Bug 77440 - Notify ChromeClient about touch-event handlers
: Notify ChromeClient about touch-event handlers
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-01-31 08:10 PST by
Modified: 2012-02-15 12:21 PST (History)


Attachments
patch (19.53 KB, patch)
2012-01-31 08:15 PST, Sadrul Habib Chowdhury
no flags Review Patch | Details | Formatted Diff | Diff
Patch (15.47 KB, patch)
2012-02-06 00:34 PST, Sadrul Habib Chowdhury
no flags Review Patch | Details | Formatted Diff | Diff
Patch (23.52 KB, patch)
2012-02-06 00:40 PST, Sadrul Habib Chowdhury
no flags Review Patch | Details | Formatted Diff | Diff
Patch (23.89 KB, patch)
2012-02-06 12:16 PST, Sadrul Habib Chowdhury
no flags Review Patch | Details | Formatted Diff | Diff
Patch (24.13 KB, patch)
2012-02-13 15:39 PST, Sadrul Habib Chowdhury
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (23.89 KB, patch)
2012-02-14 17:18 PST, Sadrul Habib Chowdhury
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (23.89 KB, patch)
2012-02-15 10:22 PST, Sadrul Habib Chowdhury
no flags Review Patch | Details | Formatted Diff | Diff
Patch (23.91 KB, patch)
2012-02-15 10:29 PST, Sadrul Habib Chowdhury
no flags Review Patch | Details | Formatted Diff | Diff
Patch (24.06 KB, patch)
2012-02-15 10:36 PST, Sadrul Habib Chowdhury
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2012-01-31 08:15:09 PST -------
Created an attachment (id=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 From 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 From 2012-01-31 08:34:46 PST -------
(From update of attachment 124738 [details])
Attachment 124738 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11354017
------- Comment #4 From 2012-02-06 00:34:43 PST -------
Created an attachment (id=125586) [details]
Patch
------- Comment #5 From 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 From 2012-02-06 00:40:54 PST -------
Created an attachment (id=125588) [details]
Patch
------- Comment #7 From 2012-02-06 11:16:00 PST -------
(From update of attachment 125588 [details])
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 From 2012-02-06 12:14:33 PST -------
(From update of attachment 125588 [details])
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 From 2012-02-06 12:16:27 PST -------
Created an attachment (id=125687) [details]
Patch
------- Comment #10 From 2012-02-06 15:35:43 PST -------
This looks largely reasonable to me minus the absence of tests.
------- Comment #11 From 2012-02-06 15:43:45 PST -------
(From update of attachment 125687 [details])
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 From 2012-02-13 15:39:52 PST -------
Created an attachment (id=126851) [details]
Patch
------- Comment #13 From 2012-02-13 15:40:38 PST -------
(From update of attachment 125687 [details])
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 From 2012-02-13 15:43:02 PST -------
(From update of attachment 126851 [details])
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 From 2012-02-14 16:55:40 PST -------
(From update of attachment 126851 [details])
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 From 2012-02-14 17:18:51 PST -------
Created an attachment (id=127086) [details]
Patch for landing
------- Comment #17 From 2012-02-14 17:19:37 PST -------
(From update of attachment 127086 [details])
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 From 2012-02-14 17:20:18 PST -------
(From update of attachment 126851 [details])
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 From 2012-02-15 10:21:43 PST -------
(From update of attachment 127086 [details])
Please update / add reviewer name.
------- Comment #20 From 2012-02-15 10:22:54 PST -------
Created an attachment (id=127194) [details]
Patch for landing
------- Comment #21 From 2012-02-15 10:23:38 PST -------
(From update of attachment 127194 [details])
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 From 2012-02-15 10:29:49 PST -------
Created an attachment (id=127195) [details]
Patch
------- Comment #23 From 2012-02-15 10:36:24 PST -------
Created an attachment (id=127198) [details]
Patch
------- Comment #24 From 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 From 2012-02-15 12:21:30 PST -------
(From update of attachment 127198 [details])
Clearing flags on attachment: 127198

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