Bug 92089 – Web Inspector: Protocol Extension: Refactor protocol extension for CSS Regions

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Andrei Poenaru]

Change the implemented protocol extension for CSS Regions.

[Andrei Poenaru]

Created attachment 154001 [details]
Protocol Extension

[Andrei Poenaru]

Created attachment 154641 [details]
Patch

[Pavel Feldman]

Comment on attachment 154641 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154641&action=review

Mostly looks good. My main concern is the temporary function.

> Source/WebCore/dom/WebKitNamedFlow.cpp:100
> +const RenderRegionList* WebKitNamedFlow::tempGetRegions()

It sounds weird that we expose non-existing methods using the protocol.

> Source/WebCore/inspector/Inspector.json:2166
> +                    { "name": "regionOverset", "type": "string", "enum": ["overset", "fit", "empty"], "description": "The \"overset\" attribute of a Named Flow." },

this is already a region, consider dropping the "region" prefix.

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

How do the overset properties of flow and the region correlate?

> Source/WebCore/inspector/Inspector.json:2180
> +                    { "name": "regions", "type": "array", "items": { "$ref": "Region" }, "description": "An array of regions associated with the Named Flow." }

contentNodeId.
For my education: how do these two correlate? (content and regions.nodeIds).

> Source/WebCore/inspector/Inspector.json:2336
> +                    { "name": "namedFlows", "type": "array", "items": { "$ref": "NamedFlow" }, "description": "An array containing the Named Flows in the document." }

You should remove getFlowByName now, right?

> Source/WebCore/inspector/InspectorCSSAgent.cpp:831
> +    WebKitNamedFlow* webKitNamedFlow = document->namedFlows()->flowByName(flowName);

I think you should drop this method.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1054
> +PassRefPtr<TypeBuilder::CSS::NamedFlow> InspectorCSSAgent::buildObjectForNamedFlow(RefPtr<WebKitNamedFlow>& webKitNamedFlow, int documentNodeId)

Use PassRefPtr instead.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1056
> +    RefPtr<NodeList> contentList = webKitNamedFlow->getContent();

According to the WebKit coding guidelines, this should be called "content()", not "getContent()".

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1062
> +    // FIXME: Should be reimplemented when WebKitNamedFlow::getRegions will be available

I think you should start with implementing the underlying functionality.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1066
> +    if (regionList)

