Bug 100550

Summary: No tests for changing mouse cursors
Product: WebKit Reporter: Rick Byers <rbyers>
Component: CSSAssignee: Rick Byers <rbyers>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aivopaas, darin, gustavo, gyuyoung.kim, hayato, philn, rakuco, tony, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 101876    
Bug Blocks: 53341, 101225, 85343, 99493, 100059, 101501, 101857    
Attachments:
Description Flags
Patch
none
Patch
none
Fix mac failure
none
Explicitly expect failure on non-chromium platforms for now
none
Update to ToT
none
Make tests port-agnostic
none
Add export
none
Ignore - wrong bug
none
More exports
none
One more export for gtk
none
Merge with ToT
none
Patch for landing
none
Patch for landing
none
Patch
none
Just the changes from the originally landed version
none
Just the changes from the originally landed version none

Description Rick Byers 2012-10-26 12:00:25 PDT
No tests for changing mouse cursors
Comment 1 Rick Byers 2012-10-26 12:05:50 PDT
Whenever we change/extend support for custom mouse cursors, we don't add any tests because there is no machinery for them.  Eg. in http://trac.webkit.org/changeset/96566 darin@ wrote 'No tests because we currently don't have any test machinery for cursors.'.

This bug tracks adding some minimal tests for changing mouse cursors.  I'm not going to attempt to cover all the scenarios and different edge cases here, but we need to stop the bleeding so that there's no excuse not to add more tests in the future.

