Bug 91006 – Unify numTouchEventHandlersChanged and needTouchEvents in the chrome client

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Pierre Rossi]

Unify numTouchEventHandlersChanged and needTouchEvents in the chrome client

[Pierre Rossi]

Created attachment 151766 [details]
Patch

[Pierre Rossi]

Created attachment 151767 [details]
Speculative patch for Chromium

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Pierre Rossi]

Created attachment 151769 [details]
Patch

previous patch was missing some uncommited chunks.

[Ryosuke Niwa]

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?

[Pierre Rossi]

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.

[WebKit Review Bot]

Comment on attachment 151769 [details]
Patch

Attachment 151769 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13200431

[Pierre Rossi]

Created attachment 151929 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Pierre Rossi]

Created attachment 151953 [details]
Patch

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Comment on attachment 151953 [details]
Patch

That is, r- for now.

[Pierre Rossi]

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.

[Pierre Rossi]

Created attachment 152754 [details]
Patch

[Ryosuke Niwa]

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?

[Pierre Rossi]

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.

[Sadrul Habib Chowdhury]

(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!

[Sadrul Habib Chowdhury]

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

[Ryosuke Niwa]

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

[Pierre Rossi]

Comment on attachment 152754 [details]
Patch

chromium_rev seems to be at 147759 in DEPS, should be safe to land.

[WebKit Review Bot]

Comment on attachment 152754 [details]
Patch

Clearing flags on attachment: 152754

Committed r123354: <http://trac.webkit.org/changeset/123354>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.