Bug 150515

Let's collect some data on web apps that have user-facing features while in background tabs, what the features are, and how they're implemented. Hopefully, we can design a way to pause tabs without breaking these features. I'll start with Facebook. Feature: Chat. Behavior: Play a sound and flash the titlebar when you get a new message. Implementation: ???

Created attachment 265649 [details] Patch

Attachment 265649 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.

Just looking for some feedback, especially re: terrible names.

Comment on attachment 265649 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265649&action=review We should add a WebKit API test for this (see Tools/TestWebKitAPI). > Source/WebCore/dom/Document.cpp:4559 > + for (auto* element : m_documentSuspensionCallbackElements) > + element->prepareElementForDocumentSuspension(); You shouldn't duplicate all these code in documentWillSuspendForPageCache. > Source/WebCore/dom/Document.cpp:4568 > + m_frame->animation().suspendAnimationsForDocument(this); It seems like suspending animation is also a good thing for page cache. > Source/WebCore/dom/Document.cpp:4575 > + m_frame->clearTimers(); This function also calls suspendAnimationsForDocument. Why do we need to call it twice? > Source/WebCore/dom/Document.cpp:4589 > + m_frame->view()->forceLayout(); Do we really need to always force layout each time we resume a document? It seems like we just need to remember whether there was a pending style recalc when we suspended the document, and schedule a recalc here if there was one. > Source/WebCore/dom/Document.cpp:4596 > + Vector<Element*> elements; Ditto about the code duplication. > Source/WebCore/dom/Document.cpp:6632 > - if (!frame() || !frame()->page()) > + if (!frame() || !frame()->page() || m_isTabSuspended) > return; We can't just ignore this timer. We need to call this function when we're resuming the document. We should do this by stopping m_didAssociateFormControlsTimer, and remembering that we stopped it. > Source/WebCore/dom/Document.h:1763 > + bool m_isTabSuspended; > + You should use { false } (new syntax) to initialize this boolean here instead of doing it manually in the constructor. > Source/WebCore/dom/Element.h:350 > - virtual void documentWillSuspendForPageCache() { } > - virtual void documentDidResumeFromPageCache() { } > + virtual void prepareElementForDocumentSuspension() { } > + virtual void resumeElementFromDocumentSuspension() { } Why don't we do this rename in a separate patch? The old name did follow WebKit naming convention of "will" and "did" nicely though. > Source/WebCore/page/DOMWindow.cpp:576 > +void DOMWindow::suspendForTabSuspension() Why do we need a separate version of these functions? What's the point of resuming & suspending between two suspension modes?

Comment on attachment 265649 [details] Patch Attachment 265649 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/439338 Number of test failures exceeded the failure limit.

Created attachment 265651 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5

Comment on attachment 265649 [details] Patch Attachment 265649 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/439331 Number of test failures exceeded the failure limit.

Created attachment 265652 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 265649 [details] Patch Attachment 265649 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/439363 Number of test failures exceeded the failure limit.

