[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 #24 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 267181
  --> https://bugs.webkit.org/attachment.cgi?id=267181
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.

-- 
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/20151211/af60d3af/attachment.html>