[webkit-reviews] review denied: [Bug 86204] Web Inspector: heap profiler should allow revealing an element which is logged to the console : [Attachment 141400] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Yury Semikhatsky
<yurys at chromium.org>'s request for review:
Bug 86204: Web Inspector: heap profiler should allow revealing an element which
is logged to the console
https://bugs.webkit.org/show_bug.cgi?id=86204

Attachment 141400: Patch
https://bugs.webkit.org/attachment.cgi?id=141400&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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.