[webkit-reviews] review denied: [Bug 86630] Web Inspector: Emulate pseudo styles (hover etc.) of non-selected elements : [Attachment 150610] 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 86630: Web Inspector: Emulate pseudo styles (hover etc.) of non-selected
elements
https://bugs.webkit.org/show_bug.cgi?id=86630

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

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


> Source/WebCore/inspector/InspectorCSSAgent.h:137
> +    typedef HashMap<Node*, unsigned> NodeToForcedPseudoState;

So you rely upon the fact that nodes in this map are alive (retained in
InspectorDOMAgent) + you use the listener to clean it up. It does look good to
me.
Another option would be to have a mapping from node id (int) to the state. It
would look less suspicious: Node* as a key makes one think that it could be
collected. It does add indirection into the recalcStyleForPseudoStateIfNeeded
though.

Anyways, I like latter a bit more, so please consider using that approach.

> Source/WebCore/inspector/front-end/ElementsPanel.js:178
> +	       if (pseudoClasses.indexOf(pseudoClass) >= 0)

I would simply use {} here, you can store pseudoclasses as object keys.

> Source/WebCore/inspector/front-end/ElementsPanel.js:186
> +	       if (!pseudoClasses.length)

This would need to be Object.keys(pseudoClasses).length, but this happens less
frequently.

> Source/WebCore/inspector/front-end/ElementsPanel.js:349
> +	   var treeElement = this.treeOutline.findTreeElement(node);

I am sure you can re-use some of this code elsewhere. Otherwise, it is not
clear why we have a whole new method operating close tab and pushing styles /
metrics updates.

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:233
> +    decorationForTreeElement: function(treeElement)

This should be private and can be declared on the tree element itself.

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:257
> +	   decorationElement.addStyleClass("gutter-decoration");

Style name could be more specific (elements-gutter-decoration)

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:259
> +	       decorationElement.addStyleClass("children-decoration");

ditto

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:642
> +	   if (!propertyValue)

This check pretty much kills the benefit of the base class. I'd merge the two.

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1168
> +	   var pseudoSubMenu =
contextMenu.appendSubMenuItem(WebInspector.UIString("Pseudoclasses"));

"Force state"?

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1181
> +	   var elementsPanel = WebInspector.panels.elements;

You should not access elements panel from here, elements tree outline should
not depend on it. r- is for this.