[webkit-reviews] review denied: [Bug 72701] Web Inspector: [Protocol] Retain a single universal method for loading a required combination of element styles : [Attachment 115765] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 72701: Web Inspector: [Protocol] Retain a single universal method for
loading a required combination of element styles
https://bugs.webkit.org/show_bug.cgi?id=72701

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=115765&action=review


> Source/WebCore/inspector/Inspector.json:1438
>		   "type": "object",

I don't see why we need this class - you should return these from
getStylesFromNode.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:256
> +	   // Early return to avoid unnecessary style recalcs.

This early return confuses me - we should treat all the flags equally.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:262
>      bool needStyleRecalc = element != m_lastElementWithPseudoState ||
forcePseudoState != m_lastPseudoState;

I would just add a check here to not recalc in case of inline query.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:311
> +		       parentStyle->setObject("inlineStyle",
styleSheet->buildObjectForStyle(styleSheet->styleForId(InspectorCSSId(styleShee
t->id(), 0))));

So asking for inherited styles will return all: inline, matched, etc? This does
not sound right.

> Source/WebCore/inspector/front-end/CSSStyleModel.js:88
> +	       if ("computedStyle" in payload)

Here and below, use "if (payload.computedStyle)" for compilability.

> Source/WebCore/inspector/front-end/CSSStyleModel.js:150
> +	   CSSAgent.getStylesForNode(nodeId, [],
WebInspector.CSSStyleModel.StyleFlags.COMPUTED, callback.bind(null,
userCallback));

Computed style should take forced pseudo classes into consideration.