Bug 66272 – [chromium] DRT: Add support for sending scroll-update events from EventSender

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Sadrul Habib Chowdhury]

Add support for sending scroll-update events from EventSender.

[Sadrul Habib Chowdhury]

Created attachment 103998 [details]
patch

Please note that the new layout test will pass once https://bugs.webkit.org/show_bug.cgi?id=66267 lands.

[WebKit Review Bot]

Attachment 103998 [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

Tools/DumpRenderTree/chromium/EventSender.cpp:1061:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 12 files


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

[Sadrul Habib Chowdhury]

Created attachment 104000 [details]
patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 104000 [details]
patch

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

> Source/WebKit/chromium/public/WebInputEvent.h:106
> +        GestureScrollUpdate,

API changes LGTM

[Robert Kroeger]

Comment on attachment 104000 [details]
patch

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

LGTM. (But I'm not a reviewer.)

> LayoutTests/fast/events/touch/gesture/gesture-scroll-expected.txt:6
> +Gesture manager not implemented on this platform or broken

nit: Maybe this should be "Gesture events not implemented..."?

[Darin Fisher (:fishd, Google)]

Just FYI in case... It looks like you aren't looking for an official review on this patch because you have not requested a review by setting the '?' flag.

[Sadrul Habib Chowdhury]

(In reply to comment #6)
> Just FYI in case... It looks like you aren't looking for an official review on this patch because you have not requested a review by setting the '?' flag.

Indeed. I was hoping to have the first patch to be reviewed first (https://bugs.webkit.org/show_bug.cgi?id=66267) since the layout test added in this patch depends on that.

[Sadrul Habib Chowdhury]

Created attachment 104418 [details]
patch

Addressed the nit from Rob. The first patch has landed (r93358). So this is ready for review.

[Robert Kroeger]

This change looks good to me.

Adam: can you review?

[Adam Barth]

Comment on attachment 104418 [details]
patch

I see that fishd already approved the API change.

[Adam Barth]

Comment on attachment 104418 [details]
patch

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

> Tools/DumpRenderTree/chromium/EventSender.cpp:1077
> +    default:
> +        ASSERT_NOT_REACHED();

One nit: WebKit style is to leave off the default in these cases and have the compiler complain when we forget one.

[Robert Kroeger]

(In reply to comment #11)
> (From update of attachment 104418 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104418&action=review
> 
> > Tools/DumpRenderTree/chromium/EventSender.cpp:1077
> > +    default:
> > +        ASSERT_NOT_REACHED();
> 
> One nit: WebKit style is to leave off the default in these cases and have the compiler complain when we forget one.

I'll fix when I add another PGE subtype to DumpRenderTree

[WebKit Review Bot]

Comment on attachment 104418 [details]
patch

Rejecting attachment 104418 [details] from commit-queue.

New failing tests:
fast/events/touch/gesture/gesture-scroll.html
Full output: http://queues.webkit.org/results/10212277

[Robert Kroeger]

Created attachment 112920 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[WebKit Review Bot]

Comment on attachment 112920 [details]
Patch

Clearing flags on attachment: 112920

Committed r98782: <http://trac.webkit.org/changeset/98782>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.