| Summary: | [chromium] Pepper Plugins don't receive mouse movement information in non-mouse-lock mode. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | yzshen | ||||||
| Component: | UI Events | Assignee: | yzshen | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | scheib, tony, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
yzshen
2012-02-07 11:54:59 PST
Created attachment 125916 [details]
Patch
LGTM, Though I'm not a reviewer. Is it possible to write a test for this? Maybe we have one in chromium? (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. :)) (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? (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 on attachment 125916 [details]
Patch
How can we test this?
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).
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.) (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. Created attachment 126843 [details]
Patch
Thanks, Tony! Uploaded a patch with revised ChangeLog. 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 on attachment 126843 [details] Patch Clearing flags on attachment: 126843 Committed r107684: <http://trac.webkit.org/changeset/107684> All reviewed patches have been landed. Closing bug. |