Bug 86630 – Web Inspector: Emulate pseudo styles (hover etc.) of non-selected elements

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Alexander Pavlov (apavlov)]

Upstreaming http://code.google.com/p/chromium/issues/detail?id=128011

[Alexander Pavlov (apavlov)]

Created attachment 150610 [details]
Patch

[Pavel Feldman]

Comment on attachment 150610 [details]
Patch

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.

[Alexander Pavlov (apavlov)]

Created attachment 150779 [details]
Patch

[Pavel Feldman]

Comment on attachment 150779 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150779&action=review

> Source/WebCore/English.lproj/localizedStrings.js:20
> +localizedStrings["%d descendant with forced pseudo state"] = "%d descendant with forced pseudo state";

i think you should remove the "pseudo" part from all the captions.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:537
> +    if (m_nodeIdToForcedPseudoState.isEmpty())

I don't think you need this check, it'll return later.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1048
> +        Element* element = static_cast<Element*>(m_domAgent->nodeForId(it->first));

toElement

> Source/WebCore/inspector/InspectorCSSAgent.h:144
> +    int boundNodeId(Node* node) { return m_domAgent->boundNodeId(node); }

I'd implement it in .cpp

> Source/WebCore/inspector/front-end/ElementsPanel.js:195
> +        this.treeOutline.updateOpenCloseTags(node);

It would be great if this was driven by the events.

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:40
> +WebInspector.ElementsTreeOutline = function(omitRootDOMNode, selectEnabled, showInElementsPanelEnabled, contextMenuCallback, setPseudoClassCallback)

I would introduce WebInspector.DOMAgent.prototype.addUserPropertyListener(name, listener) to resolve this dependency.

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:194
> +    updateOpenCloseTags: function(node)

This will become private.

[Alexander Pavlov (apavlov)]

Committed r121860: <http://trac.webkit.org/changeset/121860>