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.
Created attachment 186073 [details] Patch
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 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
Created attachment 186353 [details] Patch
Comment on attachment 186353 [details] Patch Attachment 186353 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16368242
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 on attachment 186353 [details] Patch Note, that this change will break Chromium pyauto tests
Created attachment 186380 [details] Patch
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.
(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 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?
(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.
(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
(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.
Created attachment 186874 [details] Patch
Comment on attachment 186874 [details] Patch Attachment 186874 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16396288
Comment on attachment 186874 [details] Patch Attachment 186874 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16388401
Comment on attachment 186874 [details] Patch Attachment 186874 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16390371
Created attachment 186878 [details] Patch
Comment on attachment 186878 [details] Patch Attachment 186878 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16394381
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
Created attachment 187071 [details] Patch
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 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.
Created attachment 187323 [details] Patch
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 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
Created attachment 187533 [details] Patch
Comment on attachment 187533 [details] Patch Clearing flags on attachment: 187533 Committed r142460: <http://trac.webkit.org/changeset/142460>
All reviewed patches have been landed. Closing bug.
(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
Fix landed in http://trac.webkit.org/changeset/142517 ( without any references to this bug :-/ )
(In reply to comment #32) > Fix landed in http://trac.webkit.org/changeset/142517 ( without any references to this bug :-/ ) Thanks for fixing this!