[webkit-reviews] review denied: [Bug 61179] Web Inspector: [Chromium] Add an ability to look up and explore an object from a heap profile : [Attachment 109163] JSC fix again

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Mikhail Naganov
<mnaganov at chromium.org>'s request for review:
Bug 61179: Web Inspector: [Chromium] Add an ability to look up and explore an
object from a heap profile
https://bugs.webkit.org/show_bug.cgi?id=61179

Attachment 109163: JSC fix again
https://bugs.webkit.org/attachment.cgi?id=109163&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=109163&action=review


Overall looks good. A number of nits to be fixed prior to landing.

> Source/WebCore/bindings/js/ScriptProfiler.cpp:45
> +PassRefPtr<InspectorValue> ScriptProfiler::getObjectByHeapObjectId(unsigned,
InjectedScriptManager*)

While we use "get" prefixes in the protocol, regular WebCore classes should not
have those.

> Source/WebCore/bindings/v8/ScriptProfiler.cpp:83
> +    v8::HandleScope scope;

Blank line here would be great.

> Source/WebCore/bindings/v8/ScriptProfiler.cpp:87
> +    v8::Handle<v8::Object> object(value.As<v8::Object>());

ditto

>> Source/WebCore/bindings/v8/ScriptProfiler.cpp:92
>> +	if (!injectedScript.hasNoValue())
> 
> An else statement can be removed when the prior "if" concludes with a return,
break, continue or goto statement.  [readability/control_flow] [4]

Please follow the suggested style.

> Source/WebCore/inspector/Inspector.json:1947
> +		       { "name": "objectId", "type": "integer" }

You should define class for your ids in the long run.

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:384
> +    if (m_frontend) {

no need to check for front-end.

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:386
> +	   heapObject->asObject(result);

Since this result is not optional (according to the protocol), you can't pass
null here. You should assign errorString.

> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:189
> +    } else if (node.flags & tree.snapshot.nodeFlags.canBeQueried) {

no need for {}

> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:274
> +	   if (this._type === "string") {

no need for {}

> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:275
> +	       callback({ type: "string", description: this._name });

Use WebInspector.RemoteObject.fromPrimitiveValue(this._name)

> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:279
> +		   if (object.type)

You should check for the error here.

> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:282
> +		       callback({ type: "null", description:
WebInspector.UIString("Not available") });

ditto

> Source/WebCore/inspector/front-end/HeapSnapshot.js:995
> +	   var list = [];

A test for this method would be great.

> Source/WebCore/inspector/front-end/Popover.js:292
> +WebInspector.ObjectPopoverHelper = function(panelElement, getAnchor,
queryObject, onHide, disableOnClick)

Please extract this in a separate change.