[webkit-reviews] review denied: [Bug 58652] Web Inspector: Background network events collection - add GUI to Inspector : [Attachment 91650] Fix merge conflicts in gtk

[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 91650: Fix merge conflicts in gtk
https://bugs.webkit.org/attachment.cgi?id=91650&action=review

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

> LayoutTests/http/tests/inspector/network/network-open-load-reopen.html:6
> +function reopenFrontend() {

Please move this function to inspector-test.js since it's shared by at least
two tests.

> LayoutTests/http/tests/inspector/network/network-open-load-reopen.html:35
> +	       InspectorTest.evaluateInPage("reopenFrontend()");

You might start reopening front-end before the resources have loaded.

> LayoutTests/http/tests/inspector/network/network-open-load-reopen.html:40
> +	       for (i = 0; i < resourcesCount; i++) {

How can you be sure that all the images have already loaded?

> Source/WebCore/inspector/Inspector.json:451
> +		   "name": "switchBackgroundEventsCollectionTo",

Should be setBackgroundEventCollectionEnabled(true/false).

> Source/WebCore/inspector/Inspector.json:458
> +		   "name": "backgroundEventsCollectionEnabled",

getBackgroundEventCollectionState

> Source/WebCore/inspector/InspectorResourceAgent.cpp:94
> +	   disable(0);

The contract is that ErrorString* is non-0.

> Source/WebCore/inspector/InspectorResourceAgent.cpp:-186
> -	   ErrorString error;

Please revert this.

> Source/WebCore/inspector/InspectorResourceAgent.cpp:351
> +	   InspectorFrontendChannel* client =
m_frontend->getInspectorFrontendChannel();

Please put an ASSERT(m_frontend)

> Source/WebCore/inspector/InspectorResourceAgent.cpp:355
> +	   // Take out Message Proxy from reveiver chain.

reveiver -> receiver

> Source/WebCore/inspector/InspectorResourceAgent.h:108
> +    Frame* frameForId(const String& frameId);

Should be removed?

> Source/WebCore/inspector/front-end/NetworkPanel.js:603
> +	   var self = this;

Use eventCollectionEnabled.bind(this) instead(like we do in other places).

> Source/WebCore/inspector/front-end/NetworkPanel.js:604
> +	   function callbackEventsCollectionEnabled(error, enabled)

eventCollectionEnabled

> Source/WebCore/inspector/front-end/NetworkPanel.js:610
> +	   this._backgroundEventsCollectionToggle.addEventListener("click",
this._toggleBackgroundEventsCollection.bind(this), false);

I believe we should first try adding context menu item instead of the status
bar item. We can move it to the status bar later when the feature will be
tested.