Bug 108653 - Web Inspector: Split Profiler domain in protocol into Profiler and HeapProfiler
Summary: Web Inspector: Split Profiler domain in protocol into Profiler and HeapProfiler
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: Alexei Filippov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-01 09:54 PST by Alexei Filippov
Modified: 2013-02-12 03:22 PST (History)
27 users (show)

See Also:


Attachments
Patch (61.37 KB, patch)
2013-02-01 10:19 PST, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (64.47 KB, patch)
2013-02-04 04:55 PST, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (58.64 KB, patch)
2013-02-04 08:23 PST, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (64.96 KB, patch)
2013-02-06 10:24 PST, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (65.23 KB, patch)
2013-02-06 11:08 PST, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (65.28 KB, patch)
2013-02-07 05:32 PST, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (64.89 KB, patch)
2013-02-08 08:21 PST, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (64.88 KB, patch)
2013-02-11 02:42 PST, Alexei Filippov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexei Filippov 2013-02-01 09:54:00 PST
Currently CPU and heap profilers share the same domain 'Profiler' in the protocol.
In fact these two profile types have not too much in common. So put each into its own domain.
It should also help when Profiles panel gets split into several tools.
Comment 1 Alexei Filippov 2013-02-01 10:19:01 PST
Created attachment 186073 [details]
Patch
Comment 2 WebKit Review Bot 2013-02-01 12:20:04 PST
Comment on attachment 186073 [details]
Patch

Attachment 186073 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16299982

New failing tests:
inspector-protocol/heap-profiler/heap-snapshot-with-event-listener.html
inspector-protocol/heap-profiler/heap-snapshot-with-detached-dom-tree.html
inspector-protocol/nmi-webaudio-leak-test.html
Comment 3 WebKit Review Bot 2013-02-01 13:37:42 PST
Comment on attachment 186073 [details]
Patch

Attachment 186073 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16303952

New failing tests:
inspector-protocol/heap-profiler/heap-snapshot-with-event-listener.html
inspector-protocol/heap-profiler/heap-snapshot-with-detached-dom-tree.html
inspector-protocol/nmi-webaudio-leak-test.html
Comment 4 Alexei Filippov 2013-02-04 04:55:35 PST
Created attachment 186353 [details]
Patch
Comment 5 Build Bot 2013-02-04 07:37:50 PST
Comment on attachment 186353 [details]
Patch

Attachment 186353 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16368242
Comment 6 Yury Semikhatsky 2013-02-04 07:53:34 PST
Comment on attachment 186353 [details]
Patch

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

Can you split the change into smaller pieces?

> Source/WebCore/inspector/InspectorController.h:151
> +    InspectorHeapProfilerAgent* m_heapProfilerAgent;

What is the reason for having this field?

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:285
> +        && m_profiles.begin() == m_profiles.end())

m_profiles.isEmpty() and please put the condition on one line

> Source/WebCore/inspector/InspectorProfilerAgent.h:129
> +class InspectorHeapProfilerAgent : public InspectorBaseAgent<InspectorHeapProfilerAgent>, public InspectorBackendDispatcher::HeapProfilerCommandHandler {

This should go into its own file.
Comment 7 Yury Semikhatsky 2013-02-04 07:54:57 PST
Comment on attachment 186353 [details]
Patch

Note, that this change will break Chromium pyauto tests
Comment 8 Alexei Filippov 2013-02-04 08:23:57 PST
Created attachment 186380 [details]
Patch
Comment 9 Alexei Filippov 2013-02-04 08:27:29 PST
Comment on attachment 186353 [details]
Patch

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

Moved InspectorHeapProfilerAgent functions back to their original locations in InspectorProfilerAgent, so it was easier to see the diffs. i will move them to a separate file with a next patch.

>> Source/WebCore/inspector/InspectorController.h:151
>> +    InspectorHeapProfilerAgent* m_heapProfilerAgent;
> 
> What is the reason for having this field?

No reason so far. Removed.

>> Source/WebCore/inspector/InspectorProfilerAgent.cpp:285
>> +        && m_profiles.begin() == m_profiles.end())
> 
> m_profiles.isEmpty() and please put the condition on one line

done.

