Summary: | Add settings to restrict media containers and codecs when in Captive Portal mode | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||||||||||||||
Component: | Media | Assignee: | Jer Noble <jer.noble> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | benjamin, calvaris, cdumez, cgarcia, changseok, cmarcelo, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gustavo, gyuyoung.kim, kondapallykalyan, macpherson, menard, peng.liu6, philipj, pnormand, sam, sergio, vjaquez, webkit-bug-importer | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Attachments: |
|
Description
Jer Noble
2022-02-07 08:51:15 PST
Created attachment 451125 [details]
Patch
Comment on attachment 451125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451125&action=review > Source/WTF/Scripts/Preferences/WebPreferences.yaml:1329 > +MediaAudioCodecIDsAllowedWhenCaptivePortalModeEnabled: The name looks a little strange. Is "MediaAudioCodecIDsAllowedInCaptivePortalMode" better? > Source/WebCore/page/SettingsBase.cpp:238 > + Vector<FourCC> newTypes; > + for (auto type : StringView(types).split(',')) { Nit. Maybe we can implement this in a function and reuse the function? > Source/WebKit/WebKit.xcodeproj/xcshareddata/xcschemes/WebKitSwift.xcscheme:1 > +<?xml version="1.0" encoding="UTF-8"?> Is this change relevant? Comment on attachment 451125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451125&action=review r=me once the bots are happy > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:362 > + auto& settings = document().settings(); > + if (!contentTypeMeetsContainerAndCodecTypeRequirements(contentType, settings.allowedMediaContainerTypes(), settings.allowedMediaCodecTypes())) > + return Exception { NotSupportedError }; You could add a public method to MediaSource instead of repeating the check here. (In reply to Peng Liu from comment #2) > Comment on attachment 451125 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=451125&action=review > > > Source/WTF/Scripts/Preferences/WebPreferences.yaml:1329 > > +MediaAudioCodecIDsAllowedWhenCaptivePortalModeEnabled: > > The name looks a little strange. Is > "MediaAudioCodecIDsAllowedInCaptivePortalMode" better? Ok! > > Source/WebCore/page/SettingsBase.cpp:238 > > + Vector<FourCC> newTypes; > > + for (auto type : StringView(types).split(',')) { > > Nit. Maybe we can implement this in a function and reuse the function? I dunno, it's pretty concise as it is. > > Source/WebKit/WebKit.xcodeproj/xcshareddata/xcschemes/WebKitSwift.xcscheme:1 > > +<?xml version="1.0" encoding="UTF-8"?> > > Is this change relevant? Whoops! Nope. (In reply to Eric Carlson from comment #3) > Comment on attachment 451125 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=451125&action=review > > r=me once the bots are happy > > > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:362 > > + auto& settings = document().settings(); > > + if (!contentTypeMeetsContainerAndCodecTypeRequirements(contentType, settings.allowedMediaContainerTypes(), settings.allowedMediaCodecTypes())) > > + return Exception { NotSupportedError }; > > You could add a public method to MediaSource instead of repeating the check > here. It's already been factored into a single utility method; I'm not sure it's worth further refactoring. Created attachment 451255 [details]
Patch
Created attachment 451264 [details]
Patch
Created attachment 451267 [details]
Patch
Comment on attachment 451267 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451267&action=review > Source/WTF/ChangeLog:3 > + [Cocoa] Adopt -streamDataParser:didProvideContentKeySpecifier:forTrackID: delegate callback Is the right change for this bug? The title is quite different. (In reply to Sam Weinig from comment #8) > Comment on attachment 451267 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=451267&action=review > > > Source/WTF/ChangeLog:3 > > + [Cocoa] Adopt -streamDataParser:didProvideContentKeySpecifier:forTrackID: delegate callback > > Is the right change for this bug? The title is quite different. Ugh. webkit-patch uploaded that patch to the wrong bug. Will fix. Created attachment 451271 [details]
Patch
Comment on attachment 451271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451271&action=review > Source/WTF/ChangeLog:134 > +2022-02-07 Jer Noble <jer.noble@apple.com> > + > + Add settings to restrict media containers and codecs when in Captive Portal mode > + https://bugs.webkit.org/show_bug.cgi?id=236245 > + > + Reviewed by NOBODY (OOPS!). > + > + * Scripts/Preferences/WebPreferences.yaml: > + Nit: move this to the top of the ChangeLog > LayoutTests/media/video-test.js:243 > + return waitFor(element, type, false, true); s/false, true/false, false/ Comment on attachment 451271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451271&action=review > LayoutTests/ChangeLog:373 > +2022-02-07 Jer Noble <jer.noble@apple.com> Nit: move this to the top of the ChangeLog. :-) Created attachment 451713 [details]
Patch for landing
Looks like settings are bleeding between runs. Will update with a patch that resets these new settings to consistent values between runs. Created attachment 451764 [details]
Patch for landing
Created attachment 451766 [details]
Patch for landing
Created attachment 451785 [details]
Patch for landing
Committed r289697 (247182@main): <https://commits.webkit.org/247182@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 451785 [details]. |