[webkit-reviews] review denied: [Bug 98168] Move side-effects on hover/active state out of hit-testing : [Attachment 191208] Patch

[bugzilla-daemon at webkit.org]

Allan Sandfeld Jensen <allan.jensen at digia.com> has denied Mike West
<mkwst at chromium.org>'s request for review:
Bug 98168: Move side-effects on hover/active state out of hit-testing
https://bugs.webkit.org/show_bug.cgi?id=98168

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

------- Additional Comments from Allan Sandfeld Jensen <allan.jensen at digia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=191208&action=review


Wrong assert. Note you need another reviewer once you have added my name to the
Changelog.

> Source/WebCore/ChangeLog:6
> +
> +	   Reviewed by NOBODY (OOPS!).

When continuing other peoples patches you still need to mention them in the
Changelog. For instance:
Based on patch by Allan Sandfeld Jensen.

> Source/WebCore/ChangeLog:25
> +	   Document::updateHoverActiveState is currently called during hit
testing
> +	   to update the hover and active states of elements effected by mouse
> +	   movements (or their keyboard equivalents). This conflates the hit
test
> +	   algorithm itself with side-effects associated with specific
use-cases.
> +
> +	   This conflation makes it very difficult to reuse the hover/active
logic
> +	   for things other than hit testing. 'mouseenter'/'mouseleave'
events[1]
> +	   are one example of a feature that would be simple to implement on
top of
> +	   this existing logic if we split it out from the hit testing path,
and
> +	   instead call it explicitly when necessary. An explicit split between

> +	   hit testing and its side-effects will also enable us to simplify
> +
> +	   This patch drops the call to Document::updateHoverActiveState from
> +	   RenderView::hitTest, and adjusts the three call-sites in
EventHandler
> +	   to explicitly call out to it rather than
Document::updateStyleIfNeeded.
> +	   The latter call is still necessary but has been folded into
> +	   updateHoverActiveState, as the former is never called without
calling
> +	   the latter.

Great!

> Source/WebCore/dom/Document.cpp:5847
> +    ASSERT(request.readOnly());

ASSERT(!request.readOnly());