[webkit-reviews] review granted: [Bug 129551] Split UserActivity, simplify PageThrottler : [Attachment 225575] Fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 129551: Split UserActivity, simplify PageThrottler
https://bugs.webkit.org/show_bug.cgi?id=129551

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

------- Additional Comments from Darin Adler <darin at apple.com>
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