Bug 139776 - Add strong typing to RefCounter interface, return value as a bool.
Summary: Add strong typing to RefCounter interface, return value as a bool.
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Barraclough
Depends on:
Blocks: 139744
  Show dependency treegraph
Reported: 2014-12-18 09:48 PST by Gavin Barraclough
Modified: 2014-12-18 18:12 PST (History)
2 users (show)

See Also:

Fix (31.47 KB, patch)
2014-12-18 11:24 PST, Gavin Barraclough
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2014-12-18 09:48:32 PST
Currently all token vended by a RefCounter have the same type – Ref<RefCounter::Count>. This means there is no compile time type checking to prevent mistakes. Update the count() method to token<>(), templated on type used to identify the token being returned. Calls to token<T>() will return a result of type RefCounter::Token<T>.

There are a few problems with the fact the counter will return you an exact count of the number of outstanding tokens:
  - It is desirable to only fire the callback on zero-edge changes; it is more consistent to do so if the value is only readable as a boolean.
  - It is desirable to provide the value as an argument to the callback, however to make this useful for integer values it is also necessary to indicate the direction of change (0->1 is often interesting where 2->1 is not).
  - There is a mismatch between the precision of returning a count, and the inherent imprecision of a token based mechanism, where it may be difficult to guarantee absolutely no unnecessary refcount churn, and thus unintentional counter values.
Comment 1 Gavin Barraclough 2014-12-18 11:24:14 PST
Created attachment 243505 [details]
Comment 2 Geoffrey Garen 2014-12-18 11:30:10 PST
Comment on attachment 243505 [details]

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


> Source/WTF/ChangeLog:8
> +        Currently all token vended by a RefCounter have the same type â Ref<RefCounter::Count>.

No squiggles, please.

