[WK2] Add initial implementation the Screen Wake Lock API behind an experimental flag#4733
Conversation
cdumez
added
WebCore JavaScript
|
EWS run on previous version of this PR (hash 10319a4) Details
|
|
Upstream WPT PR: web-platform-tests/wpt#36088 |
webkit-ews-buildbot
added
the
merging-blocked
cdumez
force-pushed
the
screen_wake_lock_implementation
branch
from
10319a4 to
faf5c8e
Compare
|
EWS run on previous version of this PR (hash faf5c8e) Details
|
cdumez
force-pushed
the
screen_wake_lock_implementation
branch
from
faf5c8e to
66d5776
Compare
|
EWS run on previous version of this PR (hash 66d5776) Details
|
cdumez
force-pushed
the
screen_wake_lock_implementation
branch
from
66d5776 to
d0a8f72
Compare
|
EWS run on previous version of this PR (hash d0a8f72) Details
|
cdumez
requested review from
achristensen07,
brentfulgham and
geoffreygaren
There was a problem hiding this comment.
Should we use/consume transient activation instead?
There was a problem hiding this comment.
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.
I would favor not consuming in this case; but we can have that discussion in the standards github. (I've commented there already.)
cdumez
force-pushed
the
screen_wake_lock_implementation
branch
from
d0a8f72 to
8afd079
Compare
|
EWS run on previous version of this PR (hash 8afd079) Details |
cdumez
force-pushed
the
screen_wake_lock_implementation
branch
from
8afd079 to
a6e3d1a
Compare
|
EWS run on previous version of this PR (hash a6e3d1a) Details
|
cdumez
force-pushed
the
screen_wake_lock_implementation
branch
from
a6e3d1a to
7254884
Compare
|
EWS run on previous version of this PR (hash 7254884) Details
|
webkit-ews-buildbot
added
the
merging-blocked
cdumez
force-pushed
the
screen_wake_lock_implementation
branch
from
7254884 to
74f691f
Compare
|
EWS run on previous version of this PR (hash 74f691f) Details
|
cdumez
force-pushed
the
screen_wake_lock_implementation
branch
from
74f691f to
6a683aa
Compare
|
EWS run on current version of this PR (hash 6a683aa) Details |
🧪 style
|
EWS run on previous version of this PR (hash 6a683aa) Details
|
|
Ping review? |
There was a problem hiding this comment.
I wish we could use a weak map for this instead.
There was a problem hiding this comment.
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.
I see: We do use WeakHashSet here, but WeakHashSet does not have amortizedCleanupIfNeeded(), like WeakHashMap does.
Maybe we should add amortizedCleanupIfNeeded() to WeakHashSet.
cdumez
force-pushed
the
screen_wake_lock_implementation
branch
from
6a683aa to
575238a
Compare
|
EWS run on current version of this PR (hash 575238a) Details |
|
EWS run on current version of this PR (hash 575238a) Details |
There was a problem hiding this comment.
Why do we need to downcast this here? It's just passed to the ContextDestructionObserver constructor.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I think downcast<> does just return nullptr if the type is wrong?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
I think downcast<> does just return nullptr if the type is wrong?
There was a problem hiding this comment.
It's funny that this paradigm seems to support the concept of being a context destruction observer for a nullptr.
There was a problem hiding this comment.
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.
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. |
webkit-commit-queue
force-pushed
the
screen_wake_lock_implementation
branch
from
575238a to
95606b1
Compare
webkit-commit-queue
removed
the
merge-queue
9 participants
95606b1
575238a
🛠 mac-debug🧪 api-mac🧪 mac-AS-debug-wk2