[webkit-reviews] review denied: [Bug 92089] Web Inspector: Protocol Extension: Refactor protocol extension for CSS Regions : [Attachment 156910] 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 156910: Patch
https://bugs.webkit.org/attachment.cgi?id=156910&action=review

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