[webkit-reviews] review denied: [Bug 131696] Only webkit2 pages should have a page throttler. : [Attachment 229404] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Stephanie Lewis <slewis at apple.com>'s
request for review:
Bug 131696: Only webkit2 pages should have a page throttler.
https://bugs.webkit.org/show_bug.cgi?id=131696

Attachment 229404: patch
https://bugs.webkit.org/attachment.cgi?id=229404&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.