Bug 131069 – Enable fullscreen captions selection

• Component: WebKit2
• Status: RESOLVED FIXED

[Jeremy Jones]

Enable captions button in fullscreen.

[Jeremy Jones]

Created attachment 228327 [details]
Patch

[Eric Carlson]

Comment on attachment 228327 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenInterface.h:52
> +    

Nit: extra blank line.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:290
> +    

Can't you return here if option is nil?

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:299
> +        self.delegate->selectAudioMediaOption(index != NSNotFound ? index : UINT64_MAX);

index can't be NSNotFound.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:323
> +    [_currentLegibleMediaSelectionOption release];
> +    _currentLegibleMediaSelectionOption = [option retain];
> +    
> +    ASSERT(self.delegate);
> +    
> +    NSInteger index = NSNotFound;
> +    
> +    if (option && self.legibleMediaSelectionOptions)
> +        index = [self.legibleMediaSelectionOptions indexOfObject:option];
> +    
> +    if (index != NSNotFound)
> +        self.delegate->selectLegibleMediaOption(index != NSNotFound ? index : UINT64_MAX);

Ditto the comments about setCurrentAudioMediaSelectionOption.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:488
> +    NSMutableArray* webOptions = [NSMutableArray array];

Nit: you can optimize this slightly by allocating the number of elements you will need.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:509
> +    NSMutableArray* webOptions = [NSMutableArray array];
> +    for (auto& name : options) {
> +        RetainPtr<WebAVMediaSelectionOption> webOption = adoptNS([[WebAVMediaSelectionOption alloc] init]);
> +        RetainPtr<NSString> nameString = adoptNS([[NSString alloc] initWithUTF8String:name.utf8().data()]);
> +        [webOption setLocalizedDisplayName:nameString.get()];
> +        [webOptions addObject:webOption.get()];
> +    }

Instead of duplicating this code here and in setAudioMediaSelectionOptions, why don't you put it into a shared function?

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.h:70
> +    

Nit: extra blank line.

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:89
> +    

Nit: extra spaces (hint: enable trimming "whitespace-only lines" in Xcode editing prefs).

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:208
> +void WebVideoFullscreenModelMediaElement::selectAudioMediaOption(uint64_t)
> +{
> +}

This needs a FIXME.

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:71
> +    virtual void selectAudioMediaOption(uint64_t index) override;
> +    virtual void selectLegibleMediaOption(uint64_t index) override;

Nit: "index" doesn't add anything.

[Jeremy Jones]

