Bug 77440 - Notify ChromeClient about touch-event handlers
: Notify ChromeClient about touch-event handlers
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Sadrul Habib Chowdhury
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-31 08:10 PST by Sadrul Habib Chowdhury
Modified: 2012-02-15 12:21 PST (History)
14 users (show)

See Also:


Attachments
patch (19.53 KB, patch)
2012-01-31 08:15 PST, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
Patch (15.47 KB, patch)
2012-02-06 00:34 PST, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
Patch (23.52 KB, patch)
2012-02-06 00:40 PST, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
Patch (23.89 KB, patch)
2012-02-06 12:16 PST, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
Patch (24.13 KB, patch)
2012-02-13 15:39 PST, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
Patch for landing (23.89 KB, patch)
2012-02-14 17:18 PST, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
Patch for landing (23.89 KB, patch)
2012-02-15 10:22 PST, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
Patch (23.91 KB, patch)
2012-02-15 10:29 PST, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
Patch (24.06 KB, patch)
2012-02-15 10:36 PST, Sadrul Habib Chowdhury
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 2012-01-31 08:10:16 PST
Currently ChromeClient is notified of mouse-wheel event handlers. Add support to notify the client about touch-event handlers too.
Comment 1 Sadrul Habib Chowdhury 2012-01-31 08:15:09 PST
Created attachment 124738 [details]
patch

I followed the code that deals with mouse-wheel events, and added corresponding code for touch events. Please take a look. The chromium side of this patch is at: http://codereview.chromium.org/9233058/
Comment 2 WebKit Review Bot 2012-01-31 08:21:54 PST
Attachment 124738 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Philippe Normand 2012-01-31 08:34:46 PST
Comment on attachment 124738 [details]
patch

Attachment 124738 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11354017
Comment 4 Sadrul Habib Chowdhury 2012-02-06 00:34:43 PST
Created attachment 125586 [details]
Patch
Comment 5 WebKit Review Bot 2012-02-06 00:38:55 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 6 Sadrul Habib Chowdhury 2012-02-06 00:40:54 PST
Created attachment 125588 [details]
Patch
Comment 7 Adam Barth 2012-02-06 11:16:00 PST
Comment on attachment 125588 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

This patch is missing tests.

> Source/WebCore/dom/Document.cpp:5362
> +    Frame* mainFrame = page() ? page()->mainFrame() : 0;

The usually idiom here is m_frame->tree()->top()

> Source/WebCore/page/Frame.cpp:1030
> +void Frame::notifyChromeClientTouchEventHandlerCountChanged() const

Why not just have an accessor to query for the number of touch event handlers rather than having to actively compute the value for each notification?  If the embedder doesn't care, this is a waste of time.

> Source/WebCore/page/Frame.h:198
> +        void notifyChromeClientTouchEventHandlerCountChanged() const;

Also, if a frame is removed from the document, the number of touch event handlers changes, but we don't seem to generate a notification.  (It's possible/likely that notifyChromeClientWheelEventHandlerCountChanged has similar problems.)
Comment 8 Sadrul Habib Chowdhury 2012-02-06 12:14:33 PST
Comment on attachment 125588 [details]
Patch

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

>> Source/WebCore/page/Frame.cpp:1030
>> +void Frame::notifyChromeClientTouchEventHandlerCountChanged() const
> 
> Why not just have an accessor to query for the number of touch event handlers rather than having to actively compute the value for each notification?  If the embedder doesn't care, this is a waste of time.

The count of the touch-event handlers go all the way to the client/embedder (i.e. to the browser process over IPC). Chromium does not actually care about the actual number of handlers, it just cares whether there is any handlers or not. So I think not having to count the number of handlers would be OK. However, I have intentionally kept this new code very similar to the existing code for wheel-event handlers. And it probably makes sense to keep them similar if possible. I will be happy to change both pieces of code to not do the exhaustive counting, and use a boolean to notify the existence of event handlers.

>> Source/WebCore/page/Frame.h:198
>> +        void notifyChromeClientTouchEventHandlerCountChanged() const;
> 
> Also, if a frame is removed from the document, the number of touch event handlers changes, but we don't seem to generate a notification.  (It's possible/likely that notifyChromeClientWheelEventHandlerCountChanged has similar problems.)

I notice that I missed a call to notifyChromeClientTouchEventHandlerCountChanged in Frame::setDocument. I have corrected this to make sure this is called from there (notifyChromeClientWheelEventHandlerCountChanged is already called in there). I believe this takes care of this?
Comment 9 Sadrul Habib Chowdhury 2012-02-06 12:16:27 PST
Created attachment 125687 [details]
Patch
Comment 10 Robert Kroeger 2012-02-06 15:35:43 PST
This looks largely reasonable to me minus the absence of tests.
Comment 11 Ryosuke Niwa 2012-02-06 15:43:45 PST
Comment on attachment 125687 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Please at least explain what kind of changes you're making and why you're making them. In particular, please explain how new client callback is used.
r- due to the lack of explanation.

