Currently ChromeClient is notified of mouse-wheel event handlers. Add support to notify the client about touch-event handlers too.
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/
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 on attachment 124738 [details] patch Attachment 124738 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11354017
Created attachment 125586 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Created attachment 125588 [details] Patch
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 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?
Created attachment 125687 [details] Patch
This looks largely reasonable to me minus the absence of tests.
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.
Created attachment 126851 [details] Patch
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 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 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
Created attachment 127086 [details] Patch for landing
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 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 on attachment 127086 [details] Patch for landing Please update / add reviewer name.
Created attachment 127194 [details] Patch for landing
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.
Created attachment 127195 [details] Patch
Created attachment 127198 [details] Patch
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 on attachment 127198 [details] Patch Clearing flags on attachment: 127198 Committed r107832: <http://trac.webkit.org/changeset/107832>
All reviewed patches have been landed. Closing bug.