Unify numTouchEventHandlersChanged and needTouchEvents in the chrome client
Created attachment 151766 [details] Patch
Created attachment 151767 [details] Speculative patch for Chromium
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Created attachment 151769 [details] Patch previous patch was missing some uncommited chunks.
Comment on attachment 151769 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151769&action=review > Source/WebCore/dom/Document.cpp:5926 > + m_listenerTypes &= ~TOUCH_LISTENER; Where did this code come from? > Source/WebCore/dom/Document.cpp:5928 > + if (Page* page = this->page()) > + page->chrome()->client()->needTouchEvents(false); What if we had multiple documents (with touch event handlers) in the page?
Comment on attachment 151769 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151769&action=review >> Source/WebCore/dom/Document.cpp:5926 >> + m_listenerTypes &= ~TOUCH_LISTENER; > > Where did this code come from? Mhh, just trying to make Document::hasListenerType more reliable for TOUCH, since it's used in other places for that purpose. Worthy of an inline comment maybe ? >> Source/WebCore/dom/Document.cpp:5928 >> + page->chrome()->client()->needTouchEvents(false); > > What if we had multiple documents (with touch event handlers) in the page? Good point. I guess I might need to keep the logic at the Frame level just for that.
Comment on attachment 151769 [details] Patch Attachment 151769 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13200431
Created attachment 151929 [details] Patch
Comment on attachment 151929 [details] Patch Attachment 151929 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13182794 New failing tests: fast/events/touch/touch-handler-count.html
Created attachment 151948 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 151953 [details] Patch
Comment on attachment 151953 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151953&action=review The patch looks reasonable now. Please consider adding a API test (Tools/WebKitTestAPI for m_listenerTypes change). > Source/WebCore/ChangeLog:18 > + flag for the document if needed. We don't indent comments like this. > Source/WebCore/dom/Document.cpp:5913 > + if (m_touchEventHandlerCount > 1) Nit: it should read if (m_touchEventHandlerCount) since m_touchEventHandlerCount is unsigned. > Source/WebCore/dom/Document.cpp:5928 > + // Since hasListenerType() is relied upon in other places, we should try to keep it accurate. > + m_listenerTypes &= ~TOUCH_LISTENER; It seems like we ought to be able to write a API test for this. Also, the comment here doesn't belong in the code; it should be in the change log instead since you're describing why you're making this change. > Source/WebCore/page/Frame.cpp:315 > +#if ENABLE(TOUCH_EVENTS) > + if (m_page && m_page->mainFrame() == this && m_doc && m_doc->hasListenerType(Document::TOUCH_LISTENER)) > + m_page->chrome()->client()->needTouchEvents(true); > +#endif I would have preferred nesting this inside the if statement above.
Comment on attachment 151953 [details] Patch That is, r- for now.
Comment on attachment 151953 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151953&action=review >> Source/WebCore/dom/Document.cpp:5913 >> + if (m_touchEventHandlerCount > 1) > > Nit: it should read if (m_touchEventHandlerCount) since m_touchEventHandlerCount is unsigned. wouldn't it be if (m_touchEventHandlerCount - 1) in that case ? I personally find it less readable. >> Source/WebCore/dom/Document.cpp:5928 >> + m_listenerTypes &= ~TOUCH_LISTENER; > > It seems like we ought to be able to write a API test for this. > Also, the comment here doesn't belong in the code; it should be in the change log instead since you're describing why you're making this change. After a quick discussion with Benjamin, I settled for just exposing the hasListenerType logic through the Internals. I started having concerns when I had to export 4 additional functions in WebCore.exp.in, especially since we already expose the count through Internals. >> Source/WebCore/page/Frame.cpp:315 >> +#endif > > I would have preferred nesting this inside the if statement above. Fair enough.
Created attachment 152754 [details] Patch
Comment on attachment 152754 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152754&action=review > Source/WebCore/dom/Document.cpp:5958 > + if (Page* page = this->page()) > + page->chrome()->client()->needTouchEvents(true); Shouldn't we check that we had not already called needTouchEvents with true by going through frames?
Comment on attachment 152754 [details] Patch Is there a way we can synchronize for landing this so as not to break the chromium build for too long ? View in context: https://bugs.webkit.org/attachment.cgi?id=152754&action=review >> Source/WebCore/dom/Document.cpp:5958 >> + page->chrome()->client()->needTouchEvents(true); > > Shouldn't we check that we had not already called needTouchEvents with true by going through frames? With the early return above, only when a document gets its first touch event handler will this happen, I'm not sure which one is more expensive in practice really.
(In reply to comment #17) > (From update of attachment 152754 [details]) > Is there a way we can synchronize for landing this so as not to break the chromium build for too long ? I have put up https://chromiumcodereview.appspot.com/10797043/ for review. If the chromium patch lands before this webkit patch, then chromium won't be broken. (there will need to be another patch shortly thereafter to cleanup the old code, I can take care of that too) Thanks for doing this!
(In reply to comment #18) > (In reply to comment #17) > > (From update of attachment 152754 [details] [details]) > > Is there a way we can synchronize for landing this so as not to break the chromium build for too long ? > > I have put up https://chromiumcodereview.appspot.com/10797043/ for review. If the chromium patch lands before this webkit patch, then chromium won't be broken. (there will need to be another patch shortly thereafter to cleanup the old code, I can take care of that too) The chromium patch has landed. So this can now land at any time without breaking anything.
(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #17) > > > (From update of attachment 152754 [details] [details] [details]) > > > Is there a way we can synchronize for landing this so as not to break the chromium build for too long ? > > > > I have put up https://chromiumcodereview.appspot.com/10797043/ for review. If the chromium patch lands before this webkit patch, then chromium won't be broken. (there will need to be another patch shortly thereafter to cleanup the old code, I can take care of that too) > > The chromium patch has landed. So this can now land at any time without breaking anything. We need to roll Chromium DEPS in WebKit first.
Comment on attachment 152754 [details] Patch chromium_rev seems to be at 147759 in DEPS, should be safe to land.
Comment on attachment 152754 [details] Patch Clearing flags on attachment: 152754 Committed r123354: <http://trac.webkit.org/changeset/123354>
All reviewed patches have been landed. Closing bug.