Created attachment 265653 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Comment on attachment 265649 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265649&action=review I think the problems with page cache are partly because there are no clear state transitions between the three distinct suspension states (None, Tab, PageCache). If these we tracked more explicitly as mutually exclusive states and transitions between them implemented then I think many of them would disappear or at least become more understandable. Or maybe Tab and PageCache suspension could be exactly the same? > Source/WebCore/dom/Document.cpp:4515 > + if (!renderView()) > + return; > + > + renderView()->setIsInWindow(false); > + if (renderView()->usesCompositing()) > + renderView()->compositor().cancelCompositingLayerUpdate(); > } Could put renderView() to a local. > Source/WebCore/dom/Document.h:1762 > + bool m_isTabSuspended; Is this really needed? Suspension state should be per page. Can't we just keep the suspension state in a single place, in Page? Could use modern initialization: bool m_isTabSuspended { false }; > Source/WebCore/dom/Element.h:350 > + virtual void prepareElementForDocumentSuspension() { } > + virtual void resumeElementFromDocumentSuspension() { } Word 'Element' does not add information as these functions are scoped to class Element already. 'Document' doesn't add much either. Maybe prepareForSuspension/resumeFromSuspension? > Source/WebCore/loader/DocumentLoader.cpp:1355 > +// #if ENABLE(TAB_SUSPENSION) > + if (!document() || !document()->inPageCache()) > + return; > +/** #else > // A page in the PageCache should not be able to start loads. > ASSERT_WITH_SECURITY_IMPLICATION(!document() || !document()->inPageCache()); > +#endif **/ Tab suspension shouldn't affect page cached documents in any way. It should be mutually exclusive with page cache suspension and these assert shouldn't still be valid. > Source/WebCore/page/DOMWindow.cpp:397 > , m_suspendedForPageCache(false) > + , m_suspendedForTabSuspension(false) Can we just track suspension state in a single place, Page? Trying to keep all these booleans in sync seems bug prone and unnecessary. We shouldn't ever be both suspended for page and tab suspension. If these are needed they could be combined to something like enum class SuspensionState { None, Tab, PageCache } > Source/WebCore/page/DOMWindow.cpp:485 > - willDestroyDocumentInFrame(); > + if (!m_suspendedForPageCache) > + willDestroyDocumentInFrame(); Why change m_suspendedForPageCache case? > Source/WebCore/page/DOMWindow.cpp:583 > +void DOMWindow::suspendForTabSuspension() > +{ > + ASSERT(!m_suspendedForTabSuspension); > + if (m_suspendedForPageCache) > + resumeFromPageCache(); > + disconnectDOMWindowProperties(); > + m_suspendedForTabSuspension = true; > +} I don't understand this function. Why are we suspending a tab that is in page cache? Shouldn't that just be ASSERT(!m_suspendedForPageCache)? Is disconnectDOMWindowProperties is actually something we want to do to suspend tabs? > Source/WebCore/page/DOMWindow.h:376 > + bool m_suspendedForTabSuspension; Could use modern initialization: bool m_suspendedForTabSuspension { false }; > Source/WebCore/page/Page.cpp:1270 > void Page::setViewState(ViewState::Flags viewState) > { > + if (m_isTabSuspended && !viewState) > + return; Why this? > Source/WebCore/page/Page.cpp:1304 > + if (!isVisible()) { > + if (canSuspendPageForTabSuspension()) > + suspendPageForTabSuspension(); > + } else > + resumePageFromTabSuspension(); Why isn't isVisible() test just part of canSuspendPageForTabSuspension()? Instead of calling suspendPageForTabSuspension()/resumePageFromTabSuspension() from multiple places it might be safer and cleaner to have single updateTabSuspensionState() or similar that implements this logic. All places that may cause change in the suspension state would call it. > Source/WebCore/page/Page.cpp:1315 > + resumePageFromTabSuspension(); > + } else { > + setViewState((m_pageActivityState & ~(ViewState::IsVisible | ViewState::IsVisibleOrOccluded)) | ViewState::IsVisuallyIdle); > + if (canSuspendPageForTabSuspension()) > + suspendPageForTabSuspension(); This would call it. > Source/WebCore/page/Page.cpp:1357 > + if (canSuspendPageForTabSuspension()) > + suspendPageForTabSuspension(); And this function. > Source/WebCore/page/Page.cpp:1836 > +bool Page::canSuspendPageForTabSuspension() > +{ > + return !(m_pageActivityState & (PageActivityState::AudiblePlugin | PageActivityState::PageLoadActivity)) && PageCache::singleton().canCache(this); > +} This would be more readable and expandable as multiline function if (m_pageActivityState & (PageActivityState::AudiblePlugin | PageActivityState::PageLoadActivity)) return false; if (!PageCache::singleton().canCache(this)) return false; return true; > Source/WebCore/page/Page.cpp:1848 > + setIsTabSuspended(true); You probably don't need a setter at all since you are only changing the variable internally in Page. > Source/WebCore/page/Page.cpp:1861 > + setIsTabSuspended(false); Same here. > Source/WebCore/page/Page.h:654 > + bool m_isTabSuspended; Could use modern initialization: bool m_isTabSuspended { false }; > Source/WebCore/page/Page.h:656 > + void setIsTabSuspended(bool isTabSuspended) { m_isTabSuspended = isTabSuspended; } Shouldn't be needed.

Created attachment 266016 [details] Patch

Comment on attachment 266016 [details] Patch r=me assuming it builds.

Created attachment 267092 [details] Patch

Created attachment 267120 [details] Patch

Created attachment 267122 [details] Patch

