Bug 81154 - [WK2] Add Page Visibility API support
Summary: [WK2] Add Page Visibility API support
Status: RESOLVED FIXED
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: 81355
Blocks: 69554 81164
  Show dependency treegraph
 
Reported: 2012-03-14 13:55 PDT by Jesus Sanchez-Palencia
Modified: 2012-04-04 06:10 PDT (History)
5 users (show)

See Also:


Attachments
Patch (5.44 KB, patch)
2012-03-14 15:15 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (5.72 KB, patch)
2012-03-21 10:41 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (6.01 KB, patch)
2012-03-30 13:23 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (5.30 KB, patch)
2012-04-03 08:40 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesus Sanchez-Palencia 2012-03-14 13:55:05 PDT
WebKit2 should support the Page Visibility API implementation.
Comment 1 Jesus Sanchez-Palencia 2012-03-14 15:15:01 PDT
Created attachment 131928 [details]
Patch
Comment 2 Anders Carlsson 2012-03-14 15:46:27 PDT
Comment on attachment 131928 [details]
Patch

Here are a couple of concerns with this patch:

1. There are no API tests for this.
2. This is not the right way to implement this feature. We already have information in the web process about the state of the page, so there shouldn't be a need to expose another setter for it.
Comment 3 Jesus Sanchez-Palencia 2012-03-15 12:30:24 PDT
Thanks for reviewing this!

(In reply to comment #2)
> 2. This is not the right way to implement this feature. We already have information in the web process about the state of the page, so there shouldn't be a need to expose another setter for it.

About the state of the page we already have in the web process, are you talking about active and focused (setFocused and setActive, both from WebPage)? Perhaps we could use this and call WebPage::setVisibilityState for both hidden and visible states, but what about the prerender and preview ones? (http://www.w3.org/TR/2011/WD-page-visibility-20110602/#sec-document-visibility-interface)

I can't think of a "platform-agnostic" way that could automatically set this as the current state of Page. But maybe I'm just missing something here...

That is way this patch follows previous implementations of the Page Visibility API, like WebKit1's Chromium and EFL ports, whose approach was to even provide an API to their WebViews so this could be set.

By the way, but maybe not totally related, I've just realized that after calling WebPageProxy::viewStateDidChange we keep the state information somehow in WebPage for all states (focused, active, isInWindow) but not for ViewIsVisible.
Comment 4 Jesus Sanchez-Palencia 2012-03-15 12:51:22 PDT
(In reply to comment #3)
> About the state of the page we already have in the web process, are you talking about active and focused (setFocused and setActive, both from WebPage)? Perhaps we could use this and call WebPage::setVisibilityState for both hidden and visible states, but what about the prerender and preview ones? 
(http://www.w3.org/TR/2011/WD-page-visibility-20110602/#sec-document-visibility-interface)

I'm sorry, my bad, the current spec is in http://www.w3.org/TR/page-visibility/#sec-document-visibility-interface  and the prerender was left out of it. But preview is still there and so the rest of my doubts remains.
Comment 5 Jesus Sanchez-Palencia 2012-03-21 10:41:04 PDT
Created attachment 133072 [details]
Patch
Comment 6 Jesus Sanchez-Palencia 2012-03-30 13:23:37 PDT
Created attachment 134868 [details]
Patch
Comment 7 Jesus Sanchez-Palencia 2012-03-31 07:07:32 PDT
Ping review, guys?! Thanks!
Comment 8 Kenneth Rohde Christiansen 2012-04-01 04:42:09 PDT
Comment on attachment 134868 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:810
> +    PageVisibilityState visibilityState = PageVisibilityStateHidden;
> +
> +    if (flags & ViewIsInPrerender)
> +        visibilityState = PageVisibilityStatePrerender;
> +    else if (m_pageClient->isViewVisible()) {
> +        resumeActiveDOMObjectsAndAnimations();
> +        visibilityState = PageVisibilityStateVisible;
> +    }
> +
> +    if (visibilityState != m_visibilityState) {
> +        m_visibilityState = visibilityState;
> +        process()->send(Messages::WebPage::SetVisibilityState(visibilityState, false), m_pageID);
> +    }

would it make sense to resume on the web process side as part of SetVisibilityState ?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3163
> +void WebPage::setVisibilityState(int visibilityState, bool isInitialState)
> +{
> +    if (!m_page)
> +        return;
> +    m_page->setVisibilityState(static_cast<WebCore::PageVisibilityState>(visibilityState), isInitialState);
> +}

Should the initial state be set after we crash? Couldn't this be done more "automatically". Why does it matter anyway?
Comment 9 Allan Sandfeld Jensen 2012-04-01 05:43:06 PDT
When setting the page visibility, please also use take advantage of the new visibility info to set the FrameView visibility (inherited from Widget).
Comment 10 Allan Sandfeld Jensen 2012-04-01 06:29:48 PDT
Comment on attachment 134868 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:813
> +        suspendActiveDOMObjectsAndAnimations();

I don't think suspendActiveDOMObjectsAndAnimations makes sense in this context. The use case for page visibility API is specifically that web-pages can adjust their timers to make fewer updates.

Instead use Page::willMoveOffscreen which only disables animations but not active DOM objects.
I also think it makes sense to calls ScrollView::hide() which tells the rendering logic that the page is no longer being rendered.
Comment 11 Jesus Sanchez-Palencia 2012-04-02 15:03:40 PDT
Comment on attachment 134868 [details]
Patch

I will comment on all comments here so we can discuss the raised points and so I'm clearing the review flag for now.


(In reply to comment #8)
> Should the initial state be set after we crash? Couldn't this be done more "automatically". Why does it matter anyway?

I guess Page is already setting the initial state anyway, so I will have a better look at this. IMHO, it's not needed.


(In reply to comment #9)
> When setting the page visibility, please also use take advantage of the new visibility info to set the FrameView visibility (inherited from Widget).

Perhaps we should first land this more "raw" implementation for Page Visibility in WK2, which is pretty close to how it's is implemented in WK1. After that we can have a follow-up patch that can play around with the FrameView visibility, Page::willMoveOffscreen, ScrollView::hide(), etc, as they all need to be investigated and are actually improvements that will take advantage of the visibility state of WebPage after we have settled down what is needed for the Page Visibility API implementation itself. We can leave suspend/resume of activeDOMObjects out for now as well.


(In reply to comment #10)
> I don't think suspendActiveDOMObjectsAndAnimations makes sense in this context. The use case for page visibility API is specifically that web-pages can adjust their timers to make fewer updates.
> 
> Instead use Page::willMoveOffscreen which only disables animations but not active DOM objects.
> I also think it makes sense to calls ScrollView::hide() which tells the rendering logic that the page is no longer being rendered.

Ditto.


What do you guys think?
Comment 12 Jesus Sanchez-Palencia 2012-04-03 08:40:03 PDT
Created attachment 135335 [details]
Patch
Comment 13 WebKit Review Bot 2012-04-04 06:10:42 PDT
Comment on attachment 135335 [details]
Patch

Clearing flags on attachment: 135335

Committed r113181: <http://trac.webkit.org/changeset/113181>
Comment 14 WebKit Review Bot 2012-04-04 06:10:50 PDT
All reviewed patches have been landed.  Closing bug.