Bug 42232

Summary: Make changing Cursors work in WebKit2.
Product: WebKit Reporter: Sam Weinig <sam>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric, gustavo, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 42236    
Attachments:
Description Flags
Patch
andersca: review+
Updated patch
none
Patch 3 darin: review+

Description Sam Weinig 2010-07-13 22:05:44 PDT 
Cursors should work in WebKit2.
Comment 1 Sam Weinig 2010-07-13 22:08:00 PDT 
Created attachment 61468 [details]
Patch
Comment 2 WebKit Review Bot 2010-07-13 22:12:41 PDT 
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.
Comment 3 Early Warning System Bot 2010-07-13 22:14:57 PDT 
Attachment 61468 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3448282
Comment 4 WebKit Review Bot 2010-07-13 22:25:23 PDT 
Attachment 61468 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3546035
Comment 5 mitz 2010-07-13 22:37:01 PDT 
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)?
Comment 6 Sam Weinig 2010-07-13 22:38:05 PDT 
Created attachment 61471 [details]
Updated patch
Comment 7 WebKit Review Bot 2010-07-13 22:41:04 PDT 
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.
Comment 8 Sam Weinig 2010-07-13 22:41:37 PDT 
(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.
Comment 9 WebKit Review Bot 2010-07-13 23:03:14 PDT 
Attachment 61471 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3447273
Comment 10 Sam Weinig 2010-07-14 10:17:41 PDT 
Created attachment 61533 [details]
Patch 3
Comment 11 WebKit Review Bot 2010-07-14 10:18:56 PDT 
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.
Comment 12 Darin Adler 2010-07-14 11:01:49 PDT 
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.
Comment 13 Sam Weinig 2010-07-14 11:58:36 PDT 
Landed in http://trac.webkit.org/changeset/63339.