Bug 131696 – Only webkit2 pages should have a page throttler.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Stephanie Lewis]

Created attachment 229404 [details]
patch

The page throttler controls app nap for WebKit2 web processes.  It shouldn't be created for WebKit1 views, SVG images, and the Web inspector.  This led to bugs where we would keep ourselves out of app nap because these extra page throttlers weren't tracking the correct states.

Related radars:
<rdar://problem/16473045> Visibility state counters are often incorrect
<rdar://problem/15511684> High CPU usage in WebContent
<rdar://problem/16218996> battery menu says safari using a lot of power, but i havent really used it in quite a while

[WebKit Commit Bot]

Attachment 229404 [details] did not pass style-queue:


ERROR: Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebCore/page/PageThrottler.cpp:41:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1818:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 4 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 229404 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229404&action=review

> Source/WebCore/page/Page.cpp:1116
> +    m_pageThrottler = PageThrottler::create(*this, m_viewState);

Would be nice to assert !m_pageThrottler here. And it should be using make_unique instead of reference counting:

    m_pageThrottler = std::make_unique<PageThrottler>(*this, m_viewState);

> Source/WebCore/page/Page.h:388
> +    void initializePageThrottler();

I think this function should be called createPageThrottler. “initialize” is something you do to something that already exists.

> Source/WebCore/page/Page.h:545
> +    RefPtr<PageThrottler> m_pageThrottler;

Should be std::unique_ptr<PageThrottler>.

> Source/WebCore/page/PageThrottler.cpp:35
> +#include <wtf/text/CString.h>

Why was this needed?

> Source/WebCore/page/PageThrottler.cpp:42
> +PassRefPtr<PageThrottler> PageThrottler::create(Page& page, ViewState::Flags viewState)
> +{
> +     return adoptRef(new PageThrottler(page, viewState));
> +}

Don’t need this.

> Source/WebCore/page/PageThrottler.cpp:102
> -
> +    

Please don’t add this whitespace.

> Source/WebCore/page/PageThrottler.h:44
> +class PageThrottler : public RefCounted<PageThrottler> {

Reference counting does not seem appropriate here; there’s no need for sharing. Lets use std::unique_ptr and have this be single ownership. We should just leave this class alone entirely, and use make_unique to create it.

> Source/WebCore/page/Settings.cpp:661
> +    if (m_page->pageThrottler())
> +    m_page->pageThrottler()->hiddenPageDOMTimerThrottlingStateChanged();

Need to indent the body of the if statement.

[Stephanie Lewis]

Created attachment 229500 [details]
patch with unique ptr

[WebKit Commit Bot]

Attachment 229500 [details] did not pass style-queue:


ERROR: Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebCore/page/PageThrottler.cpp:41:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1819:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 4 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephanie Lewis]

Created attachment 229502 [details]
new patch for real this time

oops that was the original patch

[WebKit Commit Bot]

Attachment 229502 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1819:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Comment on attachment 229502 [details]
new patch for real this time

View in context: https://bugs.webkit.org/attachment.cgi?id=229502&action=review

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1750
> +    if (m_page->pageThrottler())

WebPage's m_page should always have a throttler (per call to createPageThrottler, above).
This should just be an ASSERT.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1819
> +    if(m_page->pageThrottler())

ditto.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1851
> +    if (m_page->pageThrottler())

ditto.

[Stephanie Lewis]

Committed http://trac.webkit.org/changeset/167523