Bug 58064 – Web Inspector: Network events don't preserves, when inspector frontend closed and open again

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

[Sergey Vorobyev]

Monitoring of network activity start when inspector frontend created.
So It's empty in initial state.
Same problem, when user close frontend and open it again:
Network events are not saves.
So we need way to collect network events in background.

[Sergey Vorobyev]

Created attachment 88782 [details]
First version of background event collector patch

Need add condition to enable this functionality

[Sergey Vorobyev]

Created attachment 88786 [details]
Fix merge conflicts

[WebKit Review Bot]

Attachment 88786 [details] did not pass style-queue:

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

Source/WebCore/inspector/InspectorFrontendProxy.cpp:68:  Else clause should never be on same line as else (use 2 lines)  [whitespace/newline] [4]
Source/WebCore/inspector/InspectorResourceAgent.cpp:49:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/inspector/InspectorResourceAgent.cpp:54:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/inspector/EventsCollector.cpp:27:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/inspector/EventsCollector.cpp:31:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/inspector/EventsCollector.cpp:52:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/inspector/EventsCollector.cpp:55:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/inspector/InspectorFrontendProxy.h:40:  The parameter name "collector" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorFrontendProxy.h:41:  The parameter name "collector" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorFrontendProxy.h:41:  The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 10 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Attachment 88786 [details] did not build on win:
Build output: http://queues.webkit.org/results/8346958

[Sergey Vorobyev]

Created attachment 88789 [details]
Patch

[Sergey Vorobyev]

Created attachment 88792 [details]
Patch

[Build Bot]

Attachment 88792 [details] did not build on win:
Build output: http://queues.webkit.org/results/8341995

[Build Bot]

Attachment 88789 [details] did not build on win:
Build output: http://queues.webkit.org/results/8341998

[Yury Semikhatsky]

Comment on attachment 88792 [details]
Patch

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

> Source/WebCore/inspector/EventsCollector.cpp:39
> +    events = *(new Vector<String>);

Just nuke this line.

[Yury Semikhatsky]

Comment on attachment 88792 [details]
Patch

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

> Source/WebCore/inspector/Inspector.json:374
> +                "name": "domContentEventFired",

Let's move introduction of these two messages into a separate change. It would make the patch smaller.

[Sergey Vorobyev]

Created attachment 88822 [details]
Patch

[Sergey Vorobyev]

Created attachment 88994 [details]
Patch file with doc-encode to prompt vc-project pass svn-apply

[Sergey Vorobyev]

Created attachment 88997 [details]
Patch file with fuzz=2 for vc project

[Early Warning System Bot]

Attachment 88997 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8374946

[Sergey Vorobyev]

Created attachment 89009 [details]
Add new files in WebCore/WebCore.pro file

[Yury Semikhatsky]

Comment on attachment 89009 [details]
Add new files in WebCore/WebCore.pro file

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

> Source/WebCore/ChangeLog:9
> +        No new tests. (OOPS!)

Would be really nice to have a test for this change.

> Source/WebCore/inspector/EventsCollector.cpp:38
> +{ }

style: closing bracket should go on a new line.

> Source/WebCore/inspector/EventsCollector.cpp:42
> +    totalLength += (int)message.length();

we don't use c-style casts in WebCore code. You could use size_t as m_totalLength type.

> Source/WebCore/inspector/EventsCollector.h:42
> +    void collect(const String& message);

I'd rename it to addEvent

> Source/WebCore/inspector/EventsCollector.h:43
> +    void pushData(InspectorFrontendChannel*);

Consider renaming pushData to sendCollectedEvents?

> Source/WebCore/inspector/EventsCollector.h:45
> +    static const int CAPACITY = 1048576; // 1 Mb

WebCore constant names should be in camelCase, like maxCapacity. Also please use 1024*1024 instead of the number, compiler will do this evaluation for you.

> Source/WebCore/inspector/EventsCollector.h:46
> +    int totalLength;

