DevTools: Render third-party badges in timeline tree view

DevTools: Render third-party badges in timeline tree view

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

[alph, 2017-05-20 02:06:48]

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

[caseq, 2017-05-22 18:43:50]

https://codereview.chromium.org/2892333002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChartView.js
(right):

https://codereview.chromium.org/2892333002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChartView.js:102:
null, this._productRegistry, this._model.timelineModel(), this._urlToColorCache)
:
Let's avoid keeping the model here, as this mapper won't be reset when model is
reset.

https://codereview.chromium.org/2892333002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js
(right):

https://codereview.chromium.org/2892333002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js:731:
var icon = parsedURL ? this._badgePool.badgeForURL(parsedURL, false, true) :
undefined;
Can we encapsulate this logic into productByEvent() to assure the URL we're
using for the product resolution is exactly the same as the one for the badge?

[alph, 2017-05-22 22:53:26]

ptal

https://codereview.chromium.org/2892333002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChartView.js
(right):

https://codereview.chromium.org/2892333002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChartView.js:102:
null, this._productRegistry, this._model.timelineModel(), this._urlToColorCache)
:
On 2017/05/22 18:43:49, caseq wrote:
> Let's avoid keeping the model here, as this mapper won't be reset when model
is
> reset.

Done.

https://codereview.chromium.org/2892333002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js
(right):

https://codereview.chromium.org/2892333002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js:731:
var icon = parsedURL ? this._badgePool.badgeForURL(parsedURL, false, true) :
undefined;
On 2017/05/22 18:43:49, caseq wrote:
> Can we encapsulate this logic into productByEvent() to assure the URL we're
> using for the product resolution is exactly the same as the one for the badge?

Done.

[caseq, 2017-05-22 23:43:35]

https://codereview.chromium.org/2892333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js
(right):

https://codereview.chromium.org/2892333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js:25: *
@param {boolean=} ignoreSetting
Let's move this to the BadgePool level!

https://codereview.chromium.org/2892333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js:81:
if (!this._badgeElements.has(badgeElement))
Any idea if this is necessary?

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

https://codereview.chromium.org/2892333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:355:
return defaultColor;
Why don't we cache it?

[pfeldman, 2017-05-23 19:19:28]

https://codereview.chromium.org/2892333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js
(right):

https://codereview.chromium.org/2892333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js:52:
badgeElement[ProductRegistry.BadgePool._ignoreSettingSymbol] = !!ignoreSetting;
We are adding so many flags and branching points, why don't we extract couple of
methods and call them unconditionally from say alwaysVisibleBadgeForURL ?

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

https://codereview.chromium.org/2892333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js:642:
this._badgePool = new ProductRegistry.BadgePool();
You should reset it once it is no longer needed.

https://codereview.chromium.org/2892333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js:897:
return name ? {name: this._beautifyDomainName(name), badge: icon} : null;
Why do you need name?

[alph, 2017-05-24 00:53:44]

https://codereview.chromium.org/2892333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js
(right):

https://codereview.chromium.org/2892333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js:25: *
@param {boolean=} ignoreSetting
On 2017/05/22 23:43:35, caseq wrote:
> Let's move this to the BadgePool level! 

Done.

https://codereview.chromium.org/2892333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js:52:
badgeElement[ProductRegistry.BadgePool._ignoreSettingSymbol] = !!ignoreSetting;
On 2017/05/23 19:19:27, pfeldman wrote:
> We are adding so many flags and branching points, why don't we extract couple
of
> methods and call them unconditionally from say alwaysVisibleBadgeForURL ?

moved the flags to badge pool level

https://codereview.chromium.org/2892333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js:81:
if (!this._badgeElements.has(badgeElement))
On 2017/05/22 23:43:35, caseq wrote:
> Any idea if this is necessary?

Done.

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

https://codereview.chromium.org/2892333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js:642:
this._badgePool = new ProductRegistry.BadgePool();
On 2017/05/23 19:19:27, pfeldman wrote:
> You should reset it once it is no longer needed.

Done.

https://codereview.chromium.org/2892333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js:897:
return name ? {name: this._beautifyDomainName(name), badge: icon} : null;
On 2017/05/23 19:19:28, pfeldman wrote:
> Why do you need name?

We show the name in the tree next to the badge or swatch if no badge

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

