Bug 139106 – Generalize PageActivityAssertionToken

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Gavin Barraclough]

PageActivityAssertionToken is a RAII mechanism implementing a counter, used by PageThrottler to count user visible activity in progress on the page (currently page load and media playback). Use of an RAII type is prevents a number of possible errors, including double counting a single media element, or failing to decrement the count after a media element has been deallocated.

Generalizing the mechanism would both make it available for new uses (currently I want to track page load separately from media playback in PageThrottler, and track processes with active pages in WK2), and also may provide a better implementation for some existing accounting mechanisms (tracking processes suppression state across processes/contexts/globally in WK2, tracking outstanding user visible resource requests).

The current implementation has a number of drawbacks that have been addressed by this refactoring:
 - specific to single use in PageThrottler class - not reusable.
 - incomplete encapsulation - the counter and WeakPtrFactory that comprise the current implementation are not encapsulated (are in the client type, PageThrottler).
 - tokens are not shared - PageActivityAssertionToken instances are managed by std::unique, every increment requires an object allocation.
 - redundancy - the current implementation uses a WeakPtr to safely reference the PageThrottler, this is internally implemented using a reference counted type, resulting in two counters being incremented (one in the PageActivityAssertionToken, one in the PageThrottler).

In the reimplementation:
 - a callback is provided via a lambda function, which allows for easy reuse without a lot of boilerplate code.
 - the counter, callback and ownership of the otherwise weakly-owned token is encapsulated within the RefCounter type.
 - a single count within RefCounter::Counter stores the counter value, and also manage the lifetime of this object.
 - standard RefPtrs are used to manage references to the RefCounter::Counter.

[Gavin Barraclough]

Created attachment 242285 [details]
Fix

[WebKit Commit Bot]

Attachment 242285 [details] did not pass style-queue:


ERROR: Source/WebCore/page/PageThrottler.cpp:34:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/RefCounter.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/RefCounter.h:39:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/RefCounter.h:60:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/RefCounter.h:60:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 5 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Created attachment 242287 [details]
Speculative build / style fixes

[Sam Weinig]

Comment on attachment 242287 [details]
Speculative build / style fixes

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

> Source/WTF/wtf/RefCounter.h:57
> +    typedef std::function<void()> ValueDidChangeCallback;

ValueDidChangeCallback is actually more characters than std::function<void()> and I am not sure it aids the clarity here.

[Gavin Barraclough]

(In reply to comment #4)
> Comment on attachment 242287 [details]
> Speculative build / style fixes
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242287&action=review
> 
> > Source/WTF/wtf/RefCounter.h:57
> > +    typedef std::function<void()> ValueDidChangeCallback;
> 
> ValueDidChangeCallback is actually more characters than
> std::function<void()> and I am not sure it aids the clarity here.

No strong opinion on my behalf here.

I found:
    RefCounter::RefCounter(ValueDidChangeCallback valueDidChange)
to be more easily readable than:
    RefCounter::RefCounter(std::function<void()> valueDidChange)

If the code needs to be refactored it can easier if the set of members holding or parameters passing std::functions that are being used for this purpose are typed, and if any code is using lambdas for a mix of these purposes and other it might provide clarity.

On this set of grounds I felt a typedef helped on balance, but I could be convinced otherwise.

[Sam Weinig]

Comment on attachment 242287 [details]
Speculative build / style fixes

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

I like the direction of this change, but think we are loosing a lot of clarity by using the low level RefPtr<RefCounter::Counter> directly everywhere.  I think this could be fixed by wrapping it in a PageActivityAssertionToken class (doesn't have to be unique_ptr anymore, which is nice).

> Source/WTF/wtf/RefCounter.cpp:62
> +    // The Counter object has a is kept alive so long as either the RefCounter that created

Wat?

> Source/WTF/wtf/RefCounter.cpp:74
> +} // namespace WebCore
> +
> +

This has an extra newline.

> Source/WTF/wtf/RefCounter.h:62
> +    PassRefPtr<Counter> counter() const

This should probably return a RefPtr<Counter>, as is our current style.

[Chris Dumez]

Comment on attachment 242287 [details]
Speculative build / style fixes

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

>> Source/WTF/wtf/RefCounter.h:62
>> +    PassRefPtr<Counter> counter() const
> 
> This should probably return a RefPtr<Counter>, as is our current style.

Since we are not passing ownership to the caller, wouldn't it make sense to return a raw pointer instead (or even a reference instead in this case since it cannot be null it seems).

[Gavin Barraclough]

Created attachment 242368 [details]
Review comment fixes. Not for re-review yet; needs an API test.

Per conversation with Sam & Chris switched to PassRef for now. (Open debate on using a plain reference.)

[Gavin Barraclough]

Created attachment 242421 [details]
With API test

[Gavin Barraclough]

Created attachment 242422 [details]
With API test, minus non-ascii character in API test

[Gavin Barraclough]

Created attachment 242427 [details]
EWS bot has an issue with the API test that my machine didn't. :-/

[Sam Weinig]

Comment on attachment 242427 [details]
EWS bot has an issue with the API test that my machine didn't. :-/

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

> Source/WTF/wtf/RefCounter.cpp:44
> +    // The Counter object is kept alive so long as either the RefCounter that created it remains

Counter -> Count.

[Gavin Barraclough]

Committed revision 176683.

[David Kilzer (:ddkilzer)]

(In reply to comment #13)
> Committed revision 176683.

Committed r176683: <http://trac.webkit.org/r176683>

Follow-up build fix in r176832:  <http://trac.webkit.org/r176832>