Bug 81154

Created attachment 131928 [details] Patch

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.

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.

(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.

Created attachment 133072 [details] Patch

Created attachment 134868 [details] Patch

Ping review, guys?! Thanks!

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?

When setting the page visibility, please also use take advantage of the new visibility info to set the FrameView visibility (inherited from Widget).

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 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?

Created attachment 135335 [details] Patch

Comment on attachment 135335 [details] Patch Clearing flags on attachment: 135335 Committed r113181: <http://trac.webkit.org/changeset/113181>

All reviewed patches have been landed. Closing bug.