DevTools: Show badges instead of products in ConsoleContextSelector

DevTools: Show badges instead of products in ConsoleContextSelector

BUG=719058
Review-Url: https://codereview.chromium.org/2889833003
Cr-Commit-Position: refs/heads/master@{#472666}
Committed: https://chromium.googlesource.com/chromium/src/+/9eaa9ffde680190e6c32965bf66ec92be1df7ef1

[einbinder, 2017-05-16 23:08:22]

einbinder@chromium.org changed reviewers:
+ pfeldman@chromium.org

[einbinder, 2017-05-16 23:08:23]

ptal

[pfeldman, 2017-05-16 23:16:06]

https://codereview.chromium.org/2889833003/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js
(right):

https://codereview.chromium.org/2889833003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:88:
var selectedContext = UI.context.flavor(SDK.ExecutionContext);
Why do we need this?

https://codereview.chromium.org/2889833003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:270:
var resourceTreeModel = executionContext.target().model(SDK.ResourceTreeModel);
Do it for the main worlds only.

https://codereview.chromium.org/2889833003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:411:
title.appendChild(badgeElement);
When you discard the list item, it would be helpful to unlink badge from it -
otherwise you leak connected dom substree instead of leaking just one div.

https://codereview.chromium.org/2889833003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js
(right):

https://codereview.chromium.org/2889833003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js:18:
badgeForFrame(frame, notClickable) {
pointer-events: none on the outer element should do the trick.

[einbinder, 2017-05-16 23:58:27]

https://codereview.chromium.org/2889833003/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js
(right):

https://codereview.chromium.org/2889833003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:88:
var selectedContext = UI.context.flavor(SDK.ExecutionContext);
On 2017/05/16 at 23:16:05, pfeldman wrote:
> Why do we need this?

Trying to update selection in the list as late as possible. This avoids creating
dom elements before the list is shown.

https://codereview.chromium.org/2889833003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:270:
var resourceTreeModel = executionContext.target().model(SDK.ResourceTreeModel);
On 2017/05/16 at 23:16:05, pfeldman wrote:
> Do it for the main worlds only.

I don't follow.

https://codereview.chromium.org/2889833003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:411:
title.appendChild(badgeElement);
On 2017/05/16 at 23:16:05, pfeldman wrote:
> When you discard the list item, it would be helpful to unlink badge from it -
otherwise you leak connected dom substree instead of leaking just one div.

Done.

https://codereview.chromium.org/2889833003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js
(right):

https://codereview.chromium.org/2889833003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js:18:
badgeForFrame(frame, notClickable) {
On 2017/05/16 at 23:16:05, pfeldman wrote:
> pointer-events: none on the outer element should do the trick.

That removes the title hover effect.

[pfeldman, 2017-05-17 18:12:41]

https://codereview.chromium.org/2889833003/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js
(right):

https://codereview.chromium.org/2889833003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:270:
var resourceTreeModel = executionContext.target().model(SDK.ResourceTreeModel);
You are showing frame badges on the content scripts, no? I don't think you
should.

[einbinder, 2017-05-17 20:45:32]

https://codereview.chromium.org/2889833003/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js
(right):

https://codereview.chromium.org/2889833003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:270:
var resourceTreeModel = executionContext.target().model(SDK.ResourceTreeModel);
On 2017/05/17 at 18:12:41, pfeldman wrote:
> You are showing frame badges on the content scripts, no? I don't think you
should.

Done. I think?

[pfeldman, 2017-05-17 21:42:22]

>> That removes the title hover effect.

That title is in the popup anyways, so I would just nuke it.

https://codereview.chromium.org/2889833003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js
(right):

https://codereview.chromium.org/2889833003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:276:
this._disposeExecutionContextFunctions.set(executionContext, () => {
I would vote for something more conservative:

this._extensionContextBadgePools = new Map();

https://codereview.chromium.org/2889833003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:519:
var dispose =
this._disposeExecutionContextFunctions.get(this._list.itemAtIndex(i));
Why disposing when selected?

[einbinder, 2017-05-17 23:26:15]

Removed the hover effect and moved the badge into the subtitle.

https://codereview.chromium.org/2889833003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js
(right):

https://codereview.chromium.org/2889833003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:276:
this._disposeExecutionContextFunctions.set(executionContext, () => {
On 2017/05/17 at 21:42:22, pfeldman wrote:
> I would vote for something more conservative:
> 
> this._extensionContextBadgePools = new Map();

Done.

https://codereview.chromium.org/2889833003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:519:
var dispose =
this._disposeExecutionContextFunctions.get(this._list.itemAtIndex(i));
On 2017/05/17 at 21:42:22, pfeldman wrote:
> Why disposing when selected?

It remakes the element in refreshItemsInRange

[pfeldman, 2017-05-17 23:35:16]

lgtm

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

https://codereview.chromium.org/2889833003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js:43:
badgeElement.container = element;
We don't typically do that, we go all the way and use symbols. In this case you
can do parent.parent though:-------------------------------------------

badgeElement.parentNodeOrShadowHost().parentNodeOrShadowHost()

[einbinder, 2017-05-18 00:00:38]

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

https://codereview.chromium.org/2889833003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/BadgePool.js:43:
badgeElement.container = element;
On 2017/05/17 at 23:35:15, pfeldman wrote:
> We don't typically do that, we go all the way and use symbols. In this case
you can do parent.parent though:-------------------------------------------
> 
> badgeElement.parentNodeOrShadowHost().parentNodeOrShadowHost()

Done.

[einbinder, 2017-05-18 00:00:56]

The CQ bit was checked by einbinder@chromium.org

[einbinder, 2017-05-18 00:00:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2889833003/#ps80001
(title: "Don't use container")

[commit-bot: I haz the power, 2017-05-18 00:03:30]

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-18 04:39:03]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495065656807260,
"parent_rev": "027325f153ba7f269b197cac9b43cca8519f08bf", "commit_rev":
"9eaa9ffde680190e6c32965bf66ec92be1df7ef1"}

[commit-bot: I haz the power, 2017-05-18 04:39:14]

Description was changed from

==========
DevTools: Show badges instead of products in ConsoleContextSelector

BUG=719058
==========

to

==========
DevTools: Show badges instead of products in ConsoleContextSelector

BUG=719058

Review-Url: https://codereview.chromium.org/2889833003
Cr-Commit-Position: refs/heads/master@{#472666}
Committed:
https://chromium.googlesource.com/chromium/src/+/9eaa9ffde680190e6c32965bf66e...
==========

[commit-bot: I haz the power, 2017-05-18 04:39:15]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9eaa9ffde680190e6c32965bf66e...