Bug 58652 - Web Inspector: Background network events collection - add GUI to Inspector
Summary: Web Inspector: Background network events collection - add GUI to Inspector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-15 05:50 PDT by Sergey Vorobyev
Modified: 2011-05-20 09:15 PDT (History)
12 users (show)

See Also:


Attachments
Gui added. 1 simple test (18.71 KB, patch)
2011-04-28 08:11 PDT, Sergey Vorobyev
no flags Details | Formatted Diff | Diff
Fix style (18.63 KB, patch)
2011-04-28 10:37 PDT, Sergey Vorobyev
no flags Details | Formatted Diff | Diff
Fix merge conflicts in gtk (17.09 KB, patch)
2011-04-29 01:19 PDT, Sergey Vorobyev
yurys: review-
Details | Formatted Diff | Diff
fix error after review (18.79 KB, patch)
2011-04-29 06:53 PDT, Sergey Vorobyev
yurys: review-
Details | Formatted Diff | Diff
Add 3 tests (25.40 KB, patch)
2011-05-11 05:25 PDT, Sergey Vorobyev
yurys: review-
Details | Formatted Diff | Diff
Another one (26.19 KB, patch)
2011-05-12 11:19 PDT, Sergey Vorobyev
yurys: review-
Details | Formatted Diff | Diff
Split network-specific InspectorTest methods to network-test.js (26.87 KB, patch)
2011-05-13 02:49 PDT, Sergey Vorobyev
no flags Details | Formatted Diff | Diff
Minor fix (26.93 KB, patch)
2011-05-13 05:09 PDT, Sergey Vorobyev
yurys: review-
Details | Formatted Diff | Diff
Remove bool isBackgroundCollectionInit (26.81 KB, patch)
2011-05-13 06:01 PDT, Sergey Vorobyev
no flags Details | Formatted Diff | Diff
Fix wrong bug links (26.81 KB, patch)
2011-05-13 06:38 PDT, Sergey Vorobyev
no flags Details | Formatted Diff | Diff
suppressed 3 new tests on qt platform (27.73 KB, patch)
2011-05-16 07:17 PDT, Sergey Vorobyev
no flags Details | Formatted Diff | Diff
Minor fix after pfeldman review (28.00 KB, patch)
2011-05-16 09:42 PDT, Sergey Vorobyev
yurys: review+
Details | Formatted Diff | Diff
Fix style (28.13 KB, patch)
2011-05-19 00:03 PDT, Sergey Vorobyev
yurys: review+
yurys: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergey Vorobyev 2011-04-15 05:50:18 PDT
Related bugs:
https://bugs.webkit.org/show_bug.cgi?id=58064

Feature will be disable by default ('cause it's expensive)
So we need mechanism for customer to enable it from Inspector frontend
Comment 1 Sergey Vorobyev 2011-04-28 08:11:25 PDT
Created attachment 91492 [details]
Gui added. 1 simple test

Just a draft.
We need new icon for new button
Comment 2 WebKit Review Bot 2011-04-28 08:13:30 PDT
Attachment 91492 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/inspector/InspectorResourceAgent.cpp:349:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/inspector/InspectorResourceAgent.cpp:408:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Sergey Vorobyev 2011-04-28 10:37:28 PDT
Created attachment 91513 [details]
Fix style
Comment 4 Sergey Vorobyev 2011-04-29 01:19:57 PDT
Created attachment 91650 [details]
Fix merge conflicts in gtk
Comment 5 Yury Semikhatsky 2011-04-29 02:15:27 PDT
Comment on attachment 91650 [details]
Fix merge conflicts in gtk

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.
Comment 6 Sergey Vorobyev 2011-04-29 06:51:52 PDT
Comment on attachment 91650 [details]
Fix merge conflicts in gtk

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.

Done.

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

Done. Add image.onload

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

Done, I think (same as previous)

>> Source/WebCore/inspector/Inspector.json:451
>> +                "name": "switchBackgroundEventsCollectionTo",
> 
> Should be setBackgroundEventCollectionEnabled(true/false).

Done.

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

Done.

>> Source/WebCore/inspector/InspectorResourceAgent.cpp:94
>> +        disable(0);
> 
> The contract is that ErrorString* is non-0.

ok, done

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

done.

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

done

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

done

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

yes, thanks

>> Source/WebCore/inspector/front-end/NetworkPanel.js:603
>> +        var self = this;
> 
> Use eventCollectionEnabled.bind(this) instead(like we do in other places).

done

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

done

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

