Bug 77440 – Notify ChromeClient about touch-event handlers

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Sadrul Habib Chowdhury]

Currently ChromeClient is notified of mouse-wheel event handlers. Add support to notify the client about touch-event handlers too.

[Sadrul Habib Chowdhury]

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/

[WebKit Review Bot]

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.

[Philippe Normand]

Comment on attachment 124738 [details]
patch

Attachment 124738 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11354017

[Sadrul Habib Chowdhury]

Created attachment 125586 [details]
Patch

[WebKit Review Bot]

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

[Sadrul Habib Chowdhury]

Created attachment 125588 [details]
Patch

[Adam Barth]

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.)

[Sadrul Habib Chowdhury]

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?

[Sadrul Habib Chowdhury]

Created attachment 125687 [details]
Patch

[Robert Kroeger]

This looks largely reasonable to me minus the absence of tests.

[Ryosuke Niwa]

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.

[Sadrul Habib Chowdhury]

Created attachment 126851 [details]
Patch

[Sadrul Habib Chowdhury]

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.

[Ryosuke Niwa]

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.

[Darin Fisher (:fishd, Google)]

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

[Sadrul Habib Chowdhury]

Created attachment 127086 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 127086 [details]
Patch for landing

Rejecting attachment 127086 [details] from commit-queue.

[e-mail] 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.

[Sadrul Habib Chowdhury]

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)

[Ryosuke Niwa]

Comment on attachment 127086 [details]
Patch for landing

Please update / add reviewer name.

[Sadrul Habib Chowdhury]

Created attachment 127194 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 127194 [details]
Patch for landing

Rejecting attachment 127194 [details] from commit-queue.

[e-mail] 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.

[Sadrul Habib Chowdhury]

Created attachment 127195 [details]
Patch

[Sadrul Habib Chowdhury]

Created attachment 127198 [details]
Patch

[WebKit Review Bot]

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.

[WebKit Review Bot]

Comment on attachment 127198 [details]
Patch

Clearing flags on attachment: 127198

Committed r107832: <http://trac.webkit.org/changeset/107832>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.