[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





--- Comment #9 from Sergey Vorobyev <sergeyvorobyev at google.com>  2011-05-11 05:05:38 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).

done

>> LayoutTests/http/tests/inspector/inspector-test.js:318
>> +var reopenedFrontendCount = 0;
> 
> frontendReopeningCount

done

>> LayoutTests/http/tests/inspector/network/network-open-load-reopen.html:34
>> +        }
> 
> Style nit: else should go on the same line.

done

>> 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.

It's will be too complicate source:
We need save all data in page local property and get it after reopen.
Also, it is not really nesesary

>> 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.

done

>> Source/WebCore/ChangeLog:7
>> +
> 
> You should provide a brief description of the UI and semantic changes.

done

>> 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.

ok

>> 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).

done

>> Source/WebCore/inspector/InspectorResourceAgent.cpp:71
>> +static const char getBackgroundEventsCollectionState[] = "getBackgroundEventsCollectionState";
> 
> getBackgroundEventsCollectionState -> backgroundEventsCollectionEnabled (like resourceAgentEnabled above)

done

>> Source/WebCore/inspector/InspectorResourceAgent.cpp:342
>> +bool InspectorResourceAgent::getBackgroundEventsCollectionState()
> 
> getBackgroundEventsCollectionState -> backgroundEventsCollectionEnabled

done

>> Source/WebCore/inspector/InspectorResourceAgent.cpp:347
>> +void InspectorResourceAgent::setBackgroundEventsCollectionEnabled(ErrorString*, bool& enabled)
> 
> bool& -> bool

done

>> 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.

done

>> 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.

done

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