Bug 209676 – [Cocoa] Sleep disabling should be performed in the UI process

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Per Arne Vollan]

Since sleep disabling is causing communication with the power management service, it should be performed in the UI process.

[Per Arne Vollan]

Created attachment 394756 [details]
Patch

[Per Arne Vollan]

Created attachment 394758 [details]
Patch

[Per Arne Vollan]

Created attachment 394762 [details]
Patch

[Darin Adler]

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.

[Per Arne Vollan]

Created attachment 394817 [details]
Patch

[Per Arne Vollan]

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

[Per Arne Vollan]

Created attachment 394818 [details]
Patch

[Per Arne Vollan]

Created attachment 394822 [details]
Patch

[Per Arne Vollan]

Created attachment 394823 [details]
Patch

[Per Arne Vollan]

Created attachment 394824 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

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.

[Per Arne Vollan]

Created attachment 394914 [details]
Patch

[Per Arne Vollan]

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

[Per Arne Vollan]

(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!

[EWS]

Committed r259200: <https://trac.webkit.org/changeset/259200>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394914 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61055348>

[Darin Adler]

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.

[Per Arne Vollan]

Reopening to attach new patch.

[Per Arne Vollan]

Created attachment 394938 [details]
Patch

[Per Arne Vollan]

Reopening to attach new patch.

[Per Arne Vollan]

Created attachment 394939 [details]
Patch

[Per Arne Vollan]

Created attachment 394940 [details]
Patch

[Per Arne Vollan]

Created attachment 394946 [details]
Patch

[Per Arne Vollan]

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

[Per Arne Vollan]

Committed r259219: <https://trac.webkit.org/changeset/259219/webkit>