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

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> 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 203099: Patch
https://bugs.webkit.org/attachment.cgi?id=203099&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=203099&action=review


> Source/WebCore/ChangeLog:10
> +	   exiting throttling state, and adds a hysteresis to we can

"to we can" -> "so we can"

> Source/WebCore/html/HTMLMediaElement.h:759
> +    PageThrottler* getThrottler();

We don't use get in function names unless there's an out param. Can this be
|const|?

> Source/WebCore/html/HTMLMediaElement.h:760
> +    PageThrottler* throttlerIfPresent();

existingPageThrottler()?

> Source/WebCore/html/HTMLMediaElement.h:761
> +    RefPtr<PageThrottler> m_pageThrottler;

Why do you need to store a reference to this? Why not just get it from Page
when you need it?

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

What does "active" mean?

> Source/WebCore/page/Page.cpp:234
> +    m_pageThrottler->clearPage();

Why not just m_pageThrottler = nullptr?

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

PageThrottler is such an awkward name.

It's not clear to me that you gain much by putting throttle-related logic into
its own class, since it has the same lifetime as Page. You just add ambiguity
about lifetimes and aliasing.

> Source/WebCore/page/Page.h:122
> +    void suspendScriptedAnimations() { m_scriptedAnimationsSuspended = true;
}
> +    void resumeScriptedAnimations() { m_scriptedAnimationsSuspended = false;
}

suspendScriptedAnimations and resumeScriptedAnimations are function names that
imply that they do work, but these don't, so the names are confusing. Maybe
noteScriptedAnimationsSuspended() etc?

> Source/WebCore/page/Page.h:129
> +    void startUnthrottledEvent();

What does this mean?

I think it means "preventThrottling".

> Source/WebCore/page/Page.h:130
> +    void reportInterestingEvent();

"interesting"?

> Source/WebCore/page/Page.h:131
> +    void stopUnthrottledEvent();

allowThrottling?

> Source/WebCore/page/Page.h:150
> +    void throttleTheWorld();
> +    void unthrottleTheWorld();

Really, the world? All Pages? All Documents?

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

Is Page the correct scope for this throttling behavior? What happens on
navigation? What about the Page Cache?

> Source/WebKit2/Shared/mac/ChildProcessMac.mm:62
> +static const double kSuspensionHysteresis = 2.0;

Is this a time value? The name should make it clear.