[webkit-reviews] review granted: [Bug 66544] Web Inspector: getAttributes should work on a single node, not array. : [Attachment 104483] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has granted Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 66544: Web Inspector: getAttributes should work on a single node, not
array.
https://bugs.webkit.org/show_bug.cgi?id=66544

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

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=104483&action=review


>> Source/WebCore/inspector/InspectorDOMAgent.cpp:1083
>> +void InspectorDOMAgent::getAttributes(ErrorString* errorString, int nodeId,
RefPtr<InspectorArray>* result)
> 
> The parameter type should use PassRefPtr instead of RefPtr. 
[readability/pass_ptr] [5]

We should file a bug about check-webkit-style not understanding RefPtr
out-parameters.

> Source/WebCore/inspector/front-end/DOMAgent.js:463
> -	   var nodeIds = [];
>	   for (var nodeId in this._attributeLoadNodeIds)
> -	       nodeIds.push(Number(nodeId));
> -	   DOMAgent.getAttributes(nodeIds,
this._wrapClientCallback(callback.bind(this)));
> +	       DOMAgent.getAttributes(parseInt(nodeId),
this._wrapClientCallback(callback.bind(this, nodeId)));

Presumably this is worse from an efficiency standpoint. Hopefully it won't
matter in practice.