<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Safari background tabs should be fully suspended where possible."
href="https://bugs.webkit.org/show_bug.cgi?id=150515#c18">Comment # 18</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Safari background tabs should be fully suspended where possible."
href="https://bugs.webkit.org/show_bug.cgi?id=150515">bug 150515</a>
from <span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=267122&action=diff" name="attach_267122" title="Patch">attachment 267122</a> <a href="attachment.cgi?id=267122&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=267122&action=review">https://bugs.webkit.org/attachment.cgi?id=267122&action=review</a>
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.
<span class="quote">> Source/WebCore/dom/Document.cpp:4525
> + suspendActiveDOMObjects(ActiveDOMObject::PageCache);
> + suspendScriptedAnimationControllerCallbacks();</span >
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.
<span class="quote">> Source/WebCore/dom/Document.cpp:4548
> + page()->lockAllOverlayScrollbarsToHidden(true);</span >
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?
<span class="quote">> Source/WebCore/dom/Document.cpp:4554
> + m_visualUpdatesAllowed = false;
> + m_visualUpdatesSuppressionTimer.stop();
> +</span >
Why can't we do this in the page cache case?
<span class="quote">> Source/WebCore/dom/Document.cpp:4556
> + ASSERT(m_frame);
> + m_frame->clearTimers();</span >
It seems like the page cache code also clears timers.
Why can't we move this into documentWillSuspend?
<span class="quote">> Source/WebCore/dom/Document.cpp:4569
> + documentDidResume();
> +
> + resumeActiveDOMObjects(ActiveDOMObject::PageWillBeSuspended);
> + resumeScriptedAnimationControllerCallbacks();
> +
> + ASSERT(m_frame);
> + m_frame->animation().resumeAnimationsForDocument(this);
> +
> + m_visualUpdatesAllowed = true;</span >
Ditto. All of these changes should be sharable with page cache code.
<span class="quote">> Source/WebCore/history/CachedFrame.cpp:139
> + if (frame.page()->isTabSuspended())
> + frame.page()->setIsTabSuspended(false);</span >
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?
<span class="quote">> Source/WebCore/history/CachedFrame.cpp:172
> + if (frame.page()->isTabSuspended())
> + frame.page()->setIsTabSuspended(false);</span >
Why is it okay to pretend as if the page had never been tab-suspended here?
<span class="quote">> Source/WebCore/loader/FrameLoader.cpp:1825
> + if (m_frame.page()->isTabSuspended())
> + m_frame.page()->setIsTabSuspended(false);</span >
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.
<span class="quote">> Source/WebCore/page/Page.cpp:146
> +bool Page::s_tabSuspension = false;</span >
I would call this s_tabSuspensionIsEnabled.
<span class="quote">> Source/WebCore/page/Page.cpp:1301
> + m_pageActivityState = activityState;
> + setIsTabSuspended(!isVisible());</span >
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.
<span class="quote">> Source/WebCore/page/Page.cpp:1311
> +
> + setIsTabSuspended(!isVisible);</span >
setViewState calls setIsVisibleInternal so why is this even needed?
Also, this function appears to be only called in WebKit1.
<span class="quote">> Source/WebCore/page/Page.cpp:1350
> +
> + setIsTabSuspended(!isVisible);</span >
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).
<span class="quote">> Source/WebCore/page/Page.cpp:1836
> + return s_tabSuspension && !(m_pageActivityState & PageActivityState::UnsuspendableFlags) && PageCache::singleton().canCache(this);</span >
We should also exclude pre-render pages.
<span class="quote">> Source/WebCore/page/Page.cpp:1859
> +void Page::setTabSuspension(bool enable)</span >
should be called setTabSuspensionEnabled.
<span class="quote">> Source/WebCore/page/PageThrottler.h:54
> static const Flags AllFlags = UserInputActivity | AudiblePlugin | MediaActivity | PageLoadActivity;
> + static const Flags UnsuspendableFlags = MediaActivity | PageLoadActivity;</span >
Why is it okay to suspend when we have an audible plugin and/or user input activity?</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>