Bug 66272 - [chromium] DRT: Add support for sending scroll-update events from EventSender
Summary: [chromium] DRT: Add support for sending scroll-update events from EventSender
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Varun Jain
URL:
Keywords:
Depends on:
Blocks: 67822
  Show dependency treegraph
 
Reported: 2011-08-15 20:09 PDT by Sadrul Habib Chowdhury
Modified: 2011-10-28 16:44 PDT (History)
5 users (show)

See Also:


Attachments
patch (13.76 KB, patch)
2011-08-15 20:22 PDT, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
patch (14.31 KB, patch)
2011-08-15 20:29 PDT, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
patch (14.38 KB, patch)
2011-08-18 16:00 PDT, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
Patch (15.20 KB, patch)
2011-10-28 15:27 PDT, Robert Kroeger
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sadrul Habib Chowdhury 2011-08-15 20:09:23 PDT
Add support for sending scroll-update events from EventSender.
Comment 1 Sadrul Habib Chowdhury 2011-08-15 20:22:19 PDT
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.
Comment 2 WebKit Review Bot 2011-08-15 20:25:12 PDT
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.
Comment 3 Sadrul Habib Chowdhury 2011-08-15 20:29:38 PDT
Created attachment 104000 [details]
patch
Comment 4 Darin Fisher (:fishd, Google) 2011-08-15 20:52:25 PDT
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
Comment 5 Robert Kroeger 2011-08-16 08:34:16 PDT
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..."?
Comment 6 Darin Fisher (:fishd, Google) 2011-08-16 09:53:47 PDT
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.
Comment 7 Sadrul Habib Chowdhury 2011-08-16 10:06:36 PDT
(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.
Comment 8 Sadrul Habib Chowdhury 2011-08-18 16:00:09 PDT
Created attachment 104418 [details]
patch

Addressed the nit from Rob. The first patch has landed (r93358). So this is ready for review.
Comment 9 Robert Kroeger 2011-10-25 10:41:55 PDT
This change looks good to me.

Adam: can you review?
Comment 10 Adam Barth 2011-10-25 10:44:43 PDT
Comment on attachment 104418 [details]
patch

I see that fishd already approved the API change.
Comment 11 Adam Barth 2011-10-25 10:45:15 PDT
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.
Comment 12 Robert Kroeger 2011-10-25 10:52:31 PDT
(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
Comment 13 WebKit Review Bot 2011-10-25 12:12:39 PDT
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
Comment 14 Robert Kroeger 2011-10-28 15:27:52 PDT
Created attachment 112920 [details]
Patch
Comment 15 WebKit Review Bot 2011-10-28 15:30:15 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 16 WebKit Review Bot 2011-10-28 16:44:45 PDT
Comment on attachment 112920 [details]
Patch

Clearing flags on attachment: 112920

Committed r98782: <http://trac.webkit.org/changeset/98782>
Comment 17 WebKit Review Bot 2011-10-28 16:44:52 PDT
All reviewed patches have been landed.  Closing bug.