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

[bugzilla-daemon at webkit.org]

Darin Adler <darin 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 191550: Patch
https://bugs.webkit.org/attachment.cgi?id=191550&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=191550&action=review


Idea looks pretty good, but patch still needs work.

Not good that the test fails on Mac, by the way, and we should find out why
before landing.

> Source/WebCore/ChangeLog:27
> +	   (WebCore):

Please remove this bogus line.

> Source/WebCore/ChangeLog:36
> +	   (EventHandler):

Please remove this bogus line.

> Source/WebCore/page/EventHandler.cpp:149
> +// The amount of time to wait for a cursor update on style and layout
changes
> +// Set to 50Hz, no need to be faster than common screen refresh rate
> +const double cursorUpdateInterval = 0.02;

It’s a real shame that all our various timers such as the layout timer, repaint
timer, and cursor update timer are independent. Some day I’d prefer to see a
common scheme that linked them all since they are all about updating what’s
visible on-screen.

> Source/WebCore/page/EventHandler.cpp:1238
> +    Settings* settings = m_frame->settings();
> +    if (settings && !settings->deviceSupportsMouse())
> +	   return;

Why don’t I see deletion of old code that checked deviceSupportsMouse? If this
was copied and pasted from somewhere, we need to refactor so we share that code
instead of copying it.

Not new to this patch, but it’s crazy that this is a “setting”. That’s not the
right way to expose something like that to WebKit.

> Source/WebCore/page/EventHandler.cpp:1245
> +    if (!m_frame->page() || !m_frame->page()->isOnscreen() ||
!m_frame->page()->focusController()->isActive())
> +	   return;

Again, was this rule copied and pasted from somewhere? We don’t just want
another copy.

I’d like to see these checks about when it’s OK to set the cursor bundled up
into a FrameView member function called shouldSetCursor or something like that.
I think I’d include the deviceSupportsMouse check there along with these. I
hope this would cut down on code duplication; not sure where else these checks
already exist and I am not easily able to grep the code at this moment.

> Source/WebCore/page/EventHandler.cpp:1251
> +    bool shiftKey;
> +    bool ctrlKey;
> +    bool altKey;
> +    bool metaKey;
> +    PlatformKeyboardEvent::getCurrentModifierState(shiftKey, ctrlKey,
altKey, metaKey);

It’s not correct to combine the shift key state from the time of cursor
adjustment with the mouse position from m_lastKnownMousePosition. Since we need
shift key state to select the cursor, then we need to store the shift key state
along with the mouse position at the time the mouse position is stored.

> Source/WebCore/page/EventHandler.cpp:1253
> +    m_frame->document()->updateLayoutIgnorePendingStylesheets();

This is wrong. A plain updateLayout would be OK, but there’s no reason for us
to force a flash of unstyled content here, which is what
updateLayoutIgnorePendingStylesheets does.

> Source/WebCore/page/EventHandler.cpp:1257
> +    m_frame->document()->renderView()->hitTest(request, result);

Better to call view->renderView() rather than
m_frame->document()->renderView(). Also, the RenderView can be zero, so we
should do a null check.

> Source/WebCore/page/EventHandler.cpp:1269
> +    if (m_cursorUpdateTimer.isActive())
> +	   m_cursorUpdateTimer.stop();

We can just call stop unconditionally. No real reason to check isActive. Some
other call sites might do it, but they are either incorrect, or trying to
optimize.

This is not the correct place to stop the timer. We want to stop the timer when
we set the cursor, not when we decide what to do set it to. Adding a side
effect to a function that is supposed to tell us what cursor to use is not good
design. This might require a little refactoring of the selectCursor call sites;
maybe we can combine the selectCursor, setCursor, setting m_currentMouseCursor,
and stopping the m_cursorUpdateTimer and use that function in all the
appropriate places.

> Source/WebCore/page/EventHandler.cpp:1291
> -    Node* node = event.targetNode();
> -    RenderObject* renderer = node ? node->renderer() : 0;
> +    Node* node = result.targetNode();
> +    if (!node)
> +	   return NoCursorChange;
> +    bool originalIsText = node->isTextNode();
> +    if (node && originalIsText)
> +	   node = node->parentNode();
> +    if (!node)
> +	   return NoCursorChange;
> +
> +    RenderObject* renderer = node->renderer();

We added a check for a text node. Does this fix a bug? Where is the test case
for the bug it fixes? This fix should go in separately rather than being
coupled with the rest of the change, or rolled out. The change log makes no
mention of this.

> Source/WebCore/page/EventHandler.cpp:1354
> +	   bool editable = (node->rendererIsEditable());

Please remove the parentheses.

> Source/WebCore/page/EventHandler.cpp:1356
> -	   if (useHandCursor(node, event.isOverLink(),
event.event().shiftKey()))
> +	   if (useHandCursor(node, result.URLElement() &&
result.URLElement()->isLink(), shiftKey))

It’s not good that to copy the body of the isOverLink function from
MouseEventWithHitTestResults here, even though it’s simple. We don’t just want
to copy code; we want to reuse it. If we have to make a fix we don’t want to
have to fix it in two places. We could move the isOverLink function into
HitTestResult, then could use it here.

> Source/WebCore/page/EventHandler.cpp:1366
> -	   if ((editable || (renderer && renderer->isText() &&
node->canStartSelection())) && !inResizer && !scrollbar)
> +	   if ((editable || (originalIsText && node->canStartSelection())) &&
!inResizer && !result.scrollbar())

Again, does this fix a bug? Please don’t mix this in with your patch unless
it’s really new to your patch.

> LayoutTests/fast/events/mouse-cursor-no-mousemove.html:17
> +var CURSOR_UPDATE_DELAY = 50;

Not good that this test races the cursor updating code. We should find a more
reliable way to test this.