DevTools: implement release note behind an experiment

DevTools: implement release note behind an experiment

Behind a hidden experiment.

BUG=682879
Review-Url: https://codereview.chromium.org/2649023007
Cr-Commit-Position: refs/heads/master@{#452981}
Committed: https://chromium.googlesource.com/chromium/src/+/3f6f704e1dd29f27151815af2b24e5da87711546

[chenwilliam, 2017-02-09 23:38:17]

Description was changed from

==========
DevTools: What's New?


BUG=
==========

to

==========
DevTools: What's New?

Behind a hidden experiment.

BUG=682879
==========

[chenwilliam, 2017-02-09 23:38:41]

Description was changed from

==========
DevTools: What's New?

Behind a hidden experiment.

BUG=682879
==========

to

==========
DevTools: implement release notes behind an experiment

Behind a hidden experiment.

BUG=682879
==========

[chenwilliam, 2017-02-09 23:39:48]

chenwilliam@chromium.org changed reviewers:
+ einbinder@chromium.org, luoe@chromium.org

[chenwilliam, 2017-02-09 23:39:48]

ptal!

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/console/releaseNoteMessage.css
(right):

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/releaseNoteMessage.css:61:
font-family: '.SFNSDisplay-Regular', sans-serif !important;
ideas on how to override the monospace font with the platform font that we use?

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/releaseNoteMessage.css:73:
.dth-button {
shouldn't actually be in here... @einbinder is landing the buttons style in
another CL?

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right):

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1933:
UI.createExternalLink = function(url, linkText, className, preventClick,
unstyled) {
I want to convert all the optional parameters to an options object in a f/u CL.

[luoe, 2017-02-11 00:30:55]

High level comment: let's not modify 'level' and 'type', since SDK shouldn't
affect/be aware of release notes.

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
(right):

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:40:
this.registerRequiredCSS('console/releaseNoteMessage.css');
Could we move releaseNoteMessage.css into the help module?

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:531:
_formatReleaseNote(obj) {
Let's move this into the release note module.

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:534:
var container = createElement('div');
Some of this logic already exists through our DOMExtension helpers!
- createElementWithClass('div', 'release-note-container')
- There's already Element.prototype.createTextChild()
- createChild() takes className as a 2nd argument

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:552:
notSeenObserver.observe(container);
Is there any guarantee that the order in which IntersectionObserver callbacks
are fired is the same as the order in which we call observe()?

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:580:
});
css `:hover`?

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:593:
var highlightElement = highlightContainer.createChild('li');
Can we simplify the DOM and do without highlightElement?

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:595:
highlight.link, highlight.text, 'release-note-highlight release-note-link',
undefined, true);
`release-note-highlight` seems to have to rules?

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:603:
UI.createExternalLink(releaseNote.link, 'And more...', 'dth-button is-primary',
undefined, true);
Common.UIString() for 'And more...' and 'Hide'

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/console/releaseNoteMessage.css
(right):

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/releaseNoteMessage.css:8:
padding: 0 !important;
!!!are there any alternatives?

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/externs.js (right):

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/externs.js:123: * @return
{!Array.<!IntersectionObserverEntry>}
nit: remove dot

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/help/ReleaseNoteManager.js
(right):

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNoteManager.js:18: }
How about something shorter?
return _releaseNotesByVersion.find(note => note.version === chromeVersion)

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNoteManager.js:61:
note.timeout = this._timeoutDuration;
In the future, other assistance elements besides release notes could probably
benefit from the same if-hidden-before-timeout-don't-count-as-viewed logic. 
Could we move the intersectionObserver logic into this file and have the
releaseNotesManager manage it?

this._timeoutDuration is only used here.  Let's inline it or make a const.

