Bug 85650

Created attachment 140299 [details] Patch

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

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

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

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

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

Created attachment 140561 [details] Patch

(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 on attachment 140561 [details] Patch Clearing flags on attachment: 140561 Committed r116422: <http://trac.webkit.org/changeset/116422>

All reviewed patches have been landed. Closing bug.

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.

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

Created attachment 140980 [details] Patch

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

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

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

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

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