[webkit-reviews] review granted: [Bug 132033] [iOS][WK2] Split iOS touch event dispatch for the regular touch event dispatch : [Attachment 229936] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 132033: [iOS][WK2] Split iOS touch event dispatch for the regular touch
event dispatch
https://bugs.webkit.org/show_bug.cgi?id=132033

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=229936&action=review


> Source/WebKit2/UIProcess/WebPageProxy.cpp:1515
> +    m_process->responsivenessTimer()->start();
> +    bool handled = false;
> +    m_process->sendSync(Messages::WebPage::TouchEventSync(event),
Messages::WebPage::TouchEventSync::Reply(handled), m_pageID);
> +    didReceiveEvent(event.type(), handled);
> +    m_pageClient.doneWithTouchEvent(event, handled);
> +    m_process->responsivenessTimer()->stop();

Would be nice to have a RAII class for start/stop.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1531
> +    m_process->send(Messages::EventDispatcher::TouchEvent(m_pageID, event),
0);

What is the last param?

> Source/WebKit2/UIProcess/WebPageProxy.h:698
> +    void handleSynchronousTouchEvent(const NativeWebTouchEvent&);

handleTouchEventSynchronously?

> Source/WebKit2/UIProcess/WebPageProxy.h:699
> +    void sendAsynchronousTouchEvent(const NativeWebTouchEvent&);

handleTouchEventAsynchronously?

> Source/WebKit2/WebProcess/WebPage/EventDispatcher.cpp:177
> +	       TouchEventQueue& queuedEvents = addResult.iterator->value;
> +	       ASSERT(!queuedEvents.isEmpty());
> +	       const WebTouchEvent& lastTouchEvent = queuedEvents.last();
> +	       WebEvent::Type type = lastTouchEvent.type();
> +	       if (type == WebEvent::TouchMove)
> +		   queuedEvents.last() = touchEvent;

Not quite sure what you are doing here. Coalescing move events? Can you add a
comment or name the variables to clarify?

> Source/WebKit2/WebProcess/WebPage/EventDispatcher.h:77
> +    void touchEvent(uint64_t pageID, const WebTouchEvent&);

I wish we had a typedef for pageID

> Source/WebKit2/WebProcess/WebPage/EventDispatcher.h:100
> +    // FIXME: verify what is the right value for the inline capacity.

Do this?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2062
> +void WebPage::dispatchAsynchronousTouchEventsQueue(const
Vector<WebTouchEvent, 5>& queue)

You don't dispatch a queue, so dispatchAsynchronousTouchEventsInQueue or
dispatchAsynchronousTouchEvents.