[webkit-reviews] review denied: [Bug 88466] Web Inspector: [CSS Pane] Toggling :hover does not bubble up : [Attachment 157947] 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 88466: Web Inspector: [CSS Pane] Toggling :hover does not bubble up
https://bugs.webkit.org/show_bug.cgi?id=88466

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

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


> Source/WebCore/inspector/InspectorCSSAgent.cpp:935
> +    unsigned currentForcedPseudoState =
m_forcedPseudoStateManager->fullForcedPseudoState(nodeId);

Ok, this is not good - we now have both: InspectorInstrumentation and front-end
calling different overloads of forcePseudoState. It might be a good idea to
rename it back to setForcedPseudoState. Also, I think I have a pseudo code that
does what you want to achieve with this test and only takes a dozen of lines.
It introduces a convenience struct ForcedPseudoState { unsigned stateMask;
unsigned hoverCount; } to achieve that.

    if (forcedPseudoState.hover && !currentForcedPseudoState.hover) {
	for (Element* currentElement = element; currentElement; currentElement
= currentElement->parentElement())
	   
m_nodeIdToForcedPseudoState[m_domAgent->boundNodeId(parentElement)].hoverCount+
+;
    } else if (!forcedPseudoState.hover && currentForcedPseudoState.hover) {
	for (Element* currentElement = element; currentElement; currentElement
= currentElement->parentElement())
	    int id = m_domAgent->boundNodeId(parentElement);
	    PseudoState& state = m_nodeIdToForcedPseudoState[id];
	    state.hoverCount--;
	    if (!parentState.stateMask && !parentState.hoverCount)
		m_nodeIdToForcedPseudoState.remove(id);
    }

But I guess we don't even need a counter - there can only be one hovered
element at a time, so we only need to mark that element + its path to the
document.

Another point that is missing is DOM mutations - what if the node that has been
hovered has been removed from the DOM - we need to clean its path as well.