> Source/WebKit2/UIProcess/Plugins/mac/PluginProcessManagerMac.mm:37
> -void PluginProcessManager::updateProcessSuppressionState()
> +void PluginProcessManager::updateProcessSuppressionDisabled(bool disabled)
>  {
> -    bool processSuppressionEnabled = !m_processSuppressionDisabledForPageCounter.value();
> -    if (m_processSuppressionEnabled == processSuppressionEnabled)
> -        return;
> -    
> -    m_processSuppressionEnabled = processSuppressionEnabled;
> +    bool enabled = !disabled;

This is not my least favorite way not to do things.
Comment 3 Gavin Barraclough 2014-12-18 16:52:06 PST
Transmitting file data ...................
Committed revision 177541.
Comment 4 Brent Fulgham 2014-12-18 17:03:24 PST
This breaks the Windows build and perhaps others.
Comment 5 Brent Fulgham 2014-12-18 17:04:37 PST
Build errors:

5>  RefCounter.cpp
5>c:\projects\webkit\opensource\source\wtf\wtf\RefCounter.h(105): warning C4812: obsolete declaration style: please use 'WTF::RefCounter::Token<T>::Token' instead
5>c:\projects\webkit\opensource\source\wtf\wtf\RefCounter.h(105): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
5>c:\projects\webkit\opensource\source\wtf\wtf\RefCounter.h(105): error C2751: 'WTF::RefCounter::Token<T>::{ctor}' : the name of a function parameter cannot be qualified
5>c:\projects\webkit\opensource\source\wtf\wtf\RefCounter.h(105): warning C4346: 'WTF::RefCounter::Token<T>::{ctor}' : dependent name is not a type
5>          prefix with 'typename' to indicate a type
5>c:\projects\webkit\opensource\source\wtf\wtf\RefCounter.h(105): error C2143: syntax error : missing ',' before '&'
5>c:\projects\webkit\opensource\source\wtf\wtf\RefCounter.h(108): error C2244: 'WTF::RefCounter::Token<T>::{ctor}' : unable to match function definition to an existing declaration
5>          definition
5>          'WTF::RefCounter::Token<T>::Token(const int)'
5>          existing declarations
5>          'WTF::RefCounter::Token<T>::Token(WTF::RefCounter::Count *)'
5>          'WTF::RefCounter::Token<T>::Token(WTF::RefCounter::Token<T> &&)'
5>          'WTF::RefCounter::Token<T>::Token(const WTF::RefCounter::Token<T> &)'
5>          'WTF::RefCounter::Token<T>::Token(std::nullptr_t)'
5>          'WTF::RefCounter::Token<T>::Token(void)'
5>c:\projects\webkit\opensource\source\wtf\wtf\RefCounter.h(111): warning C4812: obsolete declaration style: please use 'WTF::RefCounter::Token<T>::Token' instead
5>c:\projects\webkit\opensource\source\wtf\wtf\RefCounter.h(111): warning C4551: function call missing argument list
5>c:\projects\webkit\opensource\source\wtf\wtf\RefCounter.h(111): error C2277: 'WTF::RefCounter::Token<T>::{ctor}' : cannot take address of this member function
5>c:\projects\webkit\opensource\source\wtf\wtf\RefCounter.h(111): error C2568: '&&' : unable to resolve function overload
5>          c:\projects\webkit\opensource\source\wtf\wtf\RefCounter.h(74): could be 'WTF::RefCounter::Token<T>::Token(WTF::RefCounter::Count *)'
5>          c:\projects\webkit\opensource\source\wtf\wtf\RefCounter.h(64): or       'WTF::RefCounter::Token<T>::Token(WTF::RefCounter::Token<T> &&)'
5>          c:\projects\webkit\opensource\source\wtf\wtf\RefCounter.h(63): or       'WTF::RefCounter::Token<T>::Token(const WTF::RefCounter::Token<T> &)'
5>          c:\projects\webkit\opensource\source\wtf\wtf\RefCounter.h(62): or       'WTF::RefCounter::Token<T>::Token(std::nullptr_t)'
5>          c:\projects\webkit\opensource\source\wtf\wtf\RefCounter.h(61): or       'WTF::RefCounter::Token<T>::Token(void)'
5>c:\projects\webkit\opensource\source\wtf\wtf\RefCounter.h(114): error C2433: 'WTF::RefCounter::Token<T>::{ctor}' : 'inline' not permitted on data declarations
5>c:\projects\webkit\opensource\source\wtf\wtf\RefCounter.h(114): warning C4346: 'WTF::RefCounter::Token<T>::{ctor}' : dependent name is not a type
5>          prefix with 'typename' to indicate a type
5>c:\projects\webkit\opensource\source\wtf\wtf\RefCounter.h(114): error C2350: 'WTF::RefCounter::Token<T>::{ctor}' is not a static member
Comment 6 Brent Fulgham 2014-12-18 17:11:02 PST
This compiles on Windows, is this a comparable implementation?

Index: Source/WTF/wtf/RefCounter.h
--- Source/WTF/wtf/RefCounter.h (revision 177541)
+++ Source/WTF/wtf/RefCounter.h (working copy)
@@ -102,13 +102,13 @@

 template<class T>
-inline RefCounter::Token<T>::Token(const RefCounter::Token<T>::Token<T>& token)
+inline RefCounter::Token<T>::Token(const RefCounter::Token<T>& token)
     : m_ptr(token.m_ptr)

 template<class T>
-inline RefCounter::Token<T>::Token(RefCounter::Token<T>::Token<T>&& token)
+inline RefCounter::Token<T>::Token(RefCounter::Token<T>&& token)
     : m_ptr(token.m_ptr)
Comment 7 Brent Fulgham 2014-12-18 17:15:46 PST
Reviewed my proposed change with Gavin.

Committed r177544.
Comment 8 Gavin Barraclough 2014-12-18 17:42:32 PST
Committed revision 177549.
Comment 9 Beth Dakin 2014-12-18 18:12:46 PST
Build fix: http://trac.webkit.org/changeset/177553