[webkit-reviews] review denied: [Bug 58652] Web Inspector: Background network events collection - add GUI to Inspector : [Attachment 93311] Another one

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has denied Sergey Vorobyev
<sergeyvorobyev at google.com>'s request for review:
Bug 58652: Web Inspector: Background network events collection - add GUI to
Inspector
https://bugs.webkit.org/show_bug.cgi?id=58652

Attachment 93311: Another one
https://bugs.webkit.org/attachment.cgi?id=93311&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=93311&action=review

> LayoutTests/http/tests/inspector/inspector-test.js:323
> +InspectorTest.enableBackgroundEventCollection = function()

These three methods are specific to network inspection. They should go to
network-test.js

> LayoutTests/http/tests/inspector/inspector-test.js:330
> +	   WebInspector.panels.network._toggleBackgroundEventsCollection();

It's overcomplicated. We don't need to reload page in
enableBackgroundEventCollection. If you would like to clear resources you can
send disable command followed by enable but I'd rather make the test not depend
on the previous state.

> LayoutTests/http/tests/inspector/inspector-test.js:342
> +InspectorTest.addToResultNetworkResources = function()

dumpNetworkResources

> LayoutTests/http/tests/inspector/inspector-test.js:345
> +    resources.sort(function(a, b) {return a.url.localeCompare(b.url);});

Wouldn't resources.sort() give the same result?

> LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html:6
> +function loadImages(size)

loadImagesAndReopenFrontend

> LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html:19

> +function loadImages400()

Inline this function?

> LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html:24

> +function loadImages600()

ditto