Missing {} for multiline block below.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1067
> +        for (RenderRegionList::const_iterator it = regionList->begin(); it != regionList->end(); ++it) {

Sounds like buildArrayForRegions.

[Andrei Poenaru]

(In reply to comment #3)
> (From update of attachment 154641 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154641&action=review
> 
> Mostly looks good. My main concern is the temporary function.
> 
> > Source/WebCore/dom/WebKitNamedFlow.cpp:100
> > +const RenderRegionList* WebKitNamedFlow::tempGetRegions()
> 
> It sounds weird that we expose non-existing methods using the protocol.
> 
The team I am working in at Adobe is responsible with implementing CSS Regions in WebKit. The implementation of the "getRegions" method is in progress, but it may
take some time before it gets in WebKit: they are having a hard time finding a reviewer.
http://www.w3.org/TR/css3-regions/#the-namedflow-interface

> > Source/WebCore/inspector/Inspector.json:2166
> > +                    { "name": "regionOverset", "type": "string", "enum": ["overset", "fit", "empty"], "description": "The \"overset\" attribute of a Named Flow." },
> 
> this is already a region, consider dropping the "region" prefix.
> 
Because of the "overset" value of the enum the property can not be named the same.

> > Source/WebCore/inspector/Inspector.json:2178
> > +                    { "name": "overset", "type": "boolean", "description": "The \"overset\" attribute of a Named Flow." },
> 
> How do the overset properties of flow and the region correlate?
> 
The flow has "overset: true" if the last region in the region chain has "regionOverset: overset".

> > Source/WebCore/inspector/Inspector.json:2180
> > +                    { "name": "regions", "type": "array", "items": { "$ref": "Region" }, "description": "An array of regions associated with the Named Flow." }
> 
> contentNodeId.
> For my education: how do these two correlate? (content and regions.nodeIds).
> 
The content nodes are taken from the main document flow and flowed through the regions in DOM order.
Sample (css regions should be enabled in "about://flags"): http://adobe.github.com/web-platform/samples/css-regions/basic/single-flow.html

> > Source/WebCore/inspector/Inspector.json:2336
> > +                    { "name": "namedFlows", "type": "array", "items": { "$ref": "NamedFlow" }, "description": "An array containing the Named Flows in the document." }
> 
> You should remove getFlowByName now, right?
> 
The reason why I proposed this method is to avoid the overhead of requesting all named flows when one changes, or a new flow is created.

[Alexander Pavlov (apavlov)]

Comment on attachment 154641 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154641&action=review

>>> Source/WebCore/inspector/Inspector.json:2336
>>> +                    { "name": "namedFlows", "type": "array", "items": { "$ref": "NamedFlow" }, "description": "An array containing the Named Flows in the document." }
>> 
>> You should remove getFlowByName now, right?
> 
> The reason why I proposed this method is to avoid the overhead of requesting all named flows when one changes, or a new flow is created.

What is the origin of the named flow changes? Is it CSSOM? If so, can you push the update into the frontend using an event instead?

> Source/WebCore/inspector/InspectorCSSAgent.cpp:837
> +    RefPtr<WebKitNamedFlow> refWebKitNamedFlow(webKitNamedFlow);

This line is not necessary. You could try passing webKitNamedFlow directly into buildObjectForNamedFlow(PassRefPtr<WebKitNamedFlow>...) and see if the respective PassRefPtr will get constructed automatically.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1086
> +                continue;

Should this ASSERT_NOT_REACHED() as well?

> Source/WebCore/inspector/InspectorDOMAgent.h:169
> +    int pushNodePathToFrontend(Node*);

What is the rationale behind this change? The two push* methods seem to have been properly grouped originally.

[Andrei Poenaru]

(In reply to comment #5)
> (From update of attachment 154641 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154641&action=review
> 
> >>> Source/WebCore/inspector/Inspector.json:2336
> >>> +                    { "name": "namedFlows", "type": "array", "items": { "$ref": "NamedFlow" }, "description": "An array containing the Named Flows in the document." }
> >> 
> >> You should remove getFlowByName now, right?
> > 
> > The reason why I proposed this method is to avoid the overhead of requesting all named flows when one changes, or a new flow is created.
> 
> What is the origin of the named flow changes? Is it CSSOM? If so, can you push the update into the frontend using an event instead?

I will also add some events, but, in order to keep them lightweight, they are sending only the name of the flow.

> > Source/WebCore/inspector/InspectorDOMAgent.h:169
> > +    int pushNodePathToFrontend(Node*);
> 
> What is the rationale behind this change? The two push* methods seem to have been properly grouped originally.

The reason why I made this method public is to be able to get the NodeIds for the content nodes and for the regions (they may not have been sent yet to the frontend).

[Andrei Poenaru]

Created attachment 156910 [details]
Patch

[Pavel Feldman]

Comment on attachment 156910 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156910&action=review

> Source/WebCore/inspector/Inspector.json:2199
>                      { "name": "nodeId", "$ref": "DOM.NodeId", "description": "The document node id." },

You operate document node id a lot, I was not anticipating this. I wother if it should be named documentId (or documentNodeId) consistently in the protocol.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1073
> +PassRefPtr<TypeBuilder::Array<TypeBuilder::CSS::Region> > InspectorCSSAgent::buildArrayForRegions(PassRefPtr<NodeList> prpRegionList)

We don't use prp prefixes.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1075
> +    RefPtr<NodeList> regionList = prpRegionList;

PassRefPtr overrides ->, you should not need this intermediate step.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1079
> +        const AtomicString& regionElementOverset = static_cast<Element*>(regionList->item(i))->webkitRegionOverset();

toElement(...)

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1082
> +        if (regionElementOverset == "empty")

Are there any contants for these in WebCore?

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1095
> +            .setNodeId(m_domAgent->pushNodePathToFrontend(regionList->item(i)));

FYI: pushNodePathToFrontend is private for a reason:
- we don't want to emit notifications without requests
- it would not work if say client has not yet requested the document.

In your case, you receive document nodeId in getNamedFlowCollection, so you know that the user already has requested DOM. I think we should expose public InspectorDOMAgent::pushNodeToFrontend(int documentNodeId, node) so that it could only be called with a document handle in hand. It would then check if the node belongs to that document and redirect to the private pushNodePathToFrontend if everything is Ok.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1109
> +        content->addItem(m_domAgent->pushNodePathToFrontend(contentList->item(i)));

ditto

> Source/WebCore/inspector/InspectorDOMAgent.h:172
> +    int pushNodePathToFrontend(Node*);

I don't think you should do this. See above for notes.

> Source/WebCore/inspector/front-end/CSSStyleModel.js:169
>      getNamedFlowCollectionAsync: function(nodeId, userCallback)

Sounds like this change is missing a more comprehensive test!

[Andrei Poenaru]

Created attachment 157452 [details]
Patch

[Pavel Feldman]

Comment on attachment 157452 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157452&action=review

> Source/WebCore/inspector/InspectorDOMAgent.cpp:454
> +    if (!assertDocument(errorString, documentNodeId))

Nit: you could also check whether node's document matches the one resolved.

[Andrei Poenaru]

Created attachment 157647 [details]
Patch for landing

[Pavel Feldman]

Comment on attachment 157647 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=157647&action=review

> Source/WebCore/inspector/InspectorDOMAgent.cpp:456
> +        return 0;

You need to assign the errorString here in case the latter is false.

[Andrei Poenaru]

Created attachment 157650 [details]
Patch for landing

[Andrei Poenaru]

Created attachment 157665 [details]
Patch

[WebKit Review Bot]

Comment on attachment 157665 [details]
Patch

Clearing flags on attachment: 157665

Committed r125268: <http://trac.webkit.org/changeset/125268>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.