Bug 91855

Summary:

Web Inspector: Protocol Extension: add getFlowByName command

Product:

WebKit

Reporter:

Andrei Poenaru <andreigpoenaru>

Component:

Web Inspector (Deprecated)

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

apavlov, bweinstein, gyuyoung.kim, joepeck, keishi, loislo, pfeldman, pmuellr, rakuco, rik, timothy, webkit.review.bot, yurys

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

90786

Attachments:

Description	Flags
Protocol Extension	none
Patch	none
Patch	none
Patch for landing	none

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	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
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.