<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Make every port implement MainThreadSharedTimer instead of using global functions"
href="https://bugs.webkit.org/show_bug.cgi?id=150498#c33">Comment # 33</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Make every port implement MainThreadSharedTimer instead of using global functions"
href="https://bugs.webkit.org/show_bug.cgi?id=150498">bug 150498</a>
from <span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=150498#c32">comment #32</a>)
<span class="quote">> Comment on <span class=""><a href="attachment.cgi?id=264207&action=diff" name="attach_264207" title="Try to fix win build">attachment 264207</a> <a href="attachment.cgi?id=264207&action=edit" title="Try to fix win build">[details]</a></span>
> Try to fix win build
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=264207&action=review">https://bugs.webkit.org/attachment.cgi?id=264207&action=review</a>
>
> > 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.</span >
Ok, I'll fix the comment then.
<span class="quote">> > 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.</span >
Ok.
<span class="quote">> > Source/WebCore/platform/SharedTimer.h:41
> > + SharedTimer() { }
>
> This line of code can simply be deleted. It has no effect.</span >
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.
<span class="quote">> > 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);</span >
Yes, but I'll leave this for someone who can build mac port :-)
<span class="quote">> > 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.</span >
Ok.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>