"_getReleaseNote()" sounds like it just fetches and returns something.  Can we
move the set-Timeout into the individual call sites?

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNoteManager.js:111:
Help.ReleaseNoteManager._releaseNotesByVersion = [
Can we move this data object into its own file?

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNoteManager.js:174: var
link = Help.ReleaseNoteManager.getCurrentVersionReleaseNote().link;
getCurrentVersionReleaseNote() may be null

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js (right):

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:487: Other:
'other',
nit: commas at the end of the last entry should be consistent.  It looks like
this file excluded them before

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:522: Note:
'note',
I don't like how making release-notes a ConsoleViewMessage has introduced a note
'level' and 'type', making SDK aware of it.

If we want it viewported, can we try an alternative approach where we make a new
class that implements Console.ConsoleViewportElement, something like
Help.ConsoleHelpMessage?  It would require slight modification to ConsoleView's
itemCount, itemElement, fastHeight, but avoids touching ConsoleViewMessage at
all.

If we know that the help message is always going to be at the top, it could be
similar to the 'N messages hidden by filters' element that isn't viewported.  If
we end up wanting it to always be at the bottom, we can insert it after the
viewport but above the prompt.  wdyt?

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right):

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1937: var a =
createElementWithClass('span', className);
Doesn't `a` get the className set no matter what, whether or not unstyled is
true?

[chenwilliam, 2017-02-15 00:57:09]

ptal. thanks!

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:531:
_formatReleaseNote(obj) {
On 2017/02/11 00:30:54, luoe wrote:
> Let's move this into the release note module.

Done.

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:534:
var container = createElement('div');
On 2017/02/11 00:30:54, luoe wrote:
> Some of this logic already exists through our DOMExtension helpers!
> - createElementWithClass('div', 'release-note-container')
> - There's already Element.prototype.createTextChild()
> - createChild() takes className as a 2nd argument
> 

Done.

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:552:
notSeenObserver.observe(container);
On 2017/02/11 00:30:54, luoe wrote:
> Is there any guarantee that the order in which IntersectionObserver callbacks
> are fired is the same as the order in which we call observe()?

Removed.

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:580:
});
On 2017/02/11 00:30:54, luoe wrote:
> css `:hover`?

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.

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:593:
var highlightElement = highlightContainer.createChild('li');
On 2017/02/11 00:30:54, luoe wrote:
> Can we simplify the DOM and do without highlightElement?

Tried doing without it but it makes styling the bullet point separately (e.g.
black color) tricky.

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:595:
highlight.link, highlight.text, 'release-note-highlight release-note-link',
undefined, true);
On 2017/02/11 00:30:54, luoe wrote:
> `release-note-highlight` seems to have to rules?

Done.

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:603:
UI.createExternalLink(releaseNote.link, 'And more...', 'dth-button is-primary',
undefined, true);
On 2017/02/11 00:30:54, luoe wrote:
> Common.UIString() for 'And more...' and 'Hide'

Done.

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/help/ReleaseNoteManager.js
(right):

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNoteManager.js:111:
Help.ReleaseNoteManager._releaseNotesByVersion = [
On 2017/02/11 00:30:54, luoe wrote:
> Can we move this data object into its own file?

Done.

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right):

https://codereview.chromium.org/2649023007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1937: var a =
createElementWithClass('span', className);
On 2017/02/11 00:30:54, luoe wrote:
> Doesn't `a` get the className set no matter what, whether or not unstyled is
> true?

L1960 should override based on unstyled flag.

[einbinder, 2017-02-15 20:10:45]

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js (right):

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:5:
Help.ReleaseNoteManager = class {
This doesn't look like it needs to be a class, just a static method:
getReleaseNote

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:69: return {
Let's keep the test code in the test file.

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:119:
`${Common.UIString('New in DevTools')} v${releaseNote.version}`;
Common.UIString('New in DevTools v%d', releaseNote.version)

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right):

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1930: * @param
{boolean=} unstyled
What is the styling you are trying to remove? Can you not turn it off with css?

[luoe, 2017-02-15 22:32:33]

The 'Hide' button is a few pixels taller than the 'and more...' button.

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js (right):

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:11:
this._init();
We could inline the _init function here.

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:23: */
Return value doesn't seem used.

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:38: if
(this._releaseNote.version !== browserVersion)
If we require an exact match, we don't need to do the while loop in L50

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:55: }
This is getting the newest release note before or equal to the version.  If we
make release notes in Chrome 60, stop making release notes for 61 - 63, then
start showing notes in 64, I don't think we want to show old release notes from
60 to new users.

Would we have to impose all release notes to be written in a future-proof way? 
Should devs who add a new release note update text from older release notes to
be compatible?

