Bug 85650 - Integrate Page Visibility state change with suspension/resume of Animations
Summary: Integrate Page Visibility state change with suspension/resume of Animations
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jesus Sanchez-Palencia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-04 12:25 PDT by Jesus Sanchez-Palencia
Modified: 2014-02-05 10:54 PST (History)
12 users (show)

See Also:


Attachments
Patch (2.25 KB, patch)
2012-05-04 12:34 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (3.53 KB, patch)
2012-05-07 11:29 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (5.83 KB, patch)
2012-05-09 11:06 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (5.87 MB, application/zip)
2012-05-09 12:15 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jesus Sanchez-Palencia 2012-05-04 12:25:12 PDT
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.
Comment 1 Jesus Sanchez-Palencia 2012-05-04 12:34:04 PDT
Created attachment 140299 [details]
Patch
Comment 2 Jesus Sanchez-Palencia 2012-05-04 12:40:30 PDT
(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!
Comment 3 zalan 2012-05-04 12:52:41 PDT
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()
Comment 4 Kenneth Rohde Christiansen 2012-05-04 15:22:07 PDT
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.
Comment 5 Allan Sandfeld Jensen 2012-05-06 02:22:55 PDT
(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.
Comment 6 zalan 2012-05-06 11:00:14 PDT
> 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.
Comment 7 Allan Sandfeld Jensen 2012-05-07 00:05:26 PDT
(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?
Comment 8 zalan 2012-05-07 00:38:26 PDT
(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.
Comment 9 Jesus Sanchez-Palencia 2012-05-07 11:29:44 PDT
Created attachment 140561 [details]
Patch
Comment 10 Jesus Sanchez-Palencia 2012-05-07 11:33:46 PDT
(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!
Comment 11 WebKit Review Bot 2012-05-08 07:59:14 PDT
Comment on attachment 140561 [details]
Patch

Clearing flags on attachment: 140561

Committed r116422: <http://trac.webkit.org/changeset/116422>
Comment 12 WebKit Review Bot 2012-05-08 07:59:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Darin Adler 2012-05-08 13:17:22 PDT
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.
Comment 14 Jesus Sanchez-Palencia 2012-05-09 10:54:19 PDT
I'm refactoring this on trunk so we move it to WebCore as Darin suggested.
Comment 15 Jesus Sanchez-Palencia 2012-05-09 11:06:29 PDT
Created attachment 140980 [details]
Patch
Comment 16 Jesus Sanchez-Palencia 2012-05-09 11:09:36 PDT
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?! :)
Comment 17 zalan 2012-05-09 11:51:48 PDT
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;
Comment 18 WebKit Review Bot 2012-05-09 12:15:34 PDT
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
Comment 19 WebKit Review Bot 2012-05-09 12:15:51 PDT
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
Comment 20 Jesus Sanchez-Palencia 2012-05-09 14:04:56 PDT
(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?!
Comment 21 Jesus Sanchez-Palencia 2012-05-09 14:06:20 PDT
(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... :)
Comment 22 Simon Fraser (smfr) 2012-10-05 11:00:33 PDT
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?
Comment 23 Jesus Sanchez-Palencia 2012-10-10 10:26:56 PDT
(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)?