Ok, done.
Comment 7 Sergey Vorobyev 2011-04-29 06:53:13 PDT
Created attachment 91674 [details]
fix error after review
Comment 8 Yury Semikhatsky 2011-05-06 03:12:09 PDT
Comment on attachment 91674 [details]
fix error after review

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.
Comment 9 Sergey Vorobyev 2011-05-11 05:05:38 PDT
Comment on attachment 91674 [details]
fix error after review

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
Comment 10 Sergey Vorobyev 2011-05-11 05:25:22 PDT
Created attachment 93110 [details]
Add 3 tests
Comment 11 Yury Semikhatsky 2011-05-12 08:43:48 PDT
Comment on attachment 93110 [details]
Add 3 tests

View in context: https://bugs.webkit.org/attachment.cgi?id=93110&action=review

> LayoutTests/http/tests/inspector/inspector-test.js:343
> +    setTimeout(function() {

Why do we need this setTimeout?

> LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html:8
> +    var size = 400 + 200 * loadedResourceCount;

What's the purpose of this expression? It looks like you can use some arbitrary numbers to get two different URLs.

> LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html:34
> +            WebInspector.panels.network._toggleBackgroundEventsCollection();

Which state is it supposed to go in? Is it possible that the background collection is already on? Would be better to call something like enableBackgroundEventCollection that would make sure that the new state is on, no matter what the previous state was.

> LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html:61
> +Test that network panel is empty after reopening. Even if before reopening resources are loading in background collection mode.

It's hard to understand what the test does from the description, could you change the wording?

> LayoutTests/http/tests/inspector/network/network-close-load-open.html:8
> +    size = 400;

This can be inlined.

> LayoutTests/http/tests/inspector/network/network-close-load-open.html:44
> +            WebInspector.panels.network._toggleBackgroundEventsCollection();

There should be a more reliable way of doing this so that even if the test fails background collection would be turned off.

> LayoutTests/http/tests/inspector/network/network-open-load-reopen.html:55
> +Test that when background event collection is enabled network panel correctly restores network log after the frontend reopening.

What's the difference between this test and network-close-load-open.html ?

> Source/WebCore/ChangeLog:9
> +        "Backgroune events collection". It's allows save all network events when inspector

Backgroune -> Background. It's allows save -> It allows to save

> Source/WebCore/ChangeLog:10
> +        frontend closed. Events occasion before collection enabling are not preserved after

Events that occur before...
Comment 12 Sergey Vorobyev 2011-05-12 11:08:29 PDT
Comment on attachment 93110 [details]
Add 3 tests

View in context: https://bugs.webkit.org/attachment.cgi?id=93110&action=review

>> LayoutTests/http/tests/inspector/inspector-test.js:343
>> +    setTimeout(function() {
> 
> Why do we need this setTimeout?

Probably no reasons. I remove it.

>> LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html:8
>> +    var size = 400 + 200 * loadedResourceCount;
> 
> What's the purpose of this expression? It looks like you can use some arbitrary numbers to get two different URLs.

Exactly. It's look confusing, yeah. I fix it.

>> LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html:34
>> +            WebInspector.panels.network._toggleBackgroundEventsCollection();
> 
> Which state is it supposed to go in? Is it possible that the background collection is already on? Would be better to call something like enableBackgroundEventCollection that would make sure that the new state is on, no matter what the previous state was.

done

>> LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html:61
>> +Test that network panel is empty after reopening. Even if before reopening resources are loading in background collection mode.
> 
> It's hard to understand what the test does from the description, could you change the wording?

Test that reopening clears network panel in background events collection disabled mode.

>> LayoutTests/http/tests/inspector/network/network-close-load-open.html:8
>> +    size = 400;
> 
> This can be inlined.

done

>> LayoutTests/http/tests/inspector/network/network-close-load-open.html:44
>> +            WebInspector.panels.network._toggleBackgroundEventsCollection();
> 
> There should be a more reliable way of doing this so that even if the test fails background collection would be turned off.

done. But not pure safe. We need add InspectorTest.disableBackgroundEventCollection call to InspectorTest.completeTest for really safety.

>> LayoutTests/http/tests/inspector/network/network-open-load-reopen.html:55
>> +Test that when background event collection is enabled network panel correctly restores network log after the frontend reopening.
> 
> What's the difference between this test and network-close-load-open.html ?

Different in actions sequences.
open-load-reopen test that collection work with opened frontend
close-load-open test that collection work with closed frontend

>> Source/WebCore/ChangeLog:9
>> +        "Backgroune events collection". It's allows save all network events when inspector
> 
> Backgroune -> Background. It's allows save -> It allows to save

done.

>> Source/WebCore/ChangeLog:10
>> +        frontend closed. Events occasion before collection enabling are not preserved after
> 
> Events that occur before...

done.
Comment 13 Sergey Vorobyev 2011-05-12 11:19:37 PDT
Created attachment 93311 [details]
Another one

I add try-catch statements in my tests.
They duplicate InspectorTest.disableBackengCollectionEvents() call, but I didn't think up how avoid duplicate and use finally correct in this cases.
Comment 14 Yury Semikhatsky 2011-05-13 01:46:24 PDT
Comment on attachment 93311 [details]
Another one

View in context: https://bugs.webkit.org/attachment.cgi?id=93311&action=review

> LayoutTests/http/tests/inspector/inspector-test.js:323
> +InspectorTest.enableBackgroundEventCollection = function()

These three methods are specific to network inspection. They should go to network-test.js

> LayoutTests/http/tests/inspector/inspector-test.js:330
> +        WebInspector.panels.network._toggleBackgroundEventsCollection();

It's overcomplicated. We don't need to reload page in enableBackgroundEventCollection. If you would like to clear resources you can send disable command followed by enable but I'd rather make the test not depend on the previous state.

> LayoutTests/http/tests/inspector/inspector-test.js:342
> +InspectorTest.addToResultNetworkResources = function()

dumpNetworkResources

> LayoutTests/http/tests/inspector/inspector-test.js:345
> +    resources.sort(function(a, b) {return a.url.localeCompare(b.url);});

Wouldn't resources.sort() give the same result?

> LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html:6
> +function loadImages(size)

loadImagesAndReopenFrontend

> LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html:19
> +function loadImages400()

Inline this function?

> LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html:24
> +function loadImages600()

ditto
Comment 15 Sergey Vorobyev 2011-05-13 02:48:03 PDT
Comment on attachment 93311 [details]
Another one

View in context: https://bugs.webkit.org/attachment.cgi?id=93311&action=review

>> LayoutTests/http/tests/inspector/inspector-test.js:323
>> +InspectorTest.enableBackgroundEventCollection = function()
> 
> These three methods are specific to network inspection. They should go to network-test.js

done

>> LayoutTests/http/tests/inspector/inspector-test.js:330
>> +        WebInspector.panels.network._toggleBackgroundEventsCollection();
> 
> It's overcomplicated. We don't need to reload page in enableBackgroundEventCollection. If you would like to clear resources you can send disable command followed by enable but I'd rather make the test not depend on the previous state.

done.

>> LayoutTests/http/tests/inspector/inspector-test.js:342
>> +InspectorTest.addToResultNetworkResources = function()
> 
> dumpNetworkResources

done

>> LayoutTests/http/tests/inspector/inspector-test.js:345

> 
> Wouldn't resources.sort() give the same result?

No, it wouldn't

>> LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html:6
>> +function loadImages(size)
> 
> loadImagesAndReopenFrontend

done. Same done in other tests

>> LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html:19
>> +function loadImages400()
> 
> Inline this function?

done

>> LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html:24
>> +function loadImages600()
> 
> ditto

done
Comment 16 Sergey Vorobyev 2011-05-13 02:49:09 PDT
Created attachment 93419 [details]
Split network-specific InspectorTest methods to network-test.js
Comment 17 Yury Semikhatsky 2011-05-13 04:13:59 PDT
Comment on attachment 93419 [details]
Split network-specific InspectorTest methods to network-test.js

View in context: https://bugs.webkit.org/attachment.cgi?id=93419&action=review

Overall looks good except a couple of minor suggestions.

> LayoutTests/http/tests/inspector/network-test.js:3
> +InspectorTest.enableBackgroundEventCollection = function()

This method is not used anymore, please remove it.

> LayoutTests/http/tests/inspector/network-test.js:9
> +InspectorTest.enableBackgroundEventCollectionInsure = function()

I think it can be just enableBackgroundEventCollection, or ensureBackgroundEventCollectionEnabled if you prefer the latter form.

> LayoutTests/http/tests/inspector/network/network-open-load-reopen.html:56
> +Test that when background event collection is enabled network panel correctly restores network log after the frontend reopening.

It's a good practice to put a link to the bug into the test description.
Comment 18 Sergey Vorobyev 2011-05-13 05:09:19 PDT
Comment on attachment 93419 [details]
Split network-specific InspectorTest methods to network-test.js

View in context: https://bugs.webkit.org/attachment.cgi?id=93419&action=review

>> LayoutTests/http/tests/inspector/network-test.js:3
>> +InspectorTest.enableBackgroundEventCollection = function()
> 
> This method is not used anymore, please remove it.

done.

>> LayoutTests/http/tests/inspector/network-test.js:9
>> +InspectorTest.enableBackgroundEventCollectionInsure = function()
> 
> I think it can be just enableBackgroundEventCollection, or ensureBackgroundEventCollectionEnabled if you prefer the latter form.

done.

>> LayoutTests/http/tests/inspector/network/network-open-load-reopen.html:56
>> +Test that when background event collection is enabled network panel correctly restores network log after the frontend reopening.
> 
> It's a good practice to put a link to the bug into the test description.

done.
Comment 19 Sergey Vorobyev 2011-05-13 05:09:50 PDT
Created attachment 93436 [details]
Minor fix
Comment 20 Yury Semikhatsky 2011-05-13 05:27:00 PDT
Comment on attachment 93436 [details]
Minor fix

View in context: https://bugs.webkit.org/attachment.cgi?id=93436&action=review

> Source/WebCore/inspector/InspectorResourceAgent.cpp:352
> +        if (!isBackgroundCollectionInit)

This may well be replaced with if (!m_eventsCollector), no need to introduce additional field.

> Source/WebCore/inspector/InspectorResourceAgent.cpp:397
> +void InspectorResourceAgent::backgroundCollectionInit()

initializeBackgroundCollection
Comment 21 Sergey Vorobyev 2011-05-13 06:00:43 PDT
Comment on attachment 93436 [details]
Minor fix

View in context: https://bugs.webkit.org/attachment.cgi?id=93436&action=review

>> Source/WebCore/inspector/InspectorResourceAgent.cpp:352
>> +        if (!isBackgroundCollectionInit)
> 
> This may well be replaced with if (!m_eventsCollector), no need to introduce additional field.

Yeah, I'm dumb. done

>> Source/WebCore/inspector/InspectorResourceAgent.cpp:397
>> +void InspectorResourceAgent::backgroundCollectionInit()
> 
> initializeBackgroundCollection

done
Comment 22 Sergey Vorobyev 2011-05-13 06:01:40 PDT
Created attachment 93438 [details]
Remove bool isBackgroundCollectionInit
Comment 23 Yury Semikhatsky 2011-05-13 06:27:18 PDT
Comment on attachment 93438 [details]
Remove bool isBackgroundCollectionInit

View in context: https://bugs.webkit.org/attachment.cgi?id=93438&action=review

> LayoutTests/http/tests/inspector/network/network-close-load-open.html:59
> +<a href="https://bugs.webkit.org/show_bug.cgi?id=58652">Bug 58652</a>

Both hrefs are the same.

> LayoutTests/http/tests/inspector/network/network-open-load-reopen.html:58
> +<a href="https://bugs.webkit.org/show_bug.cgi?id=58652">Bug 58652</a>

Same here.
Comment 24 Sergey Vorobyev 2011-05-13 06:34:50 PDT
Comment on attachment 93438 [details]
Remove bool isBackgroundCollectionInit

View in context: https://bugs.webkit.org/attachment.cgi?id=93438&action=review

>> LayoutTests/http/tests/inspector/network/network-close-load-open.html:59
>> +<a href="https://bugs.webkit.org/show_bug.cgi?id=58652">Bug 58652</a>
> 
> Both hrefs are the same.

Sorry. done

>> LayoutTests/http/tests/inspector/network/network-open-load-reopen.html:58
>> +<a href="https://bugs.webkit.org/show_bug.cgi?id=58652">Bug 58652</a>
> 
> Same here.

done.
Comment 25 Sergey Vorobyev 2011-05-13 06:38:08 PDT
Created attachment 93441 [details]
Fix wrong bug links
Comment 26 Yury Semikhatsky 2011-05-13 07:05:06 PDT
Comment on attachment 93441 [details]
Fix wrong bug links

Looks good but LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html times out when I run it locally.
Comment 27 Yury Semikhatsky 2011-05-13 07:05:43 PDT
(In reply to comment #26)
> (From update of attachment 93441 [details])
> Looks good but LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html times out when I run it locally.

It times out on Qt in release mode.
Comment 28 Sergey Vorobyev 2011-05-16 07:17:47 PDT
Created attachment 93638 [details]
suppressed 3 new tests on qt platform
Comment 29 Pavel Feldman 2011-05-16 08:07:21 PDT
Comment on attachment 93638 [details]
suppressed 3 new tests on qt platform

View in context: https://bugs.webkit.org/attachment.cgi?id=93638&action=review

> LayoutTests/http/tests/inspector/network-test.js:5
> +    if (!WebInspector.panels.network._backgroundCollectionEnabled) {

No need { }

> LayoutTests/http/tests/inspector/network-test.js:21
> +    //resources.sort(function(a, b) {return a.url.localeCompare(b.url);});

Please do not land commented code.

> LayoutTests/http/tests/inspector/network-test.js:24
> +    for (i = 0; i < resources.length; i++) {

No need for { }

> LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html:32
> +        InspectorTest.evaluateInPage("frontendReopeningCount", function(result) {

Please do not use anonymous functions.

> LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html:33
> +            if (result._description === "0") {

What is _description ?

> LayoutTests/http/tests/inspector/network/network-close-load-open.html:24
> +    if (loadedResourceCount === 2) {

No need for {}

> LayoutTests/http/tests/inspector/network/network-open-load-reopen.html:23
> +    if (++loadedResourceCount === 2) {

{}

> Source/WebCore/inspector/Inspector.json:455
> +                "description": "Allow turn on/off background network events collection.",

Toggles background network event collection.

> Source/WebCore/inspector/Inspector.json:461
> +                "name": "getBackgroundEventsCollectionState",

isBackgroundEventsCollectionEnabled
Comment 30 Sergey Vorobyev 2011-05-16 09:36:59 PDT
Comment on attachment 93638 [details]
suppressed 3 new tests on qt platform

View in context: https://bugs.webkit.org/attachment.cgi?id=93638&action=review

>> LayoutTests/http/tests/inspector/network-test.js:5
>> +    if (!WebInspector.panels.network._backgroundCollectionEnabled) {
> 
> No need { }

done

>> LayoutTests/http/tests/inspector/network-test.js:21
>> +    //resources.sort(function(a, b) {return a.url.localeCompare(b.url);});
> 
> Please do not land commented code.

Oh, thanks for this point. In fact commented code is coorect and resources.sort() was just experement

>> LayoutTests/http/tests/inspector/network-test.js:24
>> +    for (i = 0; i < resources.length; i++) {
> 
> No need for { }

done.

>> LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html:32
>> +        InspectorTest.evaluateInPage("frontendReopeningCount", function(result) {
> 
> Please do not use anonymous functions.

ok, done. But I just take this pattern from inspector/debugger/open-close-open.html test.

>> LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html:33
>> +            if (result._description === "0") {
> 
> What is _description ?

Again, I take it from inspector/debugger/open-close-open.htm
I tried make same thing another way, but didn't invent how...
Actually I don't know, how I can get evaluating value, except _description

>> LayoutTests/http/tests/inspector/network/network-close-load-open.html:24
>> +    if (loadedResourceCount === 2) {
> 
> No need for {}

done. But is code style strict for LayoutTests? I mean same { } in http/tests/inspector/network/network-size.html
And Tools/Scripts/check-webkit-style doesn't produce warning on this code...

>> LayoutTests/http/tests/inspector/network/network-open-load-reopen.html:23
>> +    if (++loadedResourceCount === 2) {
> 
> {}

done

>> Source/WebCore/inspector/Inspector.json:455
>> +                "description": "Allow turn on/off background network events collection.",
> 
> Toggles background network event collection.

done.

>> Source/WebCore/inspector/Inspector.json:461
>> +                "name": "getBackgroundEventsCollectionState",
> 
> isBackgroundEventsCollectionEnabled

done.
Comment 31 Sergey Vorobyev 2011-05-16 09:42:17 PDT
Created attachment 93653 [details]
Minor fix after pfeldman review
Comment 32 Yury Semikhatsky 2011-05-18 06:37:15 PDT
Comment on attachment 93653 [details]
Minor fix after pfeldman review

View in context: https://bugs.webkit.org/attachment.cgi?id=93653&action=review

> LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html:37
> +    function step123(stepNumber) {

name 'step123' looks weird

> LayoutTests/http/tests/inspector/network/network-clear-after-disabled.html:42
> +        if (stepNumber._description === "1") {

'if' should go on the same line as 'else'.
Comment 33 Sergey Vorobyev 2011-05-19 00:03:42 PDT
Created attachment 94047 [details]
Fix style
Comment 34 Yury Semikhatsky 2011-05-20 09:15:47 PDT
Committed r86959: <http://trac.webkit.org/changeset/86959>