Bug 58064 - Web Inspector: Network events don't preserves, when inspector frontend closed and open again
Summary: Web Inspector: Network events don't preserves, when inspector frontend closed...
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-07 11:18 PDT by Sergey Vorobyev
Modified: 2011-04-15 08:16 PDT (History)
13 users (show)

See Also:


Attachments
First version of background event collector patch (37.77 KB, patch)
2011-04-08 01:52 PDT, Sergey Vorobyev
no flags Details | Formatted Diff | Diff
Fix merge conflicts (36.04 KB, patch)
2011-04-08 02:29 PDT, Sergey Vorobyev
no flags Details | Formatted Diff | Diff
Patch (37.30 KB, patch)
2011-04-08 03:12 PDT, Sergey Vorobyev
no flags Details | Formatted Diff | Diff
Patch (38.46 KB, patch)
2011-04-08 03:46 PDT, Sergey Vorobyev
no flags Details | Formatted Diff | Diff
Patch (39.53 KB, patch)
2011-04-08 07:57 PDT, Sergey Vorobyev
no flags Details | Formatted Diff | Diff
Patch file with doc-encode to prompt vc-project pass svn-apply (39.24 KB, patch)
2011-04-11 05:46 PDT, Sergey Vorobyev
no flags Details | Formatted Diff | Diff
Patch file with fuzz=2 for vc project (38.38 KB, patch)
2011-04-11 06:13 PDT, Sergey Vorobyev
no flags Details | Formatted Diff | Diff
Add new files in WebCore/WebCore.pro file (39.96 KB, patch)
2011-04-11 08:23 PDT, Sergey Vorobyev
yurys: review-
Details | Formatted Diff | Diff
After firsts review. Functionality disabled by default (35.94 KB, patch)
2011-04-14 06:58 PDT, Sergey Vorobyev
yurys: review-
Details | Formatted Diff | Diff
After 2nd review. Fix memory leaks (35.94 KB, patch)
2011-04-15 06:22 PDT, Sergey Vorobyev
yurys: review-
Details | Formatted Diff | Diff
After 3th review. Revert one remove change (35.95 KB, patch)
2011-04-15 06:45 PDT, Sergey Vorobyev
no flags Details | Formatted Diff | Diff
Minor fixes (35.87 KB, patch)
2011-04-15 07:07 PDT, Sergey Vorobyev
no flags Details | Formatted Diff | Diff
Fix style (35.87 KB, patch)
2011-04-15 07:31 PDT, Sergey Vorobyev
yurys: review-
Details | Formatted Diff | Diff
Fix clearFrontend (41.08 KB, patch)
2011-04-15 08:05 PDT, Sergey Vorobyev
no flags 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-07 11:18:18 PDT
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.
Comment 1 Sergey Vorobyev 2011-04-08 01:52:39 PDT
Created attachment 88782 [details]
First version of background event collector patch

Need add condition to enable this functionality
Comment 2 Sergey Vorobyev 2011-04-08 02:29:14 PDT
Created attachment 88786 [details]
Fix merge conflicts
Comment 3 WebKit Review Bot 2011-04-08 02:32:31 PDT
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.
Comment 4 Build Bot 2011-04-08 02:56:25 PDT
Attachment 88786 [details] did not build on win:
Build output: http://queues.webkit.org/results/8346958
Comment 5 Sergey Vorobyev 2011-04-08 03:12:44 PDT
Created attachment 88789 [details]
Patch
Comment 6 Sergey Vorobyev 2011-04-08 03:46:05 PDT
Created attachment 88792 [details]
Patch
Comment 7 Build Bot 2011-04-08 04:11:17 PDT
Attachment 88792 [details] did not build on win:
Build output: http://queues.webkit.org/results/8341995
Comment 8 Build Bot 2011-04-08 04:13:31 PDT
Attachment 88789 [details] did not build on win:
Build output: http://queues.webkit.org/results/8341998
Comment 9 Yury Semikhatsky 2011-04-08 07:54:26 PDT
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.
Comment 10 Yury Semikhatsky 2011-04-08 07:56:18 PDT
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.
Comment 11 Sergey Vorobyev 2011-04-08 07:57:26 PDT
Created attachment 88822 [details]
Patch
Comment 12 Sergey Vorobyev 2011-04-11 05:46:23 PDT
Created attachment 88994 [details]
Patch file with doc-encode to prompt vc-project pass svn-apply
Comment 13 Sergey Vorobyev 2011-04-11 06:13:31 PDT
Created attachment 88997 [details]
Patch file with fuzz=2 for vc project
Comment 14 Early Warning System Bot 2011-04-11 07:27:56 PDT
Attachment 88997 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8374946
Comment 15 Sergey Vorobyev 2011-04-11 08:23:59 PDT
Created attachment 89009 [details]
Add new files in WebCore/WebCore.pro file
Comment 16 Yury Semikhatsky 2011-04-14 04:44:57 PDT
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.
Comment 17 Sergey Vorobyev 2011-04-14 06:57:19 PDT
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
Comment 18 Sergey Vorobyev 2011-04-14 06:58:40 PDT
Created attachment 89579 [details]
After firsts review. Functionality disabled by default
Comment 19 Yury Semikhatsky 2011-04-15 05:36:59 PDT
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.
Comment 20 Sergey Vorobyev 2011-04-15 06:22:15 PDT
Created attachment 89774 [details]
After 2nd review. Fix memory leaks
Comment 21 Sergey Vorobyev 2011-04-15 06:24:19 PDT
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
Comment 22 Yury Semikhatsky 2011-04-15 06:31:39 PDT
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.
Comment 23 Alexander Pavlov (apavlov) 2011-04-15 06:40:00 PDT
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.
Comment 24 Yury Semikhatsky 2011-04-15 06:43:27 PDT
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
Comment 25 Sergey Vorobyev 2011-04-15 06:45:10 PDT
Created attachment 89778 [details]
After 3th review. Revert one remove change
Comment 26 Sergey Vorobyev 2011-04-15 07:03:06 PDT
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
Comment 27 Sergey Vorobyev 2011-04-15 07:05:46 PDT
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
Comment 28 Sergey Vorobyev 2011-04-15 07:07:10 PDT
Created attachment 89783 [details]
Minor fixes
Comment 29 WebKit Review Bot 2011-04-15 07:09:19 PDT
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.
Comment 30 Sergey Vorobyev 2011-04-15 07:31:05 PDT
Created attachment 89787 [details]
Fix style
Comment 31 Yury Semikhatsky 2011-04-15 07:52:52 PDT
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.
Comment 32 Sergey Vorobyev 2011-04-15 08:05:54 PDT
Created attachment 89790 [details]
Fix clearFrontend
Comment 33 Yury Semikhatsky 2011-04-15 08:16:30 PDT
Comment on attachment 89790 [details]
Fix clearFrontend

Clearing flags on attachment: 89790

Committed r83975: <http://trac.webkit.org/changeset/83975>
Comment 34 Yury Semikhatsky 2011-04-15 08:16:59 PDT
All reviewed patches have been landed.  Closing bug.