[Webkit-unassigned] [Bug 91855] Web Inspector: Protocol Extension: add getFlowByName command

Mon Jul 23 10:56:58 PDT 2012

https://bugs.webkit.org/show_bug.cgi?id=91855

Pavel Feldman <pfeldman at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #153812|review?                     |review+
               Flag|                            |

--- Comment #8 from Pavel Feldman <pfeldman at chromium.org>  2012-07-23 10:56:57 PST ---
(From update of attachment 153812)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.