[Webkit-unassigned] [Bug 150498] Make every port implement MainThreadSharedTimer instead of using global functions

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=150498

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #263924|review?                     |review+
              Flags|                            |

--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 263924
  --> https://bugs.webkit.org/attachment.cgi?id=263924
Try to fix Mac build

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

This is a little better, although not a huge improvement. I think there is some other strangeness here that can be fixed.

> Source/WebCore/platform/SharedTimer.h:52
>      class MainThreadSharedTimer final : public SharedTimer {

This class uses global state, but instead it should be a singleton. We would replace the mainThreadSharedTimer() function from ThreadTimers with a MainThreadSharedTimer::singleton() function that returns a reference to a single global shared timer.

The code for this should go into MainThreadSharedTimer.cpp and this class should get its own header MainThreadSharedTimer.h instead of being tacked on to the bottom of SharedTimer.h. If you don’t want to add those two new files in this patch, you could instead put the code into ThreadTimers.cpp for now, but that would just be a stopgap until we can do it right.

> Source/WebCore/platform/SharedTimer.h:54
>          void setFiredFunction(void (*function)()) override

This function’s body should not be in the header. Instead it should be in MainThreadSharedTimer.cpp (or ThreadTimers.cpp for now, I guess).

> Source/WebCore/platform/SharedTimer.h:62
> +        void setFireInterval(double) override;
> +        void stop() override;
> +        void invalidate() override;

Can any of these be private?

> Source/WebCore/platform/SharedTimer.h:64
> +        static void timerFired()

This function’s body should not be in the header. Instead it should be in MainThreadSharedTimer.cpp (or ThreadTimers.cpp for now, I guess).

The class is named timer, so I think this should just be named fired(), not timerFired().

This should be a member function, not a static member function; callers can call MainThreadSharedTimer::singleton().fired(). We should also consider making it private if we can. Unfortunately doing that means all the thread-specific callback functions would need to be either static member functions or friend functions so they could access this. But it’s peculiar to have this be public.

> Source/WebCore/platform/SharedTimer.h:70
> +        static void (*s_firedFunction)();

This should be a non-static member, and also should probably be initialized to nullptr here in the header once it is like that.

> Source/WebCore/platform/cf/SharedTimerCF.cpp:41
> +void (*MainThreadSharedTimer::s_firedFunction)() = nullptr;

We don’t want code like this to be repeated for each platform. This is only here because we don’t have a MainThreadSharedTimer.cpp to put it in. For now, I suggest putting code like this in ThreadTimers.cpp or creating MainThreadSharedTimer.cpp. But we won’t need this if we change the function to be a member instead of a static member.

> Source/WebCore/platform/cf/SharedTimerCF.cpp:75
> +    MainThreadSharedTimer::timerFired();

This should be:

    MainThreadSharedTimer::singleton().timerFired();

> Source/WebCore/platform/gtk/SharedTimerGtk.cpp:40
> +    ASSERT(MainThreadSharedTimer::s_firedFunction);

No need to repeat the class name when we are inside a member function of that class.

> Source/WebCore/platform/gtk/SharedTimerGtk.cpp:44
> +    gSharedTimer.scheduleAfterDelay("[WebKit] sharedTimerTimeoutCallback", [this] { MainThreadSharedTimer::timerFired(); },

No need to repeat the class name when we are inside a member function of that class.

> Source/WebCore/platform/win/SharedTimerWin.cpp:128
> +    ASSERT(MainThreadSharedTimer::s_firedFunction);

No need to repeat the class name when we are inside a member function of that class.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151024/d3da724e/attachment.html>