Bug 138450 – De-templatize Timer

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

De-templatize Timer

[Jer Noble]

Created attachment 241110 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/Modules/geolocation/Geolocation.h:189:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/Timer.h:139:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 280 files


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

[Anders Carlsson]

Comment on attachment 241110 [details]
Patch

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

> Source/WebCore/platform/Timer.h:125
> -    Timer(TimerFiredClass* object, TimerFiredFunction function)
> +    template <typename TimerFiredClass, typename TimerFiredBaseClass>
> +    using is_compatible = typename std::enable_if<std::is_base_of<TimerFiredBaseClass, TimerFiredClass>::value>::type*;

I don't think we need to do this.

> Source/WebCore/platform/Timer.h:137
> +    template <typename TimerFiredClass, typename TimerFiredBaseClass, is_compatible<TimerFiredBaseClass, TimerFiredClass> = nullptr>
> +    Timer(TimerFiredClass* object, TimerFiredFunction<TimerFiredBaseClass> function)
>          : m_function(std::bind(function, object, std::ref(*this)))
>      {
>      }
>  
> -    Timer(TimerFiredClass* object, DeprecatedTimerFiredFunction function)
> +    template <typename TimerFiredClass, typename TimerFiredBaseClass, is_compatible<TimerFiredBaseClass, TimerFiredClass> = nullptr>
> +    Timer(TimerFiredClass* object, DeprecatedTimerFiredFunction<TimerFiredBaseClass> function)
>          : m_function(std::bind(function, object, this))
>      {
>      }

How about something like this instead?

    template<typename TimerFiredClass, typename TimerFiredMemberFunctionClass>
    Timer(TimerFiredClass* object, void (TimerFiredMemberFunctionClass::*function)(Timer<TimerFiredClass>&))
        : m_function(std::bind(function, object, std::ref(*this)))
    {
    }

    template<typename TimerFiredClass, typename TimerFiredMemberFunctionClass>
    Timer(TimerFiredClass* object, void (TimerFiredMemberFunctionClass::*function)(Timer<TimerFiredClass>*))
        : m_function(std::bind(function, object, this))
    {
    }

I also think we should deprecate both the constructors above and add

    template<typename TimerFiredClass, typename TimerFiredMemberFunctionClass>
    Timer(TimerFiredClass& object, void (TimerFiredMemberFunctionClass::*function)())
        : m_function(std::bind(function, &object))
    {

which has two advantages:
    1. It doesn't require the member function to take a Timer - we almost never do anything with the timer anyway. Those clients who want a timer can use std::bind with the constructor below.
    2. It takes the object by reference since we can guarantee that its' not null.

> Source/WebCore/platform/Timer.h:140
> +        : m_function(function)

This should use WTF::move.

[Jer Noble]

Looks like the Windows build failure is due to this MSVC bug: <https://connect.microsoft.com/VisualStudio/feedback/details/800231/c-11-alias-template-issue>

[Jer Noble]

(In reply to comment #3)
> Comment on attachment 241110 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241110&action=review
> 
> > Source/WebCore/platform/Timer.h:125
> > -    Timer(TimerFiredClass* object, TimerFiredFunction function)
> > +    template <typename TimerFiredClass, typename TimerFiredBaseClass>
> > +    using is_compatible = typename std::enable_if<std::is_base_of<TimerFiredBaseClass, TimerFiredClass>::value>::type*;
> 
> I don't think we need to do this.

Okay. I added it because, without it, passing in an incompatible ends up with a really obscure error message from std::bind(). But we're all used to obscure error messages by now. :)

> > Source/WebCore/platform/Timer.h:137
> > +    template <typename TimerFiredClass, typename TimerFiredBaseClass, is_compatible<TimerFiredBaseClass, TimerFiredClass> = nullptr>
> > +    Timer(TimerFiredClass* object, TimerFiredFunction<TimerFiredBaseClass> function)
> >          : m_function(std::bind(function, object, std::ref(*this)))
> >      {
> >      }
> >  
> > -    Timer(TimerFiredClass* object, DeprecatedTimerFiredFunction function)
> > +    template <typename TimerFiredClass, typename TimerFiredBaseClass, is_compatible<TimerFiredBaseClass, TimerFiredClass> = nullptr>
> > +    Timer(TimerFiredClass* object, DeprecatedTimerFiredFunction<TimerFiredBaseClass> function)
> >          : m_function(std::bind(function, object, this))
> >      {
> >      }
> 
> How about something like this instead?
> 
>     template<typename TimerFiredClass, typename
> TimerFiredMemberFunctionClass>
>     Timer(TimerFiredClass* object, void
> (TimerFiredMemberFunctionClass::*function)(Timer<TimerFiredClass>&))
>         : m_function(std::bind(function, object, std::ref(*this)))
>     {
>     }

You'd need "Timer&" instead of "Timer<TimerFiredClass>&", but that'd work. That should fix the Windows build too (which seems to be triggered by "using = member function ptr".

>     template<typename TimerFiredClass, typename
> TimerFiredMemberFunctionClass>
>     Timer(TimerFiredClass* object, void
> (TimerFiredMemberFunctionClass::*function)(Timer<TimerFiredClass>*))
>         : m_function(std::bind(function, object, this))
>     {
>     }
> 
> I also think we should deprecate both the constructors above and add
> 
>     template<typename TimerFiredClass, typename
> TimerFiredMemberFunctionClass>
>     Timer(TimerFiredClass& object, void
> (TimerFiredMemberFunctionClass::*function)())
>         : m_function(std::bind(function, &object))
>     {
> 
> which has two advantages:
>     1. It doesn't require the member function to take a Timer - we almost
> never do anything with the timer anyway. Those clients who want a timer can
> use std::bind with the constructor below.

Yep!

>     2. It takes the object by reference since we can guarantee that its' not
> null.

Yep!

> > Source/WebCore/platform/Timer.h:140
> > +        : m_function(function)
> 
> This should use WTF::move.

Ok.

[Jer Noble]

Created attachment 241116 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/Modules/geolocation/Geolocation.h:189:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/Timer.h:136:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 280 files


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

[Jer Noble]

Created attachment 241119 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/Modules/geolocation/Geolocation.h:189:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/Timer.h:136:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 277 files


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

[Jer Noble]

Committed r175719: <http://trac.webkit.org/changeset/175719>

[Jer Noble]

<rdar://problem/19269065>