Bug 236245 – Add settings to restrict media containers and codecs when in Captive Portal mode

• Component: Media
• Status: RESOLVED FIXED

[Jer Noble]

Add settings to restrict media containers and codecs when in Captive Portal mode

[Jer Noble]

Created attachment 451125 [details]
Patch

[Peng Liu]

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?

[Eric Carlson]

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.

[Jer Noble]

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

[Jer Noble]

Created attachment 451255 [details]
Patch

[Jer Noble]

Created attachment 451264 [details]
Patch

[Jer Noble]

Created attachment 451267 [details]
Patch

[Sam Weinig]

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.

[Jer Noble]

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

[Jer Noble]

Created attachment 451271 [details]
Patch

[Eric Carlson]

Comment on attachment 451271 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451271&action=review

> Source/WTF/ChangeLog:134
> [phone]  Jer Noble  <[e-mail]>
> +
> +        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/

[Peng Liu]

Comment on attachment 451271 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451271&action=review

> LayoutTests/ChangeLog:373
> [phone]  Jer Noble  <[e-mail]>

Nit: move this to the top of the ChangeLog. :-)

[Jer Noble]

Created attachment 451713 [details]
Patch for landing

[Jer Noble]

Looks like settings are bleeding between runs. Will update with a patch that resets these new settings to consistent values between runs.

[Jer Noble]

Created attachment 451764 [details]
Patch for landing

[Jer Noble]

Created attachment 451766 [details]
Patch for landing

[Jer Noble]

Created attachment 451785 [details]
Patch for landing

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/88857365>