DevTools: implement release note behind an experiment

third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/**
+ * @return {!Help.ReleaseNote}
+ */
+Help.latestReleaseNote = function() {

[alph, 2017/02/21 23:19:42]

The namespace Help should go into file Help.js

[chenwilliam, 2017/02/22 02:36:38]

Done.

+  if (!Help._latestReleaseNote) {
+    /** @type {!Help.ReleaseNote} */
+    Help._latestReleaseNote = Help._getReleaseNoteByVersion(Help.browserVersion());
+  }
+  return Help._latestReleaseNote;
+};
+
+/**
+ * @param {number} version
+ * @return {!Help.ReleaseNote}
+ */
+Help._getReleaseNoteByVersion = function(version) {

[alph, 2017/02/21 23:19:42]

This function looks strange, why do you need to return one of release notes,
that is <= version?

[chenwilliam, 2017/02/22 02:36:39]

It's looking for the "best" release note to show when the user explicitly opens
the "what's new" drawer.

+  const originalVersion = version;
+  var note;

[alph, 2017/02/21 23:19:42]

move it to definition.

[chenwilliam, 2017/02/22 02:36:39]

Done.

+  while (version) {
+    note = Help.releaseNotesByVersion.find(note => note.version === version);
+    if (note)
+      return note;
+    version--;
+  }
+  console.error(`Unable to find release note for version ${originalVersion} - using last release note as fallback`);
+  return Help.releaseNotesByVersion[Help.releaseNotesByVersion.length - 1];
+};
+
+/**
+ * @return {!Common.Setting}
+ */
+Help.releaseNoteVersionSetting = function() {
+  if (!Help._releaseNoteVersionSetting) {
+    /** @type {!Common.Setting} */
+    Help._releaseNoteVersionSetting = Common.settings.createSetting('releaseNoteVersionSeen', 0, false);

[alph, 2017/02/21 23:19:42]

createLocalSetting

[chenwilliam, 2017/02/22 02:36:38]

I wanted a global setting so it doesn't re-show a particular release note (e.g.
if a user has seen v58 on one computer and then uses his/her other computer, we
shouldn't show the same release note again).

+  }
+  return Help._releaseNoteVersionSetting;
+};
+
+Help.showReleaseNoteIfNeeded = function() {
+  Help._showReleaseNoteIfNeeded(Help.releaseNoteVersionSetting().get(), Help.browserVersion());
+};
+
+/**
+ * @param {number} lastSeenVersion
+ * @param {number} browserVersion
+ * @return {?Promise<?UI.Panel>}
+ */
+Help._showReleaseNoteIfNeeded = function(lastSeenVersion, browserVersion) {
+  if (!Runtime.experiments.isEnabled('releaseNote'))
+    return null;
+  if (lastSeenVersion >= browserVersion)
+    return null;
+  if (Help.latestReleaseNote().version !== browserVersion)

[alph, 2017/02/21 23:19:42]

Why's the check?
What if the latest is 57, browser is 58, last seen is 56, then we still want to
show release notes for 57. right?

[chenwilliam, 2017/02/22 02:36:39]

I'd prefer to not open the what's new drawer for them in that case. If we show
"What's New DevTools 57" - users may assume that they had just upgraded to
version 57 which is misleading. In the future, we could handle this edge case
where users skip a version, by displaying both v57 and v58 in this scenario.

+    return null;
+  Help.releaseNoteVersionSetting().set(Help.browserVersion());
+  return UI.inspectorView.showPanel(Help._releaseNoteViewId);
+};
+
+/**
+ * @return {number}
+ */
+Help.browserVersion = function() {
+  if (!Help._browserVersion) {
+    var chromeRegex = new RegExp('(?:^|\\W)Chrome/(\\S+)');
+    var chromeMatch = navigator.userAgent.match(chromeRegex);
+    /** @type {number} */
+    Help._browserVersion = parseInt(chromeMatch[1].split('.')[0], 10);

[alph, 2017/02/21 23:19:42]

No need for double parsing. Just extract the only number you need, e.g.
/Chrome\/(\d+)/
Also replace parseInt() with Number()

[chenwilliam, 2017/02/22 02:36:39]

Done.

+  }
+  return Help._browserVersion;
+};
+
+Help.ReleaseNoteView = class extends UI.VBox {
+  constructor() {
+    super(true);
+    this.registerRequiredCSS('help/releaseNote.css');
+    this._releaseNoteElement = this._createReleaseNoteElement(Help.latestReleaseNote());
+    this.contentElement.appendChild(this._releaseNoteElement);
+  }
+
+  /**
+   * @param {!Help.ReleaseNote} releaseNote
+   * @return {!Element}
+   */
+  _createReleaseNoteElement(releaseNote) {
+    var container = createElementWithClass('div', 'release-note-container');
+    insertText(container);

[alph, 2017/02/21 23:19:42]

no need to extract these into functions. They are just used once.

[chenwilliam, 2017/02/22 02:36:39]

Done.

+    insertImage(container);
+    return container;
+
+    /**
+     * @param {!Element} container
+     */
+    function insertText(container) {
+      var textContainer = container.createChild('div', 'release-note-text-container');
+      textContainer.createChild('div', 'release-note-header').textContent =
+          Common.UIString('New in DevTools v%d', releaseNote.version);

[alph, 2017/02/21 23:19:42]

drop 'v'?

[chenwilliam, 2017/02/22 02:36:39]

Done.

+      var highlightContainer = textContainer.createChild('ul', 'release-note-highlight-container');
+      for (var highlight of releaseNote.highlights) {
+        var className = highlight.featured ? 'release-note-featured-link' : 'release-note-link';
+        var highlightLink = UI.createExternalLink(highlight.link, highlight.text, className);
+        highlightContainer.createChild('li').appendChild(highlightLink);
+      }
+
+      var openReleaseNote = (event) => {

[alph, 2017/02/21 23:19:42]

no need () around event

[chenwilliam, 2017/02/22 02:36:39]

Done.

+        event.consume(true);
+        InspectorFrontendHost.openInNewTab(releaseNote.link);
+      };
+      var viewMoreButton = UI.createTextButton(Common.UIString('And more...'), openReleaseNote);

[alph, 2017/02/21 23:19:43]

just inline the handler here. It's small.

[chenwilliam, 2017/02/22 02:36:39]

Done.

+      textContainer.appendChild(viewMoreButton);
+
+      var closeDrawerTab = (event) => {
+        event.consume(true);
+        UI.inspectorView.closeDrawerTab(Help._releaseNoteViewId, true);
+      };
+      var closeButton = UI.createTextButton(Common.UIString('Hide'), closeDrawerTab, 'close-release-note');

[alph, 2017/02/21 23:19:42]

How about name it Dismiss?

[chenwilliam, 2017/02/22 02:36:39]

Done.

+      textContainer.appendChild(closeButton);
+    }
+
+    /**
+     * @param {!Element} container
+     */
+    function insertImage(container) {
+      var imageLink = UI.createExternalLink(releaseNote.link, ' ', 'release-note-image-container');
+      container.appendChild(imageLink);
+      var image = imageLink.createChild('img', 'release-note-image');
+      image.src = releaseNote.image.src;
+      image.addEventListener('mouseover', e => container.classList.add('image-hover'));

[alph, 2017/02/21 23:19:42]

Can't you do this with :hover pseudo style?

[chenwilliam, 2017/02/22 02:36:39]

I'm trying to create a selector for the featured link when the ancestor (e.g.
text-container) sibling (e.g. image-container) is hovered upon. I'm not sure how
to use css :hover in this case.

+      image.addEventListener('mouseout', e => container.classList.remove('image-hover'));
+    }
+  }
+};
+
+/**
+ * @const
+ * @type {string}
+ */
+Help._releaseNoteViewId = 'release-note';
+
+/** @typedef {!{src: string}} */
+Help.ReleaseNoteImage;
+
+/** @typedef {!{text: string, link: string, featured: (boolean | undefined)}} */
+Help.ReleaseNoteHighlight;
+
+/** @typedef {!{version: number, highlights: !Array<!Help.ReleaseNoteHighlight>, link: string, image: !Help.ReleaseNoteImage}} */

[alph, 2017/02/21 23:19:43]

Please make it less than 120 columns.

[chenwilliam, 2017/02/22 02:36:39]

Done.

+Help.ReleaseNote;