[webkit-reviews] review denied: [Bug 101857] Updating mouse cursor on style changes without emitting fake mousemove event : [Attachment 188525] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Aivo Paas
<aivopaas at gmail.com>'s request for review:
Bug 101857: Updating mouse cursor on style changes without emitting fake
mousemove event
https://bugs.webkit.org/show_bug.cgi?id=101857

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=188525&action=review


> Source/WebCore/page/EventHandler.cpp:1245
> +    m_cursorTimer.stop();

You don't need to stop the timer here.

> Source/WebCore/page/EventHandler.h:283
> +    void cursorTimerFired(Timer<EventHandler>*);

cursorUpdateTimerFired.

> Source/WebCore/page/EventHandler.h:411
> +    Timer<EventHandler> m_cursorTimer;

This should be called m_cursorUpdateTimer

> Source/WebCore/rendering/RenderObject.cpp:1813
> +    if (oldStyle.get() && !areCursorsEqual(oldStyle.get(), this->style())) {

> +	   if (Frame* frame = this->frame())
> +	       frame->eventHandler()->scheduleCursorUpdate();
> +    }

Why not leave this in styleDidChange like the old code? setStyle() should not
be polluted with  stuff like this. The hunk above would also then not be
required.

I'm still not convinced that this is the correct place to do this. I think
cursor updates should really happen after layout, and a zero-delay timer after
a style change doesn't necessarily mean that layout has happened. Some style
changes won't result in layout (just painting), so it's OK to update the cursor
then. But others do, and the layout may be delayed by layout timers in
FrameView, in which case we should also delay the cursor update.