Split UserActivity, simplify PageThrottler

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Gavin Barraclough]

The class UserActivity currently implements two things – a hysteresis mechanism, and an abstraction of NSActivity controlled by that mechanism. PageThrottler implements its own hysteresis mechanism, which directly controls DOM timer throttling and also controls a couple of UserActivities, giving a total of 3 separate hysteresis mechanisms, layered two deep.

Split UserActivity into three, with HysteresisActivity implementing an abstract hysteresis mechanism, UserActivity::Impl controlling the NSActivity, and then UserActivity combining these two back together. The interface to UserActivity is unchanged.

Remove PageThrottler's bespoke hysteresis, replacing it with a use of HysteresisActivity. Replace the two UserActivities with a single UserActivity::Impl, so there are no longer layered hysteresis mechanisms.

[Gavin Barraclough]

Created attachment 225575 [details]
Fix

[Darin Adler]

Comment on attachment 225575 [details]
Fix

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

> Source/WebCore/platform/HysteresisActivity.h:42
> +    UserActivity(const char* description);

Should mark this explicit.

> Source/WebCore/platform/HysteresisActivity.h:56
> +private:
> +#if HAVE(NS_ACTIVITY)
> +    void hysteresisTimerFired();
> +    bool isValid();
> +
> +    bool m_active;
> +    RetainPtr<NSString> m_description;
> +    RunLoop::Timer<UserActivity> m_timer;
> +    RetainPtr<id> m_activity;
> +#endif

I think the private has to go inside the #if or we won’t compile with it off. Maybe that’s one reason for the red I see below in EWS.

> Source/WebCore/platform/HysteresisActivity.h:36
> +    HysteresisActivity(Delegate* delegate, double hysteresisSeconds = DefaultHysteresisSeconds)

Can delegate be a reference instead of a pointer? I think that would be much better.

Also, I think this constructor should be marked explicit.

> Source/WebCore/platform/HysteresisActivity.h:74
> +    constexpr static const double DefaultHysteresisSeconds = 5.0;

I don’t think we have working constexpr on the Windows compiler. We maybe were working around it with a macro? I’d ask Anders about this.

> Source/WebCore/platform/UserActivity.h:45
> +        Impl(const char* description);

explicit

> Source/WebCore/platform/UserActivity.h:57
> +    UserActivity(const char* description);

explicit

[Gavin Barraclough]

Created attachment 225590 [details]
EWS fixes

[Gavin Barraclough]

Created attachment 225603 [details]
GTK fix

[Gavin Barraclough]

Transmitting file data ............
Committed revision 164948.