Bug 94524 - Remove redundant TOUCH_LISTENER event type
Summary: Remove redundant TOUCH_LISTENER event type
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Klein
URL:
Keywords:
Depends on:
Blocks: 94016
  Show dependency treegraph
 
Reported: 2012-08-20 14:05 PDT by Adam Klein
Modified: 2012-08-22 06:40 PDT (History)
7 users (show)

See Also:


Attachments
Patch (26.10 KB, patch)
2012-08-20 14:20 PDT, Adam Klein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Klein 2012-08-20 14:05:51 PDT
Remove redundant TOUCH_LISTENER event type
Comment 1 Adam Klein 2012-08-20 14:20:12 PDT
Created attachment 159517 [details]
Patch
Comment 2 Ryosuke Niwa 2012-08-20 14:34:00 PDT
Comment on attachment 159517 [details]
Patch

Okay.
Comment 3 WebKit Review Bot 2012-08-20 15:24:54 PDT
Comment on attachment 159517 [details]
Patch

Clearing flags on attachment: 159517

Committed r126080: <http://trac.webkit.org/changeset/126080>
Comment 4 WebKit Review Bot 2012-08-20 15:24:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Antonio Gomes 2012-08-21 05:12:19 PDT
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?
Comment 6 Ojan Vafai 2012-08-21 09:45:57 PDT
Looks to me like it should still work. See Document::didAddTouchEventHandler, which still sets needs touch-events on the Page.
Comment 7 Adam Klein 2012-08-21 10:42:19 PDT
(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().
Comment 8 Antonio Gomes 2012-08-22 06:40:29 PDT
> 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.