Bug 61543 – Inform ChromeClient touch events are not needed by the webpage when navigating away from the page instead of in both Document:detach/CachedFrame

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Dinu Jacob]

Currently, the chrome client is informed that the touch events are not needed in Document::detach / CachedFramed constructor. 

When loading a new page, the old page is added to page cache and the chrome client is informed that touch events are not needed anymore in the CachedFrame constructor. However, if the page cache capacity has been reached, the oldest item is marked to be removed from the cache and a timer is started. The item is removed from cache when the timer expires. When the item is removed, Document:detach is called which informs (incorrectly) chrome client that touch events are not needed. If the current page loaded has touch event listeners, it will not get touch events anymore.

As needTouchEvents method takes only a boolean, there is no indication that the flag was reset for the older document being detached. It might be better to reset the flag at the time we navigate away from the current page.

Patch to follow shortly.

[Dinu Jacob]

Created attachment 95003 [details]
First attempt

[Simon Hausmann]

CC'ing Ben and Dimitry, as this code was added in bug #34215

[Ben Murdoch]

Comment on attachment 95003 [details]
First attempt

View in context: https://bugs.webkit.org/attachment.cgi?id=95003&action=review

Thanks for spotting this bug! Have a question about the proposed patch.

> Source/WebCore/loader/FrameLoader.cpp:2074
> +    m_frame->page()->chrome()->client()->needTouchEvents(false);

Will this now trigger clearing the flag on frame navigation? For example, imagine a page with an iframe and we perform a navigation inside the frame. What will happen to touch events in the parent frame? The old code ensured that it was the main frame performing the navigation before clearing the flag.

Is it possible to write a layout test? It would be good to ensure the correct behavior in this scenario.

[Dinu Jacob]

(In reply to comment #3)
> (From update of attachment 95003 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95003&action=review
> 
> Thanks for spotting this bug! Have a question about the proposed patch.
> 
> > Source/WebCore/loader/FrameLoader.cpp:2074
> > +    m_frame->page()->chrome()->client()->needTouchEvents(false);
> 
> Will this now trigger clearing the flag on frame navigation? For example, imagine a page with an iframe and we perform a navigation inside the frame. What will happen to touch events in the parent frame? The old code ensured that it was the main frame performing the navigation before clearing the flag.
> 
> Is it possible to write a layout test? It would be good to ensure the correct behavior in this scenario.

Thanks for your feedback Ben. Yes, with the current patch it will reset the flag when we are navigating within an iframe. I can reset the flag only for the main frame to avoid resetting the flag for iframes. 

I started writing a layout test but realised that for WebKit2 I can't yet use the eventSender to send the touch events. Is there any other way I can achieve this for WebKit2?

[Dinu Jacob]

Created attachment 95770 [details]
Patch with fix for iframe navigation

[Dinu Jacob]

Hi Ben/Dimitri,

Can you please provide your input on the latest patch?

Thanks,
Dinu

[Ben Murdoch]

Comment on attachment 95770 [details]
Patch with fix for iframe navigation

View in context: https://bugs.webkit.org/attachment.cgi?id=95770&action=review

> Source/WebCore/ChangeLog:5
> +        Inform ChromeClient touch events are not needed by the webpage when navigating away from the page instead of in both Document:detach/CachedFrame

Hi Dinu, I'm a little confused about the ChangeLog message - just want to make sure I have the flow of events right. You've added the new code to FrameLoader::transitionToCommitted. Does that not mean when we are "committed" to loading the new page rather than necessarily navigating away from the old one?  So we're setting the flag to false after committing to loading a new page, but before that new page could have registered any new touch event handlers, right?

[Dinu Jacob]

(In reply to comment #7)
> (From update of attachment 95770 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95770&action=review
> 
> > Source/WebCore/ChangeLog:5
> > +        Inform ChromeClient touch events are not needed by the webpage when navigating away from the page instead of in both Document:detach/CachedFrame
> 
> Hi Dinu, I'm a little confused about the ChangeLog message - just want to make sure I have the flow of events right. You've added the new code to FrameLoader::transitionToCommitted. Does that not mean when we are "committed" to loading the new page rather than necessarily navigating away from the old one?  So we're setting the flag to false after committing to loading a new page, but before that new page could have registered any new touch event handlers, right?

Hi Ben,

Yes, that's exactly what is being done. If the use of "navigating away from the old page" is confusing I can replace it with "committed to loading a new page".

Dinu

[Ben Murdoch]

That's OK - perhaps if you need to rebase the patch for landing then you could update it.

The code LGTM, but it'd be really nice to add a test as this behavior depends on FrameLoader::transitionToCommitted getting called before CachedFrame::restore. Fine at the moment, but should something get refactored in the future it'd be really easy to break this behavior without noticing ... maybe we can have a test on a non-WebKit2 platform? I don't have powers to r+ so I will defer to someone who does :)

[Dimitri Glazkov (Google)]

Comment on attachment 95770 [details]
Patch with fix for iframe navigation

rs=me.

[Dinu Jacob]

Comment on attachment 95770 [details]
Patch with fix for iframe navigation

Forgot to request commit as well. Please commit for me.

[Dinu Jacob]

(In reply to comment #9)
> That's OK - perhaps if you need to rebase the patch for landing then you could update it.
> 
> The code LGTM, but it'd be really nice to add a test as this behavior depends on FrameLoader::transitionToCommitted getting called before CachedFrame::restore. Fine at the moment, but should something get refactored in the future it'd be really easy to break this behavior without noticing ... maybe we can have a test on a non-WebKit2 platform? I don't have powers to r+ so I will defer to someone who does :)

Thanks Ben. I will try to come up with a test that can be tested on a non-WebKit2 platform.

Dinu

[WebKit Review Bot]

Comment on attachment 95770 [details]
Patch with fix for iframe navigation

Clearing flags on attachment: 95770

Committed r88242: <http://trac.webkit.org/changeset/88242>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.