Summary: | Generalize PageActivityAssertionToken | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gavin Barraclough <barraclough> | ||||||||||||||
Component: | WebCore Misc. | Assignee: | Gavin Barraclough <barraclough> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | benjamin, calvaris, cmarcelo, commit-queue, ddkilzer, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, japhet, jer.noble, philipj, sergio | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 139108 | ||||||||||||||||
Attachments: |
|
Description
Gavin Barraclough
2014-11-29 16:20:06 PST
Created attachment 242285 [details]
Fix
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.
Created attachment 242287 [details]
Speculative build / style fixes
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. (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. 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. 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). 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.)
Created attachment 242421 [details]
With API test
Created attachment 242422 [details]
With API test, minus non-ascii character in API test
Created attachment 242427 [details]
EWS bot has an issue with the API test that my machine didn't. :-/
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. Committed revision 176683. (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> |