[webkit-reviews] review denied: [Bug 92089] Web Inspector: Protocol Extension: Refactor protocol extension for CSS Regions : [Attachment 154641] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Andrei Poenaru
<poenaru at adobe.com>'s request for review:
Bug 92089: Web Inspector: Protocol Extension: Refactor protocol extension for
CSS Regions
https://bugs.webkit.org/show_bug.cgi?id=92089

Attachment 154641: Patch
https://bugs.webkit.org/attachment.cgi?id=154641&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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.