Ever since we added gstreamer-libav to the GNOME runtime last month, I've become increasing frustrated by autoplay videos. This patch seems to work for cnn.com. Haven't managed to test youtube due to all the assertion failures. I'm not sure where the best place to do this is. I'm specifically trying to avoid adding API.
Created attachment 338472 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment on attachment 338472 [details] Patch I don't see any suggestions for how to do this better, so r?
Why not adding new API for this (and other policies)? I'm fine with changing the default behavior, but that doesn't mean it fits for all use cases. An embedded with no input will want for sure auto play videos. Apple ports even have autoplay quirks, so I'm afraid this is not so easy.
(In reply to Carlos Garcia Campos from comment #4) > Why not adding new API for this (and other policies)? I just don't have time to investigate new API right now, sorry. We'll want to do something general that allows us to expose other members of WebsitePoliciesData in the future. WebkitWebsitePolicies, perhaps.
Comment on attachment 338472 [details] Patch r- then, this is changing the behavior without a way for the user to opt in/out, it might break embedded systems where there isn't input to start video with a user gesture.
An API wrapping WebsitePoliciesData sounds like the way to go?
*** Bug 181650 has been marked as a duplicate of this bug. ***
I mean't to mark the duplicate in the opposite direction, oh well, no harm.
As of 2020, the original patch won't always work, since performing an IPC send in `webkitWebViewCreatePage' often no-ops due to the WebProcess not being alive yet so videos still autoplay regardless. The IPC wrappers for `updateWebsitePolicies' could be modified to ensure the process has started before doing an IPC send, but there's lots of APIs with this race and it's not clear what's important enough to block and spin up the process and what isn't. I'd figure that no messages should be silently dropped, but they are. As a WebKitGTK client, you've got be careful and issue these calls after `WEBKIT_LOAD_STARTED' where you're sure the WebProcess is up. Anyway, I digress... Once that is fixed, autoplay policies were still not respected because they are stored in `DocumentLoader''s, which can be replaced by the time media elements perform a variety of inconsistent checks to determine the UI client's autoplay preferences. As a summary of the state of (auto)play, - `HTMLMediaElement' manages "user interference" and orchestration of the element session, relevant web settings, relevant queries to various `Document''s regarding autoplay policy, movement of the element between documents, interactions with the readyState algorithms and of course handling of the `autoplay' attribute itself. - `MediaElementSession', which got carved out of `HTMLMediaElement', controls all sorts of behavioural properties of the video, including bits of the autoplay story. It adds things like autoplaying to "external devices" and "invisible autoplay" which is not configurable in the UIProcess side. It also controls *preloading*, which seems a strange distinction from autoplaying. Also contains hit testing logic to determine if a video is "main content enough" for autoplay eligibility. - `WebsitePoliciesData' has autoplay logic, which translates UI/API enumerations into Core versions and sets them on the loader. This lets the UIProcess control certain aspects of autoplay. - `DocumentLoader''s have autoplay policies stuck on them, there is no class logic for this extra data. Loader's come and go, and it's easy for the loader the media element queries to be different to the loader the `WebsitePoliciesData' hangs the policies off at the time its called from the UIProcess. It does feel like the `DocumentLoader' is the wrong place for autoplay policies, but it's so ingrained in WebKit/WebCore that refactoring is basically impossible for me, due to the undocumented Mac-specific autoplay behaviours that I'd almost certainly impact without knowing. - `Document''s query their loader and contain logic about what combinations of policies imply requiring user gestures. I do not think this is the right place for such logic, instead it should be in its own AutoplayPolicy class. The users of this loader API for getting the policies are `Document::audioPlaybackRequiresUserGesture', `Document::videoPlaybackRequiresUserGesture', `Document::mediaDataLoadsAutomatically', which in turn are used by MediaElement and WebAudio. - `Page''s have `allowsMediaDocumentInlinePlayback' and associated setters. They further have logic for allowing certain playback controls relating to autoplay. Also data for whether playback is suspended. - `WebSetting' s such as `RequiresUserGestureForMediaPlayback', used by `webkit_settings_set_media_playback_requires_user_gesture' is like AutoPlay::Deny, but not. `RequiresUserGestureForMediaPlayback' => {Video,Audio}PlaybackRequiresUserGesture, and these two more specific settings are also settable in the web preferences. - There's also "quirks" for documents and loaders, that influence the decisions to autoplay in myriad ways, and can be site-specific.. Then, there are inconsistencies as to where to query for the current capabilities, - A static in `MediaElementSession', `pageExplicitlyAllowsElementToAutoplayInline' which is true iff `document.isMediaDocument() && !document.ownerElement() && page && page->allowsMediaDocumentInlinePlayback();' where `page' is `mediaElement.document().page()'. - `MediaElementSession::playbackPermitted()' which checks `Document''s, "top documents" (with FIXMEs asking why specifically "top documents") and `Document' quirks. - `WebPage::updateWebsitePolicies' sets autoplay policies on loaders retrieved like so, ,---- | auto* documentLoader = m_page->mainFrame().loader().documentLoader(); `---- - `HTMLMediaElement' requests autoplay policies in in a different way to how they are set, leading to lost information, ,---- | auto& document = this->document(); | auto* page = document.page(); | | if (document.ownerElement() || !document.isMediaDocument()) { | const auto& topDocument = document.topDocument(); | const bool shouldAudioPlaybackRequireUserGesture = topDocument.audioPlaybackRequiresUserGesture() && !isProcessingUserGesture; `---- Where `topDocument' follows the parent chain to the top and `audioPlaybackRequiresUserGesture()' gets the loader by `this->loader()'. There's also a special path in `updateWebsitePolicies' to call `m_page->updateMediaElementRateChangeRestrictions();', which loops over each media element in the current page and asks it to query *its* top document playback restrictions. So, with all that as background and an inability to refactor this into a more understandable and consolidated state owing to Mac-specific behaviour I can't feasibly test and a lack of understanding of WebKit loaders and why they are the place to store autoplay policies, I decided to do a bit of a hack specifically for GTK/WPE. We already have a way to influence global autoplay settings via the `media-playback-requires-user-gesture' property. If you set this to `TRUE', nothing will autoplay. A large hammer, but to be honest behaviour I'd be happy with in my browser. However, it sounds like API users want to control this setting at a finer granularity, at least per page, and for Apple per document (although, I don't understand this motivation, iframes maybe?) So, I added some extra plumbing to add the autoplay policies to pages.
Created attachment 399119 [details] Patch
Ping
WPE EWS is red :)
Comment on attachment 399119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399119&action=review > Source/WebCore/dom/Document.cpp:5437 > +#if PLATFORM(GTK) || PLATFORM(WPE) > + if (auto *page = this->page()) { > + AutoplayPolicy policy = page->autoplayPolicy(); > + if (policy != AutoplayPolicy::Default) > + return policy == AutoplayPolicy::AllowWithoutSound || policy == AutoplayPolicy::Deny; > + } > +#endif Why is this specific to GTK and WPE? > Source/WebCore/dom/Document.cpp:5457 > +#if PLATFORM(GTK) || PLATFORM(WPE) > + if (auto *page = this->page()) { > + AutoplayPolicy policy = page->autoplayPolicy(); > + if (policy != AutoplayPolicy::Default) > + return policy == AutoplayPolicy::Deny; > + } > +#endif Ditto. > Source/WebCore/dom/Document.cpp:5476 > +#if PLATFORM(GTK) || PLATFORM(WPE) > + if (auto *page = this->page()) { > + AutoplayPolicy policy = page->autoplayPolicy(); > + if (policy != AutoplayPolicy::Default) > + return policy == AutoplayPolicy::Deny; > + } > +#endif Ditto.
Comment on attachment 399119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399119&action=review :) Charlie, you'll be proud of me, I finally bought news shoes! You were right, my old shoes were old. > Source/WebCore/ChangeLog:11 > + I have only performed manually testing so far, since this is my > + first foray in the API side of WebKit, I'm not sure how best to > + test these things without resorting to a trivial check that > + setters / getters work (which seems fruitless). Such a trivial check might not seem terribly useful... but I'm pretty sure it actually *has* caught a bug before. I don't remember what, but probably I really messed up a getter or setter somehow. :P Anyway, it's better than nothing, and meets our minimum requirements to have an API test. Some APIs are just hard to test. >> Source/WebCore/dom/Document.cpp:5437 >> +#endif > > Why is this specific to GTK and WPE? So the logic is: * Use autoplay policy from the DocumentLoader, if set * Otherwise, use autoplay policy from the WebCore::Page, but only in WPE/GTK * Otherwise, check the global setting And the reason for the difference is explained in the FIXME that Charlie added to WebKitWebPage.cpp. Storing autoplay policy in the WebPage is a workaround for bugs where the DocumentLoader's autoplay policy state gets messed up. On the one hand, I like practical solutions to implement this important feature and get it out to users. I'm really tired of autoplay crap that no other browsers allow nowadays. On the other hand, we're adding a lot of GTK/WPE-specific conditions in a lot of different places, intended only as a workaround for this problem, and without FIXME comments to remove them in the future. That's not great... certainly it might benefit from further investigation. > Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:219 > +typedef enum { > + WEBKIT_AUTOPLAY_ALLOW, > + WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND, > + WEBKIT_AUTOPLAY_DENY > +} WebKitAutoplayPolicy; It's failing WPE EWS because you forgot to add this to WPE's WebKitWebView.h. > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6350 > + // FIXME: Apply the autoplay policy to the "global" Page as well > + // as the loader, it seems there is a semantic error storing them > + // in the document loader, which can change in lots of different > + // ways by the time the media element wants to know what the UI > + // client's autoplay preferences are. A lot of autoplay logic > + // could be sensibly refactored and consolidated, but there's so > + // much undocumented Mac-specific behaviour that it's simply > + // infeasible to do such a refactoring as a GTK/WPE developer > + // without fear of breaking something on other ports. > + setAutoplayPolicyForPage(websitePolicies.autoplayPolicy); It should be possible to figure out why the state is getting messed up in the particular case you're testing though, right? We certainly don't want to break Mac, but we *should* be willing to make a change that fixes our ports and see what happens on Mac, if the change appears to be correct. Worst case, our commit gets reverted and we're no worse off than we started. We seem to have other insane state tracking issues in the media code, like bug #186971 where it plays media while thinking it's not doing so. Who knows, could be related!
(In reply to Michael Catanzaro from comment #15) > Comment on attachment 399119 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=399119&action=review > > :) > > Charlie, you'll be proud of me, I finally bought news shoes! You were right, > my old shoes were old. :) > > > Source/WebCore/ChangeLog:11 > > + I have only performed manually testing so far, since this is my > > + first foray in the API side of WebKit, I'm not sure how best to > > + test these things without resorting to a trivial check that > > + setters / getters work (which seems fruitless). > > Such a trivial check might not seem terribly useful... but I'm pretty sure > it actually *has* caught a bug before. I don't remember what, but probably I > really messed up a getter or setter somehow. :P Anyway, it's better than > nothing, and meets our minimum requirements to have an API test. Some APIs > are just hard to test. OK, I will craft a sanity check. > > >> Source/WebCore/dom/Document.cpp:5437 > >> +#endif > > > > Why is this specific to GTK and WPE? > > So the logic is: > > * Use autoplay policy from the DocumentLoader, if set > * Otherwise, use autoplay policy from the WebCore::Page, but only in WPE/GTK > * Otherwise, check the global setting > > And the reason for the difference is explained in the FIXME that Charlie > added to WebKitWebPage.cpp. You summed it up better than me, this is a practical fix. I hope the commentary I added is clear enough about why I made this GTK/WPE specific. Indeed, the page lookup seems like it would be harmless for Mac, but I documented why I don't want to modify Mac behaviour at all since I can't feasibly test it. > > Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:219 > > +typedef enum { > > + WEBKIT_AUTOPLAY_ALLOW, > > + WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND, > > + WEBKIT_AUTOPLAY_DENY > > +} WebKitAutoplayPolicy; > > It's failing WPE EWS because you forgot to add this to WPE's WebKitWebView.h. Will fix. > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6350 > > + // FIXME: Apply the autoplay policy to the "global" Page as well > > + // as the loader, it seems there is a semantic error storing them > > + // in the document loader, which can change in lots of different > > + // ways by the time the media element wants to know what the UI > > + // client's autoplay preferences are. A lot of autoplay logic > > + // could be sensibly refactored and consolidated, but there's so > > + // much undocumented Mac-specific behaviour that it's simply > > + // infeasible to do such a refactoring as a GTK/WPE developer > > + // without fear of breaking something on other ports. > > + setAutoplayPolicyForPage(websitePolicies.autoplayPolicy); > > It should be possible to figure out why the state is getting messed up in > the particular case you're testing though, right? It's not so clear anything is getting messed up per se. On our ports, the document loader queried from the media element is different to the one used by the policy setter UI-side. I'm not sure if this is a bug in the loader or if our port API codepaths make loader-specific policies for autoplay not feasible. It made more sense to me to have per-page policies rather than per-loader policies. I studied the git history to try and find out, but I still don't understand the reasons for storing such policies in the loader. It would be awesome if a loader-expert could chime in here, developing a deep understanding of the meta in this subsystem with no documentation would take me a long time from scratch, but like everyone I am disappointed I haven't found a clean implementation for this.
(In reply to Charlie Turner from comment #16) > It's not so clear anything is getting messed up per se. On our ports, the > document loader queried from the media element is different to the one used > by the policy setter UI-side. I'm not sure if this is a bug in the loader or > if our port API codepaths make loader-specific policies for autoplay not > feasible. Hm, that's odd... maybe Phil/Calvaris/Alicia would have an opinion on this. > It made more sense to me to have per-page policies rather than per-loader > policies. Yeah, Page does seem like a good place....
I've learned enough Obj-C to be able to read the WebsitePolicies.mm's autoplay tests. The mac API has a notion of "web view delegates" which would avoid the race conditions I outlined above in my use of updateWebsitePolicies. I'll find out if the GTK port supports a similar thing soon enough. This file also demonstrates some methods to properly test autoplay policies, so I think I can do a better job now.
I debugged this a bit further and found the "corruption", although, it might be my implementation of the new API as well, UIProcess: updateWebsitePolicies page 0x125e30a87000 mainframe 0x125e30a83000 loader 0x125e30a82000 documentLoader 0x53ea3aef6a00 WebCore: finishInitialization page 0x125e30a87000 mainframe 0x125e30a83000 loader 0x125e30a82000 documentloader 0x53ea3aef9100 In other words, everything but the document loader remains constant, the one place this new API attaches the policies to use... The fundamental issue is a race window between the CreateWebPage IPC message and LoadRequest message. The Mac ports get around this because they have a notion of a navigation delegate on the web view, which I assume is avoiding the trap for users of the Mac API where you can set WebPage policies that will get attached to an "empty document", which exists only inside this race window, to be trashed later when a provisional load completes and the empty document gets replaced by a real one. The only example I found in the GTK API that appeared to be predicated on PageLoadState was =webkit_web_view_set_settings=. But that will silently drop updates if the state isn't correct, leaving no clear method to implement an API that is not surprisingly sensitive to statement order in the user program. I've not any experience even using the GTK API, so any help on expectations here would be great. >>IPC CreateWebPage WebProcess::createWebPage WebPage::WebPage m_mainFrame->initWithCoreMainFrame() >>>IPC DidCreateMainFrame Frame::init() m_loader->init() FrameLoader::init() policyLoader <- inital empty document provisionalLoader <- policyLoader DocumentLoader::startLoadingMainResource() maybeLoadEmpty() finishedLoading() commitIfReady() DocumentLoader::commitIfReady() frameLoader()->commitProvisionalLoad() FrameLoader::transitionToCommitted FrameLoader::setDocumentLoader(0x53ea3aef6a00) // empty document documentLoader <- provisionalLoader (0x53ea3aef6a00) // still the empty document created way up in FrameLoader::init() ---- If the UIProcess sets any WebPage policies before this next IPC checkpoint, they will be lost. ---- Mac seems to avoid this with their "navigation delegate" abstraction, which I assume ensure policies are never set on the weird "empty document" ---- that exists between CreateWebPage and LoadRequest. >>IPC LoadRequest WebPage::loadRequest UserInputBridge::loadRequest FrameLoader::load << stack frame where the loader gets changed, m_client->createDocumentLoader WebFrameLoaderClient::createDocumentLoader(ResourceRequest, SubsituteData) WebPage::createDocumentLoader newLoader <- WebDocumnetLoader::create DocumentLoader::DocumentLoader load(newLoader) FrameLoader::load [here, the newDocumentLoader should get updated with previously set policies?] FrameLoader::loadWithDocumentLoader [if this is a subframe, encoding overrides get propagated, another potential hook?] FrameLoader::setPolicyDocumentLoader policyDocumentLoader <- 0x53ea3aef9100 [previous loader's policies now trashed] I can get rid of adding Page lookup as in earlier version of my patch by hooking into FrameLoader::load, and imitating what happens with encoding overrides, namely, propagating them to the newly created loader, if it is replacing an existing one. I will submit this patch to EWS, there's some iframe tests on Mac that should exercise this logic. It's conceptually racey because depending on the timing between frame loads and policy setting, an old policy could override a newly set one. Let's see what the Mac bot thinks...
Created attachment 400127 [details] Patch Provisional patch to see if the Mac EWS fails with this approach. ChangeLogs to be reviewed later. Adds basic API tests
Comment on attachment 400127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400127&action=review > Source/WebCore/loader/FrameLoader.cpp:1543 > - if (m_documentLoader) > + if (m_documentLoader) { > newDocumentLoader.setOverrideEncoding(m_documentLoader->overrideEncoding()); > + newDocumentLoader.setAutoplayPolicy(m_documentLoader->autoplayPolicy()); > + } Wow, nice debugging. I'd like to see what Carlos Garcia thinks. See also: comment #10.
The WebsitePoliciesAutoplayEnabled Cocoa test (which checks the iframe cases) passed as can be seen https://ews-build.webkit.org/#/builders/3/builds/25107 (so did all the other tests). So, I am more confident this is a regression-free solution.
To summarise, I have 3 options for how to implement an autoplay API, 1/ As a web setting / web context item - Toggling autoplay would toggle all related views (probably, all tabs) - Unlikely to be useful when media-playback-requires-user-gesture exists, although you can now additionally autoplay without sound. 2/ Extension to decide-policy - This would conceptually require every media load to block on the UI decision. - Policy decision could include media URL, then caching could mitigate such blocks with persistent storage managed by WebKit. 3/ A web view API as initially proposed - Caveat, you must call the API after load committal but before the load finishes. Doing so before can lose policy information due to the empty document as explained earlier. - I took at look at how Surf uses the APIs, it seems accepted practice that per URL (i.e., view, page) configuration is given after WEBKIT_LOAD_COMMITTED in the load-changed callbacks. In such a case, all the complication I documented here about IPC message drops and fake document loaders is side-stepped and the patch becomes trivial boilerplate. I wonder if this is acceptable to the GTK API r+'ers? - Then per-url handling of autoplay policies is upto the application rather than WebKit, as option 2) suggested. - Iframe's make this more complicated, because load-changed does not notify you of them. They must be handled indirectly by the app, somehow. (other confused users https://lists.webkit.org/pipermail/webkit-gtk/2017-March/002976.html) Even with caveats, option 3/ seems best.
Created attachment 400159 [details] Patch Take option 3/
Hm, I dunno, actually any one of the three options seems OK to me. Option 2 would be the easiest way for applications to implement per-origin permissions. E.g. the user could allow autoplay on YouTube but not on other websites. Would one round of IPC really have any noticeable effect on performance? Surely not? (In reply to Charlie Turner from comment #23) > 3/ A web view API as initially proposed > - Caveat, you must call the API after load committal but before the > load finishes. Doing so before can lose policy information due to > the empty document as explained earlier. This one seems tricky to use properly.
(In reply to Michael Catanzaro from comment #25) > Hm, I dunno, actually any one of the three options seems OK to me. Option 1/ is inflexible, you can't have per URL settings, which might upset your users. For example, audible autoplay is generally a nuisance, but on Netflix it's kinda expected (yes, EME is an even larger problem...) I'm sure there's lots of examples like that users would end up asking about. > > Option 2 would be the easiest way for applications to implement per-origin > permissions. E.g. the user could allow autoplay on YouTube but not on other > websites. Would one round of IPC really have any noticeable effect on > performance? Surely not? This does seem most correct, but... > > (In reply to Charlie Turner from comment #23) > > 3/ A web view API as initially proposed > > - Caveat, you must call the API after load committal but before the > > load finishes. Doing so before can lose policy information due to > > the empty document as explained earlier. > > This one seems tricky to use properly. I took a look at Epiphany, and there committed is also a special state that has staging logic. So the precedent looks firmly set. /* Warning: the URI property may remain set to the URI of the * previously-loaded page until WEBKIT_LOAD_COMMITTED! During * WEBKIT_LOAD_STARTED, it may or may not match the URI being loaded. */ Also TLS is special cased here, update_security_status_for_committed_load (EphyWebView *view, const char *uri) ^^ This looks like it could be used analogously for autoplay status per URI. Seems like a classic Worse is Better vs. Option 2/ WDYT?
I think I vote for Option 2, because decide-policy is perfect for this. :)
Wait for Carlos Garcia's opinion before changing your patch, though.
I would use the policy-decision signal too, which is the perfect place for per-url policies, I would say. Note that there's also default policies, that can be set in the page and are automatically used when no policies are given to WebFramePolicyListenerProxy::use(). So, I would add a new class por policies, since we might want to support other things in the future that fit as website policies (cocoa api allows to use a different website data store and content manager, and other policies like popup blocker and preferences like enable/disable JavaScript or user agent string). I'm not sure about the name, though WebKitWebsitePolicies is probably confusing, and WebKitWebPreferences (cocoa uses WKWebpagePreferences) could be confused with WebKitSettings too. Then I would add a construct only property to set the default policies, that would end up setting the PageConfiguration::m_defaultWebsitePolicies. This way you don't need to have a process running to set your policies and there's no race condition. We add something like webkit_policy_decision_use_with_policies (or with_the_name_we_decide). This allows to override the default policies for a specific url. And finally webkit_web_view_update_policies that allows to change the policies for the page after it has been loaded. What do you think?
Created attachment 400823 [details] Patch Thanks for the reviews, I have gone with Option 2/ now. This is only for GTK at the moment to ease review iterations. Once the API is decided, I will add the extra boilerplate for WPE as well. I went with WebKitWebsitePolicies as a name just because it internally it made the most sense, and I could not think of a better name.
Comment on attachment 400823 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400823&action=review Thanks Charlie! Nice API test. > Source/WebKit/UIProcess/API/glib/WebKitPolicyDecision.cpp:93 > + * webkit_policy_decision_use_with_policies: > + * @decision: a #WebKitPolicyDecision > + * @policies: a #WebKitWebsitePolicies Charlie, when does it make sense to use this new API? Only for navigation policy decisions? Hm, what I was expecting was a new policy decision type that would prompt the browser to decide whether certain policies should be allowed... but I see now that's not quite how this works, maybe it wasn't the right expectation. Carlos, what do you think? I'm not quite sure the benefit of having this new function after all, since it seems like the autoplay policy is going to need to be set for every load anyway? It feels like this API gives a different point in time to set the website policies, but it doesn't actually have much to do with the underlying policy decision, it's just allowing policy setting to be tied to the policy decision, but you could otherwise manually update the policies using the WebKitWebView API after each LOAD_COMMITTED and get the same result. Right? > Source/WebKit/UIProcess/API/glib/WebKitPolicyDecision.cpp:96 > + * Accept the action which triggered this decision, and continue with > + * @policies affecting subsequent loads. affecting subsequent loads of... what? The entire web view? Or just loads for the security origin corresponding to the policy decision? > Source/WebKit/UIProcess/API/glib/WebKitPolicyDecision.cpp:98 > + * For example, a navigation decicion to a video sharing website may decision > Source/WebKit/UIProcess/API/glib/WebKitPolicyDecision.cpp:101 > + * autoplay policy in this case would be set in the @policies. > + */ Since: 2.30 > Source/WebKit/UIProcess/API/glib/WebKitPolicyDecision.cpp:102 > +void webkit_policy_decision_use_with_policies(WebKitPolicyDecision* decision, WebKitWebsitePolicies *policies) WebKitWebsitePolicies* policies > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1901 > +void webkitWebContextCreatePageForWebView(WebKitWebContext* context, WebKitWebView* webView, WebKitUserContentManager* userContentManager, WebKitWebView* relatedView, WebKitWebsitePolicies *defaultWebsitePolicies) WebKitWebsitePolicies* defaultWebsitePolicies > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:849 > + gpointer websitePolicies = g_value_get_object(value); > + webView->priv->websitePolicies = websitePolicies ? WEBKIT_WEBSITE_POLICIES(websitePolicies) : nullptr; webView->priv->websitePolicies = static_cast<WebKitWebsitePolicies*>(g_value_get_object(value)); > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:951 > + webView->priv->websitePolicies = nullptr; Is there some reason it has to be manually destroyed here? Unless this is breaking a reference loop you can probably just leave it be. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3790 > - getPage(webView).runJavaScriptInMainFrame({ String::fromUTF8(script), URL { }, false, WTF::nullopt, true }, [task = WTFMove(task)](API::SerializedScriptValue* serializedScriptValue, Optional<ExceptionDetails> details, WebKit::CallbackBase::Error) { > + RunJavaScriptParameters params = { String::fromUTF8(script), URL { }, false, WTF::nullopt, false }; > + getPage(webView).runJavaScriptInMainFrame(WTFMove(params), [task = WTFMove(task)](API::SerializedScriptValue* serializedScriptValue, Optional<ExceptionDetails> details, WebKit::CallbackBase::Error) { Unrelated code change? > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4667 > +void webkit_web_view_update_website_policies(WebKitWebView* webView, WebKitWebsitePolicies *policies) WebKitWebsitePolicies* policies > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4688 > + WebsitePoliciesData policiesData { }; > + > + switch (webkit_website_policies_get_autoplay_policy(policies)) { > + case WebKitAutoplayPolicy::WEBKIT_AUTOPLAY_ALLOW: > + policiesData.autoplayPolicy = WebsiteAutoplayPolicy::Allow; > + break; > + case WebKitAutoplayPolicy::WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND: > + policiesData.autoplayPolicy = WebsiteAutoplayPolicy::AllowWithoutSound; > + break; > + case WebKitAutoplayPolicy::WEBKIT_AUTOPLAY_DENY: > + policiesData.autoplayPolicy = WebsiteAutoplayPolicy::Deny; > + break; > + default: > + policiesData.autoplayPolicy = WebsiteAutoplayPolicy::Default; > + } > + > + getPage(webView).updateWebsitePolicies(WTFMove(policiesData)); Since your WebKitWebsitePolicies contains a API::WebsitePolicies, you should be able to move this logic to WebKitWebsitePolicies, right? You could have a private function webkitWebsitePoliciesGetPoliciesData() in WebKitWebsitePoliciesPrivate.h that returns a WebsitePoliciesData to pass to WebPageProxy::updateWebsitePolicies? > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:141 > + * webkit_website_policies_new_with_policies: > + * @first_policy_name: name of the first policy to set > + * @...: value of first policy, followed by more policies, %NULL-terminated > + * > + * Returns: (transfer full): the newly created #WebKitWebsitePolicies Would be good to show a usage example here. variadic functions are hard! > Source/WebKit/UIProcess/API/glib/WebKitWebsitePoliciesPrivate.h:25 > +API::WebsitePolicies* webkitWebsitePoliciesGetAPIType(WebKitWebsitePolicies *); Hm, I think we just call these: webkitWebsitePoliciesGetWebsitePolicies > Source/WebKit/UIProcess/API/gtk/WebKitPolicyDecision.h:67 > +webkit_policy_decision_use_with_policies (WebKitPolicyDecision *decision, WebKitWebsitePolicies *policies); Each parameter should be on its own line here in the public header. > Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:319 > + "website-policies", webkit_web_view_get_website_policies(webView), Should update the doc comment: "The newly created #WebKitWebView will also have the same #WebKitUserContentManager, #WebKitSettings, and #WebKitWebsitePolicies as @web_view."
(In reply to Michael Catanzaro from comment #31) > Comment on attachment 400823 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=400823&action=review > > Thanks Charlie! > > Nice API test. Thanks! And thanks for the review. Unfortunately the test looks like it is racey given the failure in the bots and none here. I have some more work to do. As a matter of coordination I will be back Thursday this week. > > > Source/WebKit/UIProcess/API/glib/WebKitPolicyDecision.cpp:93 > > + * webkit_policy_decision_use_with_policies: > > + * @decision: a #WebKitPolicyDecision > > + * @policies: a #WebKitWebsitePolicies > > Charlie, when does it make sense to use this new API? Only for navigation > policy decisions? I would say it only makes sense for navigation decisions, the app can make use of URI information to decide defaults for things like autoplay (and in the future all the other policy stuff) per load. > > I'm not quite sure the benefit of > having this new function after all, since it seems like the autoplay policy > is going to need to be set for every load anyway? It feels like this API > gives a different point in time to set the website policies, but it doesn't > actually have much to do with the underlying policy decision, it's just > allowing policy setting to be tied to the policy decision, but you could > otherwise manually update the policies using the WebKitWebView API after > each LOAD_COMMITTED and get the same result. Right? Exactly, I tried to highlight this between options 2/ and 3/ before. They both achieve the same thing, but by using the decide-policy signal, we avoid the race conditions described above, and making the app aware of load states when calling WebKit APIs (although, this is already a practice to some extent in WebKit apps) > > > Source/WebKit/UIProcess/API/glib/WebKitPolicyDecision.cpp:96 > > + * Accept the action which triggered this decision, and continue with > > + * @policies affecting subsequent loads. > > affecting subsequent loads of... what? The entire web view? Or just loads > for the security origin corresponding to the policy decision? Good question. I should make the docs better. Subsequent loads of the *entire web view". For example, if you use_with_policy(deny) on any URI, that policy sticks to the view's core frame's loader, which AFAICT, basically means any pages loaded in any way in the current view. So, if you navigate now to a different URI, autoplay will still be denied if the application doesn't reset anything to default values, per URL navigated to. > > > Source/WebKit/UIProcess/API/glib/WebKitPolicyDecision.cpp:98 > > + * For example, a navigation decicion to a video sharing website may > > decision Fixed. > > > Source/WebKit/UIProcess/API/glib/WebKitPolicyDecision.cpp:101 > > + * autoplay policy in this case would be set in the @policies. > > + */ > > Since: 2.30 Fixed. > > > Source/WebKit/UIProcess/API/glib/WebKitPolicyDecision.cpp:102 > > +void webkit_policy_decision_use_with_policies(WebKitPolicyDecision* decision, WebKitWebsitePolicies *policies) > > WebKitWebsitePolicies* policies Fixed. > > > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1901 > > +void webkitWebContextCreatePageForWebView(WebKitWebContext* context, WebKitWebView* webView, WebKitUserContentManager* userContentManager, WebKitWebView* relatedView, WebKitWebsitePolicies *defaultWebsitePolicies) > > WebKitWebsitePolicies* defaultWebsitePolicies Fixed. > > > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:849 > > + gpointer websitePolicies = g_value_get_object(value); > > + webView->priv->websitePolicies = websitePolicies ? WEBKIT_WEBSITE_POLICIES(websitePolicies) : nullptr; > > webView->priv->websitePolicies = > static_cast<WebKitWebsitePolicies*>(g_value_get_object(value)); Fixed. > > > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:951 > > + webView->priv->websitePolicies = nullptr; > > Is there some reason it has to be manually destroyed here? Unless this is > breaking a reference loop you can probably just leave it be. Aye, overly defensive, removed. > > > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3790 > > - getPage(webView).runJavaScriptInMainFrame({ String::fromUTF8(script), URL { }, false, WTF::nullopt, true }, [task = WTFMove(task)](API::SerializedScriptValue* serializedScriptValue, Optional<ExceptionDetails> details, WebKit::CallbackBase::Error) { > > + RunJavaScriptParameters params = { String::fromUTF8(script), URL { }, false, WTF::nullopt, false }; > > + getPage(webView).runJavaScriptInMainFrame(WTFMove(params), [task = WTFMove(task)](API::SerializedScriptValue* serializedScriptValue, Optional<ExceptionDetails> details, WebKit::CallbackBase::Error) { > > Unrelated code change? This is important for the API tests to work. Without it, everytime you `runJavaScriptAndWaitUntilFinished', user gestures are defaulted to being active (for reasons that were unclear). This change makes it so user gestures are not magically activated, so that the typical behaviour of JS trying to autoplay a video in an origin that is not allowed to autoplay will be blocked. Note the true -> false change in the lifted RunJavaScriptParameters, you have to squint. > > > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4667 > > +void webkit_web_view_update_website_policies(WebKitWebView* webView, WebKitWebsitePolicies *policies) > > WebKitWebsitePolicies* policies Fixed. > > > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4688 > > + WebsitePoliciesData policiesData { }; > > + > > + switch (webkit_website_policies_get_autoplay_policy(policies)) { > > + case WebKitAutoplayPolicy::WEBKIT_AUTOPLAY_ALLOW: > > + policiesData.autoplayPolicy = WebsiteAutoplayPolicy::Allow; > > + break; > > + case WebKitAutoplayPolicy::WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND: > > + policiesData.autoplayPolicy = WebsiteAutoplayPolicy::AllowWithoutSound; > > + break; > > + case WebKitAutoplayPolicy::WEBKIT_AUTOPLAY_DENY: > > + policiesData.autoplayPolicy = WebsiteAutoplayPolicy::Deny; > > + break; > > + default: > > + policiesData.autoplayPolicy = WebsiteAutoplayPolicy::Default; > > + } > > + > > + getPage(webView).updateWebsitePolicies(WTFMove(policiesData)); > > Since your WebKitWebsitePolicies contains a API::WebsitePolicies, you should > be able to move this logic to WebKitWebsitePolicies, right? You could have a > private function webkitWebsitePoliciesGetPoliciesData() in > WebKitWebsitePoliciesPrivate.h that returns a WebsitePoliciesData to pass to > WebPageProxy::updateWebsitePolicies? Ah yes, good idea. Fixed! > > > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:141 > > + * webkit_website_policies_new_with_policies: > > + * @first_policy_name: name of the first policy to set > > + * @...: value of first policy, followed by more policies, %NULL-terminated > > + * > > + * Returns: (transfer full): the newly created #WebKitWebsitePolicies > > Would be good to show a usage example here. variadic functions are hard! OK, I've added one, I hope it's useful. > > > Source/WebKit/UIProcess/API/glib/WebKitWebsitePoliciesPrivate.h:25 > > +API::WebsitePolicies* webkitWebsitePoliciesGetAPIType(WebKitWebsitePolicies *); > > Hm, I think we just call these: webkitWebsitePoliciesGetWebsitePolicies I'm starting to stammer, but fixed :) > > > Source/WebKit/UIProcess/API/gtk/WebKitPolicyDecision.h:67 > > +webkit_policy_decision_use_with_policies (WebKitPolicyDecision *decision, WebKitWebsitePolicies *policies); > > Each parameter should be on its own line here in the public header. Fixed. > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:319 > > + "website-policies", webkit_web_view_get_website_policies(webView), > > Should update the doc comment: "The newly created #WebKitWebView will also > have the same #WebKitUserContentManager, #WebKitSettings, and > #WebKitWebsitePolicies as @web_view." Good eye! Fixed.
(In reply to Charlie Turner from comment #32) > (In reply to Michael Catanzaro from comment #31) > > Comment on attachment 400823 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=400823&action=review > > > > > > > Source/WebKit/UIProcess/API/glib/WebKitPolicyDecision.cpp:96 > > > + * Accept the action which triggered this decision, and continue with > > > + * @policies affecting subsequent loads. > > > > affecting subsequent loads of... what? The entire web view? Or just loads > > for the security origin corresponding to the policy decision? > > Good question. I should make the docs better. Subsequent loads of the > *entire web view". > > For example, if you use_with_policy(deny) on any URI, that policy sticks to > the view's core frame's loader, which AFAICT, basically means any pages > loaded in any way in the current view. So, if you navigate now to a > different URI, autoplay will still be denied if the application doesn't > reset anything to default values, per URL navigated to. Please ignore this, I got it totally wrong due to a bug in my test application. They're not sticky. The application must respond with its desired policy for every URL loaded, and that policy affects only loads of that URL. If you reload, or otherwise navigate away to another URL, you must set it again. This behaviour makes more sense, should have wondered a bit harder why I was seeing the sticky behaviour...
Created attachment 400866 [details] Patch Address Michael's comments
(In reply to Charlie Turner from comment #33) > Please ignore this, I got it totally wrong due to a bug in my test > application. They're not sticky. The application must respond with its > desired policy for every URL loaded, and that policy affects only loads of > that URL. If you reload, or otherwise navigate away to another URL, you must > set it again. This behaviour makes more sense, should have wondered a bit > harder why I was seeing the sticky behaviour... So... what does webkit_web_view_update_website_policies() do then?
(In reply to Michael Catanzaro from comment #35) > (In reply to Charlie Turner from comment #33) > > Please ignore this, I got it totally wrong due to a bug in my test > > application. They're not sticky. The application must respond with its > > desired policy for every URL loaded, and that policy affects only loads of > > that URL. If you reload, or otherwise navigate away to another URL, you must > > set it again. This behaviour makes more sense, should have wondered a bit > > harder why I was seeing the sticky behaviour... > > So... what does webkit_web_view_update_website_policies() do then? Gives you the option of updating the policies in the most flexible (and potentially dangerous) way. See the API test that uses JavaScript to try and "autoplay" a video. After autoplay is denied initially by the policy handler, the API allows you to change the policy outside of decide-policy flows without reloading the page.
The API test failures are unrelated now (a web view fullscreen test failed in the last run) I continued investigating the spurious failure seen on the bots locally. I could not reproduce, but I did notice a *very rare* failure in the autoplay test that is only triggered when the policy client tests are run as a whole. I can *not* reproduce a failure in my autoplay test in isolation. The trace looks like this, and makes no sense at first. Where has that wait() call come from? #0 0x00007fa0c9ce9c75 in _g_log_abort (breakpoint=1) at ../../../glib/gmessages.c:554 #1 0x00007fa0c9ceaf7d in g_logv (log_domain=0x7fa0c9d2f00e "GLib", log_level=G_LOG_LEVEL_CRITICAL, format=<optimized out>, args=args@entry=0x7ffdf97d08b0) at ../../../glib/gmessages.c:1371 #2 0x00007fa0c9ceb14f in g_log (log_domain=<optimized out>, log_level=<optimized out>, format=<optimized out>) at ../../../glib/gmessages.c:1413 #3 0x000000000041ec69 in WebViewTest::quitMainLoop() (this=0x63e5c0) at /home/cht/igalia/sources/WebKit/Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.cpp:176 #4 0x000000000041ff1c in WebViewTest::wait(double)::$_0::operator()(void*) const (this=0x63e5c0, userData=0x63e5c0) at /home/cht/igalia/sources/WebKit/Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.cpp:182 #5 0x000000000041fef5 in WebViewTest::wait(double)::$_0::__invoke(void*) (userData=0x63e5c0, userData@entry=<error reading variable: value has been optimized out>) at /home/cht/igalia/sources/WebKit/Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.cpp:181 #6 0x00007fa0c9ce4863 in g_timeout_dispatch (source=0x953530, callback=<optimized out>, user_data=<optimized out>) at ../../../glib/gmain.c:4667 #7 0x00007fa0c9ce3dd8 in g_main_dispatch (context=0x4d7820) at ../../../glib/gmain.c:3182 #8 0x00007fa0c9ce3dd8 in g_main_context_dispatch (context=context@entry=0x4d7820) at ../../../glib/gmain.c:3847 #9 0x00007fa0c9ce41c8 in g_main_context_iterate (context=0x4d7820, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ../../../glib/gmain.c:3920 #10 0x00007fa0c9ce44c2 in g_main_loop_run (loop=0x646840) at ../../../glib/gmain.c:4116 #11 0x0000000000410a34 in testAutoplayPolicy(PolicyClientTest*, void const*) (test=0x94be70) at /home/cht/igalia/sources/WebKit/Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:147 #12 0x00007fa0c9d0b15a in test_case_run (tc=0x7fa09c00fad0) at ../../../glib/gtestutils.c:2318 #13 0x00007fa0c9d0b15a in g_test_run_suite_internal (suite=suite@entry=0x6534e0, path=path@entry=0x0) at ../../../glib/gtestutils.c:2403 #14 0x00007fa0c9d0b014 in g_test_run_suite_internal (suite=suite@entry=0x653500, path=path@entry=0x0) at ../../../glib/gtestutils.c:2415 #15 0x00007fa0c9d0b014 in g_test_run_suite_internal (suite=suite@entry=0x653520, path=path@entry=0x0) at ../../../glib/gtestutils.c:2415 #16 0x00007fa0c9d0b412 in g_test_run_suite (suite=0x653520) at ../../../glib/gtestutils.c:2490 #17 0x00007fa0c9d0b431 in g_test_run () at ../../../glib/gtestutils.c:1755 #18 0x0000000000418a21 in main(int, char**) (argc=1, argv=0x7ffdf97d1068) at /home/cht/igalia/sources/WebKit/Tools/TestWebKitAPI/glib/WebKitGLib/TestMain.cpp:138 It turns out this only happens when /webkit/WebKitPolicyClient/new-window-policy is part of the group. Looking at that test, it at least becomes understandable why it could cause nondeterminism. It registers a callback on the webview ::create signal which quits the main loop when its dispatched, and this callback is never cleared AFAICT. It also has arbitrary wait() calls in there, which is often a bad idea in unit tests, though it appears to have been done due to constraints deep in GTK. The simple solution is to make sure this test always runs last. I couldn't find a better one.
Created attachment 401000 [details] Patch Attempt to resolve the nondeterminism in the decide-policy API tests
Aha, the seemingly unrelated fullscreen test relied on that "force user gestures when running JS" API :) Now seems like a good time to break this change into a separate bug and expose the RunJavaScriptParameters to the GTK API. The unexpected pass in the forms test seems related to that. I will investigate.
(In reply to Charlie Turner from comment #37) > It turns out this only happens when > /webkit/WebKitPolicyClient/new-window-policy is part of the group. Looking > at that test, it at least becomes understandable why it could cause > nondeterminism. It registers a callback on the webview ::create signal which > quits the main loop when its dispatched, and this callback is never cleared > AFAICT. Can't you simply disconnect the signal? Possibly also use a new web view for each test? It also has arbitrary wait() calls in there, which is often a bad > idea in unit tests, though it appears to have been done due to constraints > deep in GTK. The simple solution is to make sure this test always runs last. > I couldn't find a better one.
(In reply to Michael Catanzaro from comment #40) > (In reply to Charlie Turner from comment #37) > > It turns out this only happens when > > /webkit/WebKitPolicyClient/new-window-policy is part of the group. Looking > > at that test, it at least becomes understandable why it could cause > > nondeterminism. It registers a callback on the webview ::create signal which > > quits the main loop when its dispatched, and this callback is never cleared > > AFAICT. > > Can't you simply disconnect the signal? I tried that, but it didn't help. > Possibly also use a new web view for > each test? It's pretty twisted how the C++ test classes are intertwined into the glib test framework. Something as simple sounding as your suggestion actually turns into quite a lot of work when you take a look at the code. > > It also has arbitrary wait() calls in there, which is often a bad > > idea in unit tests, though it appears to have been done due to constraints > > deep in GTK. The simple solution is to make sure this test always runs last. > > I couldn't find a better one.
Created attachment 401347 [details] Patch Fix fullscreen test by adding new JS API to run without user gestures active
Created attachment 401348 [details] Patch webkit-patch ignored the new files, try submitting from the cached worktree instead
(In reply to Michael Catanzaro from comment #31) > Hm, what I was expecting was a new policy decision type that would prompt > the browser to decide whether certain policies should be allowed... but I > see now that's not quite how this works, maybe it wasn't the right > expectation. Carlos, what do you think?
Comment on attachment 401348 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401348&action=review Looks good in general. I would split the patch to discuss the run-js issue in a separate bug. We also need to improve a bit the documentation to make it clear how to use the new api. > Source/WebCore/ChangeLog:13 > + * html/HTMLMediaElement.cpp: > + (WebCore::HTMLMediaElement::pageMutedStateDidChange): Modernize style. > + (WebCore::HTMLMediaElement::updateShouldAutoplay): Add a note to > + refactor behaviour restriction logic into MediaElementSession These changes are unrelated to this patch, no? > Source/WebKit/UIProcess/API/glib/WebKitPolicyDecision.cpp:97 > + * @policies affecting all subsequent loads of resources in the > + * current origin. all subsequent loads? are you sure? > Source/WebKit/UIProcess/API/glib/WebKitPolicyDecision.cpp:101 > + * For example, a navigation decision to a video sharing website may > + * be accepted under the priviso no movies are allowed to autoplay. The > + * autoplay policy in this case would be set in the @policies. I don't understand this. > Source/WebKit/UIProcess/API/glib/WebKitPolicyDecision.cpp:107 > + g_return_if_fail(WEBKIT_IS_POLICY_DECISION(decision)); You should check policies too since it can't be nullptr > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3803 > + * webkit_web_view_run_javascript_without_forced_user_gestures: I would move this to a separate bug. I don't remember why we forced the user gesture, maybe we can just change that behavior. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4694 > + * Updates the website policies associated with @web_view. > + * > + * Note, it only makes sense to call this API after %WEBKIT_LOAD_COMMITTED > + * and before %WEBKIT_LOAD_FINISHED. This is confusing, IIRC this allows to change the policies after a page has already been loaded, but do this affect the next load? I guess it doesn't, and the default policies will be used again, right? It should be clear what this does, because it might look like this is changing the website-policies property. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4711 > + * Gets the website policies associated with @web_view. This is the getter of the property and says "policies associated with @web_view." the same than webkit_web_view_update_website_policies(). It should be clear that these are the policies set on construction, used by default, not affected by policies passed to policy-decision nor updated with webkit_web_view_update_website_policies(). > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:37 > + * WebKitWebsitePolicies allows you to configure per-page autoplay policies. per-page policies, even when we only support autoplay for now. > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:90 > +static void webkitWebsitePoliciesConstructed(GObject* object) > +{ > + G_OBJECT_CLASS(webkit_website_policies_parent_class)->constructed(object); > +} We don't need to override constructed just to chain up. > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:92 > +API::WebsitePolicies* webkitWebsitePoliciesGetWebsitePolicies(WebKitWebsitePolicies* policies) This should return a reference instead of a pointer, since it's never nullptr. > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:99 > + WebsitePoliciesData policiesData { }; Initialization isn't needed here, you can remove the { } > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:121 > + Remove this empty line > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:245 > + } Since it's a property, you should check whether it actually changed and emit notify signal. > Tools/MiniBrowser/gtk/main.c:632 > if (!editorMode) { You should unref defaultWebsitePolicies aftr all web view have been created.
(In reply to Michael Catanzaro from comment #44) > (In reply to Michael Catanzaro from comment #31) > > Hm, what I was expecting was a new policy decision type that would prompt > > the browser to decide whether certain policies should be allowed... but I > > see now that's not quite how this works, maybe it wasn't the right > > expectation. Carlos, what do you think? I don't think it's ever expected to ask the user, that would block the load on policy decision. Let's think about how we would implement this in ephy to understand ow the api works. - We might have a global preference for autoplay in the preferences dialog. That should affect the default policies set to a web view. When this setting changes, update_website_policies needs to be called for already created web views. This makes me think that maybe it shouldn't be a construct only property. - We might also have an option in the page popover to override the default policy for a particular website. In decide-policy we would check if the given url is in a security origin with overridden policies and call use_with_policies() instead. If this setting is changed after the page has been loaded we need to call update_website_policies().
Comment on attachment 401348 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401348&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:221 > + * webkit_website_policies_set_autoplay_policy: hmm, I think WebKitWebsitePolicies should be immutable. Allowing to change it is confusing, because it won't affect the web views already using this object. This is not like WebKitSettings.
(In reply to Carlos Garcia Campos from comment #46) > I don't think it's ever expected to ask the user, that would block the load > on policy decision. Let's think about how we would implement this in ephy to > understand ow the api works. > > - We might have a global preference for autoplay in the preferences dialog. > That should affect the default policies set to a web view. When this setting > changes, update_website_policies needs to be called for already created web > views. This makes me think that maybe it shouldn't be a construct only > property. In Ephy, we would just leave autoplay disabled always, but yes, theoretically a browser might want to do this. > - We might also have an option in the page popover to override the default > policy for a particular website. In decide-policy we would check if the > given url is in a security origin with overridden policies and call > use_with_policies() instead. If this setting is changed after the page has > been loaded we need to call update_website_policies(). I guess that makes sense... but, we do not actually need the policy decision API at all, then. It could be done just as well in load-changed when LOAD_COMMITTED is seen. So I think I've changed my mind about that, we should only add API that is actually needed to do what we want. (In reply to Carlos Garcia Campos from comment #47) > hmm, I think WebKitWebsitePolicies should be immutable. Allowing to change > it is confusing, because it won't affect the web views already using this > object. This is not like WebKitSettings. Hm, right.
(In reply to Carlos Garcia Campos from comment #45) > > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4694 > > + * Updates the website policies associated with @web_view. > > + * > > + * Note, it only makes sense to call this API after %WEBKIT_LOAD_COMMITTED > > + * and before %WEBKIT_LOAD_FINISHED. > > This is confusing, IIRC this allows to change the policies after a page has > already been loaded, but do this affect the next load? I guess it doesn't, > and the default policies will be used again, right? It should be clear what > this does, because it might look like this is changing the website-policies > property. I understand it is a permanent change to the web view's policies, which is unfortunate because that's not very natural. Would be much nicer if you are correct and the change is only temporary.
(In reply to Michael Catanzaro from comment #48) > (In reply to Carlos Garcia Campos from comment #46) > > I don't think it's ever expected to ask the user, that would block the load > > on policy decision. Let's think about how we would implement this in ephy to > > understand ow the api works. > > > > - We might have a global preference for autoplay in the preferences dialog. > > That should affect the default policies set to a web view. When this setting > > changes, update_website_policies needs to be called for already created web > > views. This makes me think that maybe it shouldn't be a construct only > > property. > > In Ephy, we would just leave autoplay disabled always, but yes, > theoretically a browser might want to do this. > > > - We might also have an option in the page popover to override the default > > policy for a particular website. In decide-policy we would check if the > > given url is in a security origin with overridden policies and call > > use_with_policies() instead. If this setting is changed after the page has > > been loaded we need to call update_website_policies(). > > I guess that makes sense... but, we do not actually need the policy decision > API at all, then. It could be done just as well in load-changed when > LOAD_COMMITTED is seen. So I think I've changed my mind about that, we > should only add API that is actually needed to do what we want. That might be racy, I think it's better to handle per-url exceptions in the policy-decision, what I'm not sure about is the update_policies function. > (In reply to Carlos Garcia Campos from comment #47) > > hmm, I think WebKitWebsitePolicies should be immutable. Allowing to change > > it is confusing, because it won't affect the web views already using this > > object. This is not like WebKitSettings. > > Hm, right.
(In reply to Carlos Garcia Campos from comment #45) > Comment on attachment 401348 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=401348&action=review > > Looks good in general. I would split the patch to discuss the run-js issue > in a separate bug. We also need to improve a bit the documentation to make > it clear how to use the new api. Opened https://bugs.webkit.org/show_bug.cgi?id=212969 > > > Source/WebCore/ChangeLog:13 > > + * html/HTMLMediaElement.cpp: > > + (WebCore::HTMLMediaElement::pageMutedStateDidChange): Modernize style. > > + (WebCore::HTMLMediaElement::updateShouldAutoplay): Add a note to > > + refactor behaviour restriction logic into MediaElementSession > > These changes are unrelated to this patch, no? Unrelated, I'll remove them, it's always tempting to to put little style fixes like this in bigger patches due to the submission overhead, sorry :) > > > Source/WebKit/UIProcess/API/glib/WebKitPolicyDecision.cpp:97 > > + * @policies affecting all subsequent loads of resources in the > > + * current origin. > > all subsequent loads? are you sure? Pretty sure? I'm referring to resource loader loads, not API URI loads, which is maybe something I should clarify, or have I misunderstood a deeper issue? > > > Source/WebKit/UIProcess/API/glib/WebKitPolicyDecision.cpp:101 > > + * For example, a navigation decision to a video sharing website may > > + * be accepted under the priviso no movies are allowed to autoplay. The > > + * autoplay policy in this case would be set in the @policies. > > I don't understand this. I'm trying to motivate why this API would be used by giving an anticipated example. Maybe I could reword it. > > > Source/WebKit/UIProcess/API/glib/WebKitPolicyDecision.cpp:107 > > + g_return_if_fail(WEBKIT_IS_POLICY_DECISION(decision)); > > You should check policies too since it can't be nullptr Done > > > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3803 > > + * webkit_web_view_run_javascript_without_forced_user_gestures: > > I would move this to a separate bug. I don't remember why we forced the user > gesture, maybe we can just change that behavior. https://bugs.webkit.org/show_bug.cgi?id=212969 > > > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4694 > > + * Updates the website policies associated with @web_view. > > + * > > + * Note, it only makes sense to call this API after %WEBKIT_LOAD_COMMITTED > > + * and before %WEBKIT_LOAD_FINISHED. > > This is confusing, IIRC this allows to change the policies after a page has > already been loaded, but do this affect the next load? I guess it doesn't, > and the default policies will be used again, right? It should be clear what > this does, because it might look like this is changing the website-policies > property. Agreed. It might be better to not even expose this API. It's use is basically confined to cases as shown in the unit tests (unlocking JS play() calls and vice versa). Assuming it didn't exist, API users would simply have to change their per URL policies and expect the person using the browser/app to reload the page. Not a big deal imo. > > > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4711 > > + * Gets the website policies associated with @web_view. > > This is the getter of the property and says "policies associated with > @web_view." the same than webkit_web_view_update_website_policies(). It > should be clear that these are the policies set on construction, used by > default, not affected by policies passed to policy-decision nor updated with > webkit_web_view_update_website_policies(). Agreed. I'm voting for getting rid of webkit_web_view_update_website_policies now though as above. > > > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:37 > > + * WebKitWebsitePolicies allows you to configure per-page autoplay policies. > > per-page policies, even when we only support autoplay for now. > > > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:90 > > +static void webkitWebsitePoliciesConstructed(GObject* object) > > +{ > > + G_OBJECT_CLASS(webkit_website_policies_parent_class)->constructed(object); > > +} > > We don't need to override constructed just to chain up. Fixed. > > > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:92 > > +API::WebsitePolicies* webkitWebsitePoliciesGetWebsitePolicies(WebKitWebsitePolicies* policies) > > This should return a reference instead of a pointer, since it's never > nullptr. Fixed. > > > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:99 > > + WebsitePoliciesData policiesData { }; > > Initialization isn't needed here, you can remove the { } Fixed. > > > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:121 > > + > > Remove this empty line Fixed. > > > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:245 > > + } > > Since it's a property, you should check whether it actually changed and emit > notify signal. OK, fixed. > > > Tools/MiniBrowser/gtk/main.c:632 > > if (!editorMode) { > > You should unref defaultWebsitePolicies aftr all web view have been created. Fixed. I was following the pattern of userContentManager, which seems to also have a missing unref, I added that one in too.
(In reply to Carlos Garcia Campos from comment #47) > Comment on attachment 401348 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=401348&action=review > > > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:221 > > + * webkit_website_policies_set_autoplay_policy: > > hmm, I think WebKitWebsitePolicies should be immutable. Allowing to change > it is confusing, because it won't affect the web views already using this > object. This is not like WebKitSettings. Yeah, the update API for the loader policies seem like they will only add confusion. This setter is here simply to allow resuing the same policy object in multiple tests. It's not big deal to create a new one each time instead. I'll get rid of this and the update API if you both agree. The update() API is there to solve a fairly test-specific detail imo, and in return there's a lot of potential for confusion.
(In reply to Michael Catanzaro from comment #48) > (In reply to Carlos Garcia Campos from comment #46) > > I don't think it's ever expected to ask the user, that would block the load > > on policy decision. Let's think about how we would implement this in ephy to > > understand ow the api works. > > > > - We might have a global preference for autoplay in the preferences dialog. > > That should affect the default policies set to a web view. When this setting > > changes, update_website_policies needs to be called for already created web > > views. This makes me think that maybe it shouldn't be a construct only > > property. > > In Ephy, we would just leave autoplay disabled always, but yes, > theoretically a browser might want to do this. I'd recommend ALLOW_WITHOUT_SOUND rather than DENY as your browser default FWIW. I'd personally run with DENY, but it can be confusing when some websites completely break because of the strong choice.
(In reply to Carlos Garcia Campos from comment #50) > (In reply to Michael Catanzaro from comment #48) > > (In reply to Carlos Garcia Campos from comment #46) > > > I don't think it's ever expected to ask the user, that would block the load > > > on policy decision. Let's think about how we would implement this in ephy to > > > understand ow the api works. > > > > > > - We might have a global preference for autoplay in the preferences dialog. > > > That should affect the default policies set to a web view. When this setting > > > changes, update_website_policies needs to be called for already created web > > > views. This makes me think that maybe it shouldn't be a construct only > > > property. > > > > In Ephy, we would just leave autoplay disabled always, but yes, > > theoretically a browser might want to do this. > > > > > - We might also have an option in the page popover to override the default > > > policy for a particular website. In decide-policy we would check if the > > > given url is in a security origin with overridden policies and call > > > use_with_policies() instead. If this setting is changed after the page has > > > been loaded we need to call update_website_policies(). > > > > I guess that makes sense... but, we do not actually need the policy decision > > API at all, then. It could be done just as well in load-changed when > > LOAD_COMMITTED is seen. So I think I've changed my mind about that, we > > should only add API that is actually needed to do what we want. > > That might be racy, I think it's better to handle per-url exceptions in the > policy-decision, Right, the only benefit to the more complicated decide-policy approach is that it's not racey. As we discussed before, there is some precedent to racey APIs (like webkit_web_view_get_tls_info), but it seems like a good idea to avoid them when possible. > what I'm not sure about is the update_policies function. Let me know if you agree with getting rid of update_policies, it doesn't sound like our API users would have any use for it, the only use I've got at the moment is unit test specific.
(In reply to Charlie Turner from comment #53) > I'd recommend ALLOW_WITHOUT_SOUND rather than DENY as your browser default > FWIW. I'd personally run with DENY, but it can be confusing when some > websites completely break because of the strong choice. OK, good point, I think that's what other browsers are doing nowadays anyway. (In reply to Charlie Turner from comment #54) > Right, the only benefit to the more complicated decide-policy approach is > that it's not racey. As we discussed before, there is some precedent to > racey APIs (like webkit_web_view_get_tls_info), but it seems like a good > idea to avoid them when possible. Hm, TLS info is available after LOAD_COMMITTED (or LOAD_REDIRECTED if you want the result for a redirection), so at least that API should always work properly. But yes, I see you're right about this, if we try to update the polices after LOAD_COMMITTED, it could be too late because the web process is already loading the web content at that point. > > what I'm not sure about is the update_policies function. > > Let me know if you agree with getting rid of update_policies, it doesn't > sound like our API users would have any use for it, the only use I've got at > the moment is unit test specific. I agree.
Created attachment 401723 [details] Patch Address comments and rebase over 262940. Also attempt #1 of copying boilerplate for WPE
Comment on attachment 401723 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401723&action=review > Source/WebKit/UIProcess/API/glib/WebKitPolicyDecision.cpp:101 > + * > + * Accept the action which triggered this decision, and continue with > + * @policies affecting all subsequent loads of resources in the > + * current origin. > + * > + * For example, a navigation decision to a video sharing website may > + * be accepted under the priviso no movies are allowed to autoplay. The > + * autoplay policy in this case would be set in the @policies. We should be more clear that this is really only expected to be used with navigation policy decisions. Also: @policies will affect all subsequent loads of resources in the origin associated with the navigation, not the current origin (because we have not navigated to the new origin until after the policy decision is accepted). > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4667 > + > + One blank line between functions
Created attachment 401730 [details] Patch Address comments, attempt #2 at WPE boilerplate
Comment on attachment 401730 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401730&action=review Let's start with this API, then we will see if we really need the function to update policies for existing web views. I think we should try to implement the feature in ephy to ensure the API is enough (I can help with that if needed). > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4678 > + * associated with the view. Indent this line with 4 spaces. > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:106 > + policies->priv->websitePolicies->setAutoplayPolicy(WebKit::WebsiteAutoplayPolicy::Allow); No need for WebKit:: here > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:109 > + policies->priv->websitePolicies->setAutoplayPolicy(WebKit::WebsiteAutoplayPolicy::AllowWithoutSound); Ditto. > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:112 > + policies->priv->websitePolicies->setAutoplayPolicy(WebKit::WebsiteAutoplayPolicy::Deny); Ditto. > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:115 > + default: > + ASSERT_NOT_REACHED(); Do not add default, since we are handling all the possible cases. > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:127 > + case PROP_AUTOPLAY_POLICY: { > + webkitWebsitePoliciesSetAutoplayPolicy(policies, static_cast<WebKitAutoplayPolicy>(g_value_get_enum(value))); > + break; > + } Brackets aren't needed here. > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:155 > + WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND, I'm not sure about changing the default behavior here. I guess it's ok, since it doesn't require apps to handle any new api. > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:161 > + * Add some body here even if it's just Create a new #WebKitWebsitePolicies > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:175 > + * Ditto. > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:220 > + case WebKit::WebsiteAutoplayPolicy::Allow: No need for WebKit:: > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:227 > + case WebKit::WebsiteAutoplayPolicy::Default: > + return WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND; Is it really possible? This is a construct-only parameter so the setter is always called on construction with the default value WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:229 > + default: > + ASSERT_NOT_REACHED(); Do not add default when handling all possible values, we want the compiler to complain if new values are added to the enum. > Source/WebKit/UIProcess/API/gtk/WebKitWebsitePolicies.h:61 > + * @WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND: Allow videos to autoplay if > + * they have no audio track, or if their audio track is muted. I thought this allowed to autoplay all videos, but the all started muted, like chrome does in android. Indent the second line with 4 spaces. > Tools/MiniBrowser/gtk/main.c:574 > + WebKitWebsitePolicies *defaultWebsitePolicies = webkit_website_policies_new_with_policies( > + "autoplay-policy", WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND, > + NULL); We don't need to do this as long as it's the default option already. I would a command line option to set any autoplay-policy so that we can manually test with MiniBrowser. > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:117 > + return m_autoplayed && *m_autoplayed; m_autoplayed.valueOr(False); > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:320 > + test->m_autoplayed = WTF::nullopt; > + test->m_policyDecisionResponse = PolicyClientTest::UseWithPolicy; > + test->m_respondToPolicyDecisionAsynchronously = true; > + test->loadURI(resourceURL.get()); > + // Run until the user content messages come back from autoplay-check.html > + g_main_loop_run(test->m_mainLoop); What we normally do it adding a method to the test class, in this case something like bool loadURIAndWaitForAutoPlayed(const char* uri) { m_autoplayed = WTF::nullopt; m_policyDecisionResponse = PolicyClientTest::UseWithPolicy; loadURI(uri); g_main_loop_run(m_mainLoop); return m_autoplayed.valueOr(False); } > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:324 > + test->m_websitePolicies = webkit_website_policies_new_with_policies("autoplay-policy", WEBKIT_AUTOPLAY_ALLOW, nullptr); adoptGRef() > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:334 > + test->m_websitePolicies = webkit_website_policies_new_with_policies("autoplay-policy", WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND, nullptr); adoptGRef() > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:343 > + // Now check dynamically updating the loader policies This comment should be Test with WEBKIT_AUTOPLAY_DENY I guess. > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:344 > + test->m_websitePolicies = webkit_website_policies_new_with_policies("autoplay-policy", WEBKIT_AUTOPLAY_DENY, nullptr); adoptGRef() > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:407 > + PolicyClientTest::add("WebKitPolicyClient", "autoplay-policy", testAutoplayPolicy); > + // WARNING: This test must come last, it uses racey constructs that > + // interfere nondeterminisically with any test running after it. What other tests do fail if run after this one? We should try to avoid tests to depend on each other > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:409 > } We should also tests the default website policies in WebKitWebView API.
Comment on attachment 401730 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401730&action=review Thanks for the review. I will take a look at Ephy and see if I can test it there as well. >> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4678 >> + * associated with the view. > > Indent this line with 4 spaces. Done >> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:106 >> + policies->priv->websitePolicies->setAutoplayPolicy(WebKit::WebsiteAutoplayPolicy::Allow); > > No need for WebKit:: here Done. >> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:109 >> + policies->priv->websitePolicies->setAutoplayPolicy(WebKit::WebsiteAutoplayPolicy::AllowWithoutSound); > > Ditto. Done. >> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:112 >> + policies->priv->websitePolicies->setAutoplayPolicy(WebKit::WebsiteAutoplayPolicy::Deny); > > Ditto. Done. >> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:115 >> + ASSERT_NOT_REACHED(); > > Do not add default, since we are handling all the possible cases. This was C instincts to always be careful about unwanted int->enum conversions. Hopefully the implementation will always pass valid values in :). Changed. >> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:127 >> + } > > Brackets aren't needed here. Done. >> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:155 >> + WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND, > > I'm not sure about changing the default behavior here. I guess it's ok, since it doesn't require apps to handle any new api. This is in keeping with other browser defaults. Would you prefer I change it to ALLOW? >> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:161 >> + * > > Add some body here even if it's just Create a new #WebKitWebsitePolicies Done. >> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:175 >> + * > > Ditto. Done. >> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:227 >> + return WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND; > > Is it really possible? This is a construct-only parameter so the setter is always called on construction with the default value WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND Not now. It was in the past. Also avoids compiler warnings about unhandled switch values, and it adds a bit of implicit documentation about our default autoplay policy. Didn't seem necessary to ASSERT() it. >> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:229 >> + ASSERT_NOT_REACHED(); > > Do not add default when handling all possible values, we want the compiler to complain if new values are added to the enum. Done. >> Source/WebKit/UIProcess/API/gtk/WebKitWebsitePolicies.h:61 >> + * they have no audio track, or if their audio track is muted. > > I thought this allowed to autoplay all videos, but the all started muted, like chrome does in android. > > Indent the second line with 4 spaces. It will only start if the movie has no audio track, or if the website has arranged for the audio to be muted or the volume to be 0. The media player doesn't go out of its way to mute and/or set volume=0 to attempt an autoplay without sound. Indentation done. >> Tools/MiniBrowser/gtk/main.c:574 >> + NULL); > > We don't need to do this as long as it's the default option already. I would a command line option to set any autoplay-policy so that we can manually test with MiniBrowser. OK. I have added a command-line option, but this code remains (to set the option), even though as you point out it's redundant with the default set in WebKitWebsitePolicy. I can condition this out on whether the command-line was really passed if you'd prefer. >> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:117 >> + return m_autoplayed && *m_autoplayed; > > m_autoplayed.valueOr(False); Oo, nice. Done. >> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:320 >> + g_main_loop_run(test->m_mainLoop); > > What we normally do it adding a method to the test class, in this case something like > > bool loadURIAndWaitForAutoPlayed(const char* uri) > { > m_autoplayed = WTF::nullopt; > m_policyDecisionResponse = PolicyClientTest::UseWithPolicy; > loadURI(uri); > g_main_loop_run(m_mainLoop); > return m_autoplayed.valueOr(False); > } Done. >> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:324 >> + test->m_websitePolicies = webkit_website_policies_new_with_policies("autoplay-policy", WEBKIT_AUTOPLAY_ALLOW, nullptr); > > adoptGRef() Done, this was factored into the new loadURIAndWaitForAutoPlayed. >> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:334 >> + test->m_websitePolicies = webkit_website_policies_new_with_policies("autoplay-policy", WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND, nullptr); > > adoptGRef() Done. >> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:343 >> + // Now check dynamically updating the loader policies > > This comment should be Test with WEBKIT_AUTOPLAY_DENY I guess. Should be clearer now I refactored this test case, but it's as intended. >> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:344 >> + test->m_websitePolicies = webkit_website_policies_new_with_policies("autoplay-policy", WEBKIT_AUTOPLAY_DENY, nullptr); > > adoptGRef() Done. >> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:407 >> + // interfere nondeterminisically with any test running after it. > > What other tests do fail if run after this one? We should try to avoid tests to depend on each other It's specifically the ordering in this test. When I move testNewWindowPolicy to be the first test run here, it hits various assertions. I couldn't figure out why. https://bugs.webkit.org/show_bug.cgi?id=213190 >> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:409 >> } > > We should also tests the default website policies in WebKitWebView API. I added a test to check it's ALLOW_WITHOUT_SOUND.
Created attachment 401891 [details] Patch Address review comments
Comment on attachment 401891 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401891&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:71 > + case WebKitAutoplayPolicy::WEBKIT_AUTOPLAY_ALLOW: Didn't notice this before but you don't need the WebKitAutoplayPolicy:: since it's not a C++ enum class. > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:74 > + case WebKitAutoplayPolicy::WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND: Ditto. > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:77 > + case WebKitAutoplayPolicy::WEBKIT_AUTOPLAY_DENY: Ditto. > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:152 > + WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND, Let's keep this as the default, yes. > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:228 > + case WebsiteAutoplayPolicy::Default: I still don't see how Default can be set at this point. > Source/WebKit/UIProcess/API/glib/WebKitWebsitePoliciesPrivate.h:26 > +API::WebsitePolicies* webkitWebsitePoliciesGetWebsitePolicies(WebKitWebsitePolicies *); WebKitWebsitePolicies * -> WebKitWebsitePolicies* > Source/WebKit/UIProcess/API/glib/WebKitWebsitePoliciesPrivate.h:27 > +WebKit::WebsitePoliciesData webkitWebsitePoliciesGetPoliciesData(WebKitWebsitePolicies *); Ditto. > Source/WebKit/UIProcess/API/gtk/WebKitWebsitePolicies.h:85 > +webkit_website_policies_get_autoplay_policy (WebKitWebsitePolicies* policies); WebKitWebsitePolicies* -> WebKitWebsitePolicies * > Source/WebKit/UIProcess/API/wpe/WebKitWebsitePolicies.h:85 > +webkit_website_policies_get_autoplay_policy (WebKitWebsitePolicies* policies); Ditto. > Tools/MiniBrowser/gtk/main.c:132 > + { "autoplay-policy", 0, 0, G_OPTION_ARG_CALLBACK, parseAutoplayPolicy, "Autoplay policy", NULL }, Add the possible options to the help string "Autoplay policy (allow, allow-without-sound or deny)" for example > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:118 > + m_websitePolicies = adoptGRef(webkit_website_policies_new_with_policies("autoplay-policy", policy, nullptr)); hmm, now that I see this used, I think autoplay-policy as a property of class named WebKitWebsitePolicies is a bit redundant, what do you think about using just autoplay?
(In reply to Carlos Garcia Campos from comment #62) > hmm, now that I see this used, I think autoplay-policy as a property of > class named WebKitWebsitePolicies is a bit redundant, what do you think > about using just autoplay? I think it's OK for every property name to end in "-policy"
Created attachment 402009 [details] Patch Address remaining comments for cq+
(In reply to Carlos Garcia Campos from comment #62) > Comment on attachment 401891 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=401891&action=review > > > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:71 > > + case WebKitAutoplayPolicy::WEBKIT_AUTOPLAY_ALLOW: > > Didn't notice this before but you don't need the WebKitAutoplayPolicy:: > since it's not a C++ enum class. Done. (and the others you pointed out) > > Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:228 > > + case WebsiteAutoplayPolicy::Default: > > I still don't see how Default can be set at this point. Removed. It's not actually used API side, was there for completeness. I'll simply fold it into the default: handler. > > > Source/WebKit/UIProcess/API/glib/WebKitWebsitePoliciesPrivate.h:26 > > +API::WebsitePolicies* webkitWebsitePoliciesGetWebsitePolicies(WebKitWebsitePolicies *); > > WebKitWebsitePolicies * -> WebKitWebsitePolicies* > > > Source/WebKit/UIProcess/API/glib/WebKitWebsitePoliciesPrivate.h:27 > > +WebKit::WebsitePoliciesData webkitWebsitePoliciesGetPoliciesData(WebKitWebsitePolicies *); > > Ditto. Done. > > > Source/WebKit/UIProcess/API/gtk/WebKitWebsitePolicies.h:85 > > +webkit_website_policies_get_autoplay_policy (WebKitWebsitePolicies* policies); > > WebKitWebsitePolicies* -> WebKitWebsitePolicies * > > > Source/WebKit/UIProcess/API/wpe/WebKitWebsitePolicies.h:85 > > +webkit_website_policies_get_autoplay_policy (WebKitWebsitePolicies* policies); > > Ditto. Done. > > > Tools/MiniBrowser/gtk/main.c:132 > > + { "autoplay-policy", 0, 0, G_OPTION_ARG_CALLBACK, parseAutoplayPolicy, "Autoplay policy", NULL }, > > Add the possible options to the help string "Autoplay policy (allow, > allow-without-sound or deny)" for example Done. > > > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:118 > > + m_websitePolicies = adoptGRef(webkit_website_policies_new_with_policies("autoplay-policy", policy, nullptr)); > > hmm, now that I see this used, I think autoplay-policy as a property of > class named WebKitWebsitePolicies is a bit redundant, what do you think > about using just autoplay? I see your point, there's a lot of stuttering going on. I changed to "autoplay" as the property name, but kept --autoplay-policy for the browser option (in keeping with other browsers).
Created attachment 402010 [details] Patch Add reviewed-by lines
Committed r263095: <https://trac.webkit.org/changeset/263095> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402010 [details].