Bug 131696 - Only webkit2 pages should have a page throttler.
Summary: Only webkit2 pages should have a page throttler.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 131697
  Show dependency treegraph
 
Reported: 2014-04-15 14:49 PDT by Stephanie Lewis
Modified: 2014-04-18 17:50 PDT (History)
12 users (show)

See Also:


Attachments
patch (15.67 KB, patch)
2014-04-15 14:49 PDT, Stephanie Lewis
darin: review-
Details | Formatted Diff | Diff
patch with unique ptr (15.67 KB, patch)
2014-04-16 17:14 PDT, Stephanie Lewis
no flags Details | Formatted Diff | Diff
new patch for real this time (13.91 KB, patch)
2014-04-16 17:18 PDT, Stephanie Lewis
barraclough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephanie Lewis 2014-04-15 14:49:48 PDT
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
Comment 1 WebKit Commit Bot 2014-04-15 14:50:55 PDT
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.
Comment 2 Darin Adler 2014-04-16 10:41:10 PDT
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.
Comment 3 Stephanie Lewis 2014-04-16 17:14:32 PDT
Created attachment 229500 [details]
patch with unique ptr
Comment 4 WebKit Commit Bot 2014-04-16 17:16:18 PDT
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.
Comment 5 Stephanie Lewis 2014-04-16 17:18:09 PDT
Created attachment 229502 [details]
new patch for real this time

oops that was the original patch
Comment 6 WebKit Commit Bot 2014-04-16 17:19:46 PDT
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.
Comment 7 Gavin Barraclough 2014-04-16 19:12:38 PDT
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.
Comment 8 Stephanie Lewis 2014-04-18 17:50:17 PDT
Committed http://trac.webkit.org/changeset/167523