Bug 104545 – Web Inspector: Profiler: split "getProfile" to "getCPUProfile" and "loadHeapSnapshot"

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

[Eugene Klyuchnikov]

This change will allow stronger typing of profiler output.

[Eugene Klyuchnikov]

Created attachment 178532 [details]
Patch

[Pavel Feldman]

Comment on attachment 178532 [details]
Patch

@yurys, do you want to review this?

[Yury Semikhatsky]

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.

[Yury Semikhatsky]

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.

[Eugene Klyuchnikov]

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.

[Eugene Klyuchnikov]

Created attachment 179483 [details]
Patch

[Eugene Klyuchnikov]

(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

[Eugene Klyuchnikov]

> Patch: https://chromiumcodereview.appspot.com/11583002/
> CrBug: http://code.google.com/p/chromium/issues/detail?id=166160

Reworked patch: https://chromiumcodereview.appspot.com/11615021/

[Eugene Klyuchnikov]

Created attachment 180290 [details]
Patch

rebased

[Eugene Klyuchnikov]

Created attachment 183120 [details]
Patch

[WebKit Review Bot]

Comment on attachment 183120 [details]
Patch

Clearing flags on attachment: 183120

Committed r139998: <http://trac.webkit.org/changeset/139998>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This patch broke LeaksViewer :(

Now we're getting erros like:
Uncaught TypeError: Object #<Object> has no method 'getProfile'

[Ryosuke Niwa]

Reverted the change in http://trac.webkit.org/changeset/140639 since the Leaks Viewer pulls in the inspector code at r80565.

[Eugene Klyuchnikov]

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

[Ryosuke Niwa]

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.

[Eugene Klyuchnikov]

leaks-finder do not depend on LeaksViewer, it depend on inspector protocol.

[Ryosuke Niwa]

(In reply to comment #17)
> leaks-finder do not depend on LeaksViewer, it depend on inspector protocol.

Then nothing is broken.

[Eugene Klyuchnikov]

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

[Ryosuke Niwa]

(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

[Eugene Klyuchnikov]

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