-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[WK2] Add initial implementation the Screen Wake Lock API behind an experimental flag #4733
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WK2] Add initial implementation the Screen Wake Lock API behind an experimental flag #4733
Conversation
EWS run on previous version of this PR (hash 10319a4)
|
Upstream WPT PR: web-platform-tests/wpt#36088 |
10319a4
to
faf5c8e
Compare
EWS run on previous version of this PR (hash faf5c8e)
|
faf5c8e
to
66d5776
Compare
EWS run on previous version of this PR (hash 66d5776)
|
66d5776
to
d0a8f72
Compare
EWS run on previous version of this PR (hash d0a8f72)
|
} | ||
|
||
// We should be requesting permission but at the moment, we merely require a user gesture. | ||
bool hasUserGesture = UserGestureIndicator::processingUserGesture(document); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use/consume transient activation instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the patch to use transient activation. Let me know if you think we should consume instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would favor not consuming in this case; but we can have that discussion in the standards github. (I've commented there already.)
d0a8f72
to
8afd079
Compare
EWS run on previous version of this PR (hash 8afd079) |
8afd079
to
a6e3d1a
Compare
EWS run on previous version of this PR (hash a6e3d1a)
|
a6e3d1a
to
7254884
Compare
EWS run on previous version of this PR (hash 7254884)
|
7254884
to
74f691f
Compare
EWS run on previous version of this PR (hash 74f691f)
|
74f691f
to
6a683aa
Compare
EWS run on current version of this PR (hash 6a683aa) |
EWS run on previous version of this PR (hash 6a683aa)
|
Ping review? |
|
||
WakeLockManager::~WakeLockManager() | ||
{ | ||
m_document.unregisterForVisibilityStateChangedCallbacks(*this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we could use a weak map for this instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean. It is already a WeakHashSet:
WeakHashSet<VisibilityChangeClient> m_visibilityStateCallbackClients;
This doesn't mean we shouldn't remove the client when it is gone as far as I know. Our WeakHashSet implementation doesn't aggressively prune dead entries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see: We do use WeakHashSet here, but WeakHashSet does not have amortizedCleanupIfNeeded(), like WeakHashMap does.
Maybe we should add amortizedCleanupIfNeeded() to WeakHashSet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me
6a683aa
to
575238a
Compare
EWS run on current version of this PR (hash 575238a) |
EWS run on current version of this PR (hash 575238a) |
@@ -61,7 +62,7 @@ WakeLock& NavigatorScreenWakeLock::wakeLock(Navigator& navigator) | |||
WakeLock& NavigatorScreenWakeLock::wakeLock() | |||
{ | |||
if (!m_wakeLock) | |||
m_wakeLock = WakeLock::create(); | |||
m_wakeLock = WakeLock::create(downcast<Document>(m_navigator.scriptExecutionContext())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to downcast this here? It's just passed to the ContextDestructionObserver constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am purposefully using tight typing (Document instead of ScriptExecutionContext) to make it clear that WakeLock is only exposed to Window contexts and not workers.
I am not trying to future proof it to make it work with workers because I don't know that there is such plan. In particular, the spec doesn't currently expose this to workers. Also, if somebody were to add support for workers, there would likely be a lot more work involved than just changing the type here.
{ | ||
promise->reject(Exception { NotSupportedError }); | ||
return downcast<Document>(scriptExecutionContext()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be conservative, could we return null if it's not a Document? (just in case someone in the future adds this to workers)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think downcast<> does just return nullptr if the type is wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the type is not wrong. The WakeLock constructor takes a Document (because WakeLock is not exposed to workers) and thus its script execution context is always a Document.
private: | ||
void visibilityStateChanged() final; | ||
|
||
Document& m_document; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is safe right now because the Document owns WakeLockManager with a std::unique_ptr. We could make this a little more future-proof and make this a WeakPtr to protect against someone in the future making WakeLockManager RefCounted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hesitated. The reason I went this way is that it makes it clear the document cannot be null. With WeakPtr, it is tempting to null check it everywhere.
{ | ||
promise->reject(Exception { NotSupportedError }); | ||
return downcast<Document>(scriptExecutionContext()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think downcast<> does just return nullptr if the type is wrong?
|
||
namespace WebCore { | ||
|
||
WakeLock::WakeLock() = default; | ||
WakeLock::WakeLock(Document* document) | ||
: ContextDestructionObserver(document) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's funny that this paradigm seems to support the concept of being a context destruction observer for a nullptr.
return; | ||
|
||
switch (type) { | ||
case WakeLockType::Screen: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems silly and inefficient to have a WakeLockType enum with a single value. I realize that this is how the specification is defined, but it is strange! Why pay the cost of a switch/case/branch here when it can never be anything else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote it this way to match the spec and for future extensibility as different types of lock can be supported (I think Blink has a system lock for e.g.). Hopefully the compiler is smart enough to optimize this out but if it isn't, this is not hot code at all.
…xperimental flag https://bugs.webkit.org/show_bug.cgi?id=245712 Reviewed by Alex Christensen, Brent Fulgham and Geoffrey Garen. Add initial implementation the Screen Wake Lock API for WebKit2, behind an experimental flag: - https://w3c.github.io/screen-wake-lock The implementation should be complete but it is currently behind an experimental feature flag that is off by default. Some tests are still failing. The reason for that seems to be that our support for Feature Policy is only partial. In particular, we seem to support the `allow` attribute on iframes but not the Feature-Policy HTTP header. We also have to make a decision about whether or not we want to prompt the user when the Website tries to take a screen wake lock and if we need an indicator in the browser UI. Currently, my implementation only requires a user gesture. * LayoutTests/TestExpectations: * LayoutTests/imported/w3c/web-platform-tests/feature-policy/resources/feature-policy-screen-wakelock.html: Added. * LayoutTests/imported/w3c/web-platform-tests/feature-policy/resources/featurepolicy.js: (expect_reports): * LayoutTests/imported/w3c/web-platform-tests/resources/testdriver-vendor.js: (window.test_driver_internal.set_permission): * LayoutTests/imported/w3c/web-platform-tests/screen-wake-lock/idlharness.https.window-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/screen-wake-lock/wakelock-active-document.https.window-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/screen-wake-lock/wakelock-disabled-by-feature-policy.https.sub-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/screen-wake-lock/wakelock-disabled-by-feature-policy.https.sub.html: * LayoutTests/imported/w3c/web-platform-tests/screen-wake-lock/wakelock-enabled-by-feature-policy-attribute-redirect-on-load.https.sub-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/screen-wake-lock/wakelock-enabled-by-feature-policy-attribute-redirect-on-load.https.sub.html: * LayoutTests/imported/w3c/web-platform-tests/screen-wake-lock/wakelock-enabled-by-feature-policy-attribute.https.sub-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/screen-wake-lock/wakelock-enabled-by-feature-policy-attribute.https.sub.html: * LayoutTests/imported/w3c/web-platform-tests/screen-wake-lock/wakelock-enabled-by-feature-policy.https.sub-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/screen-wake-lock/wakelock-enabled-by-feature-policy.https.sub.html: * LayoutTests/imported/w3c/web-platform-tests/screen-wake-lock/wakelock-enabled-on-self-origin-by-feature-policy.https.sub-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/screen-wake-lock/wakelock-enabled-on-self-origin-by-feature-policy.https.sub.html: * LayoutTests/imported/w3c/web-platform-tests/screen-wake-lock/wakelock-onrelease.https-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/screen-wake-lock/wakelock-released.https-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/screen-wake-lock/wakelock-request-denied.https-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/screen-wake-lock/wakelock-type.https.window-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/screen-wake-lock/wakelockpermissiondescriptor.https-expected.txt: * LayoutTests/platform/mac-wk1/TestExpectations: * LayoutTests/platform/win/TestExpectations: * Source/WebCore/Modules/permissions/PermissionName.h: * Source/WebCore/Modules/permissions/PermissionName.idl: * Source/WebCore/Modules/screen-wake-lock/NavigatorScreenWakeLock.cpp: (WebCore::NavigatorScreenWakeLock::NavigatorScreenWakeLock): (WebCore::NavigatorScreenWakeLock::wakeLock): * Source/WebCore/Modules/screen-wake-lock/NavigatorScreenWakeLock.h: * Source/WebCore/Modules/screen-wake-lock/WakeLock.cpp: (WebCore::WakeLock::WakeLock): (WebCore::WakeLock::request): (WebCore::WakeLock::document): * Source/WebCore/Modules/screen-wake-lock/WakeLock.h: (WebCore::WakeLock::create): * Source/WebCore/Modules/screen-wake-lock/WakeLockManager.cpp: Added. (WebCore::WakeLockManager::WakeLockManager): (WebCore::WakeLockManager::~WakeLockManager): (WebCore::WakeLockManager::addWakeLock): (WebCore::WakeLockManager::removeWakeLock): (WebCore::WakeLockManager::visibilityStateChanged): (WebCore::WakeLockManager::releaseAllLocks): * Source/WebCore/Modules/screen-wake-lock/WakeLockManager.h: Copied from Source/WebCore/Modules/screen-wake-lock/WakeLock.h. * Source/WebCore/Modules/screen-wake-lock/WakeLockSentinel.cpp: (WebCore::WakeLockSentinel::release): (WebCore::WakeLockSentinel::virtualHasPendingActivity const): (WebCore::WakeLockSentinel::eventListenersDidChange): (WebCore::WakeLockSentinel::wakeLockManager): * Source/WebCore/Modules/screen-wake-lock/WakeLockSentinel.h: * Source/WebCore/Modules/screen-wake-lock/WakeLockType.h: * Source/WebCore/Sources.txt: * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebCore/dom/Document.cpp: (WebCore::Document::wakeLockManager): (WebCore::Document::stopActiveDOMObjects): * Source/WebCore/dom/Document.h: * Source/WebCore/dom/TaskSource.h: * Source/WebCore/html/FeaturePolicy.cpp: (WebCore::policyTypeName): (WebCore::FeaturePolicy::parse): (WebCore::FeaturePolicy::allows const): * Source/WebCore/html/FeaturePolicy.h: * Source/WebKit/UIProcess/WebPageProxy.cpp: (WebKit::WebPageProxy::queryPermission): * Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl: * Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp: (WTR::InjectedBundle::setScreenWakeLockPermission): * Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h: * Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp: (WTR::TestRunner::setScreenWakeLockPermission): * Tools/WebKitTestRunner/InjectedBundle/TestRunner.h: * Tools/WebKitTestRunner/TestController.cpp: (WTR::TestController::handleQueryPermission): (WTR::TestController::resetStateToConsistentValues): (WTR::TestController::setScreenWakeLockPermission): * Tools/WebKitTestRunner/TestController.h: * Tools/WebKitTestRunner/TestInvocation.cpp: (WTR::TestInvocation::didReceiveMessageFromInjectedBundle): Canonical link: https://commits.webkit.org/255019@main
Committed 255019@main (95606b1): https://commits.webkit.org/255019@main Reviewed commits have been landed. Closing PR #4733 and removing active labels. |
575238a
to
95606b1
Compare
95606b1
575238a
🛠 mac-debug🧪 api-mac🧪 mac-AS-debug-wk2