[webkit-reviews] review granted: [Bug 91855] Web Inspector: Protocol Extension: add getFlowByName command : [Attachment 153812] Patch

Mon Jul 23 10:56:50 PDT 2012

Pavel Feldman <pfeldman at chromium.org> has granted Andrei Poenaru
<poenaru at adobe.com>'s request for review:
Bug 91855: Web Inspector: Protocol Extension: add getFlowByName command
https://bugs.webkit.org/show_bug.cgi?id=91855

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

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

I am r+ -ing this change, but please fix the nits prior to landing it. I also
think that you could combine named flow getters into single method.

> LayoutTests/inspector/styles/protocol-css-regions-commands.html:22
> +    InspectorTest.evaluateInPage("createDynamicElements()", testStep);

Nit: for the tests that have setup work on the layout page I typically do:

function createDynamicElements()
{
    ...
    runTest();
}

<body onload="createDynamicElements()">

> LayoutTests/inspector/styles/protocol-css-regions-commands.html:34
> +		       {

{ on previous line

> LayoutTests/inspector/styles/protocol-css-regions-commands.html:53
> +		   WebInspector.domAgent.requestDocument(documentCallback);

Nit: we switched from declaring callbacks before the function to declaring them
after. That way test reads better, while functions can be referenced above
declaration in their scope.

> LayoutTests/inspector/styles/protocol-css-regions-commands.html:90
> +		       if (namedFlow)

ditto

> Source/WebCore/inspector/Inspector.json:2138
> +		       { "name": "overset", "type": "boolean", "description":
"The \"overset\" attribute of a Named Flow." }

Is there a reason we want to query for names collection first and get the
details later? We can transmit magabytes of data over the protocol. If
performance is not super-important here, I'd prefer to return flows instantly.

> Source/WebCore/inspector/front-end/CSSStyleModel.js:204
> +	   CSSAgent.getFlowByName(nodeId, flowName, callback.bind(null,
userCallback));

userCallback is already in the scope, just use it directly. You can also bind
to "this". The snippet you were copying was not designed well.