[Webkit-unassigned] [Bug 150515] Safari background tabs should be fully suspended where possible.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=150515

--- Comment #18 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 267122
  --> https://bugs.webkit.org/attachment.cgi?id=267122
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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151210/2b74c11d/attachment-0001.html>