Comment on attachment 267122 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267122&action=review I don't think the approach of suspending a tab synchronously when the viability state changes is sound. We probably want to schedule a one shot timer when the visibility state changes, and suspend the page only when that timer fires. > Source/WebCore/dom/Document.cpp:4525 > + suspendActiveDOMObjects(ActiveDOMObject::PageCache); > + suspendScriptedAnimationControllerCallbacks(); Why did you change the order in which these functions are called? You should explain that in per-function change log comment if it's intentional. Also, it's weird that documentWillSuspend suspend active dom objects but documentDidResume does not resume. I think it's better to keep calling these two functions in the caller side instead. > Source/WebCore/dom/Document.cpp:4548 > + page()->lockAllOverlayScrollbarsToHidden(true); Why is this only needed in the tab suspension case? It seems that Document::setInPageCache already calls this. Why can't we just call this in documentWillSuspend instead? > Source/WebCore/dom/Document.cpp:4554 > + m_visualUpdatesAllowed = false; > + m_visualUpdatesSuppressionTimer.stop(); > + Why can't we do this in the page cache case? > Source/WebCore/dom/Document.cpp:4556 > + ASSERT(m_frame); > + m_frame->clearTimers(); It seems like the page cache code also clears timers. Why can't we move this into documentWillSuspend? > Source/WebCore/dom/Document.cpp:4569 > + documentDidResume(); > + > + resumeActiveDOMObjects(ActiveDOMObject::PageWillBeSuspended); > + resumeScriptedAnimationControllerCallbacks(); > + > + ASSERT(m_frame); > + m_frame->animation().resumeAnimationsForDocument(this); > + > + m_visualUpdatesAllowed = true; Ditto. All of these changes should be sharable with page cache code. > Source/WebCore/history/CachedFrame.cpp:139 > + if (frame.page()->isTabSuspended()) > + frame.page()->setIsTabSuspended(false); How can a page be in a page cache and be tab-suspended given we're already clearing this flag in CachedFrame::CachedFrame? And why is it okay for it to be resumed as if it was in the page cache? > Source/WebCore/history/CachedFrame.cpp:172 > + if (frame.page()->isTabSuspended()) > + frame.page()->setIsTabSuspended(false); Why is it okay to pretend as if the page had never been tab-suspended here? > Source/WebCore/loader/FrameLoader.cpp:1825 > + if (m_frame.page()->isTabSuspended()) > + m_frame.page()->setIsTabSuspended(false); When can we get into this state? Either CachedFrameBase::restore or this function is getting called first, and only one or the other should be needed. > Source/WebCore/page/Page.cpp:146 > +bool Page::s_tabSuspension = false; I would call this s_tabSuspensionIsEnabled. > Source/WebCore/page/Page.cpp:1301 > + m_pageActivityState = activityState; > + setIsTabSuspended(!isVisible()); I don't think it's safe to synchronously suspend a page here in the case a media RefCounter token issued by PageThrottler got deleted because that could happen during a page destruction and other undesirable timing. > Source/WebCore/page/Page.cpp:1311 > + > + setIsTabSuspended(!isVisible); setViewState calls setIsVisibleInternal so why is this even needed? Also, this function appears to be only called in WebKit1. > Source/WebCore/page/Page.cpp:1350 > + > + setIsTabSuspended(!isVisible); Again, I don't think we can synchronously suspend a document here. For example, a page created by pre-render will be an invisible page but I don't think we want to suspend that here. But we can't check whether we're in the pre-render mode or not until setIsPrersnder is called in WebPage::WebPage (WebKit2/WebProcess/WebPage/WebPage.cpp). > Source/WebCore/page/Page.cpp:1836 > + return s_tabSuspension && !(m_pageActivityState & PageActivityState::UnsuspendableFlags) && PageCache::singleton().canCache(this); We should also exclude pre-render pages. > Source/WebCore/page/Page.cpp:1859 > +void Page::setTabSuspension(bool enable) should be called setTabSuspensionEnabled. > Source/WebCore/page/PageThrottler.h:54 > static const Flags AllFlags = UserInputActivity | AudiblePlugin | MediaActivity | PageLoadActivity; > + static const Flags UnsuspendableFlags = MediaActivity | PageLoadActivity; Why is it okay to suspend when we have an audible plugin and/or user input activity?

Created attachment 267153 [details] Patch

