Bug 78014 - [chromium] Pepper Plugins don't receive mouse movement information in non-mouse-lock mode.
Summary: [chromium] Pepper Plugins don't receive mouse movement information in non-mou...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: yzshen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-07 11:54 PST by yzshen
Modified: 2012-02-14 00:55 PST (History)
3 users (show)

See Also:


Attachments
Patch (1.58 KB, patch)
2012-02-07 13:33 PST, yzshen
no flags Details | Formatted Diff | Diff
Patch (1.95 KB, patch)
2012-02-13 14:53 PST, yzshen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yzshen 2012-02-07 11:54:59 PST
WebMouseEventBuilder doesn't pass along mouse movement information properly.
Comment 1 yzshen 2012-02-07 13:33:08 PST
Created attachment 125916 [details]
Patch
Comment 2 Vincent Scheib 2012-02-07 14:03:39 PST
LGTM, Though I'm not a reviewer.
Comment 3 Tony Chang 2012-02-07 14:13:36 PST
Is it possible to write a test for this?  Maybe we have one in chromium?
Comment 4 yzshen 2012-02-07 14:20:39 PST
(In reply to comment #3)
> Is it possible to write a test for this?  Maybe we have one in chromium?

For now, we have a manual test at the chromium side.
(I agree that automatic tests are better. :))
Comment 5 Tony Chang 2012-02-07 15:07:40 PST
(In reply to comment #4)
> (In reply to comment #3)
> > Is it possible to write a test for this?  Maybe we have one in chromium?
> 
> For now, we have a manual test at the chromium side.
> (I agree that automatic tests are better. :))

Is it possible to write a layout test for this?
Comment 6 yzshen 2012-02-07 15:09:52 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Is it possible to write a test for this?  Maybe we have one in chromium?
> > 
> > For now, we have a manual test at the chromium side.
> > (I agree that automatic tests are better. :))
> 
> Is it possible to write a layout test for this?

I will look into it. Thanks!
Comment 7 Eric Seidel (no email) 2012-02-07 15:48:27 PST
Comment on attachment 125916 [details]
Patch

How can we test this?
Comment 8 Eric Seidel (no email) 2012-02-07 15:49:20 PST
Comment on attachment 125916 [details]
Patch

Ah, I see now that test investigation is underway.  Marking r- to remove from the queue.  Feel free to remark r? if you find testing impossible/impractical (ideally you'd update the changelog to say such).
Comment 9 yzshen 2012-02-13 14:28:17 PST
Hi, Tony and Eric.

I took a look and think that it is probably not easy/useful to add a test for this change:
- there isn't any test for WebMouseEventBuilder right now; moreover, the movement information added is only used by pepper, and there isn't any test for pepper at the WebKit side. (AFAIK, WebKit doesn't know anything about pepper.)
- this change is trivial: it copies two newly-added data fields from MouseEvent to WebMouseEventBuilder during conversion.

Please let me know if you think it is okay to commit without a test. (And I will be happy to make changes if you know how to add a test.)
Comment 10 Tony Chang 2012-02-13 14:33:23 PST
(In reply to comment #9)
> Hi, Tony and Eric.
> 
> I took a look and think that it is probably not easy/useful to add a test for this change:
> - there isn't any test for WebMouseEventBuilder right now; moreover, the movement information added is only used by pepper, and there isn't any test for pepper at the WebKit side. (AFAIK, WebKit doesn't know anything about pepper.)
> - this change is trivial: it copies two newly-added data fields from MouseEvent to WebMouseEventBuilder during conversion.
> 
> Please let me know if you think it is okay to commit without a test. (And I will be happy to make changes if you know how to add a test.)

I think it's fine to land the change without a test.  Thanks for looking into why it doesn't make sense.  Please put the reason for no layout test in the ChangeLog.
Comment 11 yzshen 2012-02-13 14:53:12 PST
Created attachment 126843 [details]
Patch
Comment 12 yzshen 2012-02-13 14:54:36 PST
Thanks, Tony!

Uploaded a patch with revised ChangeLog.
Comment 13 Tony Chang 2012-02-13 15:15:24 PST
BTW, it doesn't really matter that the change is trivial.  One reason to have layout tests is to verify that there are no regressions in the future and even trivial parts of the code base can regress (e.g., when refactoring).
Comment 14 WebKit Review Bot 2012-02-14 00:55:51 PST
Comment on attachment 126843 [details]
Patch

Clearing flags on attachment: 126843

Committed r107684: <http://trac.webkit.org/changeset/107684>
Comment 15 WebKit Review Bot 2012-02-14 00:55:55 PST
All reviewed patches have been landed.  Closing bug.