[Devtools] New structure and colorize rows for network products

[Devtools] New structure and colorize rows for network products

Refactored the way the dependency chain is going for the products
module. We now have a shared module that is bundled and can be used to
resolve things like frame and product for url.

We are also adding background coloring to network rows.

R=pfeldman
BUG=712876
Review-Url: https://codereview.chromium.org/2839273003
Cr-Commit-Position: refs/heads/master@{#468203}
Committed: https://chromium.googlesource.com/chromium/src/+/56637492051aec80a5023fc0bd8314c0143241db

[allada, 2017-04-26 21:56:34]

PTL

[pfeldman, 2017-04-27 22:20:49]

A good size change, was easy to review. See some comments below.

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js
(right):

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:339:
static ProductComparitor(a, b) {
ProductComparator.

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:349:
return aName.compareTo(bName) || aRequest.indentityCompare(bRequest);
localeCompare

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:720:
this._renderProductCell(cell);
This should be chained after the promise.

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:873:
var productRegistry = ProductRegistry.existingInstance();
Don't use it here, just chain.

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js
(right):

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:84:
sortingFunction: Network.NetworkRequestNode.ProductComparitor
You need to either make sorting async or create this whole control after you
resolve the products.

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js
(right):

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js:191: var
extensions = self.runtime.extensions(Network.GroupLookupInterface);
This code should no longer exist.

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/module.json (right):

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/module.json:97: "type":
"@Network.GroupLookupInterface",
Don't use dependency injection when you depends on yourself.

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

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/ProductRegistry.js:8:
if (ProductRegistry._instancePromise)
Replace all this text with:

return self.runtime.extension(ProductRegistry.Registry).instance();

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/ProductRegistry.js:21:
ProductRegistry.existingInstance = function() {
You should not need this.

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/ProductRegistry.js:26:
ProductRegistry._instancePromise = null;
drop these.

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/product_registry_impl/ProductRegistryImpl.js
(right):

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry_impl/ProductRegistryImpl.js:4:
/**
blank line above.

[allada, 2017-04-28 01:20:08]

PTaL

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js
(right):

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:339:
static ProductComparitor(a, b) {
On 2017/04/27 22:20:48, pfeldman wrote:
> ProductComparator.

Done.

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:349:
return aName.compareTo(bName) || aRequest.indentityCompare(bRequest);
On 2017/04/27 22:20:48, pfeldman wrote:
> localeCompare

Done.

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:720:
this._renderProductCell(cell);
On 2017/04/27 22:20:48, pfeldman wrote:
> This should be chained after the promise.

Done.

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:873:
var productRegistry = ProductRegistry.existingInstance();
On 2017/04/27 22:20:48, pfeldman wrote:
> Don't use it here, just chain.

Done.

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js
(right):

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:84:
sortingFunction: Network.NetworkRequestNode.ProductComparitor
On 2017/04/27 22:20:49, pfeldman wrote:
> You need to either make sorting async or create this whole control after you
> resolve the products.

Sorting is a pain point of network grid as it is. I want to resolve it after
products.

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js
(right):

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js:191: var
extensions = self.runtime.extensions(Network.GroupLookupInterface);
On 2017/04/27 22:20:49, pfeldman wrote:
> This code should no longer exist.

It does exist, it is just a simple interface to know what group extensions
exist. (drop down)

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/module.json (right):

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/module.json:97: "type":
"@Network.GroupLookupInterface",
On 2017/04/27 22:20:49, pfeldman wrote:
> Don't use dependency injection when you depends on yourself.

These are extensions, it is only used to handle the drop down grouper. Do you
want me to hard code them instead?

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

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/ProductRegistry.js:8:
if (ProductRegistry._instancePromise)
On 2017/04/27 22:20:49, pfeldman wrote:
> Replace all this text with:
> 
> return self.runtime.extension(ProductRegistry.Registry).instance();

Done.

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/ProductRegistry.js:21:
ProductRegistry.existingInstance = function() {
On 2017/04/27 22:20:49, pfeldman wrote:
> You should not need this.

Done.

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry/ProductRegistry.js:26:
ProductRegistry._instancePromise = null;
On 2017/04/27 22:20:49, pfeldman wrote:
> drop these.

Done.

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/product_registry_impl/ProductRegistryImpl.js
(right):

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/product_registry_impl/ProductRegistryImpl.js:4:
/**
On 2017/04/27 22:20:49, pfeldman wrote:
> blank line above.

Done.

[pfeldman, 2017-04-28 21:08:36]

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/module.json (right):

https://codereview.chromium.org/2839273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/module.json:97: "type":
"@Network.GroupLookupInterface",
On 2017/04/28 01:20:07, allada wrote:
> On 2017/04/27 22:20:49, pfeldman wrote:
> > Don't use dependency injection when you depends on yourself.
> 
> These are extensions, it is only used to handle the drop down grouper. Do you
> want me to hard code them instead?

I want you to instantiate them via new instead of looking them up using runtime.

https://codereview.chromium.org/2839273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js
(right):

https://codereview.chromium.org/2839273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:881:
if (!element)
When does this happen?

https://codereview.chromium.org/2839273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/NetworkGroupers.js
(right):

https://codereview.chromium.org/2839273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkGroupers.js:4: /**
Where is my line? :)

[pfeldman, 2017-04-28 21:08:46]

lgtm % nits

[allada, 2017-04-28 22:21:09]

https://codereview.chromium.org/2839273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js
(right):

https://codereview.chromium.org/2839273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:881:
if (!element)
On 2017/04/28 21:08:35, pfeldman wrote:
> When does this happen?

It should not happen, however if I call element() when and existingElement() is
null it will construct it calling this function again (ie: infinite loop).

Since this function does not need to do anything in the event that the element
does not exist we simply bail if we don't have an element.

Plus... Closure complains if you don't do this check.

https://codereview.chromium.org/2839273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/NetworkGroupers.js
(right):

https://codereview.chromium.org/2839273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkGroupers.js:4: /**
On 2017/04/28 21:08:35, pfeldman wrote:
> Where is my line? :)

Done.

[allada, 2017-04-28 22:21:15]

The CQ bit was checked by allada@chromium.org

[allada, 2017-04-28 22:21:15]

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/2839273003/#ps80001
(title: "Merge remote-tracking branch 'origin/master' into
NEW_DEPENDENCY_PRODUCTS")

[commit-bot: I haz the power, 2017-04-28 22:22:06]

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-04-28 22:35:27]

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

[commit-bot: I haz the power, 2017-04-28 22:35:28]

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

[allada, 2017-04-28 22:49:58]

The CQ bit was checked by allada@chromium.org

[allada, 2017-04-28 22:49:59]

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/2839273003/#ps100001
(title: "Merge remote-tracking branch 'origin/master' into
NEW_DEPENDENCY_PRODUCTS")

[commit-bot: I haz the power, 2017-04-28 22:50:32]

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-04-29 00:04:13]

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

[commit-bot: I haz the power, 2017-04-29 00:04:14]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[allada, 2017-04-29 00:06:09]

The CQ bit was checked by allada@chromium.org

[commit-bot: I haz the power, 2017-04-29 00:07:02]

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-04-29 01:14:55]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493424369055930,
"parent_rev": "86ca67f5423cdb1e2ef6f193e2eebdb0abf83ab0", "commit_rev":
"56637492051aec80a5023fc0bd8314c0143241db"}

[commit-bot: I haz the power, 2017-04-29 01:15:07]

Description was changed from

==========
[Devtools] New structure and colorize rows for network products

Refactored the way the dependency chain is going for the products
module. We now have a shared module that is bundled and can be used to
resolve things like frame and product for url.

We are also adding background coloring to network rows.

R=pfeldman
BUG=712876
==========

to

==========
[Devtools] New structure and colorize rows for network products

Refactored the way the dependency chain is going for the products
module. We now have a shared module that is bundled and can be used to
resolve things like frame and product for url.

We are also adding background coloring to network rows.

R=pfeldman
BUG=712876

Review-Url: https://codereview.chromium.org/2839273003
Cr-Commit-Position: refs/heads/master@{#468203}
Committed:
https://chromium.googlesource.com/chromium/src/+/56637492051aec80a5023fc0bd83...
==========

[commit-bot: I haz the power, 2017-04-29 01:15:09]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/56637492051aec80a5023fc0bd83...