[webkit-reviews] review denied: [Bug 58652] Web Inspector: Background network events collection - add GUI to Inspector : [Attachment 91674] fix error after review
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri May 6 03:12:09 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 91674: fix error after review
https://bugs.webkit.org/attachment.cgi?id=91674&action=review
------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=91674&action=review
> LayoutTests/ChangeLog:8
> + Web Inspector: Background network events collection - add GUI to
Inspector.
Bug description and URL should go first(see other log entries).
> LayoutTests/http/tests/inspector/inspector-test.js:318
> +var reopenedFrontendCount = 0;
frontendReopeningCount
> LayoutTests/http/tests/inspector/network/network-open-load-reopen.html:34
> + }
Style nit: else should go on the same line.
> LayoutTests/http/tests/inspector/network/network-open-load-reopen.html:38
> + InspectorTest.addResult("resources count = " + resourcesCount);
Could you also dump the resources before frontend reopening. Ideally the two
sets should also be compared after the reopening.
> LayoutTests/http/tests/inspector/network/network-open-load-reopen.html:54
> +Tests that network panel displays resource content correctly after the open
- load - reopen sequence.
Test that when background event collection is enabled network panel correctly
restores network log after the frontend reopening.
> Source/WebCore/ChangeLog:7
> +
You should provide a brief description of the UI and semantic changes.
> Source/WebCore/ChangeLog:8
> + Test: http/tests/inspector/network/network-open-load-reopen.html
This test doesn't check event collection with closed frontend, it only verifies
that the network log will be restore after the frontend reopening. I'd
recommend you add a test that would test the background event collection.
> Source/WebCore/inspector/InspectorFrontendProxy.cpp:58
> +InspectorFrontendChannel*
InspectorFrontendProxy::getInspectorFrontendChannel()
getInspectorFrontendChannel -> inspectorFrontendChannel (we don't use get
prefix for getters in the native part of WebCore).
> Source/WebCore/inspector/InspectorResourceAgent.cpp:71
> +static const char getBackgroundEventsCollectionState[] =
"getBackgroundEventsCollectionState";
getBackgroundEventsCollectionState -> backgroundEventsCollectionEnabled (like
resourceAgentEnabled above)
> Source/WebCore/inspector/InspectorResourceAgent.cpp:342
> +bool InspectorResourceAgent::getBackgroundEventsCollectionState()
getBackgroundEventsCollectionState -> backgroundEventsCollectionEnabled
> Source/WebCore/inspector/InspectorResourceAgent.cpp:347
> +void
InspectorResourceAgent::setBackgroundEventsCollectionEnabled(ErrorString*,
bool& enabled)
bool& -> bool
> Source/WebCore/inspector/InspectorResourceAgent.cpp:399
> + , m_eventsCollector(adoptPtr(new EventsCollector()))
I think these objects may be created lazily only when the option is enabled.
> Source/WebCore/inspector/front-end/NetworkPanel.js:33
> + var eventsCollectionEnabled = function(error, enabled)
Please use a local named function and then bind it to 'this' like we do in
other places in the front-end.
More information about the webkit-reviews
mailing list