DevTools: Add Product field in timeline event details.

DevTools: Add Product field in timeline event details.

The product field is inferred from the event url using product registry.

BUG=718655
Review-Url: https://codereview.chromium.org/2859343002
Cr-Commit-Position: refs/heads/master@{#469813}
Committed: https://chromium.googlesource.com/chromium/src/+/a7dafdea8ae94ec96b4f279482e57c0df65fd695

[alph, 2017-05-05 00:03:02]

alph@chromium.org changed reviewers:
+ allada@chromium.org, caseq@chromium.org, pfeldman@chromium.org

[caseq, 2017-05-05 00:58:15]

lgtm

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/tracing/timeline-misc/timeline-event-details.html
(right):

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/tracing/timeline-misc/timeline-event-details.html:76:
Checks the details of timeline event nodes.
Let's make name more specific to the product perhaps? Other details are being
tested in their corresponding type-specific tests.

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js
(right):

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:938: if
(!parsedURL)
_maybeAppend... then?

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:1138:
var preview = request.previewElement;
let's drop preview and inline request.previewElement everywhere.

[allada, 2017-05-05 01:27:38]

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/tracing/timeline-misc/timeline-event-details-expected.txt
(right):

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/tracing/timeline-misc/timeline-event-details-expected.txt:4:
Product: Google CDN
I would rather not ever check a specific product since this list can change at
any point.

Instead lets hijack the ProductRegistry.instance() or something similar to
always resolve something specific.

I am almost in favor of not writing a test yet since I think we should add
something generic for anyone using product registry to get consistent results.

Plus... ProductRegistryImpl is a remote module, meaning it [probably] will never
resolve on bots.

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js
(right):

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:691:
var relatedNodesMap = null;
nit: When you assign null to a variable like this closure cannot type check it
at all any more. I suggest expressing the type as nullable above it (it also
makes easier for other people to come in to change things).

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:704:
var nodeIdsToResolve = new Set();
nit: also here, closure only knows it's a set, but not what should be in it.

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:943:
var name = registry.nameForUrl(parsedURL);
If we do not find a name here, shouldn't we have not created/appended a
productNode?

[dgozman, 2017-05-05 05:57:49]

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

[dgozman, 2017-05-05 05:57:50]

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/tracing/timeline-misc/timeline-event-details-expected.txt
(right):

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/tracing/timeline-misc/timeline-event-details-expected.txt:4:
Product: Google CDN
On 2017/05/05 01:27:38, allada wrote:
> I would rather not ever check a specific product since this list can change at
> any point.
> 
> Instead lets hijack the ProductRegistry.instance() or something similar to
> always resolve something specific.
> 
> I am almost in favor of not writing a test yet since I think we should add
> something generic for anyone using product registry to get consistent results.
If we don't have something already, let alph@ come up with testing
infrastructure in this patch.

> 
> Plus... ProductRegistryImpl is a remote module, meaning it [probably] will
never
> resolve on bots.
Remote modules should load from local source in layout tests.

[alph, 2017-05-05 21:46:41]

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/tracing/timeline-misc/timeline-event-details-expected.txt
(right):

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/tracing/timeline-misc/timeline-event-details-expected.txt:4:
Product: Google CDN
On 2017/05/05 01:27:38, allada wrote:
> I would rather not ever check a specific product since this list can change at
> any point.
> 
> Instead lets hijack the ProductRegistry.instance() or something similar to
> always resolve something specific.
> 
> I am almost in favor of not writing a test yet since I think we should add
> something generic for anyone using product registry to get consistent results.
> 
> Plus... ProductRegistryImpl is a remote module, meaning it [probably] will
never
> resolve on bots.

I would like to add a test for it to test the feature is still there.
If the string becomes unstable (which I doubt) we can then update the test to
match any product string.

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/tracing/timeline-misc/timeline-event-details-expected.txt:4:
Product: Google CDN
On 2017/05/05 05:57:50, dgozman wrote:
> On 2017/05/05 01:27:38, allada wrote:
> > I would rather not ever check a specific product since this list can change
at
> > any point.
> > 
> > Instead lets hijack the ProductRegistry.instance() or something similar to
> > always resolve something specific.
> > 
> > I am almost in favor of not writing a test yet since I think we should add
> > something generic for anyone using product registry to get consistent
results.
> If we don't have something already, let alph@ come up with testing
> infrastructure in this patch.
> 
> > 
> > Plus... ProductRegistryImpl is a remote module, meaning it [probably] will
> never
> > resolve on bots.
> Remote modules should load from local source in layout tests.

I believe it should work as long as network tests for products work.

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/tracing/timeline-misc/timeline-event-details.html
(right):

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/tracing/timeline-misc/timeline-event-details.html:76:
Checks the details of timeline event nodes.
On 2017/05/05 00:58:15, caseq wrote:
> Let's make name more specific to the product perhaps? Other details are being
> tested in their corresponding type-specific tests.

Done.

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js
(right):

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:691:
var relatedNodesMap = null;
On 2017/05/05 01:27:38, allada wrote:
> nit: When you assign null to a variable like this closure cannot type check it
> at all any more. I suggest expressing the type as nullable above it (it also
> makes easier for other people to come in to change things).

Done.

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:704:
var nodeIdsToResolve = new Set();
On 2017/05/05 01:27:38, allada wrote:
> nit: also here, closure only knows it's a set, but not what should be in it.

Done. Though I just moved the code. ;-)

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:938: if
(!parsedURL)
On 2017/05/05 00:58:15, caseq wrote:
> _maybeAppend... then?

Done.

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:943:
var name = registry.nameForUrl(parsedURL);
On 2017/05/05 01:27:38, allada wrote:
> If we do not find a name here, shouldn't we have not created/appended a
> productNode?

Done.

https://codereview.chromium.org/2859343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:1138:
var preview = request.previewElement;
On 2017/05/05 00:58:15, caseq wrote:
> let's drop preview and inline request.previewElement everywhere.

Done.

[alph, 2017-05-05 21:46:45]

The CQ bit was checked by alph@chromium.org

[alph, 2017-05-05 21:46:46]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org
Link to the patchset: https://codereview.chromium.org/2859343002/#ps40001
(title: "addressing comments")

[commit-bot: I haz the power, 2017-05-05 21:47: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-05-05 23:52:11]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494020805933790,
"parent_rev": "788812f7788c07aa58487523ec70f9e921d78543", "commit_rev":
"a7dafdea8ae94ec96b4f279482e57c0df65fd695"}

[commit-bot: I haz the power, 2017-05-05 23:52:23]

Description was changed from

==========
DevTools: Add Product field in timeline event details.

The product field is inferred from the event url using product registry.

BUG=718655
==========

to

==========
DevTools: Add Product field in timeline event details.

The product field is inferred from the event url using product registry.

BUG=718655

Review-Url: https://codereview.chromium.org/2859343002
Cr-Commit-Position: refs/heads/master@{#469813}
Committed:
https://chromium.googlesource.com/chromium/src/+/a7dafdea8ae94ec96b4f279482e5...
==========

[commit-bot: I haz the power, 2017-05-05 23:52:24]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/a7dafdea8ae94ec96b4f279482e5...