[Webkit-unassigned] [Bug 57731] [CSS 3] missing cursor support for 'none' on Windows

[bugzilla-daemon at webkit.org]

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


Adam Roben (:aroben) <aroben at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #109410|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #19 from Adam Roben (:aroben) <aroben at apple.com>  2011-12-21 12:14:14 PST ---
(From update of attachment 109410)
View in context: https://bugs.webkit.org/attachment.cgi?id=109410&action=review

Sorry for the ridiculously long delay in reviewing this! I think you're really close!

> Source/WebKit2/UIProcess/win/WebView.cpp:1048
> +    // Only remove the cursor from the screen (by passing a null parameter
> +    // to ::SetCursor) if the WebCore cursor is of type None.
> +    ASSERT(m_lastCursorSet || m_webCoreCursor.type() == Cursor::None || !m_page->isValid());
> +    if (m_lastCursorSet && m_webCoreCursor.type() != Cursor::None)
> +        return false;

The code doesn't seem to match the comment. Code that matches the comment would be:
ASSERT(m_lastCursorSet || m_webCoreCursor.type() == Cursor::None || !m_page->isValid());
if (!m_lastCursorSet && m_webCoreCursor.type() != Cursor::None)
    return false;

Looks like we can get rid of m_lastCursorSet entirely now. It's only used within this function!

I think this would all be a bit clearer if this function bailed out immediately at the start if the page isn't valid. So something like this:

if (!m_page->isValid())
    return false;

HCURSOR cursor = cursorToShow();
ASSERT(!cursor == (m_webCoreCursor.type() == Cursor::None));
::SetCursor(cursor);
return true;

> Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:808
> +        // A null platformCursor should only be used to represent Cursor::None.
> +        // Catch other invalid usages here.
> +        ASSERT(platformCursor || cursor.type() == Cursor::None);

I think it would be slightly better to put this assertion just after line 794. You could even use ASSERT_ARG to get a slightly clearer error message:

ASSERT_ARG(cursor, platformCursor || cursor.type() == Cursor::None);

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