(In reply to comment #2)
> (From update of attachment 228327 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228327&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterface.h:52
> > +    
> 
> Nit: extra blank line.

Done.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:290
> > +    
> 
> Can't you return here if option is nil?

No, because we still need to inform the delegate of the change.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:299
> > +        self.delegate->selectAudioMediaOption(index != NSNotFound ? index : UINT64_MAX);
> 
> index can't be NSNotFound.

Fixed.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:323
> > +    [_currentLegibleMediaSelectionOption release];
> > +    _currentLegibleMediaSelectionOption = [option retain];
> > +    
> > +    ASSERT(self.delegate);
> > +    
> > +    NSInteger index = NSNotFound;
> > +    
> > +    if (option && self.legibleMediaSelectionOptions)
> > +        index = [self.legibleMediaSelectionOptions indexOfObject:option];
> > +    
> > +    if (index != NSNotFound)
> > +        self.delegate->selectLegibleMediaOption(index != NSNotFound ? index : UINT64_MAX);
> 
> Ditto the comments about setCurrentAudioMediaSelectionOption.

Ditto.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:488
> > +    NSMutableArray* webOptions = [NSMutableArray array];
> 
> Nit: you can optimize this slightly by allocating the number of elements you will need.

Done.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:509
> > +    NSMutableArray* webOptions = [NSMutableArray array];
> > +    for (auto& name : options) {
> > +        RetainPtr<WebAVMediaSelectionOption> webOption = adoptNS([[WebAVMediaSelectionOption alloc] init]);
> > +        RetainPtr<NSString> nameString = adoptNS([[NSString alloc] initWithUTF8String:name.utf8().data()]);
> > +        [webOption setLocalizedDisplayName:nameString.get()];
> > +        [webOptions addObject:webOption.get()];
> > +    }
> 
> Instead of duplicating this code here and in setAudioMediaSelectionOptions, why don't you put it into a shared function?

Done.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.h:70
> > +    
> 
> Nit: extra blank line.

Deleted.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:89
> > +    
> 
> Nit: extra spaces (hint: enable trimming "whitespace-only lines" in Xcode editing prefs).

Done and done.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:208
> > +void WebVideoFullscreenModelMediaElement::selectAudioMediaOption(uint64_t)
> > +{
> > +}
> 
> This needs a FIXME.

Added // FIXME: 131236 Implement audio track selection

> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:71
> > +    virtual void selectAudioMediaOption(uint64_t index) override;
> > +    virtual void selectLegibleMediaOption(uint64_t index) override;
> 
> Nit: "index" doesn't add anything.

Deleted.

[Jeremy Jones]

Created attachment 228618 [details]
Patch

[Eric Carlson]

Comment on attachment 228618 [details]
Patch

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

The WebCore changes look OK to me modulo my comments, which can be addressed in a follow up. I am not a WK2 reviewer so I won't mark this r+.

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:93
> +        legibleOptions.append(textTrack->label());

This is wrong, not all tracks have a label but every menu item needs to have a label. We calculate the display name in CaptionPreferences::displayNameForTrack. Probably the right thing to do here is to add a displayName method to PlatformTextTrack and have TextTrack::platformTextTrack call CaptionPreferences::displayNameForTrack.

I think this can be done in a follow-up bug.

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:216
> +        platformTrack = m_mediaElement->platformTextTracks()[static_cast<size_t>(index)];

I should have noticed this before, but why pass a uint64_t when we need a size_t?

[Jeremy Jones]

(In reply to comment #5)
> (From update of attachment 228618 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228618&action=review
> 
> The WebCore changes look OK to me modulo my comments, which can be addressed in a follow up. I am not a WK2 reviewer so I won't mark this r+.
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:93
> > +        legibleOptions.append(textTrack->label());
> 
> This is wrong, not all tracks have a label but every menu item needs to have a label. We calculate the display name in CaptionPreferences::displayNameForTrack. Probably the right thing to do here is to add a displayName method to PlatformTextTrack and have TextTrack::platformTextTrack call CaptionPreferences::displayNameForTrack.
> 
> I think this can be done in a follow-up bug.

Filed:
https://bugs.webkit.org/show_bug.cgi?id=131311
Use displayNameForTrack instead of textTrack->label() for captions.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:216
> > +        platformTrack = m_mediaElement->platformTextTracks()[static_cast<size_t>(index)];
> 
> I should have noticed this before, but why pass a uint64_t when we need a size_t?

I tried size_t first. It is unprecedented and has no encoder/decoder. My hypothesis is that it is not used because web process and UI process aren't required to be the same architecture. In theory, there could be a 32-bit UI process talking to a 64-bit web process. They would disagree on how this value is serialized. So it makes sense to require more explicit data sizes. Just a hypothesis.

[Tim Horton]

Comment on attachment 228618 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:106
> +@property (retain) NSArray *audioMediaSelectionOptions; // readonly

what’s up with the // readonly?

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:250
> +    return ([self hasAudioMediaSelectionOptions] || [self hasLegibleMediaSelectionOptions]);

no need for parens

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:260
> +    return ([[self audioMediaSelectionOptions] count] > 0);

nor here nor in a few other places.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:484
> +static NSMutableArray* mediaSelectionOptions(const Vector<String>& options)

all of the ObjC stars are on the wrong side!

[Jeremy Jones]

Created attachment 228937 [details]
Patch

[Jeremy Jones]

(In reply to comment #7)
> (From update of attachment 228618 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228618&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:106
> > +@property (retain) NSArray *audioMediaSelectionOptions; // readonly
> 
> what’s up with the // readonly?

Deleted

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:250
> > +    return ([self hasAudioMediaSelectionOptions] || [self hasLegibleMediaSelectionOptions]);
> 
> no need for parens

Deleted.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:260
> > +    return ([[self audioMediaSelectionOptions] count] > 0);
> 
> nor here nor in a few other places.

Deleted. 

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:484
> > +static NSMutableArray* mediaSelectionOptions(const Vector<String>& options)
> 
> all of the ObjC stars are on the wrong side!

Moved.

[WebKit Commit Bot]

Comment on attachment 228937 [details]
Patch

Rejecting attachment 228937 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 228937, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
VideoFullscreenManagerProxy.messages.in
patching file Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm
patching file Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.h
patching file Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.messages.in
patching file Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Eric Carlson']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/5561260254953472

[Jeremy Jones]

Created attachment 228986 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 228986 [details]
Patch

Clearing flags on attachment: 228986

Committed r167044: <http://trac.webkit.org/changeset/167044>