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
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 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.
Created attachment 229500 [details] patch with unique ptr
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.
Created attachment 229502 [details] new patch for real this time oops that was the original patch
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 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.
Committed http://trac.webkit.org/changeset/167523