> > 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.
<rdar://problem/16552360>
Created attachment 230618 [details] Patch
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()));
(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.
Created attachment 230669 [details] Patch
Created attachment 230701 [details] Patch for landing
Comment on attachment 230701 [details] Patch for landing Rejecting attachment 230701 [details] from commit-queue. jeremyj-wk@apple.com 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.
Comment on attachment 230701 [details] Patch for landing Clearing flags on attachment: 230701 Committed r168216: <http://trac.webkit.org/changeset/168216>
All reviewed patches have been landed. Closing bug.