Bug 153761 – Allow ports to disable automatic text track selection

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

Add a "manual" selection mode.

[Eric Carlson]

<rdar://problem/24416768>

[Eric Carlson]

Created attachment 270459 [details]
Proposed patch

[WebKit Commit Bot]

Attachment 270459 [details] did not pass style-queue:


ERROR: Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:120:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 270460 [details]
Updated patch.

[Darin Adler]

Comment on attachment 270460 [details]
Updated patch.

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

> Source/WebCore/html/HTMLMediaElement.cpp:3582
> +        if (document.page())
> +            m_captionDisplayMode = document.page()->group().captionPreferences()->captionDisplayMode();

- Is it OK to leave m_captionDisplayMode uninitialized here if document.page() is null? Maybe we should set it to Automatic in that case?
- What guarantees captionPreferences() is non-null?
- I’d put the page into a local variable rather than saying document.page() twice.

> Source/WebCore/page/CaptionUserPreferences.cpp:70
> +void CaptionUserPreferences::endBlockingNotificaitons()

Typo: endBlockingNotificaitons misspells notifications

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:80
> +static MTEnableCaption2015BehaviorPtrType MTEnableCaption2015BehaviorPtr = nullptr;

This should just be const, since it will never be non-null, right?

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:118
> +        if (!MTEnableCaption2015BehaviorPtr())
> +            return;

Looks wrong to me. I’d think this would be checking the pointer for null, not calling the pointer and checking that for null. I don’t see code that looks like this in other code that uses SOFT_LINK_OPTIONAL. Is this really the correct idiom?

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:120
> +        if (MTEnableCaption2015BehaviorPtr()()) {

Similarly looks wrong, but maybe not.

I also suggest doing this as an early return.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:746
> +    static bool manualSelectionMode;
> +    static std::once_flag initializeSelectionMode;
> +
> +    std::call_once(initializeSelectionMode, [] {
> +        if (MTEnableCaption2015BehaviorPtr())
> +            manualSelectionMode = MTEnableCaption2015BehaviorPtr()();
> +    });

There’s no need to use std::call_once if we aren’t trying to use this function from multiple threads. Could just use normal initialization:

    static bool manualSelectionMode = MTEnableCaption2015BehaviorPtr() && MTEnableCaption2015BehaviorPtr()();

But also, this use looks so peculiar. Is that really how SOFT_LINK_OPTIONAL is used?

It’s really annoying to have this code twice. Could we write it once somewhere and share it?

[Eric Carlson]

Comment on attachment 270460 [details]
Updated patch.

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

>> Source/WebCore/html/HTMLMediaElement.cpp:3582
>> +            m_captionDisplayMode = document.page()->group().captionPreferences()->captionDisplayMode();
> 
> - Is it OK to leave m_captionDisplayMode uninitialized here if document.page() is null? Maybe we should set it to Automatic in that case?
> - What guarantees captionPreferences() is non-null?
> - I’d put the page into a local variable rather than saying document.page() twice.

> Is it OK to leave m_captionDisplayMode uninitialized here if document.page() is null? Maybe we should set it to Automatic in that case?
m_captionDisplayMode is initialized in the constructor.

> What guarantees captionPreferences() is non-null?
captionPreferences() allocates the preferences lazily on demand. It can't be null (unless new fails) so it should return a reference. I will fix that in a followup.

> I’d put the page into a local variable rather than saying document.page() twice.
Done.

>> Source/WebCore/page/CaptionUserPreferences.cpp:70
>> +void CaptionUserPreferences::endBlockingNotificaitons()
> 
> Typo: endBlockingNotificaitons misspells notifications

Fixed.

>> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:80
>> +static MTEnableCaption2015BehaviorPtrType MTEnableCaption2015BehaviorPtr = nullptr;
> 
> This should just be const, since it will never be non-null, right?

This is just wrong, MTEnableCaption2015BehaviorPtr is a function. Fixed.

>> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:118
>> +            return;
> 
> Looks wrong to me. I’d think this would be checking the pointer for null, not calling the pointer and checking that for null. I don’t see code that looks like this in other code that uses SOFT_LINK_OPTIONAL. Is this really the correct idiom?

MTEnableCaption2015BehaviorPtr is a function defined by the SOFT_LINK_OPTIONAL macro, which returns a pointer to the framework function or NULL. I have changed this to make that more obvious.

>> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:120
>> +        if (MTEnableCaption2015BehaviorPtr()()) {
> 
> Similarly looks wrong, but maybe not.
> 
> I also suggest doing this as an early return.

Done.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:746
>> +    });
> 
> There’s no need to use std::call_once if we aren’t trying to use this function from multiple threads. Could just use normal initialization:
> 
>     static bool manualSelectionMode = MTEnableCaption2015BehaviorPtr() && MTEnableCaption2015BehaviorPtr()();
> 
> But also, this use looks so peculiar. Is that really how SOFT_LINK_OPTIONAL is used?
> 
> It’s really annoying to have this code twice. Could we write it once somewhere and share it?

> There’s no need to use std::call_once if we aren’t trying to use this function from multiple threads. Could just use normal initialization:
> 
>     static bool manualSelectionMode = MTEnableCaption2015BehaviorPtr() && MTEnableCaption2015BehaviorPtr()();

Done.

> But also, this use looks so peculiar. Is that really how SOFT_LINK_OPTIONAL is used?
> 
> It’s really annoying to have this code twice. Could we write it once somewhere and share it?

I will do that in a followup.

[Eric Carlson]

Created attachment 270486 [details]
Patch for landing.

[Eric Carlson]

Committed r196010: https://trac.webkit.org/r196010