Bug 34215

Summary: Need functionality in the ChromeClient to be informed when touch events are and are not needed by the webpage.
Product: WebKit Reporter: Ben Murdoch <benm>
Component: WebCore Misc.Assignee: Ben Murdoch <benm>
Status: RESOLVED FIXED    
Severity: Normal CC: android-webkit-unforking, commit-queue, eric, hausmann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Android   
OS: Android   
Bug Depends on:    
Bug Blocks: 32485    
Attachments:
Description Flags
Proposed Patch.
none
Take 2. none

Description Ben Murdoch 2010-01-27 06:35:19 PST
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.
Comment 1 Ben Murdoch 2010-01-27 07:30:00 PST
Created attachment 47531 [details]
Proposed Patch.
Comment 2 Eric Seidel (no email) 2010-01-27 07:35:54 PST
Attachment 47531 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/215110
Comment 3 Ben Murdoch 2010-01-27 07:40:55 PST
Comment on attachment 47531 [details]
Proposed Patch.

Will upload a new patch.
Comment 4 Ben Murdoch 2010-01-27 07:52:50 PST
Created attachment 47535 [details]
Take 2.
Comment 5 Simon Hausmann 2010-01-27 15:10:22 PST
Adjusted bug title as per recent webkit-dev discussion, to make it clear that this isn't really port specific.
Comment 6 Simon Hausmann 2010-01-27 15:14:19 PST
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.
Comment 7 Ben Murdoch 2010-01-28 06:39:34 PST
(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
Comment 8 Simon Hausmann 2010-01-28 07:42:35 PST
(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.
Comment 9 Dimitri Glazkov (Google) 2010-01-29 09:02:37 PST
Comment on attachment 47535 [details]
Take 2.

seems reasonable.
Comment 10 WebKit Commit Bot 2010-01-29 09:56:51 PST
Comment on attachment 47535 [details]
Take 2.

Clearing flags on attachment: 47535

Committed r54069: <http://trac.webkit.org/changeset/54069>
Comment 11 WebKit Commit Bot 2010-01-29 09:56:57 PST
All reviewed patches have been landed.  Closing bug.