Add more accurate activity state tracking

• Component: New Bugs
• Status: RESOLVED FIXED

[Oliver Hunt]

Add more accurate activity state tracking

[Oliver Hunt]

Created attachment 203098 [details]
Patch

[Early Warning System Bot]

Comment on attachment 203098 [details]
Patch

Attachment 203098 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/740117

[EFL EWS Bot]

Comment on attachment 203098 [details]
Patch

Attachment 203098 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/700108

[Oliver Hunt]

Created attachment 203099 [details]
Patch

[Oliver Hunt]

rdar://13920766

[Oliver Hunt]

<rdar://13785764>

[Anders Carlsson]

Comment on attachment 203099 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203099&action=review

Here are some high-level comments.

> Source/WebCore/html/HTMLMediaElement.cpp:350
> +        m_pageThrottler.clear();

Please use = nullptr instead of .clear().

> Source/WebCore/html/HTMLMediaElement.cpp:2485
> +PageThrottler* HTMLMediaElement::getThrottler()

We don’t use the get prefix for simple getters. I think this should be called throttler().

> Source/WebCore/html/HTMLMediaElement.cpp:2557
> +        if (PageThrottler* throttler = throttlerIfPresent())

I think you can just access m_pageThrottler here.

> Source/WebCore/page/Page.h:116
> +    static PassRefPtr<PageThrottler> create(Page* page, ChromeClient* chromeClient)

This doesn’t have to take both a page and a chrome client. You can get to the chrome client from its page.

> Source/WebCore/page/Page.h:143
> +    static const unsigned kThrottleHysteresis = 2;

This should just go into the implementation file. We also don’t use the k prefix fro constants in WebKit.

> Source/WebCore/page/Page.h:158
> +    PageThrottler(Page*, ChromeClient*);
> +    Page* m_page;
> +    ChromeClient* m_chromeClient;
> +
> +    void throttleTheWorld();
> +    void unthrottleTheWorld();
> +
> +    bool m_scriptedAnimationsSuspended;
> +    unsigned m_activeThrottleBlockers;
> +    PageThrottleState m_throttleState;
> +    void startThrottleHysteresisTimer();
> +    void stopThrottleHysteresisTimer();
> +    void throttleHysteresisTimerFired(DeferrableOneShotTimer<PageThrottler>*);
> +    DeferrableOneShotTimer<PageThrottler> m_throttleHysteresisTimer;

Don’t mix member variables and member function declarations, put the member functions first and then the member variables.

[Simon Fraser (smfr)]

Comment on attachment 203099 [details]
Patch

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.

[Oliver Hunt]

Created attachment 203107 [details]
Patch

[Lucas Forschler]

The content of attachment 203107 [details] has been deleted by
    Lucas Forschler <[e-mail]>
who provided the following reason:

invalid patch

The token used to delete this attachment was generated at 2013-05-28 17:05:10 PST.

[Oliver Hunt]

(In reply to comment #8)
> (From update of attachment 203099 [details])
> 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|?
>

Can't be const, this does real work.

 
> > 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?
> 

Because we may have been detached from the page.

> > Source/WebCore/page/Page.cpp:234
> > +    m_pageThrottler->clearPage();
> 
> Why not just m_pageThrottler = nullptr?
>

Because PageThrottler has a weak reference to page, so those need to be cleared out in the Page destructor.
 
> > Source/WebCore/page/Page.h:114
> > +class PageThrottler : public RefCounted<PageThrottler> {
> 
> PageThrottler is such an awkward name.

Yes

> 
> 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.
> 

No it doesn't - elements can float around without being attached to a page.

> > 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?

Throttling is per page.

> 
> > Source/WebKit2/Shared/mac/ChildProcessMac.mm:62
> > +static const double kSuspensionHysteresis = 2.0;
> 
> Is this a time value? The name should make it clear.

righto

[Oliver Hunt]

Created attachment 203219 [details]
Patch

[Sam Weinig]

Comment on attachment 203219 [details]
Patch

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?

[Oliver Hunt]

(In reply to comment #13)
> (From update of attachment 203219 [details])
> 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?

See my earlier comment - we don't necessarily have a 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.


Urgh, okay.

> 
> > 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?
> 

Will change, there was a time that i thought chrome client was per process, so thought i could use that alone.

> > 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?
>

I thought we might want to use it in other processes in the future.
 
> > Source/WebKit2/WebProcess/Plugins/Netscape/NPRuntimeObjectMap.h:102
> > +    RefPtr<WebCore::PageThrottler> m_pageThrottler;
> 
> Why can't we just access this view the PluginView?

Because i can't find any evidence to indicate that the page will outlast the plugin view.

[Oliver Hunt]

Created attachment 203266 [details]
Patch

[Gavin Barraclough]

Comment on attachment 203266 [details]
Patch

R+ with the discussed fix.

[Oliver Hunt]

Committed r150935: <http://trac.webkit.org/changeset/150935>