Bug 58652 – Web Inspector: Background network events collection - add GUI to Inspector

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Sergey Vorobyev]

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

[Sergey Vorobyev]

Created attachment 91492 [details]
Gui added. 1 simple test

Just a draft.
We need new icon for new button

[WebKit Review Bot]

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.

[Sergey Vorobyev]

Created attachment 91513 [details]
Fix style

[Sergey Vorobyev]

Created attachment 91650 [details]
Fix merge conflicts in gtk

[Yury Semikhatsky]

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.

[Sergey Vorobyev]

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.

[Sergey Vorobyev]

Created attachment 91674 [details]
fix error after review

[Yury Semikhatsky]

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.

[Sergey Vorobyev]

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

[Sergey Vorobyev]

Created attachment 93110 [details]
Add 3 tests

[Yury Semikhatsky]

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

[Sergey Vorobyev]

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.

[Sergey Vorobyev]

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.

[Yury Semikhatsky]

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

[Sergey Vorobyev]

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

[Sergey Vorobyev]

Created attachment 93419 [details]
Split network-specific InspectorTest methods to network-test.js

[Yury Semikhatsky]

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.

[Sergey Vorobyev]

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.

[Sergey Vorobyev]

Created attachment 93436 [details]
Minor fix

[Yury Semikhatsky]

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

[Sergey Vorobyev]

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

[Sergey Vorobyev]

Created attachment 93438 [details]
Remove bool isBackgroundCollectionInit

[Yury Semikhatsky]

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.

[Sergey Vorobyev]

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.

[Sergey Vorobyev]

Created attachment 93441 [details]
Fix wrong bug links

[Yury Semikhatsky]

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.

[Yury Semikhatsky]

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

[Sergey Vorobyev]

Created attachment 93638 [details]
suppressed 3 new tests on qt platform

[Pavel Feldman]

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

[Sergey Vorobyev]

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.

[Sergey Vorobyev]

Created attachment 93653 [details]
Minor fix after pfeldman review

[Yury Semikhatsky]

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

[Sergey Vorobyev]

Created attachment 94047 [details]
Fix style

[Yury Semikhatsky]

Committed r86959: <http://trac.webkit.org/changeset/86959>