Bug 94524 – Remove redundant TOUCH_LISTENER event type

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

[Adam Klein]

Remove redundant TOUCH_LISTENER event type

[Adam Klein]

Created attachment 159517 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 159517 [details]
Patch

Okay.

[WebKit Review Bot]

Comment on attachment 159517 [details]
Patch

Clearing flags on attachment: 159517

Committed r126080: <http://trac.webkit.org/changeset/126080>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antonio Gomes]

Comment on attachment 159517 [details]
Patch

The BlackBerry was checking the ChromeClient 'needs touch event' notification to a whole logic we have to handle touch events, if there is no listeners attached to any document in the page. ChromeClient::needTouchEvent used to be updated when any touch event was attached to any document in the page, but now it is only called by CachedFrame::restore and  Frame::setDocument.

Now you changed that logic and made the notification useless (from a brief look at the patch), and if so it breaks the behavior that we had. Please correct me if I am wrong.

To make use of Document::touchEventHandlerCount(), I have to manually recursively check all documents in the page, right?

[Ojan Vafai]

Looks to me like it should still work. See Document::didAddTouchEventHandler, which still sets needs touch-events on the Page.

[Adam Klein]

(In reply to comment #6)
> Looks to me like it should still work. See Document::didAddTouchEventHandler, which still sets needs touch-events on the Page.

It should be pointed out that didAddTouchEventHandler is called by both Node::addEventListener and DOMWindow::addEventListener, in just the same cases that addListenerTypeIfNeeded() used to call it (that is, when EventNames::isTouchEventType() is true). It may be that addListenerTypeIfNeeded should be made responsible for calling didAddTouchEventHandler; that would be a fine followup change, imho. The main point of this change was to get rid of the TOUCH_LISTENER enum which doesn't match up with the usage of the other ListenerType enums.

I will say that there was one functional change here: it used to be that every call to addEventListener() with a touch event would call into ChromeClient, whereas now we only call for the _first_ event handler on the page. From my archaeology of the patches that added this functionality, that seemed to fit the use cases, and it does for Chromium (which simply sets a bool on the browser side when the first touch listener is added). But if some ChromeClients want to be notified every time, the proper was is to modify the logic in didAddTouchEventHandler, since that catches more cases than addEventListenerTypeIfNeeded().

[Antonio Gomes]

> I will say that there was one functional change here: it used to be that every call to addEventListener() with a touch event would call into ChromeClient, whereas now we only call for the _first_ event handler on the page. 

That is ok. Thanks guys.