Integrate Page Visibility state change with suspension/resume of Animations

• Component: WebKit2
• Status: REOPENED ---

[Jesus Sanchez-Palencia]

We could use state changes of the Page Visibility API to trigger the automatic suspension/resume of a WebPage and it's main frame, in the same fashion as the Suspend/Resume API for WK2.

I believe this can be done at least for animations, but maybe not for timers and other active DOM objects.

[Jesus Sanchez-Palencia]

Created attachment 140299 [details]
Patch

[Jesus Sanchez-Palencia]

(In reply to comment #1)
> Created an attachment (id=140299) [details]
> Patch

I would like to get some feedback on this, not only on the implementation but also if this is something that we _really_ want to have. We need to keep in mind that this is not Qt specific but we can always protect it with #ifdefs for the Qt port only if needed.

It is not covered by tests because I couldn't think of a way to do so. However I also couldn't find the way how the original Suspend/Resume WK2 is being tested, so perhaps this is a non-issue.

Thanks in advance!

[zalan]

Comment on attachment 140299 [details]
Patch

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

We've done something similar on for N9, but the implementation logic (which was a little bit more extensive, like including load deferring) was on the host application level. I prefer to stay that way as even on the same platform (Qt), different hosting apps can have different view on what to do when page visibility changes.

> Source/WebKit2/ChangeLog:9
> +        automatic suspension/resume of a WebPage and it's main frame, in the same

its main frame

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3174
>  }

nitpicking

FrameView* view = m_page->mainFrame() ? m_page->mainFrame()->view() : NULL;

if (view)
  view->show()

if (view)
  view->hide()

[Kenneth Rohde Christiansen]

Comment on attachment 140299 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3162
> +    if (state == WebCore::PageVisibilityStateVisible) {
> +        m_page->didMoveOnscreen();

What if this is the same as the previous state?

>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3174
>>  }
> 
> nitpicking
> 
> FrameView* view = m_page->mainFrame() ? m_page->mainFrame()->view() : NULL;
> 
> if (view)
>   view->show()
> 
> if (view)
>   view->hide()

We don't use NULL, just 0.

[Allan Sandfeld Jensen]

