[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 bugzilla-daemon at webkit.org
Fri Apr 29 02:15:26 PDT 2011


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.


More information about the webkit-reviews mailing list