We could explore something like, never show release notes if they are more than
X versions old?  wdyt?

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/help/ReleaseNoteText.js
(right):

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNoteText.js:6: // be
shown in Canary (e.g. make sure the release notes are accurate)
nit: period at the end.

I think it would be nice if we had a link directing people to crbug/github docs
that explained the process behind adding new release notes.  There, we could
explain how to time the notes with major releases, limiting the number of notes
to N, etc.

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/help/module.json (right):

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/module.json:7: "title":
"What's New?",
My personal preference is to not include the '?' question mark at the end

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right):

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1930: * @param
{boolean=} unstyled
+1

[chenwilliam, 2017-02-16 21:32:41]

chenwilliam@chromium.org changed reviewers:
+ dgozman@chromium.org, pfeldman@chromium.org

[chenwilliam, 2017-02-16 21:32:43]

@dgozman and @pfeldman ptal. screenshot in bug.

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js (right):

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:5:
Help.ReleaseNoteManager = class {
On 2017/02/15 20:10:45, einbinder wrote:
> This doesn't look like it needs to be a class, just a static method:
> getReleaseNote

Done.

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:11:
this._init();
On 2017/02/15 22:32:29, luoe wrote:
> We could inline the _init function here.

Ack. Used for testing.

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:23: */
On 2017/02/15 22:32:29, luoe wrote:
> Return value doesn't seem used.

Done.

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:38: if
(this._releaseNote.version !== browserVersion)
On 2017/02/15 22:32:29, luoe wrote:
> If we require an exact match, we don't need to do the while loop in L50

Acknowledged.

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:55: }
On 2017/02/15 22:32:29, luoe wrote:
> This is getting the newest release note before or equal to the version.  If we
> make release notes in Chrome 60, stop making release notes for 61 - 63, then
> start showing notes in 64, I don't think we want to show old release notes
from
> 60 to new users.
> 
> Would we have to impose all release notes to be written in a future-proof way?

> Should devs who add a new release note update text from older release notes to
> be compatible?
> 
> We could explore something like, never show release notes if they are more
than
> X versions old?  wdyt?

Discussed in person

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:69: return {
On 2017/02/15 20:10:45, einbinder wrote:
> Let's keep the test code in the test file.

Done.

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:119:
`${Common.UIString('New in DevTools')} v${releaseNote.version}`;
On 2017/02/15 20:10:45, einbinder wrote:
> Common.UIString('New in DevTools v%d', releaseNote.version)

Done.

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/help/ReleaseNoteText.js
(right):

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNoteText.js:6: // be
shown in Canary (e.g. make sure the release notes are accurate)
On 2017/02/15 22:32:29, luoe wrote:
> nit: period at the end.
> 
> I think it would be nice if we had a link directing people to crbug/github
docs
> that explained the process behind adding new release notes.  There, we could
> explain how to time the notes with major releases, limiting the number of
notes
> to N, etc.

Done.

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right):

https://codereview.chromium.org/2649023007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1930: * @param
{boolean=} unstyled
On 2017/02/15 20:10:45, einbinder wrote:
> What is the styling you are trying to remove? Can you not turn it off with
css?

Done.

[dgozman, 2017-02-17 22:27:04]

Some high-level comments. Will take a closer look at UI code soon.

https://codereview.chromium.org/2649023007/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js (right):