(In reply to comment #3)
> (From update of attachment 140299 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140299&action=review
> 
> We've done something similar on for N9, but the implementation logic (which was a little bit more extensive, like including load deferring) was on the host application level. I prefer to stay that way as even on the same platform (Qt), different hosting apps can have different view on what to do when page visibility changes.
> 
This was something I requested. The point is to not send 2-3 separate calls but keep the logic in one place.

Please also note that this code does not suspend scripting like on the N9, this only suspends things that you would also want suspended. In this case CSS animations, scripted animations and some layout logic.

If a host wants to suspend more than this, it should use the suspendActiveDomObjectsAndAnimations API I wrote earlier, but I don't think we should use that by default, this version is much better.

[zalan]

> If a host wants to suspend more than this, it should use the suspendActiveDomObjectsAndAnimations API I wrote earlier, but I don't think we should use that by default, this version is much better.
And what if the host does not want to have suspend/resume activity at all? There might be such use cases (specific host apps), where page visibility does not necessary involve suspend/resume activity. I just think that pairing up these functionalities at the engine level is a little bit of stretch driven by some specific mobile use case.

[Allan Sandfeld Jensen]

(In reply to comment #6)
> > If a host wants to suspend more than this, it should use the suspendActiveDomObjectsAndAnimations API I wrote earlier, but I don't think we should use that by default, this version is much better.
> And what if the host does not want to have suspend/resume activity at all? There might be such use cases (specific host apps), where page visibility does not necessary involve suspend/resume activity. I just think that pairing up these functionalities at the engine level is a little bit of stretch driven by some specific mobile use case.

What is suspended here is essentially animated painting. If you change visibility you will stop receiving paint updates anyway, so there is no reason to keep animations going that will never be painted, or?

[zalan]

(In reply to comment #7)
> (In reply to comment #6)
> > > If a host wants to suspend more than this, it should use the suspendActiveDomObjectsAndAnimations API I wrote earlier, but I don't think we should use that by default, this version is much better.
> > And what if the host does not want to have suspend/resume activity at all? There might be such use cases (specific host apps), where page visibility does not necessary involve suspend/resume activity. I just think that pairing up these functionalities at the engine level is a little bit of stretch driven by some specific mobile use case.
> 
> What is suspended here is essentially animated painting. If you change visibility you will stop receiving paint updates anyway, so there is no reason to keep animations going that will never be painted, or?
Ok, requestAnimationFrame and alikes are fine. They are meant to be inactive when page is not visible. I probably did read too much into the Changelog entry, without tracing down those show/hide functions. 
"This patch uses state changes of the Page Visibility API to trigger the automatic suspension/resume of a WebPage and it's main frame, in the same fashion of the Suspend/Resume API of WebKit2."
I think this should change a little bit to emphasize what it really does.

[Jesus Sanchez-Palencia]

Created attachment 140561 [details]
Patch

[Jesus Sanchez-Palencia]

(In reply to comment #3)
> > Source/WebKit2/ChangeLog:9
> > +        automatic suspension/resume of a WebPage and it's main frame, in the same
> 
> its main frame

Fixed.

> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3174
> >  }
> 
> nitpicking

Fixed ;) .


(In reply to comment #4)
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3162
> > +    if (state == WebCore::PageVisibilityStateVisible) {
> > +        m_page->didMoveOnscreen();
> 
> What if this is the same as the previous state?

Fixed.


(In reply to comment #8)
> I think this should change a little bit to emphasize what it really does.

Fixed.


Thanks for the review, everyone!

[WebKit Review Bot]

Comment on attachment 140561 [details]
Patch

Clearing flags on attachment: 140561

Committed r116422: <http://trac.webkit.org/changeset/116422>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 140561 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3184
> +    WebCore::PageVisibilityState state = static_cast<WebCore::PageVisibilityState>(visibilityState);
> +
> +    if (m_visibilityState == state)
> +        return;
> +
> +    FrameView* view = m_page->mainFrame() ? m_page->mainFrame()->view() : 0;
> +
> +    if (state == WebCore::PageVisibilityStateVisible) {
> +        m_page->didMoveOnscreen();
> +        if (view)
> +            view->show();
> +    }
> +
> +    m_page->setVisibilityState(state, isInitialState);
> +    m_visibilityState = state;
> +
> +    if (state == WebCore::PageVisibilityStateHidden) {
> +        m_page->willMoveOffscreen();
> +        if (view)
> +            view->hide();
> +    }

To me this looks like code that should be in WebCore. Can we factor this so the work is in WebCore?

> Source/WebKit2/WebProcess/WebPage/WebPage.h:57
> +#if ENABLE(PAGE_VISIBILITY_API)
> +#include <WebCore/PageVisibilityState.h>
> +#endif

Includes that are in #if should go in their own separate paragraph rather than being sorted into the main include paragraph.

[Jesus Sanchez-Palencia]

I'm refactoring this on trunk so we move it to WebCore as Darin suggested.

[Jesus Sanchez-Palencia]

Created attachment 140980 [details]
Patch

[Jesus Sanchez-Palencia]

Comment on attachment 140980 [details]
Patch

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

What do you guys think of this patch now?

> Source/WebCore/ChangeLog:8
> +        Covered by previous tests.

Am I being too naive here?! :)

[zalan]

Comment on attachment 140980 [details]
Patch

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

> Source/WebCore/page/Page.cpp:1037
> +        return;

Can m_mainframe really be NULL here?

> Source/WebCore/page/Page.cpp:1039
> +    if (!isInitialState) {

Wouldn't an early return be better?
if (isInitialState) 
    return;

[WebKit Review Bot]

Comment on attachment 140980 [details]
Patch

Attachment 140980 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12655652

New failing tests:
compositing/video-page-visibility.html

[WebKit Review Bot]

Created attachment 140996 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Jesus Sanchez-Palencia]

(In reply to comment #18)
> (From update of attachment 140980 [details])
> Attachment 140980 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12655652
> 
> New failing tests:
> compositing/video-page-visibility.html

I can't reproduce this locally for the Qt port, the test is passing. I've downloaded the layout-test-results.zip provided by chromium-ews but there are no files related to this specific test. If I open the output link above, it also doesn't mention this test anywhere.

What am I missing here?!

[Jesus Sanchez-Palencia]

(In reply to comment #17)
> Can m_mainframe really be NULL here?

There was a check for it there already, so I just kept it the way it was.

> 
> > Source/WebCore/page/Page.cpp:1039
> > +    if (!isInitialState) {
> 
> Wouldn't an early return be better?
> if (isInitialState) 
>     return;

I also kept the original implementation, but yeah, while I'm there... :)

[Simon Fraser (smfr)]

Comment on attachment 140980 [details]
Patch

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

> Source/WebCore/page/Page.cpp:1050
> +        if (visibilityState == PageVisibilityStateVisible) {
> +            didMoveOnscreen();
> +            m_mainFrame->view()->show();
> +        } else if (visibilityState == PageVisibilityStateHidden) {
>              ArenaSize size = renderTreeSize();
>              HistogramSupport::histogramCustomCounts("WebCore.Page.renderTreeSizeBytes", size.treeSize, 1000, 500000000, 50);
>              HistogramSupport::histogramCustomCounts("WebCore.Page.renderTreeAllocatedBytes", size.allocated, 1000, 500000000, 50);
> +
> +            willMoveOffscreen();
> +            m_mainFrame->view()->hide();
>          }

Were show() and hide() already being called in WK1? Does this change cause them to be called twice?

[Jesus Sanchez-Palencia]

(In reply to comment #22)
> (From update of attachment 140980 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140980&action=review
> 
> > Source/WebCore/page/Page.cpp:1050
> > +        if (visibilityState == PageVisibilityStateVisible) {
> > +            didMoveOnscreen();
> > +            m_mainFrame->view()->show();
> > +        } else if (visibilityState == PageVisibilityStateHidden) {
> >              ArenaSize size = renderTreeSize();
> >              HistogramSupport::histogramCustomCounts("WebCore.Page.renderTreeSizeBytes", size.treeSize, 1000, 500000000, 50);
> >              HistogramSupport::histogramCustomCounts("WebCore.Page.renderTreeAllocatedBytes", size.allocated, 1000, 500000000, 50);
> > +
> > +            willMoveOffscreen();
> > +            m_mainFrame->view()->hide();
> >          }
> 
> Were show() and hide() already being called in WK1? Does this change cause them to be called twice?

I don't think so, but in any case both ScrollView::show() and ScrollView::hide() have checkers for isSelfVisible() before doing anything else and before calling Widget::show()/hide(). The latter is then a per platform implementation in which a few ports do the same early check and others don't or don't even implement.

Are you asking due to the risk of calling it twice or do you think the test failure from chromium can be somehow related (please check comment #20)?