[Webkit-unassigned] [Bug 58652] Web Inspector: Background network events collection - add GUI to Inspector

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=58652


Yury Semikhatsky <yurys at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #91674|review?                     |review-
               Flag|                            |




--- Comment #8 from Yury Semikhatsky <yurys at chromium.org>  2011-05-06 03:12:09 PST ---
(From update of attachment 91674)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.