Bug 236245 - Add settings to restrict media containers and codecs when in Captive Portal mode
Summary: Add settings to restrict media containers and codecs when in Captive Portal mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-07 08:51 PST by Jer Noble
Modified: 2022-02-12 10:19 PST (History)
22 users (show)

See Also:


Attachments
Patch (88.69 KB, patch)
2022-02-07 10:48 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (86.37 KB, patch)
2022-02-08 08:52 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (86.40 KB, patch)
2022-02-08 09:49 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (82.54 KB, patch)
2022-02-08 10:01 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (86.36 KB, patch)
2022-02-08 10:38 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (86.37 KB, patch)
2022-02-11 09:42 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (88.63 KB, patch)
2022-02-11 18:04 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (88.63 KB, patch)
2022-02-11 18:14 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (89.30 KB, patch)
2022-02-12 09:05 PST, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2022-02-07 08:51:15 PST
Add settings to restrict media containers and codecs when in Captive Portal mode
Comment 1 Jer Noble 2022-02-07 10:48:02 PST
Created attachment 451125 [details]
Patch
Comment 2 Peng Liu 2022-02-07 13:15:41 PST
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 3 Eric Carlson 2022-02-07 14:49:29 PST
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.
Comment 4 Jer Noble 2022-02-07 23:50:38 PST
(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.
Comment 5 Jer Noble 2022-02-08 08:52:21 PST
Created attachment 451255 [details]
Patch
Comment 6 Jer Noble 2022-02-08 09:49:23 PST
Created attachment 451264 [details]
Patch
Comment 7 Jer Noble 2022-02-08 10:01:03 PST
Created attachment 451267 [details]
Patch
Comment 8 Sam Weinig 2022-02-08 10:05:08 PST
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.
Comment 9 Jer Noble 2022-02-08 10:34:06 PST
(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.
Comment 10 Jer Noble 2022-02-08 10:38:53 PST
Created attachment 451271 [details]
Patch
Comment 11 Eric Carlson 2022-02-08 10:55:05 PST
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 12 Peng Liu 2022-02-08 11:35:34 PST
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. :-)
Comment 13 Jer Noble 2022-02-11 09:42:58 PST
Created attachment 451713 [details]
Patch for landing
Comment 14 Jer Noble 2022-02-11 13:46:11 PST
Looks like settings are bleeding between runs. Will update with a patch that resets these new settings to consistent values between runs.
Comment 15 Jer Noble 2022-02-11 18:04:35 PST
Created attachment 451764 [details]
Patch for landing
Comment 16 Jer Noble 2022-02-11 18:14:28 PST
Created attachment 451766 [details]
Patch for landing
Comment 17 Jer Noble 2022-02-12 09:05:36 PST
Created attachment 451785 [details]
Patch for landing
Comment 18 EWS 2022-02-12 10:18:04 PST
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].
Comment 19 Radar WebKit Bug Importer 2022-02-12 10:19:20 PST
<rdar://problem/88857365>