Bug 104545 - Web Inspector: Profiler: split "getProfile" to "getCPUProfile" and "loadHeapSnapshot"
Summary: Web Inspector: Profiler: split "getProfile" to "getCPUProfile" and "loadHeapS...
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: Eugene Klyuchnikov
URL:
Keywords:
Depends on:
Blocks: 102792
  Show dependency treegraph
 
Reported: 2012-12-10 06:15 PST by Eugene Klyuchnikov
Modified: 2013-01-23 21:07 PST (History)
13 users (show)

See Also:


Attachments
Patch (15.52 KB, patch)
2012-12-10 06:22 PST, Eugene Klyuchnikov
yurys: review+
Details | Formatted Diff | Diff
Patch (15.52 KB, patch)
2012-12-14 07:55 PST, Eugene Klyuchnikov
no flags Details | Formatted Diff | Diff
Patch (15.65 KB, patch)
2012-12-20 00:33 PST, Eugene Klyuchnikov
no flags Details | Formatted Diff | Diff
Patch (15.65 KB, patch)
2013-01-16 22:12 PST, Eugene Klyuchnikov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eugene Klyuchnikov 2012-12-10 06:15:28 PST
This change will allow stronger typing of profiler output.
Comment 1 Eugene Klyuchnikov 2012-12-10 06:22:45 PST
Created attachment 178532 [details]
Patch
Comment 2 Pavel Feldman 2012-12-14 00:28:36 PST
Comment on attachment 178532 [details]
Patch

@yurys, do you want to review this?
Comment 3 Yury Semikhatsky 2012-12-14 02:11:56 PST
Comment on attachment 178532 [details]
Patch

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

> Source/WebCore/inspector/Inspector.json:3035
> +                "name": "loadHeapSnapshot",

I'd name it getHeapSnapshot to avoid confusion with loading from filesystem and for consistency with getCPUProfile.
Comment 4 Yury Semikhatsky 2012-12-14 02:18:17 PST
This change will likely break  chrome/test/pyautolib/remote_inspector_client.py and related automation tests. We should update the script when this change is committed and rolled.
Comment 5 Eugene Klyuchnikov 2012-12-14 07:54:00 PST
Comment on attachment 178532 [details]
Patch

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

https://code.google.com/p/chromium/issues/detail?id=166160

>> Source/WebCore/inspector/Inspector.json:3035
>> +                "name": "loadHeapSnapshot",
> 
> I'd name it getHeapSnapshot to avoid confusion with loading from filesystem and for consistency with getCPUProfile.

Done.
Comment 6 Eugene Klyuchnikov 2012-12-14 07:55:58 PST
Created attachment 179483 [details]
Patch
Comment 7 Eugene Klyuchnikov 2012-12-17 00:31:00 PST
(In reply to comment #4)
> This change will likely break  chrome/test/pyautolib/remote_inspector_client.py and related automation tests. We should update the script when this change is committed and rolled.

Patch: https://chromiumcodereview.appspot.com/11583002/
CrBug: http://code.google.com/p/chromium/issues/detail?id=166160
Comment 9 Eugene Klyuchnikov 2012-12-20 00:33:10 PST
Created attachment 180290 [details]
Patch

rebased
Comment 10 Eugene Klyuchnikov 2013-01-16 22:12:41 PST
Created attachment 183120 [details]
Patch
Comment 11 WebKit Review Bot 2013-01-17 10:52:25 PST
Comment on attachment 183120 [details]
Patch

Clearing flags on attachment: 183120

Committed r139998: <http://trac.webkit.org/changeset/139998>
Comment 12 WebKit Review Bot 2013-01-17 10:52:29 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Ryosuke Niwa 2013-01-23 20:05:19 PST
This patch broke LeaksViewer :(

Now we're getting erros like:
Uncaught TypeError: Object #<Object> has no method 'getProfile'
Comment 14 Ryosuke Niwa 2013-01-23 20:11:29 PST
Reverted the change in http://trac.webkit.org/changeset/140639 since the Leaks Viewer pulls in the inspector code at r80565.
Comment 15 Eugene Klyuchnikov 2013-01-23 20:36:41 PST
(In reply to comment #14)
> Reverted the change in http://trac.webkit.org/changeset/140639 since the Leaks Viewer pulls in the inspector code at r80565.

I think that LeaksViewer should be fixed, as it was done with leaks-finder https://codereview.chromium.org/11615021/

Reverting this change you broke leaks-finder!!!
Comment 16 Ryosuke Niwa 2013-01-23 20:42:46 PST
Another fixed landed in http://trac.webkit.org/changeset/140641.

(In reply to comment #15)
> (In reply to comment #14)
> > Reverted the change in http://trac.webkit.org/changeset/140639 since the Leaks Viewer pulls in the inspector code at r80565.
> 
> I think that LeaksViewer should be fixed, as it was done with leaks-finder https://codereview.chromium.org/11615021/
> 
> Reverting this change you broke leaks-finder!!!

Why is this random Chromium tool depending on Tools/BuildSlaveSupport/build.webkit.org-config/public_html/LeaksViewer/?  Also, we can't break WebKit tool in order to fix Chromium tools. That's just not how it works.
Comment 17 Eugene Klyuchnikov 2013-01-23 20:45:37 PST
leaks-finder do not depend on LeaksViewer, it depend on inspector protocol.
Comment 18 Ryosuke Niwa 2013-01-23 20:51:14 PST
(In reply to comment #17)
> leaks-finder do not depend on LeaksViewer, it depend on inspector protocol.

Then nothing is broken.
Comment 19 Eugene Klyuchnikov 2013-01-23 20:58:18 PST
(In reply to comment #18)
> (In reply to comment #17)
> > leaks-finder do not depend on LeaksViewer, it depend on inspector protocol.
> 
> Then nothing is broken.

This patch contains changes to inspector protocol.

All references to "getProfile" in this patch were replaced with "getCPUProfile" and "loadHeapShapshot".

WebinspectorShims contains something that simulates WebInspector.
This was also changed to conform new interface.

The actual problem is: why LeaksViewer uses pulls web inspector code at r80565 and WebinspectorShims code at tip-of-tree?
Comment 20 Ryosuke Niwa 2013-01-23 21:05:43 PST
(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > leaks-finder do not depend on LeaksViewer, it depend on inspector protocol.
> > 
> > Then nothing is broken.
> 
> This patch contains changes to inspector protocol.

It does not!  Did you even look at the changesets?  All changes were made to:
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/LeaksViewer/WebInspectorShims.js
Comment 21 Eugene Klyuchnikov 2013-01-23 21:07:20 PST
(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > (In reply to comment #17)
> > > > leaks-finder do not depend on LeaksViewer, it depend on inspector protocol.
> > > 
> > > Then nothing is broken.
> > 
> > This patch contains changes to inspector protocol.
> 
> It does not!  Did you even look at the changesets?  All changes were made to:
> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/LeaksViewer/WebInspectorShims.js

Ooops. I'm really deeply sorry!

Thank you for your assistance.