[webkit-reviews] review denied: [Bug 57731] [CSS 3] missing cursor support for 'none' on Windows : [Attachment 104642] Revised patch addressing comments.

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has denied Ryan <aikavanak at gmail.com>'s
request for review:
Bug 57731: [CSS 3] missing cursor support for 'none' on Windows
https://bugs.webkit.org/show_bug.cgi?id=57731

Attachment 104642: Revised patch addressing comments.
https://bugs.webkit.org/attachment.cgi?id=104642&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=104642&action=review


> Source/WebKit2/UIProcess/win/WebView.cpp:800
> +    return handled? TRUE: FALSE;

WebKit coding style says to put spaces on both sides of ? and :, like this:
return handled ? TRUE : FALSE;

> Source/WebKit2/UIProcess/win/WebView.cpp:1033
> +    static const HCURSOR arrowCursor = ::LoadCursor(0, IDC_ARROW);
> +    const HCURSOR webCoreNativeCursor =
m_webCoreCursor.platformCursor()->nativeCursor();

We tend not to use "const" in this manner. We normally only used const for
objects, not pointers. (I.e., we'll say "const int* foo" but not "int* const
foo", which is essentially what "const HCURSOR" means.) So I'd remove the
"const"s here.

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

Since we know the exact conditions in which we expect to get a null HCURSOR, I
think we should assert about it in addition to this bail out:

ASSERT(m_lastCursorSet || m_webCoreCursor.type() == Cursor::None ||
!m_page->isValid());
if (m_lastCursorSet && m_webCoreCursor.type() != Cursor::None)
    return false;

> Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:808
> +	   ASSERT(platformCursor || (!platformCursor && (cursor.type() ==
Cursor::None)));

This can be simplified to: ASSERT(platformCursor || cursor.type() ==
Cursor::None)