Bug 153761 - Allow ports to disable automatic text track selection
Summary: Allow ports to disable automatic text track selection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-02-01 13:30 PST by Eric Carlson
Modified: 2016-02-02 08:39 PST (History)
2 users (show)

See Also:


Attachments
Proposed patch (28.55 KB, patch)
2016-02-01 17:48 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Updated patch. (28.55 KB, patch)
2016-02-01 17:54 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch for landing. (28.35 KB, patch)
2016-02-02 07:47 PST, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2016-02-01 13:30:10 PST
Add a "manual" selection mode.
Comment 1 Eric Carlson 2016-02-01 13:31:14 PST
<rdar://problem/24416768>
Comment 2 Eric Carlson 2016-02-01 17:48:59 PST
Created attachment 270459 [details]
Proposed patch
Comment 3 WebKit Commit Bot 2016-02-01 17:50:35 PST
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.
Comment 4 Eric Carlson 2016-02-01 17:54:48 PST
Created attachment 270460 [details]
Updated patch.
Comment 5 Darin Adler 2016-02-01 18:09:06 PST
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?
Comment 6 Eric Carlson 2016-02-02 07:46:36 PST
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.
Comment 7 Eric Carlson 2016-02-02 07:47:33 PST
Created attachment 270486 [details]
Patch for landing.
Comment 8 Eric Carlson 2016-02-02 08:39:11 PST
Committed r196010: https://trac.webkit.org/r196010