Bug 34215 – Need functionality in the ChromeClient to be informed when touch events are and are not needed by the webpage.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Ben Murdoch]

We have an optimisation on Android that allows us to tell the platform to stop forwarding touch events to WebCore if the current web page has not registered a touch event listener.

I would like to contribute this upstream to reduce our diff.

Patch to follow shortly.

[Ben Murdoch]

Created attachment 47531 [details]
Proposed Patch.

[Eric Seidel (no email)]

Attachment 47531 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/215110

[Ben Murdoch]

Comment on attachment 47531 [details]
Proposed Patch.

Will upload a new patch.

[Ben Murdoch]

Created attachment 47535 [details]
Take 2.

[Simon Hausmann]

Adjusted bug title as per recent webkit-dev discussion, to make it clear that this isn't really port specific.

[Simon Hausmann]

Comment on attachment 47535 [details]
Take 2.

I don't feel comfortable enough with these parts of the WebCore code to fully review this patch :-(, but I noticed one thing that looks suspicious :-)

> @@ -100,6 +106,10 @@ void CachedFrameBase::restore()
>          m_childFrames[i]->open();
>  
>      m_document->dispatchWindowEvent(PageTransitionEvent::create(eventNames().pageshowEvent, true), m_document);
> +#if ENABLE(TOUCH_EVENTS)
> +    if (m_document->hasListenerType(Document::TOUCH_LISTENER))
> +        m_document->page()->chrome()->client()->needTouchEvents(true);
> +#endif
>  }
>  
>  CachedFrame::CachedFrame(Frame* frame)
> @@ -145,6 +155,11 @@ CachedFrame::CachedFrame(Frame* frame)
>      else
>          LOG(PageCache, "Finished creating CachedFrame for child frame with url '%s' and DocumentLoader %p\n", m_url.string().utf8().data(), m_documentLoader.get());
>  #endif
> +
> +#if ENABLE(TOUCH_EVENTS)
> +    if (m_document->hasListenerType(Document::TOUCH_LISTENER))
> +        m_document->page()->chrome()->client()->needTouchEvents(false);
> +#endif
>  }

In these two hunks, shouldn't the logic be the other way around? If there are touch listeners, then the client _does_ need touch events.

[Ben Murdoch]

(In reply to comment #6)

Thanks for taking a look at this Simon!

> (From update of attachment 47535 [details])
> I don't feel comfortable enough with these parts of the WebCore code to fully
> review this patch :-(, but I noticed one thing that looks suspicious :-)
> 
> > @@ -100,6 +106,10 @@ void CachedFrameBase::restore()
> >          m_childFrames[i]->open();
> >  
> >      m_document->dispatchWindowEvent(PageTransitionEvent::create(eventNames().pageshowEvent, true), m_document);
> > +#if ENABLE(TOUCH_EVENTS)
> > +    if (m_document->hasListenerType(Document::TOUCH_LISTENER))
> > +        m_document->page()->chrome()->client()->needTouchEvents(true);
> > +#endif
> >  }
> >  
> >  CachedFrame::CachedFrame(Frame* frame)
> > @@ -145,6 +155,11 @@ CachedFrame::CachedFrame(Frame* frame)
> >      else
> >          LOG(PageCache, "Finished creating CachedFrame for child frame with url '%s' and DocumentLoader %p\n", m_url.string().utf8().data(), m_documentLoader.get());
> >  #endif
> > +
> > +#if ENABLE(TOUCH_EVENTS)
> > +    if (m_document->hasListenerType(Document::TOUCH_LISTENER))
> > +        m_document->page()->chrome()->client()->needTouchEvents(false);
> > +#endif
> >  }
> 
> In these two hunks, shouldn't the logic be the other way around? If there are
> touch listeners, then the client _does_ need touch events.

In the first hunk, the code is added to the restore() method on CachedFrame, i.e. when we are pulling a page from the page cache to show it again. As we don't reinstall the event listeners (they are there already and part of the cached frame's document state) we need to tell the ChromeClient to start sending touch events again if the document uses them.

In the second hunk we create a CachedFrame ready to put it into the PageCache and so the platform can *stop* sending us touch events if the document is using them (and we will turn them on again when the CachedFrame is restore()d.

I think it's the correct behavior.

Cheers, Ben

[Simon Hausmann]

(In reply to comment #7)

> I think it's the correct behavior.

Ahh, you're fully right. I misread the code as if it was calling needsTouchEvents() with "false" in both cases, but I see that it's calling it with true and false in the right order.

[Dimitri Glazkov (Google)]

Comment on attachment 47535 [details]
Take 2.

seems reasonable.

[WebKit Commit Bot]

Comment on attachment 47535 [details]
Take 2.

Clearing flags on attachment: 47535

Committed r54069: <http://trac.webkit.org/changeset/54069>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.