https://codereview.chromium.org/2892333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:355:
return defaultColor;
On 2017/05/22 23:43:35, caseq wrote:
> Why don't we cache it?

Done.

[caseq, 2017-05-24 01:36:26]

lgtm

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

https://codereview.chromium.org/2892333002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js:8: *
@param {boolean=} persistent
forceShow?

https://codereview.chromium.org/2892333002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js:12:
this._persistent = persistent;
Do you need it? Perhaps just check this._setting?

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

https://codereview.chromium.org/2892333002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js:656:
willHide() {
Let's do it upon setModel() instead.

https://codereview.chromium.org/2892333002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js:902:
var name = parsedURL && this._productRegistry.nameForUrl(parsedURL) ||
this._domainByEvent(true, event) || '';
if (!name) return null; here. Also, you can drop || '';

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

https://codereview.chromium.org/2892333002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:345: if
(!color) {
if (color) return color;

[alph, 2017-05-24 02:00:49]

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

https://codereview.chromium.org/2892333002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js:8: *
@param {boolean=} persistent
On 2017/05/24 01:36:25, caseq wrote:
> forceShow?

Done.

https://codereview.chromium.org/2892333002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js:12:
this._persistent = persistent;
On 2017/05/24 01:36:25, caseq wrote:
> Do you need it? Perhaps just check this._setting?

Done.

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

https://codereview.chromium.org/2892333002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js:656:
willHide() {
On 2017/05/24 01:36:25, caseq wrote:
> Let's do it upon setModel() instead.

Done.

https://codereview.chromium.org/2892333002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js:902:
var name = parsedURL && this._productRegistry.nameForUrl(parsedURL) ||
this._domainByEvent(true, event) || '';
On 2017/05/24 01:36:25, caseq wrote:
> if (!name) return null; here. Also, you can drop || '';

Done.

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

https://codereview.chromium.org/2892333002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:345: if
(!color) {
On 2017/05/24 01:36:26, caseq wrote:
> if (color) return color;

Done.

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

The CQ bit was checked by alph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 02:05:11]

Dry run: 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-24 07:26:17]

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

[commit-bot: I haz the power, 2017-05-24 07:26:18]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2017-05-24 19:00:23]

https://codereview.chromium.org/2892333002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js
(right):

https://codereview.chromium.org/2892333002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js:10:
constructor(showTitles, forceShow) {
Convert non-essential args to setters?

https://codereview.chromium.org/2892333002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js:125:
_setBadgeElementVisibility(badgeElement) {
_update*

[alph, 2017-05-24 22:04:39]

https://codereview.chromium.org/2892333002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js
(right):

https://codereview.chromium.org/2892333002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js:10:
constructor(showTitles, forceShow) {
On 2017/05/24 19:00:22, pfeldman wrote:
> Convert non-essential args to setters?

Done.

https://codereview.chromium.org/2892333002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js:125:
_setBadgeElementVisibility(badgeElement) {
On 2017/05/24 19:00:22, pfeldman wrote:
> _update*

Done.

[alph, 2017-05-24 22:05:52]

The CQ bit was checked by alph@chromium.org

[alph, 2017-05-24 22:05:53]

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/2892333002/#ps90001
(title: ".")

[commit-bot: I haz the power, 2017-05-24 22:07:09]

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-25 00:22:17]

CQ is committing da patch.

Bot data: {"patchset_id": 90001, "attempt_start_ts": 1495663552964550,
"parent_rev": "574fe09c0a5017c087b7b856404d021830f88e53", "commit_rev":
"9697039dce6102f4ef9caf403dbe0ee481ae3e7a"}

[commit-bot: I haz the power, 2017-05-25 00:22:25]

Description was changed from

==========
DevTools: Render third-party badges in timeline tree view

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

to

==========
DevTools: Render third-party badges in timeline tree view

BUG=718655

Review-Url: https://codereview.chromium.org/2892333002
Cr-Commit-Position: refs/heads/master@{#474489}
Committed:
https://chromium.googlesource.com/chromium/src/+/9697039dce6102f4ef9caf403dbe...
==========

[commit-bot: I haz the power, 2017-05-25 00:22:26]

Committed patchset #6 (id:90001) as
https://chromium.googlesource.com/chromium/src/+/9697039dce6102f4ef9caf403dbe...