[Webkit-unassigned] [Bug 101857] Updating mouse cursor on style changes without emitting fake mousemove event

Tue Mar 5 14:31:53 PST 2013

https://bugs.webkit.org/show_bug.cgi?id=101857

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #191550|review?                     |review-
               Flag|                            |

--- Comment #116 from Darin Adler <darin at apple.com>  2013-03-05 14:34:17 PST ---
(From update of attachment 191550)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.