<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span> changed
<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>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #263924 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<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#c9">Comment # 9</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:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=263924&action=diff" name="attach_263924" title="Try to fix Mac build">attachment 263924</a> <a href="attachment.cgi?id=263924&action=edit" title="Try to fix Mac build">[details]</a></span>
Try to fix Mac build
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=263924&action=review">https://bugs.webkit.org/attachment.cgi?id=263924&action=review</a>
This is a little better, although not a huge improvement. I think there is some other strangeness here that can be fixed.
<span class="quote">> Source/WebCore/platform/SharedTimer.h:52
> class MainThreadSharedTimer final : public SharedTimer {</span >
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.
<span class="quote">> Source/WebCore/platform/SharedTimer.h:54
> void setFiredFunction(void (*function)()) override</span >
This function’s body should not be in the header. Instead it should be in MainThreadSharedTimer.cpp (or ThreadTimers.cpp for now, I guess).
<span class="quote">> Source/WebCore/platform/SharedTimer.h:62
> + void setFireInterval(double) override;
> + void stop() override;
> + void invalidate() override;</span >
Can any of these be private?
<span class="quote">> Source/WebCore/platform/SharedTimer.h:64
> + static void timerFired()</span >
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.
<span class="quote">> Source/WebCore/platform/SharedTimer.h:70
> + static void (*s_firedFunction)();</span >
This should be a non-static member, and also should probably be initialized to nullptr here in the header once it is like that.
<span class="quote">> Source/WebCore/platform/cf/SharedTimerCF.cpp:41
> +void (*MainThreadSharedTimer::s_firedFunction)() = nullptr;</span >
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.
<span class="quote">> Source/WebCore/platform/cf/SharedTimerCF.cpp:75
> + MainThreadSharedTimer::timerFired();</span >
This should be:
MainThreadSharedTimer::singleton().timerFired();
<span class="quote">> Source/WebCore/platform/gtk/SharedTimerGtk.cpp:40
> + ASSERT(MainThreadSharedTimer::s_firedFunction);</span >
No need to repeat the class name when we are inside a member function of that class.
<span class="quote">> Source/WebCore/platform/gtk/SharedTimerGtk.cpp:44
> + gSharedTimer.scheduleAfterDelay("[WebKit] sharedTimerTimeoutCallback", [this] { MainThreadSharedTimer::timerFired(); },</span >
No need to repeat the class name when we are inside a member function of that class.
<span class="quote">> Source/WebCore/platform/win/SharedTimerWin.cpp:128
> + ASSERT(MainThreadSharedTimer::s_firedFunction);</span >
No need to repeat the class name when we are inside a member function of that class.</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>