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

-- 
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/20151117/9ddd7155/attachment.html>