Bug 110751 – [Qt][WK2] Remove unneeded method didResumeContent from PageViewportController clients

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andras Becsi]

[Qt][WK2] Remove unneeded method didResumeContent from PageViewportController clients

[Andras Becsi]

Created attachment 190041 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 190041 [details]
Patch

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

> Source/WebKit2/UIProcess/PageViewportController.cpp:320
>      if (!m_hasSuspendedContent)
>          return;

Cool, the only purpose I then see left of suspend/resumeContent in PageViewportController is headed by the following comment in pageDidRequestScroll:
// Ignore the request if suspended. Can only happen due to delay in event delivery.

So do you think we could go even further and remove all knowledge of page suspension in PVC?

[Andras Becsi]

Comment on attachment 190041 [details]
Patch

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

>> Source/WebKit2/UIProcess/PageViewportController.cpp:320
>>          return;
> 
> Cool, the only purpose I then see left of suspend/resumeContent in PageViewportController is headed by the following comment in pageDidRequestScroll:
> // Ignore the request if suspended. Can only happen due to delay in event delivery.
> 
> So do you think we could go even further and remove all knowledge of page suspension in PVC?

I'll check. I think we can, since the WebPageProxy already has early returns to not send unnecessary events.

[Andras Becsi]

(In reply to comment #3)
> (From update of attachment 190041 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190041&action=review
> 
> >> Source/WebKit2/UIProcess/PageViewportController.cpp:320
> >>          return;
> > 
> > Cool, the only purpose I then see left of suspend/resumeContent in PageViewportController is headed by the following comment in pageDidRequestScroll:
> > // Ignore the request if suspended. Can only happen due to delay in event delivery.
> > 
> > So do you think we could go even further and remove all knowledge of page suspension in PVC?
> 
> I'll check. I think we can, since the WebPageProxy already has early returns to not send unnecessary events.

Actually there are references to m_controller->hasSuspendedContent() from the client (at least for Qt).

[Andras Becsi]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 190041 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=190041&action=review
> > 
> > >> Source/WebKit2/UIProcess/PageViewportController.cpp:320
> > >>          return;
> > > 
> > > Cool, the only purpose I then see left of suspend/resumeContent in PageViewportController is headed by the following comment in pageDidRequestScroll:
> > > // Ignore the request if suspended. Can only happen due to delay in event delivery.
> > > 
> > > So do you think we could go even further and remove all knowledge of page suspension in PVC?
> > 
> > I'll check. I think we can, since the WebPageProxy already has early returns to not send unnecessary events.
> 
> Actually there are references to m_controller->hasSuspendedContent() from the client (at least for Qt).

On second thought, it looks like only PageViewportControllerClientQt::didChangeContentsSize really depend on knowing about the suspend state, all the other references could be substituted with checking if the scale animation is active. Let's see.

[Andras Becsi]

(In reply to comment #2)
> (From update of attachment 190041 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190041&action=review
> 
> > Source/WebKit2/UIProcess/PageViewportController.cpp:320
> >      if (!m_hasSuspendedContent)
> >          return;
> 
> Cool, the only purpose I then see left of suspend/resumeContent in PageViewportController is headed by the following comment in pageDidRequestScroll:
> // Ignore the request if suspended. Can only happen due to delay in event delivery.

I is also happening when there are mouse wheel events during a double tap to zoom
animation, so the comment is not completely accurate.

> 
> So do you think we could go even further and remove all knowledge of page suspension in PVC?

As discussed on IRC, we can try to make the PVC slimmer, or event merge the shareable functionality somewhere reasonable later, once we know where things are heading with the shared WebView and and specific API.

[Jocelyn Turcotte]

Comment on attachment 190041 [details]
Patch

Looks good then, r=me

[Andras Becsi]

Comment on attachment 190041 [details]
Patch

Clearing flags on attachment: 190041

Committed r143935: <http://trac.webkit.org/changeset/143935>

[Andras Becsi]

All reviewed patches have been landed.  Closing bug.

[Zoltan Arvai]

Please check API tests:

Before the patch: 
 189 passed, 3 failed, 0 skipped, 0 crashed
http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release%20WebKit2%20%28Amazon%20EC2%29/builds/14249

After the patch:
 182 passed, 10 failed, 0 skipped, 0 crashed
http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release%20WebKit2%20%28Amazon%20EC2%29/builds/14250

[Andras Becsi]

(In reply to comment #10)
> Please check API tests:
> 
> Before the patch: 
>  189 passed, 3 failed, 0 skipped, 0 crashed
> http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release%20WebKit2%20%28Amazon%20EC2%29/builds/14249
> 
> After the patch:
>  182 passed, 10 failed, 0 skipped, 0 crashed
> http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release%20WebKit2%20%28Amazon%20EC2%29/builds/14250

Thanks. Investigating.

[Andras Becsi]

(In reply to comment #11)
> (In reply to comment #10)
> > Please check API tests:
> > 
> > Before the patch: 
> >  189 passed, 3 failed, 0 skipped, 0 crashed
> > http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release%20WebKit2%20%28Amazon%20EC2%29/builds/14249
> > 
> > After the patch:
> >  182 passed, 10 failed, 0 skipped, 0 crashed
> > http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release%20WebKit2%20%28Amazon%20EC2%29/builds/14250
> 
> Thanks. Investigating.

Fixed in https://bugs.webkit.org/show_bug.cgi?id=111086