It looks like this should probably be done in a port-specific fashion so we can see the effects of any port-specific code (eg. see chromium's WebCursor).  See bug 99493 for more discussion.
Comment 2 Rick Byers 2012-10-26 12:10:30 PDT
Created attachment 170982 [details]
Patch
Comment 3 Alexey Proskuryakov 2012-10-26 14:47:37 PDT
There is a number of tests in ManualTests, can these be converted using this machinery?
Comment 4 Rick Byers 2012-10-26 21:03:03 PDT
(In reply to comment #3)
> There is a number of tests in ManualTests, can these be converted using this machinery?

Thanks, I didn't know about those.  Yes it looks easy to cover all those cases in this test.  There is, of course, some additional coverage from the manual tests (eg. embedder and os code, image appearance, etc.) but it's easy to get most of the value.  In particular:

cur-hotspot.html: I intended to add this case when fixing it in chromium - bug 19059
cursor-empty-url.html: trivial, I'll add it
cursor-maxsize.html: I cover most of this already, I can add the few extra variations but not sure it has any real value.  This test also seems to expect behavior that doesn't match the code - I'll check a couple ports.
cursor.html: This exhaustively lists all standard cursors.  I considered doing this but decided it wasn't worth it - but maybe it is.
cursorfallback.xml: looks easy to add svg cases - I'll do it
custom-cursors.html: already covered here
Comment 5 Rick Byers 2012-10-29 07:09:10 PDT
(In reply to comment #4)
> cursor-maxsize.html: I cover most of this already, I can add the few extra variations but not sure it has any real value.  This test also seems to expect behavior that doesn't match the code - I'll check a couple ports.

I've confirmed this test doesn't currently pass on Safari on Mac (we get the help cursor).  It expects that hotspots outside the boundary of the cursor will make the entire cursor invalid.  This probably isn't reasonable since we won't necessarily have loaded the image by the time we're choosing which cursor to use.  Current behavior for safari port is to just ignore the hotspot (fall back to 0,0).  On chromium we instead snap the hotspot to the image boundaries but I'll probably change that to be consistent with safari in bug 100059.  

Also, this  manual test incorrectly passes (shows no custom cursor) on chromium, apparently because we don't support TIFF images for cursors.  All the sites I can find on-line recommend CUR, PNG, GIF, etc cursors not tiff - so this is probably a pretty bogus test anyway.
Comment 6 Rick Byers 2012-10-29 13:59:34 PDT
Created attachment 171307 [details]
Patch
Comment 7 Rick Byers 2012-10-29 14:07:32 PDT
A couple more manual tests I missed:

canvas-cursor.html - this is really cool, but it's explicitly looking for flickering which I can't test here.  It also doesn't work at all on chromium for some reason, and doesn't animate smoothly on safari unless you're actively moving the mouse (we change the cursor on mouse events).

css3-cursor-fallback-quirks.html/css3-cursor-fallback-strict.html: I've included a couple of the relevant cases here.  I haven't attempted here to test the subtle differences between quirks and strict mode.  Some of this difference appears to no longer exist (parsing of hotspots is done in both cases) - neither of these tests seems to pass completely on current safari builds.

link-cursor-auto.html - interesting regression case, I've added it
Comment 8 Build Bot 2012-10-29 14:32:23 PDT
Comment on attachment 171307 [details]
Patch

Attachment 171307 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14620544

New failing tests:
fast/events/mouse-cursor.html
Comment 9 Rick Byers 2012-10-30 11:08:59 PDT
Created attachment 171485 [details]
Fix mac failure
Comment 10 Build Bot 2012-10-30 13:01:46 PDT
Comment on attachment 171485 [details]
Fix mac failure

Attachment 171485 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14629636

New failing tests:
fast/events/mouse-cursor.html
Comment 11 Rick Byers 2012-11-05 09:00:04 PST
Created attachment 172347 [details]
Explicitly expect failure on non-chromium platforms for now
Comment 12 Alexey Proskuryakov 2012-11-05 10:19:14 PST
Comment on attachment 172347 [details]
Explicitly expect failure on non-chromium platforms for now

View in context: https://bugs.webkit.org/attachment.cgi?id=172347&action=review

The latest patch doesn't apply, so EWS didn't test it.

> Tools/ChangeLog:10
> +        * DumpRenderTree/chromium/DRTTestRunner.cpp:

How much platform logic is in the code path you are testing? It might be meaningful to put getCurrentCursorInfo in Internals if you are mostly testing WebCore.
Comment 13 Rick Byers 2012-11-05 10:43:01 PST
Created attachment 172360 [details]
Update to ToT
Comment 14 Rick Byers 2012-11-05 10:48:20 PST
(In reply to comment #12)
> (From update of attachment 172347 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172347&action=review
> 
> The latest patch doesn't apply, so EWS didn't test it.

Thanks.  TestExpectations change often I guess.

> > Tools/ChangeLog:10
> > +        * DumpRenderTree/chromium/DRTTestRunner.cpp:
> 
> How much platform logic is in the code path you are testing? It might be meaningful to put getCurrentCursorInfo in Internals if you are mostly testing WebCore.

I debated this in some detail in bug 99493.  There isn't a ton of port-specific code, but there is some and it is precisely that code which I need to test in bugs like bug 100059.  So I definitely need to add some port-specific hook here, the only question is if it's worth having both mechanisms and testing at both layers.  Thoughts?
Comment 15 WebKit Review Bot 2012-11-05 10:50:05 PST
Attachment 172360 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/platform/gtk/TestExpectations:1315:  Path does not exist.  [test/expectations] [5]
LayoutTests/platform/mac/TestExpectations:804:  Path does not exist.  [test/expectations] [5]
LayoutTests/platform/win/TestExpectations:1999:  Path does not exist.  [test/expectations] [5]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Rick Byers 2012-11-05 11:09:42 PST
(In reply to comment #15)
> Attachment 172360 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
> LayoutTests/platform/gtk/TestExpectations:1315:  Path does not exist.  [test/expectations] [5]
> LayoutTests/platform/mac/TestExpectations:804:  Path does not exist.  [test/expectations] [5]
> LayoutTests/platform/win/TestExpectations:1999:  Path does not exist.  [test/expectations] [5]
> Total errors found: 3 in 11 files

This is failing upstream at the moment (these aren't the lines I added).
Comment 17 Adam Barth 2012-11-05 13:57:20 PST
Comment on attachment 172347 [details]
Explicitly expect failure on non-chromium platforms for now

View in context: https://bugs.webkit.org/attachment.cgi?id=172347&action=review

> Tools/DumpRenderTree/chromium/DRTTestRunner.cpp:2306
> +static const char* cursorTypeToString(WebCursorInfo::Type cursorType)

This function is kind of ugly.  Will we need to replicate it for every port?
Comment 18 Adam Barth 2012-11-05 14:00:49 PST
Looks like the enum comes from Cursor.h eventually.  If we need to test port-specific behavior, as you wrote in Comment #14, I wonder if we can avoid replicating this blob of code somehow...  One approach is to try implementing this for another port to see how it might work there.
Comment 19 EFL EWS Bot 2012-11-05 16:22:48 PST
Comment on attachment 172360 [details]
Update to ToT

Attachment 172360 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14721850
Comment 20 Rick Byers 2012-11-06 14:02:04 PST
(In reply to comment #18)
> Looks like the enum comes from Cursor.h eventually.  If we need to test port-specific behavior, as you wrote in Comment #14, I wonder if we can avoid replicating this blob of code somehow...  One approach is to try implementing this for another port to see how it might work there.

Thanks Adam.  I've started to look at implementing this for the mac port as well.  Ideally I think we'd share more than just this function - we could also share some of the logic for constructing the cursor info string (just pass in the values, have common string formatting).

Unless I'm missing something, it looks like we don't currently share much code between the mac DRT and chromium DRT.  Is it OK to just add this stuff to DumpRenderTreeCommon.cpp?
Comment 21 Adam Barth 2012-11-06 15:02:13 PST
We don't share much code.  The question is more of a design question.  On the surface, this TestRunner method seems very tightly coupled with the Chromium WebKit API.  A better design would be one that we could run on many different ports, either because the functionality was part of Internals or because the TestRunner method made sense.
Comment 22 Adam Barth 2012-11-06 15:02:44 PST
Comment on attachment 172360 [details]
Update to ToT

We should be able to avoid the giant switch statement.
Comment 23 Rick Byers 2012-11-07 12:29:33 PST
In terms of the port-specific nature of the testing: It looks like most/all other ports have USE(LAZY_NATIVE_CURSOR), which gives a Cursor object which can be tested in a port-agnostic fashion.

Without this, Cursor is entirely port-specific (just a wrapper around a port-specific PlatformCursor type), and so testing would have to be completely port-specific (really doesn't make sense to try to share test code across ports).

Talking offline with Adam, I'm going to look into switching chromium to use the LAZY_NATIVE_CURSOR path just for the sake of unification.  See bug 101501.
Comment 24 Rick Byers 2012-11-08 12:51:18 PST
Created attachment 173100 [details]
Make tests port-agnostic
Comment 25 Rick Byers 2012-11-08 12:53:37 PST
This new patch uses the same basic approach, except that it adds an API to internals.idl that works for all ports that have USE(LAZY_NATIVE_CURSOR) (nearly all of them).  I talked with Adam offline about using a formatting string and the need for a big switch statement to convert the enum to a string, and he said as long as it's not in chromium-specific code then it's fine.

Adam, how does this look now?
Comment 26 Build Bot 2012-11-08 14:02:37 PST
Comment on attachment 173100 [details]
Make tests port-agnostic

Attachment 173100 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14765648
Comment 27 Build Bot 2012-11-08 14:24:15 PST
Comment on attachment 173100 [details]
Make tests port-agnostic

Attachment 173100 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14765653
Comment 28 Rick Byers 2012-11-08 19:03:14 PST
Created attachment 173174 [details]
Add export
Comment 29 Rick Byers 2012-11-08 19:08:29 PST
Created attachment 173175 [details]
Ignore - wrong bug
Comment 30 Build Bot 2012-11-08 19:38:32 PST
Comment on attachment 173174 [details]
Add export

Attachment 173174 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14763862
Comment 31 Rick Byers 2012-11-09 05:51:12 PST
Created attachment 173281 [details]
More exports
Comment 32 Rick Byers 2012-11-09 06:45:52 PST
Created attachment 173297 [details]
One more export for gtk
Comment 33 Rick Byers 2012-11-09 14:33:56 PST
Created attachment 173368 [details]
Merge with ToT
Comment 34 Adam Barth 2012-11-09 14:48:21 PST
Comment on attachment 173368 [details]
Merge with ToT

View in context: https://bugs.webkit.org/attachment.cgi?id=173368&action=review

> Source/WebCore/testing/Internals.cpp:1531
> +    return "UNKNOWN";

This should be ASSERT_NOT_REACHED(), right?

> Source/WebCore/testing/Internals.cpp:1545
> +    String result = String("type=") + cursorTypeToString(cursor.type())

You should use StringBuilder because you're got a conditional in here.
Comment 35 Rick Byers 2012-11-09 15:29:15 PST
Created attachment 173383 [details]
Patch for landing
Comment 36 Rick Byers 2012-11-09 16:28:21 PST
Created attachment 173395 [details]
Patch for landing
Comment 37 WebKit Review Bot 2012-11-09 18:45:54 PST
Comment on attachment 173395 [details]
Patch for landing

Clearing flags on attachment: 173395

Committed r134144: <http://trac.webkit.org/changeset/134144>
Comment 38 WebKit Review Bot 2012-11-09 18:46:02 PST
All reviewed patches have been landed.  Closing bug.
Comment 39 WebKit Review Bot 2012-11-11 18:12:41 PST
Re-opened since this is blocked by bug 101876
Comment 40 Aivo Paas 2012-11-11 23:41:21 PST
Spotted missing braces.

@@ -1862,7 +1862,8 @@ bool EventHandler::handleMouseMoveEvent(const PlatformMouseEvent& mouseEvent, Hi
         if (FrameView* view = m_frame->view()) {
             OptionalCursor optionalCursor = selectCursor(mev, scrollbar);
             if (optionalCursor.isCursorChange())
-                view->setCursor(optionalCursor.cursor());
+                m_currentMouseCursor = optionalCursor.cursor();
+                view->setCursor(m_currentMouseCursor);
         }
     }
Comment 41 Rick Byers 2012-11-12 06:57:29 PST
(In reply to comment #40)
> Spotted missing braces.
> 
> @@ -1862,7 +1862,8 @@ bool EventHandler::handleMouseMoveEvent(const PlatformMouseEvent& mouseEvent, Hi
>          if (FrameView* view = m_frame->view()) {
>              OptionalCursor optionalCursor = selectCursor(mev, scrollbar);
>              if (optionalCursor.isCursorChange())
> -                view->setCursor(optionalCursor.cursor());
> +                m_currentMouseCursor = optionalCursor.cursor();
> +                view->setCursor(m_currentMouseCursor);
>          }
>      }

Dammit, thank you!
Comment 42 Rick Byers 2012-11-12 07:19:20 PST
(In reply to comment #41)
> (In reply to comment #40)
> > Spotted missing braces.
> > 
> > @@ -1862,7 +1862,8 @@ bool EventHandler::handleMouseMoveEvent(const PlatformMouseEvent& mouseEvent, Hi
> >          if (FrameView* view = m_frame->view()) {
> >              OptionalCursor optionalCursor = selectCursor(mev, scrollbar);
> >              if (optionalCursor.isCursorChange())
> > -                view->setCursor(optionalCursor.cursor());
> > +                m_currentMouseCursor = optionalCursor.cursor();
> > +                view->setCursor(m_currentMouseCursor);
> >          }
> >      }
> 
> Dammit, thank you!

I'm just figuring out how to add a test case for the no-cursor-change scenario (don't want to rely on chromium browser tests here), and then I'll resubmit with that new case.

Sorry for the trouble this stupid error caused.
Comment 43 Darin Adler 2012-11-12 10:30:52 PST
Comment on attachment 173395 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=173395&action=review

> Source/WebCore/testing/Internals.cpp:1530
> +    case Cursor::Pointer: return "TypePointer";
> +    case Cursor::Cross: return "TypeCross";
> +    case Cursor::Hand: return "TypeHand";
> +    case Cursor::IBeam: return "TypeIBeam";
> +    case Cursor::Wait: return "TypeWait";
> +    case Cursor::Help: return "TypeHelp";
> +    case Cursor::EastResize: return "TypeEastResize";
> +    case Cursor::NorthResize: return "TypeNorthResize";
> +    case Cursor::NorthEastResize: return "TypeNorthEastResize";
> +    case Cursor::NorthWestResize: return "TypeNorthWestResize";
> +    case Cursor::SouthResize: return "TypeSouthResize";
> +    case Cursor::SouthEastResize: return "TypeSouthEastResize";
> +    case Cursor::SouthWestResize: return "TypeSouthWestResize";
> +    case Cursor::WestResize: return "TypeWestResize";
> +    case Cursor::NorthSouthResize: return "TypeNorthSouthResize";
> +    case Cursor::EastWestResize: return "TypeEastWestResize";
> +    case Cursor::NorthEastSouthWestResize: return "TypeNorthEastSouthWestResize";
> +    case Cursor::NorthWestSouthEastResize: return "TypeNorthWestSouthEastResize";
> +    case Cursor::ColumnResize: return "TypeColumnResize";
> +    case Cursor::RowResize: return "TypeRowResize";
> +    case Cursor::MiddlePanning: return "TypeMiddlePanning";
> +    case Cursor::EastPanning: return "TypeEastPanning";
> +    case Cursor::NorthPanning: return "TypeNorthPanning";
> +    case Cursor::NorthEastPanning: return "TypeNorthEastPanning";
> +    case Cursor::NorthWestPanning: return "TypeNorthWestPanning";
> +    case Cursor::SouthPanning: return "TypeSouthPanning";
> +    case Cursor::SouthEastPanning: return "TypeSouthEastPanning";
> +    case Cursor::SouthWestPanning: return "TypeSouthWestPanning";
> +    case Cursor::WestPanning: return "TypeWestPanning";
> +    case Cursor::Move: return "TypeMove";
> +    case Cursor::VerticalText: return "TypeVerticalText";
> +    case Cursor::Cell: return "TypeCell";
> +    case Cursor::ContextMenu: return "TypeContextMenu";
> +    case Cursor::Alias: return "TypeAlias";
> +    case Cursor::Progress: return "TypeProgress";
> +    case Cursor::NoDrop: return "TypeNoDrop";
> +    case Cursor::Copy: return "TypeCopy";
> +    case Cursor::None: return "TypeNone";
> +    case Cursor::NotAllowed: return "TypeNotAllowed";
> +    case Cursor::ZoomIn: return "TypeZoomIn";
> +    case Cursor::ZoomOut: return "TypeZoomOut";
> +    case Cursor::Grab: return "TypeGrab";
> +    case Cursor::Grabbing: return "TypeGrabbing";
> +    case Cursor::Custom: return "TypeCustom";

Could we drop all these Type prefixes?
Comment 44 Rick Byers 2012-11-15 08:16:13 PST
Created attachment 174444 [details]
Patch
Comment 45 Rick Byers 2012-11-15 08:17:00 PST
(In reply to comment #43)
> 
> Could we drop all these Type prefixes?

Done.  Sorry, those were a hold-over from when this code was chromium specific (the Chromium version of that enum uses Type prefixes).
Comment 46 Rick Byers 2012-11-15 08:18:24 PST
Created attachment 174446 [details]
Just the changes from the originally landed version
Comment 47 Rick Byers 2012-11-15 08:22:45 PST
The stupid error I made (missing braces) was a little hard to write a direct test for, because it resulted in a Cursor object with an uninitialized field (unpredictable value).

So instead I added an explicit initialization and ASSERTs to the code to check that such uninitialized Cursor objects are never used.  I verified that (before fixing the missing braces) some existing WebKit layout tests failed due to these ASSERTs.  In particular, plugins/resize-from-plugin.html hits the rare NoCursorChange case, and hit this ASSERT until I added the missing braces.

I've attached both a new full patch, as well as just the diff from what I previously landed.  Adam, can you please review?  Sorry again for missing this stupid typo.
Comment 48 Rick Byers 2012-11-15 08:24:51 PST
Created attachment 174448 [details]
Just the changes from the originally landed version
Comment 49 Brent Fulgham 2012-11-15 10:27:35 PST
Comment on attachment 174444 [details]
Patch

r=me.  Rollout if the tests fail again.
Comment 50 WebKit Review Bot 2012-11-15 11:15:51 PST
Comment on attachment 174444 [details]
Patch

Clearing flags on attachment: 174444

Committed r134803: <http://trac.webkit.org/changeset/134803>
Comment 51 WebKit Review Bot 2012-11-15 11:15:59 PST
All reviewed patches have been landed.  Closing bug.