>> Source/WebCore/inspector/InspectorProfilerAgent.h:129
>> +class InspectorHeapProfilerAgent : public InspectorBaseAgent<InspectorHeapProfilerAgent>, public InspectorBackendDispatcher::HeapProfilerCommandHandler {
> 
> This should go into its own file.

Yes. Will do this in a separate patch.
Comment 10 Alexei Filippov 2013-02-04 08:28:42 PST
(In reply to comment #7)
> (From update of attachment 186353 [details])
> Note, that this change will break Chromium pyauto tests

I'm readying patch to remote_inspector_client.py
that will be able to work with both versions.
Comment 11 Yury Semikhatsky 2013-02-05 07:36:27 PST
Comment on attachment 186380 [details]
Patch

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

> Source/WebCore/inspector/InspectorController.cpp:156
> +    OwnPtr<InspectorHeapProfilerAgent> heapProfilerAgentPtr(InspectorHeapProfilerAgent::create(m_instrumentingAgents.get(), m_state.get(), m_injectedScriptManager.get()));

Inline this call.

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:571
> +    m_frontend = 0;

You should reset m_headersRequested field here. Or even better get rid of it as it looks like redundant optimization. If so, can you remove it in a separate patch to reduce the size of this change?

> Source/WebCore/inspector/front-end/ProfilesPanel.js:1115
> +        function populateHeapSnapshotsCallback(error, profileHeaders) {

This function is almost identical to populateProfilesCallback, can we reuse common part?
Comment 12 Yury Semikhatsky 2013-02-05 07:38:32 PST
(In reply to comment #10)
> (In reply to comment #7)
> > (From update of attachment 186353 [details] [details])
> > Note, that this change will break Chromium pyauto tests
> 
> I'm readying patch to remote_inspector_client.py
> that will be able to work with both versions.

Can you share a link to the patch? I don't quite understand how you can remove the old command and add the new one to the protocol in one patch without breaking the client for a couple of days.
Comment 13 Alexei Filippov 2013-02-05 23:53:39 PST
(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #7)
> > > (From update of attachment 186353 [details] [details] [details])
> > > Note, that this change will break Chromium pyauto tests
> > 
> > I'm readying patch to remote_inspector_client.py
> > that will be able to work with both versions.
> 
> Can you share a link to the patch? I don't quite understand how you can remove the old command and add the new one to the protocol in one patch without breaking the client for a couple of days.

It will be broken for couple days. I'm doing it the same way Zhenya did that in https://bugs.webkit.org/show_bug.cgi?id=104545
Comment 14 Yury Semikhatsky 2013-02-05 23:57:20 PST
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > (In reply to comment #7)
> > > > (From update of attachment 186353 [details] [details] [details] [details])
> > > > Note, that this change will break Chromium pyauto tests
> > > 
> > > I'm readying patch to remote_inspector_client.py
> > > that will be able to work with both versions.
> > 
> > Can you share a link to the patch? I don't quite understand how you can remove the old command and add the new one to the protocol in one patch without breaking the client for a couple of days.
> 
> It will be broken for couple days. I'm doing it the same way Zhenya did that in https://bugs.webkit.org/show_bug.cgi?id=104545

As I understand Zhenya had a different premise as he could break the clients for a short period until the next version is rolled.
Comment 15 Alexei Filippov 2013-02-06 10:24:31 PST
Created attachment 186874 [details]
Patch
Comment 16 Early Warning System Bot 2013-02-06 10:36:51 PST
Comment on attachment 186874 [details]
Patch

Attachment 186874 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16396288
Comment 17 Early Warning System Bot 2013-02-06 10:38:00 PST
Comment on attachment 186874 [details]
Patch

Attachment 186874 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16388401
Comment 18 Build Bot 2013-02-06 10:56:46 PST
Comment on attachment 186874 [details]
Patch

Attachment 186874 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16390371
Comment 19 Alexei Filippov 2013-02-06 11:08:45 PST
Created attachment 186878 [details]
Patch
Comment 20 Build Bot 2013-02-06 14:33:52 PST
Comment on attachment 186878 [details]
Patch

Attachment 186878 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16394381
Comment 21 Alexei Filippov 2013-02-07 00:53:04 PST
Comment on attachment 186380 [details]
Patch

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

Kept the InspectorProfilerAgent heap snapshotting functionality. Will drop it in the phase 2 patch.

>> Source/WebCore/inspector/InspectorController.cpp:156
>> +    OwnPtr<InspectorHeapProfilerAgent> heapProfilerAgentPtr(InspectorHeapProfilerAgent::create(m_instrumentingAgents.get(), m_state.get(), m_injectedScriptManager.get()));
> 
> Inline this call.

done.

>> Source/WebCore/inspector/InspectorProfilerAgent.cpp:571
>> +    m_frontend = 0;
> 
> You should reset m_headersRequested field here. Or even better get rid of it as it looks like redundant optimization. If so, can you remove it in a separate patch to reduce the size of this change?

reset it for now.

>> Source/WebCore/inspector/front-end/ProfilesPanel.js:1115
>> +        function populateHeapSnapshotsCallback(error, profileHeaders) {
> 
> This function is almost identical to populateProfilesCallback, can we reuse common part?

done
Comment 22 Alexei Filippov 2013-02-07 05:32:29 PST
Created attachment 187071 [details]
Patch
Comment 23 Yury Semikhatsky 2013-02-08 07:41:10 PST
Comment on attachment 187071 [details]
Patch

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

> Source/WebCore/inspector/InspectorHeapProfilerAgent.cpp:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.

2013. Please use Google copyright header for new files(it is slightly different from this one).

> Source/WebCore/inspector/InspectorHeapProfilerAgent.cpp:131
> +public:

Please move classes in the anonymous namespace to the top of the file.

> Source/WebCore/inspector/InspectorHeapProfilerAgent.h:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.

Same comment about copyright header as above.

> Source/WebCore/inspector/InspectorHeapProfilerAgent.h:58
> +    void addProfileFinishedMessageToConsole(PassRefPtr<ScriptProfile>, unsigned lineNumber, const String& sourceURL);

Remove these methods, they don't even have implementation in this class.

> Source/WebCore/inspector/InspectorHeapProfilerAgent.h:85
> +    InspectorHeapProfilerAgent(InstrumentingAgents*, InspectorCompositeState*, InjectedScriptManager*);

Why is it not private?

> Source/WebCore/inspector/InspectorHeapProfilerAgent.h:88
> +    typedef HashMap<unsigned, RefPtr<ScriptHeapSnapshot> > HeapSnapshotsMap;

IsToHeapSnapshotMap
Comment 24 Alexei Filippov 2013-02-08 08:11:04 PST
Comment on attachment 187071 [details]
Patch

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

>> Source/WebCore/inspector/InspectorHeapProfilerAgent.cpp:2
>> + * Copyright (C) 2010 Apple Inc. All rights reserved.
> 
> 2013. Please use Google copyright header for new files(it is slightly different from this one).

done

>> Source/WebCore/inspector/InspectorHeapProfilerAgent.cpp:131
>> +public:
> 
> Please move classes in the anonymous namespace to the top of the file.

Moved them into functions.

>> Source/WebCore/inspector/InspectorHeapProfilerAgent.h:2
>> + * Copyright (C) 2010 Apple Inc. All rights reserved.
> 
> Same comment about copyright header as above.

done

>> Source/WebCore/inspector/InspectorHeapProfilerAgent.h:58
>> +    void addProfileFinishedMessageToConsole(PassRefPtr<ScriptProfile>, unsigned lineNumber, const String& sourceURL);
> 
> Remove these methods, they don't even have implementation in this class.

done

>> Source/WebCore/inspector/InspectorHeapProfilerAgent.h:85
>> +    InspectorHeapProfilerAgent(InstrumentingAgents*, InspectorCompositeState*, InjectedScriptManager*);
> 
> Why is it not private?

made private

>> Source/WebCore/inspector/InspectorHeapProfilerAgent.h:88
>> +    typedef HashMap<unsigned, RefPtr<ScriptHeapSnapshot> > HeapSnapshotsMap;
> 
> IsToHeapSnapshotMap

You meant IdToHeapSnapshotMap?
done.
Comment 25 Alexei Filippov 2013-02-08 08:21:47 PST
Created attachment 187323 [details]
Patch
Comment 26 Yury Semikhatsky 2013-02-11 02:14:49 PST
Comment on attachment 187323 [details]
Patch

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

> Source/WebCore/inspector/InspectorHeapProfilerAgent.h:79
> +    void willProcessTask();

Please remove declaration of these methods, they don't have implementation.
Comment 27 Alexei Filippov 2013-02-11 02:41:13 PST
Comment on attachment 187323 [details]
Patch

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

>> Source/WebCore/inspector/InspectorHeapProfilerAgent.h:79
>> +    void willProcessTask();
> 
> Please remove declaration of these methods, they don't have implementation.

done
Comment 28 Alexei Filippov 2013-02-11 02:42:56 PST
Created attachment 187533 [details]
Patch
Comment 29 WebKit Review Bot 2013-02-11 05:55:40 PST
Comment on attachment 187533 [details]
Patch

Clearing flags on attachment: 187533

Committed r142460: <http://trac.webkit.org/changeset/142460>
Comment 30 WebKit Review Bot 2013-02-11 05:55:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 31 Csaba Osztrogonác 2013-02-11 08:43:17 PST
(In reply to comment #29)
> (From update of attachment 187533 [details])
> Clearing flags on attachment: 187533
> 
> Committed r142460: <http://trac.webkit.org/changeset/142460>

It broke the Windows builds: (Apple Windows and Qt Windows)

6>c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\inspector\InspectorProfilerAgent.cpp(67) : error C2370: 'WebCore::UserInitiatedProfileName' : redefinition; different storage class
6>        c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\inspector\InspectorHeapProfilerAgent.cpp(45) : see declaration of 'WebCore::UserInitiatedProfileName'
6>c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\inspector\InspectorProfilerAgent.cpp(69) : error C2370: 'WebCore::HeapProfileType' : redefinition; different storage class
6>        c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\inspector\InspectorHeapProfilerAgent.cpp(46) : see declaration of 'WebCore::HeapProfileType'
6>c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\inspector\InspectorProfilerAgent.cpp(323) : error C2088: '==' : illegal for class
Comment 32 Csaba Osztrogonác 2013-02-11 16:59:21 PST
Fix landed in http://trac.webkit.org/changeset/142517 ( without any references to this bug :-/ )
Comment 33 Yury Semikhatsky 2013-02-12 03:22:15 PST
(In reply to comment #32)
> Fix landed in http://trac.webkit.org/changeset/142517 ( without any references to this bug :-/ )

Thanks for fixing this!