Bug 108653 – Web Inspector: Split Profiler domain in protocol into Profiler and HeapProfiler

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

[Alexei Filippov]

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.

[Alexei Filippov]

Created attachment 186073 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Alexei Filippov]

Created attachment 186353 [details]
Patch

[Build Bot]

Comment on attachment 186353 [details]
Patch

Attachment 186353 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16368242

[Yury Semikhatsky]

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.

[Yury Semikhatsky]

Comment on attachment 186353 [details]
Patch

Note, that this change will break Chromium pyauto tests

[Alexei Filippov]

Created attachment 186380 [details]
Patch

[Alexei Filippov]

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.

[Alexei Filippov]

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

[Yury Semikhatsky]

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?

[Yury Semikhatsky]

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

[Alexei Filippov]

(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

[Yury Semikhatsky]

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

[Alexei Filippov]

Created attachment 186874 [details]
Patch

[Early Warning System Bot]

Comment on attachment 186874 [details]
Patch

Attachment 186874 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16396288

[Early Warning System Bot]

Comment on attachment 186874 [details]
Patch

Attachment 186874 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16388401

[Build Bot]

Comment on attachment 186874 [details]
Patch

Attachment 186874 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16390371

[Alexei Filippov]

Created attachment 186878 [details]
Patch

[Build Bot]

Comment on attachment 186878 [details]
Patch

Attachment 186878 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16394381

[Alexei Filippov]

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

[Alexei Filippov]

Created attachment 187071 [details]
Patch

[Yury Semikhatsky]

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

[Alexei Filippov]

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.

[Alexei Filippov]

Created attachment 187323 [details]
Patch

[Yury Semikhatsky]

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.

[Alexei Filippov]

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

[Alexei Filippov]

Created attachment 187533 [details]
Patch

[WebKit Review Bot]

Comment on attachment 187533 [details]
Patch

Clearing flags on attachment: 187533

Committed r142460: <http://trac.webkit.org/changeset/142460>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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

[Csaba Osztrogonác]

Fix landed in http://trac.webkit.org/changeset/142517 ( without any references to this bug :-/ )

[Yury Semikhatsky]

(In reply to comment #32)
> Fix landed in http://trac.webkit.org/changeset/142517 ( without any references to this bug :-/ )

Thanks for fixing this!