Summary: | Make every port implement MainThreadSharedTimer instead of using global functions | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||||||||||||||||||||||||
Component: | Platform | Assignee: | 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
Carlos Garcia Campos
2015-10-23 05:40:39 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.
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. 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. Created attachment 263919 [details]
Updated patch
Switched to use a static member, that should work for windows too.
Created attachment 263920 [details]
Try to fix Mac build
Created attachment 263922 [details]
Try to fix Mac build
Created attachment 263923 [details]
Try to fix Mac build
Created attachment 263924 [details]
Try to fix Mac build
Hopefully the last one, sorry for the noise.
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. (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. Created attachment 264004 [details]
New patch
It still needs the changes in the XCode files. Could someone with a mac add them, please?
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.
Created attachment 264005 [details]
Fix EFL build
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.
Created attachment 264007 [details]
Try to fix win build
It seems windows needs functional to be included
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.
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? Created attachment 264053 [details]
Try to fix win build
Let's see if this fixes the win build.
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.
hmm, the current win failure doesn't seem related to this patch. Created attachment 264126 [details]
Try to build on Mac
Now with the xcode changes, thanks Philippe!
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.
Created attachment 264127 [details]
Try to fix mac build
It's failing when linking due to undefined symbols, trying with WEBCORE_EXPORT
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.
I'm lost with the build failures. Darin, could you help me with the mac builds? 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. (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. Created attachment 264203 [details]
Try to fix mac builds
Updated the xcode changes
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.
Created attachment 264207 [details]
Try to fix win build
I think std::function<void()> = 0 was the problem in windows.
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 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. (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. Committed r191789: <http://trac.webkit.org/changeset/191789> |