Bug 150498

Summary: Make every port implement MainThreadSharedTimer instead of using global functions
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, zan
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Updated patch
none
Try to fix Mac build
none
Try to fix Mac build
none
Try to fix Mac build
none
Try to fix Mac build
darin: review+
New patch
none
Fix EFL build
none
Try to fix win build
none
Try to fix win build
none
Try to build on Mac
none
Try to fix mac build
none
Try to fix mac builds
none
Try to fix win build darin: review+

Description Carlos Garcia Campos 2015-10-23 05:40:39 PDT
Keep common code in MainThreadSharedTimer and implement the rest of the members in the platform specific files. It simplifies both the header and the port implementations.
Comment 1 Carlos Garcia Campos 2015-10-23 05:44:57 PDT
Created attachment 263917 [details]
Patch

I'm not setting r? because the windows part is missing. I need help with that because I don't know how to pass the instance pointer to TimerWindowWndProc.
Comment 2 Carlos Garcia Campos 2015-10-23 05:47:37 PDT
I plan to use RunLoop::Timer for the GTK+ implementation, since our RunLoop::Timer implementation is basically the same to what we do in SharedTimerGtk. If this is the case of the other ports we could have a cross-platform implementation of MainThreadSharedTimer.
Comment 3 Carlos Garcia Campos 2015-10-23 06:04:29 PDT
hmm, the idea of moving the shared function to the class as a member to make setFiredFunction doesn't work for CF because of the restartSharedTimer, so I'll leave it as global var in every port file for now.
Comment 4 Carlos Garcia Campos 2015-10-23 06:26:17 PDT
Created attachment 263919 [details]
Updated patch

Switched to use a static member, that should work for windows too.
Comment 5 Carlos Garcia Campos 2015-10-23 06:30:11 PDT
Created attachment 263920 [details]
Try to fix Mac build
Comment 6 Carlos Garcia Campos 2015-10-23 06:46:10 PDT
Created attachment 263922 [details]
Try to fix Mac build
Comment 7 Carlos Garcia Campos 2015-10-23 06:53:11 PDT
Created attachment 263923 [details]
Try to fix Mac build
Comment 8 Carlos Garcia Campos 2015-10-23 06:58:02 PDT
Created attachment 263924 [details]
Try to fix Mac build

Hopefully the last one, sorry for the noise.
Comment 9 Darin Adler 2015-10-24 14:42:19 PDT
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.

