Bug 91855 - Web Inspector: Protocol Extension: add getFlowByName command
Summary: Web Inspector: Protocol Extension: add getFlowByName command
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 90786
  Show dependency treegraph
 
Reported: 2012-07-20 06:44 PDT by Andrei Poenaru
Modified: 2012-07-31 01:19 PDT (History)
13 users (show)

See Also:


Attachments
Protocol Extension (3.88 KB, application/json)
2012-07-20 06:45 PDT, Andrei Poenaru
no flags Details
Patch (17.77 KB, patch)
2012-07-23 09:15 PDT, Andrei Poenaru
no flags Details | Formatted Diff | Diff
Patch (17.81 KB, patch)
2012-07-23 09:36 PDT, Andrei Poenaru
no flags Details | Formatted Diff | Diff
Patch for landing (18.39 KB, patch)
2012-07-24 02:44 PDT, Andrei Poenaru
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrei Poenaru 2012-07-20 06:44:18 PDT
Implement the getFlowByName command from this proposed protocol extension.
Comment 1 Andrei Poenaru 2012-07-20 06:45:23 PDT
Created attachment 153494 [details]
Protocol Extension
Comment 2 Pavel Feldman 2012-07-20 06:55:21 PDT
I wonder what flow properties are available via web-facing API (if any). I am a bit concerned with the protocol API surface here. We need to assess the added value vs the complexity / surface of the API. There is a lot of features we don't implement in order to keep the code manageable. Maybe we need to extract a dedicated domain for the CSS flows ("CSSFlow") to make it more modular.
Comment 3 Andrei Poenaru 2012-07-20 07:03:24 PDT
http://www.w3.org/TR/css3-regions/#cssom_view_and_css_regions

For the moment only "document.webkitGetFlowByName" is implemented.  Also, "firstEmptyRegionIndex" will be removed because it does not a have a use case.
Comment 4 Pavel Feldman 2012-07-20 07:38:25 PDT
(In reply to comment #3)
> http://www.w3.org/TR/css3-regions/#cssom_view_and_css_regions
> 
> For the moment only "document.webkitGetFlowByName" is implemented.  Also, "firstEmptyRegionIndex" will be removed because it does not a have a use case.

Ok. Do you have an idea on the overall API that you will need? I'm trying to understand whether there is a strong connection to the rest of the CSS domain. It might be that you should form everything in a separate agent / domain.
Comment 5 Andrei Poenaru 2012-07-20 07:56:11 PDT
In order to implement the design discussed on bug 90789, all the elements in the proposed protocol extension are needed.
I think there is a strong connection to the CSS domain since in it reside the functions used to modify/create named flows.
Comment 6 Andrei Poenaru 2012-07-23 09:15:57 PDT
Created attachment 153810 [details]
Patch
Comment 7 Andrei Poenaru 2012-07-23 09:36:45 PDT
Created attachment 153812 [details]
Patch
Comment 8 Pavel Feldman 2012-07-23 10:56:57 PDT
Comment on attachment 153812 [details]
Patch

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.
Comment 9 Andrei Poenaru 2012-07-24 02:44:31 PDT
Created attachment 153998 [details]
Patch for landing
Comment 10 Andrei Poenaru 2012-07-24 02:49:34 PDT
(In reply to comment #8)
> (From update of attachment 153812 [details])
> 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.

I will create another bug for refactoring the "getNamedFlowCollection" and "getFlowByName" commands so that there will be no need for "getNamedFlowContent" and "getNamedFlowRegions".
Comment 11 WebKit Review Bot 2012-07-24 05:00:52 PDT
Comment on attachment 153998 [details]
Patch for landing

Clearing flags on attachment: 153998

Committed r123459: <http://trac.webkit.org/changeset/123459>
Comment 12 WebKit Review Bot 2012-07-24 05:01:00 PDT
All reviewed patches have been landed.  Closing bug.