Summary: | Remove redundant TOUCH_LISTENER event type | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Klein <adamk> | ||||
Component: | New Bugs | Assignee: | Adam Klein <adamk> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | benjamin, mifenton, ojan, rniwa, tkent, tonikitoo, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 94016 | ||||||
Attachments: |
|
Description
Adam Klein
2012-08-20 14:05:51 PDT
Created attachment 159517 [details]
Patch
Comment on attachment 159517 [details]
Patch
Okay.
Comment on attachment 159517 [details] Patch Clearing flags on attachment: 159517 Committed r126080: <http://trac.webkit.org/changeset/126080> All reviewed patches have been landed. Closing bug. 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?
Looks to me like it should still work. See Document::didAddTouchEventHandler, which still sets needs touch-events on the Page. (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().
> 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.
|