Since sleep disabling is causing communication with the power management service, it should be performed in the UI process.
Created attachment 394756 [details] Patch
Created attachment 394758 [details] Patch
Created attachment 394762 [details] Patch
Comment on attachment 394762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394762&action=review Started reviewing. Didn’t finish yet, but some initial comments. > Source/WebCore/platform/SleepDisabler.cpp:39 > +void setSleepDisablerClient(std::unique_ptr<SleepDisablerClient>&& client) It seems puzzling that the function body is in SleepDisabler.cpp but the function is declared in SleepDisablerClient.h. That’s not great long term for future code maintainability even though it might be handy for sharing a file-local sleepDisablerClient() function. > Source/WebCore/platform/SleepDisabler.cpp:42 > + auto& c = sleepDisablerClient(); > + c = WTFMove(client); Why the local variable? Why name it "c"? sleepDisablerClient() = WTFMove(client); > Source/WebCore/platform/SleepDisabler.cpp:54 > + sleepDisablerClient()->didCreateSleepDisabler(reinterpret_cast<uintptr_t>(this), reason, type == Type::Display); This is not a good pattern. We do not want to expose pointers across process boundaries. Instead we should use the normal ObjectIdentifier pattern we use elsewhere. > Source/WebCore/platform/SleepDisabler.h:32 > +class SleepDisabler : public PAL::SleepDisabler { I don’t understand why this class both derives from PAL::SleepDisabler, and also stores a pointer to a PAL::SleepDisabler. Doesn’t make logical sense to me. > Source/WebCore/platform/SleepDisabler.h:34 > + WEBCORE_EXPORT static std::unique_ptr<SleepDisabler> create(const char*, PAL::SleepDisabler::Type); This design pattern is not needed for unique_ptr. Instead we should provide a public constructor; the caller should use makeUnique<SleepDisabler>. This pattern is for classes where the constructor can’t do all the necessary work, such as classes that need an adoptRef to safely make a Ref<> or have some work that has to be done with function calls after the constructor is called. > Source/WebCore/platform/SleepDisablerClient.h:37 > + virtual ~SleepDisablerClient() { }; > + virtual void didCreateSleepDisabler(uintptr_t, const String&, bool) { }; > + virtual void didDestroySleepDisabler(uintptr_t) { }; No semicolons needed. Could didCreate/Destroy be pure virtual functions instead of function sixth empty bodies? Need to document what the arguments are with argument names. > Source/WebCore/testing/Internals.h:968 > + uint64_t createSleepDisabler(const String& reason, bool display); > + bool destroySleepDisabler(uint64_t identifier); Seems like a really peculiar pattern to expose pointers through the internals object. This seems to be the first time we have ever done this. Why not just generate incrementing identifiers instead of using the pointer? For one thing, they could then just be 32-bit integers instead of 64-bit ones.
Created attachment 394817 [details] Patch
(In reply to Darin Adler from comment #4) > Comment on attachment 394762 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394762&action=review > > Started reviewing. Didn’t finish yet, but some initial comments. > > > Source/WebCore/platform/SleepDisabler.cpp:39 > > +void setSleepDisablerClient(std::unique_ptr<SleepDisablerClient>&& client) > > It seems puzzling that the function body is in SleepDisabler.cpp but the > function is declared in SleepDisablerClient.h. That’s not great long term > for future code maintainability even though it might be handy for sharing a > file-local sleepDisablerClient() function. > > > Source/WebCore/platform/SleepDisabler.cpp:42 > > + auto& c = sleepDisablerClient(); > > + c = WTFMove(client); > > Why the local variable? Why name it "c"? > > sleepDisablerClient() = WTFMove(client); > > > Source/WebCore/platform/SleepDisabler.cpp:54 > > + sleepDisablerClient()->didCreateSleepDisabler(reinterpret_cast<uintptr_t>(this), reason, type == Type::Display); > > This is not a good pattern. We do not want to expose pointers across process > boundaries. Instead we should use the normal ObjectIdentifier pattern we use > elsewhere. > > > Source/WebCore/platform/SleepDisabler.h:32 > > +class SleepDisabler : public PAL::SleepDisabler { > > I don’t understand why this class both derives from PAL::SleepDisabler, and > also stores a pointer to a PAL::SleepDisabler. Doesn’t make logical sense to > me. > > > Source/WebCore/platform/SleepDisabler.h:34 > > + WEBCORE_EXPORT static std::unique_ptr<SleepDisabler> create(const char*, PAL::SleepDisabler::Type); > > This design pattern is not needed for unique_ptr. Instead we should provide > a public constructor; the caller should use makeUnique<SleepDisabler>. > > This pattern is for classes where the constructor can’t do all the necessary > work, such as classes that need an adoptRef to safely make a Ref<> or have > some work that has to be done with function calls after the constructor is > called. > > > Source/WebCore/platform/SleepDisablerClient.h:37 > > + virtual ~SleepDisablerClient() { }; > > + virtual void didCreateSleepDisabler(uintptr_t, const String&, bool) { }; > > + virtual void didDestroySleepDisabler(uintptr_t) { }; > > No semicolons needed. > > Could didCreate/Destroy be pure virtual functions instead of function sixth > empty bodies? > > Need to document what the arguments are with argument names. > > > Source/WebCore/testing/Internals.h:968 > > + uint64_t createSleepDisabler(const String& reason, bool display); > > + bool destroySleepDisabler(uint64_t identifier); > > Seems like a really peculiar pattern to expose pointers through the > internals object. This seems to be the first time we have ever done this. > Why not just generate incrementing identifiers instead of using the pointer? > For one thing, they could then just be 32-bit integers instead of 64-bit > ones. Thanks for reviewing! The latest patch should address all of the issues above.
Created attachment 394818 [details] Patch
Created attachment 394822 [details] Patch
Created attachment 394823 [details] Patch
Created attachment 394824 [details] Patch
Comment on attachment 394824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394824&action=review Looks good > Source/WebCore/platform/SleepDisablerClient.h:32 > +#include "SleepDisablerIdentifier.h" > + > +#include <pal/system/SleepDisabler.h> > +#include <wtf/ObjectIdentifier.h> > +#include <wtf/text/WTFString.h> Seems like this is too many includes. <wtf/Forward.h> should be all we need for a const String& argument. I don’t see any reason we’d need the ObjectIdentifer.h or SleepDisabler.h include. Might need to include <memory> so we can use std::unique_ptr. Also, headers should have one paragraph of includes, not two. > Source/WebCore/testing/Internals.cpp:5570 > + static unsigned identifier = 0; This is a pretty good name. I also like the name "lastUsedIdentifier" for globals like this one. > Source/WebKit/UIProcess/WebProcessProxy.cpp:1797 > + return m_sleepDisablers.size() > 0; This should use !isEmpty() instead of size() > 0. > Source/WebKit/UIProcess/WebProcessProxy.h:382 > + bool hasSleepDisabler(); This should be const. > Source/WebKit/UIProcess/API/Cocoa/WKWebViewTesting.mm:225 > + return _page ? _page->process().hasSleepDisabler() : false; This can use &&, which I think is clearer than ? : false. > Source/WebKit/WebProcess/GPU/media/RemoteLegacyCDM.cpp:33 > +#include "GPUProcessConnection.h" > + > #include "RemoteLegacyCDMFactory.h" New include should not be in a separate paragraph.
Comment on attachment 394824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394824&action=review > Source/WebKit/UIProcess/WebProcessProxy.cpp:1788 > +void WebProcessProxy::didCreateSleepDisabler(SleepDisablerIdentifier identifier, const String& reason, bool display) > +{ > + auto sleepDisabler = makeUnique<WebCore::SleepDisabler>(reason.utf8().data(), display ? PAL::SleepDisabler::Type::Display : PAL::SleepDisabler::Type::System); > + m_sleepDisablers.add(identifier, WTFMove(sleepDisabler)); > +} Oh, this needs a MESSAGE_CHECK that the identifier is a good one, I think. > Source/WebKit/UIProcess/WebProcessProxy.cpp:1793 > +void WebProcessProxy::didDestroySleepDisabler(SleepDisablerIdentifier identifier) > +{ > + m_sleepDisablers.remove(identifier); > +} Ditto.
Created attachment 394914 [details] Patch
(In reply to Darin Adler from comment #12) > Comment on attachment 394824 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394824&action=review > > > Source/WebKit/UIProcess/WebProcessProxy.cpp:1788 > > +void WebProcessProxy::didCreateSleepDisabler(SleepDisablerIdentifier identifier, const String& reason, bool display) > > +{ > > + auto sleepDisabler = makeUnique<WebCore::SleepDisabler>(reason.utf8().data(), display ? PAL::SleepDisabler::Type::Display : PAL::SleepDisabler::Type::System); > > + m_sleepDisablers.add(identifier, WTFMove(sleepDisabler)); > > +} > > Oh, this needs a MESSAGE_CHECK that the identifier is a good one, I think. > > > Source/WebKit/UIProcess/WebProcessProxy.cpp:1793 > > +void WebProcessProxy::didDestroySleepDisabler(SleepDisablerIdentifier identifier) > > +{ > > + m_sleepDisablers.remove(identifier); > > +} > > Ditto. Fixed.
(In reply to Darin Adler from comment #11) > Comment on attachment 394824 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394824&action=review > > Looks good > > > Source/WebCore/platform/SleepDisablerClient.h:32 > > +#include "SleepDisablerIdentifier.h" > > + > > +#include <pal/system/SleepDisabler.h> > > +#include <wtf/ObjectIdentifier.h> > > +#include <wtf/text/WTFString.h> > > Seems like this is too many includes. <wtf/Forward.h> should be all we need > for a const String& argument. I don’t see any reason we’d need the > ObjectIdentifer.h or SleepDisabler.h include. Might need to include <memory> > so we can use std::unique_ptr. > > Also, headers should have one paragraph of includes, not two. > Fixed. > > Source/WebCore/testing/Internals.cpp:5570 > > + static unsigned identifier = 0; > > This is a pretty good name. I also like the name "lastUsedIdentifier" for > globals like this one. > Fixed. > > Source/WebKit/UIProcess/WebProcessProxy.cpp:1797 > > + return m_sleepDisablers.size() > 0; > > This should use !isEmpty() instead of size() > 0. > Done. > > Source/WebKit/UIProcess/WebProcessProxy.h:382 > > + bool hasSleepDisabler(); > > This should be const. > Fixed. > > Source/WebKit/UIProcess/API/Cocoa/WKWebViewTesting.mm:225 > > + return _page ? _page->process().hasSleepDisabler() : false; > > This can use &&, which I think is clearer than ? : false. > Fixed. > > Source/WebKit/WebProcess/GPU/media/RemoteLegacyCDM.cpp:33 > > +#include "GPUProcessConnection.h" > > + > > #include "RemoteLegacyCDMFactory.h" > > New include should not be in a separate paragraph. Done. Thanks for reviewing!
Committed r259200: <https://trac.webkit.org/changeset/259200> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394914 [details].
<rdar://problem/61055348>
Comment on attachment 394914 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394914&action=review > Source/WebCore/platform/SleepDisabler.h:31 > +#include <pal/system/SleepDisabler.h> > +#include <wtf/ObjectIdentifier.h> Still don’t think these includes are needed.
Reopening to attach new patch.
Created attachment 394938 [details] Patch
Created attachment 394939 [details] Patch
Created attachment 394940 [details] Patch
Created attachment 394946 [details] Patch
(In reply to Darin Adler from comment #18) > Comment on attachment 394914 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394914&action=review > > > Source/WebCore/platform/SleepDisabler.h:31 > > +#include <pal/system/SleepDisabler.h> > > +#include <wtf/ObjectIdentifier.h> > > Still don’t think these includes are needed. Thanks! Removed <wtf/ObjectIdentifier.h>, but kept <pal/system/SleepDisabler.h>, since this was needed for the type argument (PAL::SleepDisabler::Type) in the WebCore::SleepDisabler constructor.
Committed r259219: <https://trac.webkit.org/changeset/259219/webkit>