> 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.
Comment 10 Carlos Garcia Campos 2015-10-25 03:37:33 PDT
(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.
Comment 11 Carlos Garcia Campos 2015-10-25 03:48:15 PDT
Created attachment 264004 [details]
New patch

It still needs the changes in the XCode files. Could someone with a mac add them, please?
Comment 12 WebKit Commit Bot 2015-10-25 03:50:54 PDT
Attachment 264004 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:72:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:73:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:113:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/ThreadTimers.cpp:73:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Carlos Garcia Campos 2015-10-25 03:51:51 PDT
Created attachment 264005 [details]
Fix EFL build
Comment 14 WebKit Commit Bot 2015-10-25 03:54:09 PDT
Attachment 264005 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:72:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:73:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:113:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/ThreadTimers.cpp:73:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Carlos Garcia Campos 2015-10-25 05:59:36 PDT
Created attachment 264007 [details]
Try to fix win build

It seems windows needs functional to be included
Comment 16 WebKit Commit Bot 2015-10-25 06:03:11 PDT
Attachment 264007 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:72:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:73:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:113:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/ThreadTimers.cpp:73:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Carlos Garcia Campos 2015-10-25 23:31:58 PDT
I don't understand the windows build error:

C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/NeverDestroyed.h(53): error C2248: 'WebCore::MainThreadSharedTimer::MainThreadSharedTimer': cannot access private member declared in class 'WebCore::MainThreadSharedTimer' (compiling source file C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\MainThreadSharedTimer.cpp) [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
  C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\MainThreadSharedTimer.cpp(37): note: see declaration of 'WebCore::MainThreadSharedTimer::MainThreadSharedTimer' (compiling source file C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\MainThreadSharedTimer.cpp)
  c:\cygwin\home\buildbot\webkit\source\webcore\platform\MainThreadSharedTimer.h(35): note: see declaration of 'WebCore::MainThreadSharedTimer' (compiling source file C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\MainThreadSharedTimer.cpp)
  C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\MainThreadSharedTimer.cpp(33): note: see reference to function template instantiation 'WTF::NeverDestroyed<WebCore::MainThreadSharedTimer>::NeverDestroyed<>(void)' being compiled
  C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\MainThreadSharedTimer.cpp(33): note: see reference to function template instantiation 'WTF::NeverDestroyed<WebCore::MainThreadSharedTimer>::NeverDestroyed<>(void)' being compiled

Doesn't friend class work in windows?
Comment 18 Carlos Garcia Campos 2015-10-26 10:58:52 PDT
Created attachment 264053 [details]
Try to fix win build

Let's see if this fixes the win build.
Comment 19 WebKit Commit Bot 2015-10-26 11:01:46 PDT
Attachment 264053 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:72:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:73:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:113:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/ThreadTimers.cpp:73:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Carlos Garcia Campos 2015-10-26 12:00:03 PDT
hmm, the current win failure doesn't seem related to this patch.
Comment 21 Carlos Garcia Campos 2015-10-27 02:12:21 PDT
Created attachment 264126 [details]
Try to build on Mac

Now with the xcode changes, thanks Philippe!
Comment 22 WebKit Commit Bot 2015-10-27 02:14:35 PDT
Attachment 264126 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:72:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:73:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:113:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/ThreadTimers.cpp:73:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Carlos Garcia Campos 2015-10-27 02:38:17 PDT
Created attachment 264127 [details]
Try to fix mac build

It's failing when linking due to undefined symbols, trying with WEBCORE_EXPORT
Comment 24 WebKit Commit Bot 2015-10-27 02:40:04 PDT
Attachment 264127 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:72:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:73:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:113:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/ThreadTimers.cpp:73:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 Carlos Garcia Campos 2015-10-27 10:51:21 PDT
I'm lost with the build failures. Darin, could you help me with the mac builds?
Comment 26 Darin Adler 2015-10-27 16:29:57 PDT
I believe the Mac build failure is a mistake in the project file patch; MainThreadSharedTimer.cpp was not added to the list of files to compile. You’ll note that the old SharedTimerCF.cpp had a PBXBuildFile line that was added but the patch doesn’t add new PBXBuildFile for the new files. Whoever generated the Xcode project files has to redo it, or someone else needs to apply the patch and do that.
Comment 27 Carlos Garcia Campos 2015-10-28 01:38:44 PDT
(In reply to comment #26)
> I believe the Mac build failure is a mistake in the project file patch;
> MainThreadSharedTimer.cpp was not added to the list of files to compile.
> You’ll note that the old SharedTimerCF.cpp had a PBXBuildFile line that was
> added but the patch doesn’t add new PBXBuildFile for the new files. Whoever
> generated the Xcode project files has to redo it, or someone else needs to
> apply the patch and do that.

Thanks Darin, it's amazing how difficult is to add new files to xcode project files even using xcode. I'll try to get a new patch.
Comment 28 Carlos Garcia Campos 2015-10-28 02:43:00 PDT
Created attachment 264203 [details]
Try to fix mac builds

Updated the xcode changes
Comment 29 WebKit Commit Bot 2015-10-28 02:45:55 PDT
Attachment 264203 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:72:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:73:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:113:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/ThreadTimers.cpp:73:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 30 Carlos Garcia Campos 2015-10-28 04:38:40 PDT
Created attachment 264207 [details]
Try to fix win build

I think std::function<void()> = 0 was the problem in windows.
Comment 31 WebKit Commit Bot 2015-10-28 04:40:04 PDT
Attachment 264207 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:72:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:73:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:113:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/ThreadTimers.cpp:73:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 32 Darin Adler 2015-10-28 09:28:30 PDT
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.

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

> Source/WebCore/platform/SharedTimer.h:41
> +    SharedTimer() { }

This line of code can simply be deleted. It has no effect.

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

> 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.
Comment 33 Carlos Garcia Campos 2015-10-30 06:18:10 PDT
(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.
Comment 34 Carlos Garcia Campos 2015-10-30 06:26:23 PDT
Committed r191789: <http://trac.webkit.org/changeset/191789>