style: private fields should start with m_ prefix(http://www.webkit.org/coding/coding-style.html)

> Source/WebCore/inspector/InspectorFrontendProxy.h:40
> +    InspectorFrontendProxy(EventsCollector*);

Constructors with single argument should be marked as "explicit".

> Source/WebCore/inspector/InspectorResourceAgent.cpp:549
> +    m_frontend = new InspectorFrontend::Network(m_inspectorFrontendProxy.get());

You're leaking the Network object here. Instead it should be owned by the agent and created only when background resource tracking is enabled.

[Sergey Vorobyev]

Comment on attachment 89009 [details]
Add new files in WebCore/WebCore.pro file

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

>> Source/WebCore/ChangeLog:9
>> +        No new tests. (OOPS!)
> 
> Would be really nice to have a test for this change.

First commit would be without test and it will contain disable functionality
Next commit would contain tests and machinery for enable functionality

>> Source/WebCore/inspector/EventsCollector.cpp:38
>> +{ }
> 
> style: closing bracket should go on a new line.

done

>> Source/WebCore/inspector/EventsCollector.cpp:42
>> +    totalLength += (int)message.length();
> 
> we don't use c-style casts in WebCore code. You could use size_t as m_totalLength type.

done

>> Source/WebCore/inspector/EventsCollector.h:42
>> +    void collect(const String& message);
> 
> I'd rename it to addEvent

done

>> Source/WebCore/inspector/EventsCollector.h:43
>> +    void pushData(InspectorFrontendChannel*);
> 
> Consider renaming pushData to sendCollectedEvents?

It's much good, done

>> Source/WebCore/inspector/EventsCollector.h:45
>> +    static const int CAPACITY = 1048576; // 1 Mb
> 
> WebCore constant names should be in camelCase, like maxCapacity. Also please use 1024*1024 instead of the number, compiler will do this evaluation for you.

done

>> Source/WebCore/inspector/EventsCollector.h:46
>> +    int totalLength;
> 
> style: private fields should start with m_ prefix(http://www.webkit.org/coding/coding-style.html)

done.

>> Source/WebCore/inspector/InspectorFrontendProxy.h:40
>> +    InspectorFrontendProxy(EventsCollector*);
> 
> Constructors with single argument should be marked as "explicit".

done

>> Source/WebCore/inspector/InspectorResourceAgent.cpp:549
>> +    m_frontend = new InspectorFrontend::Network(m_inspectorFrontendProxy.get());
> 
> You're leaking the Network object here. Instead it should be owned by the agent and created only when background resource tracking is enabled.

done

[Sergey Vorobyev]

Created attachment 89579 [details]
After firsts review. Functionality disabled by default

[Yury Semikhatsky]

Comment on attachment 89579 [details]
After firsts review. Functionality disabled by default

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

> Source/WebCore/inspector/InspectorResourceAgent.cpp:94
> +    if (enabledBackgoundEventsCoollection()) {

enabledBackgoundEventsCoollection -> backgoundEventsCoollectionEnabled

> Source/WebCore/inspector/InspectorResourceAgent.cpp:-281
> -    ASSERT(!m_instrumentingAgents->inspectorResourceAgent());

This assert should be preserved if it fails you should figure out when to clear pointer to this agent from the instrumenting agents structure.

> Source/WebCore/inspector/InspectorResourceAgent.cpp:504
> +    // TODO(vors):

Please file a bug on this and add FIXME(bug number) instead of the TODO.

> Source/WebCore/inspector/InspectorResourceAgent.h:134
> +    OwnPtr<InspectorFrontend::Network> m_frontend;

This is wrong since the frontend passed in setFrontend method is owned by the caller, r- for this.

[Sergey Vorobyev]

Created attachment 89774 [details]
After 2nd review. Fix memory leaks

[Sergey Vorobyev]

Comment on attachment 89579 [details]
After firsts review. Functionality disabled by default

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

>> Source/WebCore/inspector/InspectorResourceAgent.cpp:94
>> +    if (enabledBackgoundEventsCoollection()) {
> 
> enabledBackgoundEventsCoollection -> backgoundEventsCoollectionEnabled

done

>> Source/WebCore/inspector/InspectorResourceAgent.cpp:-281
>> -    ASSERT(!m_instrumentingAgents->inspectorResourceAgent());
> 
> This assert should be preserved if it fails you should figure out when to clear pointer to this agent from the instrumenting agents structure.

done

>> Source/WebCore/inspector/InspectorResourceAgent.cpp:504
>> +    // TODO(vors):
> 
> Please file a bug on this and add FIXME(bug number) instead of the TODO.

done.

>> Source/WebCore/inspector/InspectorResourceAgent.h:134
>> +    OwnPtr<InspectorFrontend::Network> m_frontend;
> 
> This is wrong since the frontend passed in setFrontend method is owned by the caller, r- for this.

Done, add m_mockFrontend

[Yury Semikhatsky]

Comment on attachment 89774 [details]
After 2nd review. Fix memory leaks

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

> Source/WebCore/ChangeLog:8
> +

Please provide a brief description of the changes made.

> Source/WebCore/inspector/InspectorResourceAgent.cpp:-493
> -    if (!m_frontend)

Please revert this.

[Alexander Pavlov (apavlov)]

Comment on attachment 89774 [details]
After 2nd review. Fix memory leaks

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

> Source/WebCore/ChangeLog:9
> +        No new tests. (OOPS!)

You will not be able to commit a patch with this line.

> Source/WebCore/inspector/InspectorResourceAgent.cpp:94
> +    if (backgoundEventsCoollectionEnabled()) {

The method name has two mistypes, please fix.

> Source/WebCore/inspector/InspectorResourceAgent.cpp:-319
> -

I believe this line was removed inadvertently.

> Source/WebCore/inspector/InspectorResourceAgent.cpp:513
> +    // Now this function disable.

Now this function is disabled.

[Yury Semikhatsky]

Comment on attachment 89774 [details]
After 2nd review. Fix memory leaks

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

> Source/WebCore/inspector/InspectorResourceAgent.cpp:94
> +    if (backgoundEventsCoollectionEnabled()) {

I think that when event collection is enabled you could simply set inspectorFrotnedChannel to the proxy object and clear it in clearFrontend. In fact you can completely ignore passed frontend object.

If you proceed with the current approach, you should reset the agent and frontend states as they were before setFrontend including setting m_frontend to m_mockFrontend.get

[Sergey Vorobyev]

Created attachment 89778 [details]
After 3th review. Revert one remove change

[Sergey Vorobyev]

Comment on attachment 89774 [details]
After 2nd review. Fix memory leaks

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

>> Source/WebCore/ChangeLog:8
>> +
> 
> Please provide a brief description of the changes made.

done.

>> Source/WebCore/ChangeLog:9
>> +        No new tests. (OOPS!)
> 
> You will not be able to commit a patch with this line.

done.

>>> Source/WebCore/inspector/InspectorResourceAgent.cpp:94
>>> +    if (backgoundEventsCoollectionEnabled()) {
>> 
>> The method name has two mistypes, please fix.
> 
> I think that when event collection is enabled you could simply set inspectorFrotnedChannel to the proxy object and clear it in clearFrontend. In fact you can completely ignore passed frontend object.
> 
> If you proceed with the current approach, you should reset the agent and frontend states as they were before setFrontend including setting m_frontend to m_mockFrontend.get

@apavlov done, thank you

[Sergey Vorobyev]

Comment on attachment 89774 [details]
After 2nd review. Fix memory leaks

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

>> Source/WebCore/inspector/InspectorResourceAgent.cpp:-319
>> -
> 
> I believe this line was removed inadvertently.

done

>> Source/WebCore/inspector/InspectorResourceAgent.cpp:513
>> +    // Now this function disable.
> 
> Now this function is disabled.

done

[Sergey Vorobyev]

Created attachment 89783 [details]
Minor fixes

[WebKit Review Bot]

Attachment 89783 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebCore/inspector/InspectorResourceAgent.cpp:107:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sergey Vorobyev]

Created attachment 89787 [details]
Fix style

[Yury Semikhatsky]

Comment on attachment 89787 [details]
Fix style

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

> Source/WebCore/inspector/InspectorResourceAgent.cpp:106
> +        m_frontend = m_mockFrontend.get();

You should clear pointer from the proxy to the channel, otherwise m_inspectorFrontendProxy will continue sending events into the channel. r- for this.

[Sergey Vorobyev]

Created attachment 89790 [details]
Fix clearFrontend

[Yury Semikhatsky]

Comment on attachment 89790 [details]
Fix clearFrontend

Clearing flags on attachment: 89790

Committed r83975: <http://trac.webkit.org/changeset/83975>

[Yury Semikhatsky]

All reviewed patches have been landed.  Closing bug.