Bug 131311 – Use displayNameForTrack instead of textTrack->label() for captions.

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

[Jeremy Jones]

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

[Radar WebKit Bug Importer]

<rdar://problem/16552360>

[Jeremy Jones]

Created attachment 230618 [details]
Patch

[Darin Adler]

Comment on attachment 230618 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.h:53
> +    void updateLegibleOptions();

This class has a strange organization. Normally in WebKit we put the private members last and explicitly mark the section private.

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:126
> +    } else if (event->type() == eventNames().addtrackEvent
> +        || event->type() == eventNames().removetrackEvent)

I think this would read better on a single line.

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:231
> +    CaptionUserPreferences* captionPreferences = m_mediaElement->document().page()->group().captionPreferences();

If this can never be null it should be a reference instead of a pointer.

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:238
> +    for (unsigned i = 0, length = m_legibleTracksForMenu.size(); i < length; ++i) {
> +        RefPtr<TextTrack> textTrack = m_legibleTracksForMenu[i];
> +        legibleOptions.append(captionPreferences->displayNameForTrack(textTrack.get()));
> +    }

This should use a modern C++ for loop:

    for (auto& track : m_legibleTracksForMenu)
        legibleOptions.append(preferences.displayNameForTrack(track.get()));

[Jeremy Jones]

(In reply to comment #3)
> (From update of attachment 230618 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230618&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.h:53
> > +    void updateLegibleOptions();
> 
> This class has a strange organization. Normally in WebKit we put the private members last and explicitly mark the section private.

Reordered.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:126
> > +    } else if (event->type() == eventNames().addtrackEvent
> > +        || event->type() == eventNames().removetrackEvent)
> 
> I think this would read better on a single line.

Joined.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:231
> > +    CaptionUserPreferences* captionPreferences = m_mediaElement->document().page()->group().captionPreferences();
> 
> If this can never be null it should be a reference instead of a pointer.

Changed to reference.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:238
> > +    for (unsigned i = 0, length = m_legibleTracksForMenu.size(); i < length; ++i) {
> > +        RefPtr<TextTrack> textTrack = m_legibleTracksForMenu[i];
> > +        legibleOptions.append(captionPreferences->displayNameForTrack(textTrack.get()));
> > +    }
> 
> This should use a modern C++ for loop:
> 
>     for (auto& track : m_legibleTracksForMenu)
>         legibleOptions.append(preferences.displayNameForTrack(track.get()));

Modernized.

[Jeremy Jones]

Created attachment 230669 [details]
Patch

[Jeremy Jones]

Created attachment 230701 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 230701 [details]
Patch for landing

Rejecting attachment 230701 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 230701 [details]
Patch for landing

Rejecting attachment 230701 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 230701 [details]
Patch for landing

Rejecting attachment 230701 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 230701 [details]
Patch for landing

Clearing flags on attachment: 230701

Committed r168216: <http://trac.webkit.org/changeset/168216>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.