Bug 42232 – Make changing Cursors work in WebKit2.

• Component: WebKit2
• Status: RESOLVED FIXED

[Sam Weinig]

Cursors should work in WebKit2.

[Sam Weinig]

Created attachment 61468 [details]
Patch

[WebKit Review Bot]

Attachment 61468 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Shared/WebCoreArgumentCoders.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/Cursor.cpp:52:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/platform/Cursor.cpp:147:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/platform/win/CursorWin.cpp:143:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 4 in 54 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 61468 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3448282

[WebKit Review Bot]

Attachment 61468 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3546035

Comment on attachment 61468 [details]
Patch

> +        - Rework Windows cursor code to use less global variables.

fewer?

> Index: WebCore/page/Chrome.h
> ===================================================================
> --- WebCore/page/Chrome.h	(revision 63102)
> +++ WebCore/page/Chrome.h	(working copy)
> @@ -69,6 +69,7 @@ namespace WebCore {
>          virtual IntRect windowToScreen(const IntRect&) const;
>          virtual PlatformPageClient platformPageClient() const;
>          virtual void scrollbarsModeDidChange() const;
> +        virtual void setCursor(const Cursor&);

Does this method need to be virtual?

> +        PlatformCursor impl() const { return m_platformCursor; }

I’d rename this method to platformCursor() as well.

> +    const char* nameForCursorType(Cursor::Type);

How is this going to be used? Should it return 0 instead of the constant string "ERROR"?

> -    return [[NSCursor alloc] initWithImage:img hotSpot:determineHotSpot(image, hotSpot)];
> +    return [[NSCursor alloc] initWithImage:img hotSpot:hotSpot];

You should explain in the change log how hotSpot changed from being an out parameter in this function (and its caller) to being an in parameter.

> +Cursor::Cursor(Image* image, const IntPoint& hotSpot)

> +Cursor::Cursor(Type type)

> +Cursor::Cursor(const Cursor& other)

> +Cursor& Cursor::operator=(const Cursor& other)

> +PlatformCursor Cursor::platformCursor() const

These methods have identical implementations in CursorMac.mm and CursorWin.cpp. Can they be shared in Cursor.cpp inside #if USE(LAZY_NATIVE_CURSOR)?

>  void Widget::setCursor(const Cursor& cursor)

I think this method has identical implementations in WidgetMac.mm and WidgetWin.cpp. Can they be shared in Widget.cpp inside #if USE(LAZY_NATIVE_CURSOR)?

[Sam Weinig]

Created attachment 61471 [details]
Updated patch

[WebKit Review Bot]

Attachment 61471 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/wx/CursorWx.cpp:45:  Should have a space between // and comment  [whitespace/comments] [4]
WebCore/platform/Cursor.cpp:52:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/platform/Cursor.cpp:147:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/platform/win/CursorWin.cpp:143:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 4 in 61 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

(In reply to comment #5)
> (From update of attachment 61468 [details])
> > +        - Rework Windows cursor code to use less global variables.
> 
> fewer?

Will change.

> 
> > Index: WebCore/page/Chrome.h
> > ===================================================================
> > --- WebCore/page/Chrome.h	(revision 63102)
> > +++ WebCore/page/Chrome.h	(working copy)
> > @@ -69,6 +69,7 @@ namespace WebCore {
> >          virtual IntRect windowToScreen(const IntRect&) const;
> >          virtual PlatformPageClient platformPageClient() const;
> >          virtual void scrollbarsModeDidChange() const;
> > +        virtual void setCursor(const Cursor&);
> 
> Does this method need to be virtual?

Yes, since it is from HostWindow.

> 
> > +        PlatformCursor impl() const { return m_platformCursor; }
> 
> I’d rename this method to platformCursor() as well.

I would, but that would require changing a lot of places.  I can do it in a follow up though.

> > +    const char* nameForCursorType(Cursor::Type);
> 
> How is this going to be used? Should it return 0 instead of the constant string "ERROR"?

I used it for debugging.  Perhaps I should remove it.

> > -    return [[NSCursor alloc] initWithImage:img hotSpot:determineHotSpot(image, hotSpot)];
> > +    return [[NSCursor alloc] initWithImage:img hotSpot:hotSpot];
> 
> You should explain in the change log how hotSpot changed from being an out parameter in this function (and its caller) to being an in parameter.

Ok.

> 
> > +Cursor::Cursor(Image* image, const IntPoint& hotSpot)
> 
> > +Cursor::Cursor(Type type)
> 
> > +Cursor::Cursor(const Cursor& other)
> 
> > +Cursor& Cursor::operator=(const Cursor& other)
> 
> > +PlatformCursor Cursor::platformCursor() const
> 
> These methods have identical implementations in CursorMac.mm and CursorWin.cpp. Can they be shared in Cursor.cpp inside #if USE(LAZY_NATIVE_CURSOR)?

Yes!

> 
> >  void Widget::setCursor(const Cursor& cursor)
> 
> I think this method has identical implementations in WidgetMac.mm and WidgetWin.cpp. Can they be shared in Widget.cpp inside #if USE(LAZY_NATIVE_CURSOR)?

They don't unfortunately, the windows one has bit extra.

[WebKit Review Bot]

Attachment 61471 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3447273

[Sam Weinig]

Created attachment 61533 [details]
Patch 3

[WebKit Review Bot]

Attachment 61533 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/wx/CursorWx.cpp:45:  Should have a space between // and comment  [whitespace/comments] [4]
WebCore/platform/Cursor.cpp:52:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/platform/Cursor.cpp:147:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/platform/win/CursorWin.cpp:146:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 4 in 63 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 61533 [details]
Patch 3

> +    switch (type) {
> +        case Cursor::Pointer:
> +            return pointerCursor();

Wrong indentation style.

> +const char* nameForCursorType(Cursor::Type type)
> +{
> +    switch (type) {
> +        case Cursor::Pointer:
> +            return "Pointer";

Same.

> +const Cursor& pointerCursor()
> +{
> +    DEFINE_STATIC_LOCAL(Cursor, c, (Cursor::Pointer));
> +    return c;
> +}

How about using the word "cursor" or "staticLocalCursor" instead of "c"?

> +    switch (m_type) {
> +        case Cursor::Pointer:
> +            m_platformCursor = HardRetain([NSCursor arrowCursor]);
> +            break;

Switch style again.

> +            m_platformCursor = HardRetain(leakNamedCursor("crossHairCursor", 11, 11));

Given the word leak in the name of this function, it seems wrong or unnecessary to do another HardRetain.

> +    switch (m_type) {
> +        case Cursor::Pointer:

Another switch statement.

> +    LRESULT onSetCursor(HWND hWnd, UINT message, WPARAM, LPARAM, bool& handled);

Why does the HWND argument need names in all of these?

Otherwise looks good.

[Sam Weinig]

Landed in http://trac.webkit.org/changeset/63339.

[WebKit Review Bot]

http://trac.webkit.org/changeset/63339 might have broken Chromium Mac Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/63339
http://trac.webkit.org/changeset/63340
http://trac.webkit.org/changeset/63341
http://trac.webkit.org/changeset/63342