[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

--- Comment #10 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #9)
> Comment on attachment 263924 [details]
> 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.

Thanks for the review.

> > 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.

I don't mind to add new files as long as someone can help me with the XCode files.

> > 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).

Ok.

> > Source/WebCore/platform/SharedTimer.h:62
> > +        void setFireInterval(double) override;
> > +        void stop() override;
> > +        void invalidate() override;
> 
> Can any of these be private?

No, all of them are used by ThreadTimers.

> > 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().

Sure.

> 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.

Yes, making it private would require changes in CF and win implementations that I'm afraid I can't do my self easily. So, I'll leave a FIXME.

> > 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.

Yes, and not a pointer function but a std::function instead as well, I guess.

> > 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.

Right.

> > 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.

Ok, I'll submit a new patch adding all new files.

-- 
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/20151025/2dad8634/attachment-0001.html>