[webkit-reviews] review denied: [Bug 116893] Add more accurate activity state tracking : [Attachment 203219] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Oliver Hunt <oliver at apple.com>'s request
for review:
Bug 116893: Add more accurate activity state tracking
https://bugs.webkit.org/show_bug.cgi?id=116893

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

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=203219&action=review


> Source/WebCore/html/HTMLMediaElement.h:760
> +    PageThrottler* pageThrottlerIfPossible();
> +    RefPtr<PageThrottler> m_pageThrottler;

Why does this need a member? Can't it just be accessed through the page?

> Source/WebCore/page/ChromeClient.h:370
> +    virtual void incrementActivePageCount() { }
> +    virtual void decrementActivePageCount() { }

These aren't the most descriptive names.  If we can't come up with something
more descriptive, can you add comments explaining what these mean?

> Source/WebCore/page/Page.h:115
> +class Page;
> +
> +class PageThrottler : public RefCounted<PageThrottler> {
> +public:

Please put this in its own header.

> Source/WebCore/page/Page.h:119
> +    static PassRefPtr<PageThrottler> create(Page* page, ChromeClient*
chromeClient)
> +    {
> +	   return adoptRef(new PageThrottler(page, chromeClient));
> +    }

If you have the Page, why do you need the ChromeClient?

> Source/WebCore/page/Page.h:138
> +    typedef enum {
> +	   PageNotThrottledState,
> +	   PageWaitingToThrottleState,
> +	   PageThrottledState
> +    } PageThrottleState;

C++ doesn't require enum's to use this typedef notation.

> Source/WebKit2/Shared/ChildProcess.h:74
> +#else
> +    void incrementActiveTaskCount() { }
> +    void decrementActiveTaskCount() { }
>  #endif

If these are called from shared code, please put their empty implementations in
ChildProcess.cpp with other functions that only have Mac implementations.

> Source/WebKit2/Shared/ChildProcess.h:122
> +    void suspensionHysteresisTimerFired();
> +    void setProcessSuppressionEnabledInternal(bool);
> +    size_t m_activeTaskCount;
> +    bool m_shouldSuspend;
> +    WebCore::RunLoop::Timer<ChildProcess> m_suspensionHysteresisTimer;
>      RetainPtr<id> m_processSuppressionAssertion;

Are we doing this for more than the WebProcess?  If not, can this move to
WebProcess?

> Source/WebKit2/WebProcess/Plugins/Netscape/NPRuntimeObjectMap.h:102
> +    RefPtr<WebCore::PageThrottler> m_pageThrottler;

Why can't we just access this view the PluginView?