[webkit-reviews] review denied: [Bug 77440] Notify ChromeClient about touch-event handlers : [Attachment 125588] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied sadrul at chromium.org's request for
review:
Bug 77440: Notify ChromeClient about touch-event handlers
https://bugs.webkit.org/show_bug.cgi?id=77440

Attachment 125588: Patch
https://bugs.webkit.org/attachment.cgi?id=125588&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=125588&action=review


> Source/WebCore/ChangeLog:8
> +	   No new tests. (OOPS!)

This patch is missing tests.

> Source/WebCore/dom/Document.cpp:5362
> +    Frame* mainFrame = page() ? page()->mainFrame() : 0;

The usually idiom here is m_frame->tree()->top()

> Source/WebCore/page/Frame.cpp:1030
> +void Frame::notifyChromeClientTouchEventHandlerCountChanged() const

Why not just have an accessor to query for the number of touch event handlers
rather than having to actively compute the value for each notification?  If the
embedder doesn't care, this is a waste of time.

> Source/WebCore/page/Frame.h:198
> +	   void notifyChromeClientTouchEventHandlerCountChanged() const;

Also, if a frame is removed from the document, the number of touch event
handlers changes, but we don't seem to generate a notification.  (It's
possible/likely that notifyChromeClientWheelEventHandlerCountChanged has
similar problems.)