https://codereview.chromium.org/2649023007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:10:
Help.ReleaseNoteManager.releaseNote = function() {
Help.latestReleaseNote = function() {}

https://codereview.chromium.org/2649023007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:14:
Help.ReleaseNoteManager.init = function() {
showIfNeeded

https://codereview.chromium.org/2649023007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:16:
Help.ReleaseNoteManager._releaseNote =
I'd init this lazily on the first call to latestReleaseNote().

https://codereview.chromium.org/2649023007/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/2649023007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/main/Main.js:286:
Extensions.extensionServer.initializeExtensions();
Move it here.

[chenwilliam, 2017-02-17 23:23:49]

updated per @dgozman comments

https://codereview.chromium.org/2649023007/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js (right):

https://codereview.chromium.org/2649023007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:10:
Help.ReleaseNoteManager.releaseNote = function() {
On 2017/02/17 22:27:03, dgozman wrote:
> Help.latestReleaseNote = function() {}

Done.

https://codereview.chromium.org/2649023007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:14:
Help.ReleaseNoteManager.init = function() {
On 2017/02/17 22:27:04, dgozman wrote:
> showIfNeeded

Done.

https://codereview.chromium.org/2649023007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:16:
Help.ReleaseNoteManager._releaseNote =
On 2017/02/17 22:27:03, dgozman wrote:
> I'd init this lazily on the first call to latestReleaseNote().

Done.

https://codereview.chromium.org/2649023007/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/2649023007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/main/Main.js:286:
Extensions.extensionServer.initializeExtensions();
On 2017/02/17 22:27:04, dgozman wrote:
> Move it here.

Done.

[dgozman, 2017-02-21 19:31:07]

dgozman@chromium.org changed reviewers:
+ alph@chromium.org

[dgozman, 2017-02-21 19:31:08]

alph@ is very enthusiastic about reviewing this patch, so I'm deferring to him.

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

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/inspector/help/release-note.html (right):

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/help/release-note.html:11: text: 'Test
bullet #1',
We use double quotes in tests.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/help/release-note.html:28: const
originalShowPanel = UI.inspectorView.showPanel.bind(UI.inspectorView);
Why do you need original one? the sniffer will do the work on calling the
original.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/help/release-note.html:39: const
releaseNoteView = UI.viewManager.view('release-note');
We don't use const. It turned out to be slow.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/BUILD.gn (right):

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/BUILD.gn:251:
"front_end/help/ReleaseNote.js",
nit: ReleaseNotes

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/BUILD.gn:828:
"$resources_out_dir/help/help_module.js",
Do you need this?

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js (right):

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:8:
Help.latestReleaseNote = function() {
The namespace Help should go into file Help.js

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:20:
Help._getReleaseNoteByVersion = function(version) {
This function looks strange, why do you need to return one of release notes,
that is <= version?

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:22: var note;
move it to definition.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:39:
Help._releaseNoteVersionSetting =
Common.settings.createSetting('releaseNoteVersionSeen', 0, false);
createLocalSetting

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:58: if
(Help.latestReleaseNote().version !== browserVersion)
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?

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:72:
Help._browserVersion = parseInt(chromeMatch[1].split('.')[0], 10);
No need for double parsing. Just extract the only number you need, e.g.
/Chrome\/(\d+)/
Also replace parseInt() with Number()

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:91:
insertText(container);
no need to extract these into functions. They are just used once.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:101:
Common.UIString('New in DevTools v%d', releaseNote.version);
drop 'v'?

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:109: var
openReleaseNote = (event) => {
no need () around event

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:113: var
viewMoreButton = UI.createTextButton(Common.UIString('And more...'),
openReleaseNote);
just inline the handler here. It's small.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:120: var
closeButton = UI.createTextButton(Common.UIString('Hide'), closeDrawerTab,
'close-release-note');
How about name it Dismiss?

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:132:
image.addEventListener('mouseover', e =>
container.classList.add('image-hover'));
Can't you do this with :hover pseudo style?

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:150: /**
@typedef {!{version: number, highlights: !Array<!Help.ReleaseNoteHighlight>,
link: string, image: !Help.ReleaseNoteImage}} */
Please make it less than 120 columns.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/help/ReleaseNoteText.js
(right):

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNoteText.js:10:
Help.releaseNotesByVersion = [
The name is misleading. This is not a map by version. This is just a list.

[chenwilliam, 2017-02-22 02:35:50]

The CQ bit was checked by chenwilliam@chromium.org

[chenwilliam, 2017-02-22 02:35:55]

The CQ bit was unchecked by chenwilliam@chromium.org

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

@alph, could you take another look please?

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/inspector/help/release-note.html (right):

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/help/release-note.html:11: text: 'Test
bullet #1',
On 2017/02/21 23:19:42, alph wrote:
> We use double quotes in tests.

Done.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/help/release-note.html:28: const
originalShowPanel = UI.inspectorView.showPanel.bind(UI.inspectorView);
On 2017/02/21 23:19:42, alph wrote:
> Why do you need original one? the sniffer will do the work on calling the
> original.

Done. I misunderstood how the sniffer worked.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/help/release-note.html:39: const
releaseNoteView = UI.viewManager.view('release-note');
On 2017/02/21 23:19:42, alph wrote:
> We don't use const. It turned out to be slow.

Done.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/BUILD.gn (right):

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/BUILD.gn:251:
"front_end/help/ReleaseNote.js",
On 2017/02/21 23:19:42, alph wrote:
> nit: ReleaseNotes

It seems more consistent to keep it singular everywhere? If not, I can rename
it.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/BUILD.gn:828:
"$resources_out_dir/help/help_module.js",
On 2017/02/21 23:19:42, alph wrote:
> Do you need this?

Removed. I forgot to remove this when I made this module auto-start.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js (right):

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:8:
Help.latestReleaseNote = function() {
On 2017/02/21 23:19:42, alph wrote:
> The namespace Help should go into file Help.js

Done.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:20:
Help._getReleaseNoteByVersion = function(version) {
On 2017/02/21 23:19:42, alph wrote:
> This function looks strange, why do you need to return one of release notes,
> that is <= version?

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

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:22: var note;
On 2017/02/21 23:19:42, alph wrote:
> move it to definition.

Done.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:39:
Help._releaseNoteVersionSetting =
Common.settings.createSetting('releaseNoteVersionSeen', 0, false);
On 2017/02/21 23:19:42, alph wrote:
> createLocalSetting

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).

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:58: if
(Help.latestReleaseNote().version !== browserVersion)
On 2017/02/21 23:19:42, alph wrote:
> 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?

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.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:72:
Help._browserVersion = parseInt(chromeMatch[1].split('.')[0], 10);
On 2017/02/21 23:19:42, alph wrote:
> No need for double parsing. Just extract the only number you need, e.g.
> /Chrome\/(\d+)/
> Also replace parseInt() with Number()

Done.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:91:
insertText(container);
On 2017/02/21 23:19:42, alph wrote:
> no need to extract these into functions. They are just used once.

Done.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:101:
Common.UIString('New in DevTools v%d', releaseNote.version);
On 2017/02/21 23:19:42, alph wrote:
> drop 'v'?

Done.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:109: var
openReleaseNote = (event) => {
On 2017/02/21 23:19:42, alph wrote:
> no need () around event

Done.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:113: var
viewMoreButton = UI.createTextButton(Common.UIString('And more...'),
openReleaseNote);
On 2017/02/21 23:19:43, alph wrote:
> just inline the handler here. It's small.

Done.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:120: var
closeButton = UI.createTextButton(Common.UIString('Hide'), closeDrawerTab,
'close-release-note');
On 2017/02/21 23:19:42, alph wrote:
> How about name it Dismiss?

Done.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:132:
image.addEventListener('mouseover', e =>
container.classList.add('image-hover'));
On 2017/02/21 23:19:42, alph wrote:
> Can't you do this with :hover pseudo style?

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.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:150: /**
@typedef {!{version: number, highlights: !Array<!Help.ReleaseNoteHighlight>,
link: string, image: !Help.ReleaseNoteImage}} */
On 2017/02/21 23:19:43, alph wrote:
> Please make it less than 120 columns.

Done.

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/help/ReleaseNoteText.js
(right):

https://codereview.chromium.org/2649023007/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNoteText.js:10:
Help.releaseNotesByVersion = [
On 2017/02/21 23:19:43, alph wrote:
> The name is misleading. This is not a map by version. This is just a list.

Done.

[alph, 2017-02-22 21:59:45]

I have a concern with the chosen approach. Will it work for our Canary users?
If I got it right, in Canary we will increases the browser version first and
then incrementally populate the items list,
so basically the user won't see any items.

Did you consider having a list with each item having a sequential number (not
related to the version),
Given the last shown number, you just show items not seen by the user.
This way the Canary users would see items as soon as they hit Canary.
Stable users would see the release items all together once they upgrade. Wdyt?

[alph, 2017-02-23 00:39:08]

almost looks good

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/help/Help.js (right):

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/Help.js:21: const
originalVersion = version;
please avoid using const

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/Help.js:29: return
Help.releaseNoteText[Help.releaseNoteText.length - 1];
peekLast

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/Help.js:50: * @return
{?Promise<?UI.Panel>}
Can't see where the Promise is used

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/Help.js:70: /** @type {number}
*/
is it needed?

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js (right):

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:9:
this._releaseNoteElement =
this._createReleaseNoteElement(Help.latestReleaseNote());
make it a var

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:29: var
viewMoreButton = UI.createTextButton(Common.UIString('And more...'), event => {
Let's make it the last item in the list instead of a button. Wdyt?

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/help/releaseNote.css (right):

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/releaseNote.css:37: padding: 0
0 12px 0;
do you need the bottom padding?

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/releaseNote.css:45: width:
300px;
max-width perhaps?

[chenwilliam, 2017-02-23 02:31:05]

@alph - please take another look.

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/help/Help.js (right):

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/Help.js:21: const
originalVersion = version;
On 2017/02/23 00:39:07, alph wrote:
> please avoid using const

Done.

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/Help.js:29: return
Help.releaseNoteText[Help.releaseNoteText.length - 1];
On 2017/02/23 00:39:07, alph wrote:
> peekLast

Done. Had to add a cast because of this Closure warning:

found   : (undefined|{
  highlights: Array<{
  featured: (boolean|undefined),
  link: string,
  text: string
}>,
  image: {src: string},
  link: string,
  version: number
})

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/Help.js:50: * @return
{?Promise<?UI.Panel>}
On 2017/02/23 00:39:07, alph wrote:
> Can't see where the Promise is used

Used it for testing in release-note-unit.html. I've removed it and used a
sniffer instead.

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/Help.js:70: /** @type {number}
*/
On 2017/02/23 00:39:07, alph wrote:
> is it needed?

Not strictly needed, but just to make sure I don't assign it as another type
somewhere else.

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js (right):

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:9:
this._releaseNoteElement =
this._createReleaseNoteElement(Help.latestReleaseNote());
On 2017/02/23 00:39:08, alph wrote:
> make it a var

Done.

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/ReleaseNote.js:29: var
viewMoreButton = UI.createTextButton(Common.UIString('And more...'), event => {
On 2017/02/23 00:39:08, alph wrote:
> Let's make it the last item in the list instead of a button. Wdyt?

This is based on Chris's design. We can polish UI after this CL lands?

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/help/releaseNote.css (right):

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/releaseNote.css:37: padding: 0
0 12px 0;
On 2017/02/23 00:39:08, alph wrote:
> do you need the bottom padding?

It's used when DevTools width is small and the image is wrapped above the text.
Added comment.

https://codereview.chromium.org/2649023007/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/help/releaseNote.css:45: width:
300px;
On 2017/02/23 00:39:08, alph wrote:
> max-width perhaps?

Done.

[chenwilliam, 2017-02-23 02:31:36]

Description was changed from

==========
DevTools: implement release notes behind an experiment

Behind a hidden experiment.

BUG=682879
==========

to

==========
DevTools: implement release note behind an experiment

Behind a hidden experiment.

BUG=682879
==========

[alph, 2017-02-23 21:48:47]

lgtm

[chenwilliam, 2017-02-23 22:07:07]

The CQ bit was checked by chenwilliam@chromium.org

[commit-bot: I haz the power, 2017-02-23 22:07:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 00:25:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 00:25:43]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chenwilliam, 2017-02-24 19:16:55]

The CQ bit was checked by chenwilliam@chromium.org

[chenwilliam, 2017-02-24 19:16:55]

The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org
Link to the patchset: https://codereview.chromium.org/2649023007/#ps280001
(title: "make test platform independent")

[commit-bot: I haz the power, 2017-02-24 19:17:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 23:09:17]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1487963815239690,
"parent_rev": "533c31e78be276dcaa843dc66efd4fee697d9573", "commit_rev":
"3f6f704e1dd29f27151815af2b24e5da87711546"}

[commit-bot: I haz the power, 2017-02-24 23:10:00]

Description was changed from

==========
DevTools: implement release note behind an experiment

Behind a hidden experiment.

BUG=682879
==========

to

==========
DevTools: implement release note behind an experiment

Behind a hidden experiment.

BUG=682879

Review-Url: https://codereview.chromium.org/2649023007
Cr-Commit-Position: refs/heads/master@{#452981}
Committed:
https://chromium.googlesource.com/chromium/src/+/3f6f704e1dd29f27151815af2b24...
==========

[commit-bot: I haz the power, 2017-02-24 23:10:02]

Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/3f6f704e1dd29f27151815af2b24...