RESOLVED FIXED Bug 91855
Web Inspector: Protocol Extension: add getFlowByName command 
https://bugs.webkit.org/show_bug.cgi?id=91855
Summary Web Inspector: Protocol Extension: add getFlowByName command
Andrei Poenaru
Reported 2012-07-20 06:44:18 PDT
Implement the getFlowByName command from this proposed protocol extension.
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
DetailsFormatted DiffDiff
Patch (17.81 KB, patch)
2012-07-23 09:36 PDT, Andrei Poenaru 		no flags
DetailsFormatted DiffDiff
Patch for landing (18.39 KB, patch)
2012-07-24 02:44 PDT, Andrei Poenaru 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Andrei Poenaru
Comment 1 2012-07-20 06:45:23 PDT
Created attachment 153494 [details] Protocol Extension
Pavel Feldman
Comment 2 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.
Andrei Poenaru
Comment 3 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.
Pavel Feldman
Comment 4 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.
Andrei Poenaru
Comment 5 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.
Andrei Poenaru
Comment 6 2012-07-23 09:15:57 PDT
Created attachment 153810 [details] Patch
Andrei Poenaru
Comment 7 2012-07-23 09:36:45 PDT
Created attachment 153812 [details] Patch
Pavel Feldman
Comment 8 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.
Andrei Poenaru
Comment 9 2012-07-24 02:44:31 PDT
Created attachment 153998 [details] Patch for landing
Andrei Poenaru
Comment 10 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".
WebKit Review Bot
Comment 11 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>
WebKit Review Bot
Comment 12 2012-07-24 05:01:00 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.