Bug 86204

Summary: Web Inspector: heap profiler should allow revealing an element which is logged to the console
Product: WebKit Reporter: Yury Semikhatsky <yurys>
Component: Web Inspector (Deprecated)Assignee: Yury Semikhatsky <yurys>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, apavlov, bweinstein, gustavo, haraken, japhet, joepeck, keishi, loislo, pfeldman, philn, pmuellr, rik, timothy, webkit.review.bot, xan.lopez, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 86211, 86230    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch pfeldman: review+

Description Yury Semikhatsky 2012-05-11 05:56:41 PDT
At the moment we can select an element in a heap snapshot and than access the element in the console expressions using $0 variable. It is logical to have backward navigation: from an object in the console to the corresponding heap snapshot entry.
Comment 1 Yury Semikhatsky 2012-05-11 06:03:20 PDT
Created attachment 141391 [details]
Patch
Comment 2 Early Warning System Bot 2012-05-11 06:23:06 PDT
Comment on attachment 141391 [details]
Patch

Attachment 141391 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12674164
Comment 3 Build Bot 2012-05-11 06:30:17 PDT
Comment on attachment 141391 [details]
Patch

Attachment 141391 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12663686
Comment 4 Gyuyoung Kim 2012-05-11 06:30:35 PDT
Comment on attachment 141391 [details]
Patch

Attachment 141391 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12664666
Comment 5 Early Warning System Bot 2012-05-11 06:31:01 PDT
Comment on attachment 141391 [details]
Patch

Attachment 141391 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12672307
Comment 6 Yury Semikhatsky 2012-05-11 06:32:11 PDT
Created attachment 141400 [details]
Patch
Comment 7 Pavel Feldman 2012-05-11 07:12:57 PDT
Comment on attachment 141400 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        JS objects in the console have context menu item that allows to reveal them in a heap snapshot view.

Please be more verbose here, what is SnapshotObjectId?

> Source/WebCore/inspector/InjectedScript.cpp:193
> +ScriptValue InjectedScript::findObjectById(const String& objectId) const

objectForId?

> Source/WebCore/inspector/Inspector.json:2680
> +                    { "name": "heapSnapshotObjectId", "type": "string", "description": "Heap snapshot object id." }

Please define a type for object id.

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:455
> +    ScriptValue jsValue = injectedScript.findObjectById(objectId);

jsValue name is misleading - I started thinking it was jsc-specific.

> Source/WebCore/inspector/front-end/HeapSnapshot.js:1569
> +    serializeItemsRange: function(begin, end)

Please annotate.

> Source/WebCore/inspector/front-end/HeapSnapshot.js:1572
> +        if (end >= this._iterationOrder.length)

Is this a part of this change?

> Source/WebCore/inspector/front-end/HeapSnapshot.js:1626
> +    serializeItem: function(edge)

Please annotate.

> Source/WebCore/inspector/front-end/HeapSnapshot.js:1721
> +    getNodePosition: function(snapshotObjectId)

Please annotate.

> Source/WebCore/inspector/front-end/HeapSnapshotDataGrids.js:73
> +    selectObjectByHeapSnapshotId: function(heapSnapshotObjectId)

Please annotate.

> Source/WebCore/inspector/front-end/HeapSnapshotDataGrids.js:392
> +    selectObjectByHeapSnapshotId: function(id)

Please annotate.

> Source/WebCore/inspector/front-end/HeapSnapshotGridNodes.js:158
> +            if (!this._positionRanges.length) {

Is it the part of this change?

> Source/WebCore/inspector/front-end/HeapSnapshotGridNodes.js:724
> +        WebInspector.log("findAndSerializeItems " + snapshotObjectId);

Please remove logging.

> Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:64
> +            WebInspector.log("revealInDominatorsView " + objectId);

Please remove logging.

> Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:68
> +            ProfilerAgent.getHeapObjectId(objectId, didReceiveHeapObjectId.bind(this));

ObjectPropertiesSection is in components module, outsite profiler module. Ideally, this should not compile.

> Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:73
> +            WebInspector.log("didReceiveHeapObjectId error = " + error + " result = " + result);

logging.
Comment 8 Yury Semikhatsky 2012-05-12 08:55:07 PDT
Created attachment 141581 [details]
Patch
Comment 9 Pavel Feldman 2012-05-12 09:24:25 PDT
Comment on attachment 141581 [details]
Patch

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

> Source/WebCore/inspector/front-end/HeapSnapshot.js:1289
> +    getNodeClassName: function(snapshotObjectId)

nodeClassName:

> Source/WebCore/inspector/front-end/HeapSnapshot.js:1605
> +    getNodePosition: function(snapshotObjectId)

nodePosition

> Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:80
> +WebInspector.ObjectPropertiesSection.addContextMenuProvier = function(provider)

Provider

> Source/WebCore/inspector/front-end/ProfilesPanel.js:1066
> +WebInspector.RevealInHeapSnapshotContextMenuProvier = function()

Provider
Comment 10 Yury Semikhatsky 2012-05-12 09:31:08 PDT
(In reply to comment #9)
> (From update of attachment 141581 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141581&action=review
> 
> > Source/WebCore/inspector/front-end/HeapSnapshot.js:1289
> > +    getNodeClassName: function(snapshotObjectId)
> 
> nodeClassName:
> 
Done.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:1605
> > +    getNodePosition: function(snapshotObjectId)
> 
> nodePosition
> 
Done.

> > Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:80
> > +WebInspector.ObjectPropertiesSection.addContextMenuProvier = function(provider)
> 
> Provider
> 
Done.

> > Source/WebCore/inspector/front-end/ProfilesPanel.js:1066
> > +WebInspector.RevealInHeapSnapshotContextMenuProvier = function()
> 
> Provider
Done.
Comment 11 Yury Semikhatsky 2012-05-12 09:35:06 PDT
Committed r116857: <http://trac.webkit.org/changeset/116857>