Comment on attachment 267153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267153&action=review > Source/WebCore/dom/Document.cpp:4507 > + if (m_isSuspended) > + return; > + You should explain why you're adding this check in the change log as a per-function comment. > Source/WebCore/dom/Document.cpp:4520 > + page()->lockAllOverlayScrollbarsToHidden(true); It's a bit weird that we lock overlay scrollbars whenever a page is in the background but I suppose we can fix it later if someone complains. > Source/WebCore/loader/FrameLoader.cpp:1823 > + You should remove this superfluous change. > Source/WebCore/page/Page.cpp:1835 > + return s_tabSuspensionIsEnabled && !m_isPrerender && !m_pageActivityState && PageCache::singleton().canCache(this); You should compare m_pageActivityState against NoFlags. > Source/WebCore/page/Page.cpp:1868 > +void Page::timerFired() This function name is too generic for Page. How about tabSuspensionTimerFired? > Source/WebCore/page/Page.cpp:1870 > + setIsTabSuspended(m_isTabSuspended); We should check canTabSuspend() again here because it may have changed since the last time we checked. Also, we should probably not call setIsTabSuspended if m_isTabSuspended was false. > Source/WebCore/page/Page.h:662 > + Timer m_suspensionTimer; For consistency, this timer should probably be called m_tabSuspensionTimer.

Created attachment 267167 [details] Patch

Comment on attachment 267167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267167&action=review > Source/WebKit2/ChangeLog:7 > + You should mention that you're adding a runtime flag to enable this feature via user defaults. > Source/WebCore/page/Page.cpp:1855 > +void Page::scheduleTabSuspension(bool shouldSuspend) It's weird for a function that can synchronously resume a document to be called scheduleTabSuspension but I can't think of a better name now. > Source/WebCore/page/Page.cpp:1870 > +void Page::timerFired() This should be renamed to tabSuspensionTimerFired.

Created attachment 267181 [details] Patch

Comment on attachment 267181 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267181&action=review > Source/WebCore/page/Page.cpp:146 > +bool Page::s_tabSuspensionIsEnabled = false; Shouldn't this be a setting instead? > Source/WebCore/page/Page.cpp:227 > + , m_tabSuspensionTimer(*this, &Page::tabSuspensionTimerFired) nit: Could be a lambda to avoid adding a new method, considering the implementation is trivial. > Source/WebCore/page/Page.cpp:1297 > + if (m_pageActivityState == activityState) How can this happen? I see one call site in PageThrottler::setActivityFlag() and it already does this check. > Source/WebCore/page/Page.cpp:1301 > + m_pageActivityState = activityState; Seems to me we could ask m_pageThrottler for the current pageActivityState when needed and avoid adding a data member to Page for this. > Source/WebCore/page/Page.cpp:1850 > +void Page::setTabSuspensionEnabled(bool enable) setting? > Source/WebCore/page/Page.cpp:1867 > + m_isTabSuspensionScheduled = shouldSuspend; This seems confusing. If canTabSuspend() returns false, this may be true even though we did not schedule anything. I think the problem is the naming, how about m_shouldSuspendTab? > Source/WebCore/page/Page.h:663 > + bool canTabSuspend(); We normally try to avoid mixing methods and members. The methods should probably be declared higher in the class.

Created attachment 267222 [details] Patch

Comment on attachment 267222 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267222&action=review > Source/WebCore/history/PageCache.cpp:152 > + if (frame.document()&&!frame.document()->canSuspendActiveDOMObjectsForDocumentSuspension(&unsuspendableObjects)) { Nit: Need spaces around &&. > Source/WebCore/page/Page.cpp:1840 > + for (Frame* frame = &mainFrame(); frame; frame = frame->tree().traverseNext()) { We can use auto* for frame as well. > Source/WebCore/page/Page.cpp:1874 > + setIsTabSuspended(m_shouldTabSuspend); We don't need to check m_shouldTabSuspend since m_shouldTabSuspend should always be true at this point. Why don't we ASSERT(m_shouldTabSuspend) instead?

Created attachment 267224 [details] Patch

Comment on attachment 267224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267224&action=review > Source/WebCore/page/Page.cpp:1299 > + if (!activityState && !isVisible()) We should check against PageActivityState::NoFlags as well.

Created attachment 267225 [details] Patch

Created attachment 267227 [details] Patch

Comment on attachment 267227 [details] Patch Clearing flags on attachment: 267227 Committed r194006: <http://trac.webkit.org/changeset/194006>

All reviewed patches have been landed. Closing bug.