Bug 108653

Summary:

Web Inspector: Split Profiler domain in protocol into Profiler and HeapProfiler

Product:

WebKit

Reporter:

Alexei Filippov <alph>

Component:

Web Inspector (Deprecated)

Assignee:

Alexei Filippov <alph>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

apavlov, buildbot, dglazkov, eric.carlson, feature-media-reviews, gyuyoung.kim, hausmann, joepeck, jturcotte, kadam, keishi, loislo, marja, ossy, pfeldman, philn, pmuellr, rakuco, rniwa, timothy, vsevik, web-inspector-bugs, webkit-ews, webkit.review.bot, xan.lopez, yurys, zarvai

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

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!