[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 #33 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #32)
> Comment on attachment 264207 [details]
> Try to fix win build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264207&action=review
> 
> > Source/WebCore/platform/MainThreadSharedTimer.h:46
> > +    // FIXME: this should be private, but CF and Windows implementations
> > +    // need to call this from static methods at the moment.
> 
> I wouldn’t bother with this FIXME even though I agree with what it says.
> 
> If we were keeping it: we normally use sentence capitalization so "this"
> would be capitalized, and I don't think "static methods" is the term we'd
> want to use here, since that is a way to refer to static member functions,
> but if these were in fact static member functions then there would be no
> problem. I think we mean "non-member functions". The fact that those
> functions are also declared with "static" so they get internal linkage is
> not relevant.

Ok, I'll fix the comment then.

> > Source/WebCore/platform/MainThreadSharedTimer.h:52
> > +    std::function<void()> m_firedFunction { nullptr };
> 
> A std::function will be initialized to null without explicitly specifying
> nullptr. Because of that we can simply omit the { nullptr } here.

Ok.

> > Source/WebCore/platform/SharedTimer.h:41
> > +    SharedTimer() { }
> 
> This line of code can simply be deleted. It has no effect.

It seems to be needed by derived classes constructor, if I remove it I get a build failure:

../../Source/WebCore/platform/MainThreadSharedTimer.cpp: In constructor 'WebCore::MainThreadSharedTimer::MainThreadSharedTimer()':
../../Source/WebCore/platform/MainThreadSharedTimer.cpp:37:46: error: no matching function for call to 'WebCore::SharedTimer::SharedTimer()'
In file included from ../../Source/WebCore/platform/MainThreadSharedTimer.h:30:0,
                 from ../../Source/WebCore/platform/MainThreadSharedTimer.cpp:27:
../../Source/WebCore/platform/SharedTimer.h:39:14: note: candidate: WebCore::SharedTimer::SharedTimer(const WebCore::SharedTimer&) <deleted>
../../Source/WebCore/platform/SharedTimer.h:39:14: note:   candidate expects 1 argument, 0 provided

I guess I can do SharedTimer() = default; instead, though.

> > Source/WebCore/platform/cf/MainThreadSharedTimerCF.cpp:60
> > +    static PowerObserver* powerObserver;
> > +    if (!powerObserver)
> > +        powerObserver = std::make_unique<PowerObserver>(restartSharedTimer).release();
> 
> Not new code, but someone should return to this, since this is a peculiar
> way to write it. I’d write this:
> 
>     static NeverDestroyed<PowerObserver> powerObserver(restartSharedTimer);

Yes, but I'll leave this for someone who can build mac port :-)

> > Source/WebCore/workers/WorkerRunLoop.cpp:56
> > +    std::function<void()> m_sharedTimerFunction { nullptr };
> 
> No need for the { nullptr } here since std::function will initialize itself
> to null.

Ok.

-- 
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/20151030/005b8760/attachment.html>