[webkit-reviews] review denied: [Bug 116893] Add more accurate activity state tracking : [Attachment 203099] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue May 28 16:20:18 PDT 2013
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.
More information about the webkit-reviews
mailing list