[Webkit-unassigned] [Bug 92089] Web Inspector: Protocol Extension: Refactor protocol extension for CSS Regions

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=92089


Pavel Feldman <pfeldman at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #154641|review?                     |review-
               Flag|                            |




--- Comment #3 from Pavel Feldman <pfeldman at chromium.org>  2012-07-26 09:09:12 PST ---
(From update of attachment 154641)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.