> Source/WebCore/dom/Node.cpp:2508
> +        else if (eventType == eventNames().touchstartEvent || eventType == eventNames().touchmoveEvent || eventType == eventNames().touchendEvent || eventType == eventNames().touchcancelEvent)

Please wrap this into an inline function e.g. isTouchEvent(event).

> Source/WebCore/dom/Node.cpp:2559
> +        else if (eventType == eventNames().touchstartEvent || eventType == eventNames().touchmoveEvent || eventType == eventNames().touchendEvent || eventType == eventNames().touchcancelEvent)

Ditto.

> Source/WebCore/loader/EmptyClients.h:237
> +    virtual void numTouchEventHandlersChanged(unsigned) { }

Please use OVERRIDE keyword.
Comment 12 Sadrul Habib Chowdhury 2012-02-13 15:39:52 PST
Created attachment 126851 [details]
Patch
Comment 13 Sadrul Habib Chowdhury 2012-02-13 15:40:38 PST
Comment on attachment 125687 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> Please at least explain what kind of changes you're making and why you're making them. In particular, please explain how new client callback is used.
> r- due to the lack of explanation.

Added an explanation.

>> Source/WebCore/dom/Node.cpp:2508
>> +        else if (eventType == eventNames().touchstartEvent || eventType == eventNames().touchmoveEvent || eventType == eventNames().touchendEvent || eventType == eventNames().touchcancelEvent)
> 
> Please wrap this into an inline function e.g. isTouchEvent(event).

Done. I have added this in EventNames, instead of adding a static function in Node.cpp. I am not entirely sure if it fits in there, though.

>> Source/WebCore/loader/EmptyClients.h:237
>> +    virtual void numTouchEventHandlersChanged(unsigned) { }
> 
> Please use OVERRIDE keyword.

I have added the keyword just for these two functions. It looks like the keyword would apply to a number of other functions in here. But I have left them alone in this patch.
Comment 14 Ryosuke Niwa 2012-02-13 15:43:02 PST
Comment on attachment 126851 [details]
Patch

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

> Source/WebCore/dom/EventNames.h:226
> +        bool isTouchEventType(const AtomicString& eventType)
> +        {
> +            return eventType == touchstartEvent || eventType == touchmoveEvent || eventType == touchendEvent || eventType == touchcancelEvent;
> +        }

This function is only used in EventHandler.cpp. Please move it there.
Comment 15 Darin Fisher (:fishd, Google) 2012-02-14 16:55:40 PST
Comment on attachment 126851 [details]
Patch

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

> Source/WebKit/chromium/public/WebViewClient.h:266
> +    virtual void numberOfTouchEventHandlersChanged(unsigned) { }

LGTM
Comment 16 Sadrul Habib Chowdhury 2012-02-14 17:18:51 PST
Created attachment 127086 [details]
Patch for landing
Comment 17 WebKit Review Bot 2012-02-14 17:19:37 PST
Comment on attachment 127086 [details]
Patch for landing

Rejecting attachment 127086 [details] from commit-queue.

sadrul@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 18 Sadrul Habib Chowdhury 2012-02-14 17:20:18 PST
Comment on attachment 126851 [details]
Patch

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

>> Source/WebCore/dom/EventNames.h:226
>> +        }
> 
> This function is only used in EventHandler.cpp. Please move it there.

Done! (in Node.cpp)
Comment 19 Ryosuke Niwa 2012-02-15 10:21:43 PST
Comment on attachment 127086 [details]
Patch for landing

Please update / add reviewer name.
Comment 20 Sadrul Habib Chowdhury 2012-02-15 10:22:54 PST
Created attachment 127194 [details]
Patch for landing
Comment 21 WebKit Review Bot 2012-02-15 10:23:38 PST
Comment on attachment 127194 [details]
Patch for landing

Rejecting attachment 127194 [details] from commit-queue.

sadrul@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 22 Sadrul Habib Chowdhury 2012-02-15 10:29:49 PST
Created attachment 127195 [details]
Patch
Comment 23 Sadrul Habib Chowdhury 2012-02-15 10:36:24 PST
Created attachment 127198 [details]
Patch
Comment 24 WebKit Review Bot 2012-02-15 10:38:37 PST
Attachment 127198 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2

Updating OpenSource
git.webkit.org[0: 17.254.20.231]: errno=Connection refused
fatal: unable to connect a socket (Connection refused)
Died at Tools/Scripts/update-webkit line 162.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 WebKit Review Bot 2012-02-15 12:21:30 PST
Comment on attachment 127198 [details]
Patch

Clearing flags on attachment: 127198

Committed r107832: <http://trac.webkit.org/changeset/107832>
Comment 26 WebKit Review Bot 2012-02-15 12:21:37 PST
All reviewed patches have been landed.  Closing bug.