[webkit-reviews] review granted: [Bug 112800] Allow port specific text track menu : [Attachment 194067] Updated patch.

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 112800: Allow port specific text track menu
https://bugs.webkit.org/show_bug.cgi?id=112800

Attachment 194067: Updated patch.
https://bugs.webkit.org/attachment.cgi?id=194067&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=194067&action=review


> Source/WebCore/html/HTMLMediaElement.cpp:3199
> +	   else {
>	       track->setMode(TextTrack::showingKeyword());
> -	       if (captionPreferences && track->language().length())
> -		   captionPreferences->setPreferredLanguage(track->language());

>	   }

Nit: does this end up with a single line else {} ?

> Source/WebCore/html/HTMLMediaElement.cpp:3206
> +    CaptionUserPreferences* captionPreferences = document()->page() ?
document()->page()->group().captionPreferences() : 0;
> +    if (captionPreferences) {
> +	   captionPreferences->setShouldShowCaptions(trackToSelect);
> +	   if (trackToSelect && trackToSelect->language().length())
> +	      
captionPreferences->setPreferredLanguage(trackToSelect->language());

Should this be wrapped in a PLATFORM(MAC) in case not every platform wants to
set the preferred language based on the selected track?

> Source/WebCore/html/shadow/MediaControlElements.cpp:887
> +	   menuItem->setAttribute(trackIndexAttributeName(), String::number(i),
ASSERT_NO_EXCEPTION);

Do we still need this now we have the menu to track map?

> Source/WebCore/page/CaptionUserPreferencesMac.mm:123
> +	   return CaptionUserPreferences::shouldShowCaptions();
> +    
>      return
MACaptionAppearanceGetShowCaptions(kMACaptionAppearanceDomainUser);

Nit: I think you're adding whitespace to the line here.

> Source/WebCore/page/CaptionUserPreferencesMac.mm:133
> +	   CaptionUserPreferences::setShouldShowCaptions(preference);
>	   return;
>      }
> -
> +    
>      MACaptionAppearanceSetShowCaptions(kMACaptionAppearanceDomainUser,
preference);

Nit: and maybe here.

> Source/WebCore/page/CaptionUserPreferencesMac.mm:528
> +#endif  // HAVE(MEDIA_ACCESSIBILITY_FRAMEWORK)
> +    
> +static String trackDisplayName(TextTrack* track)

Nit: and maybe there too.

> Source/WebCore/page/CaptionUserPreferencesMac.mm:607
> +    bool aIsPreferredLanguage = !codePointCompare(aLanguageDisplayName,
preferredLanguageDisplayName);

Can you use equalIgnoringCase here?

> Source/WebCore/page/CaptionUserPreferencesMac.mm:610
> +	   return aIsPreferredLanguage ? true : false;

return aIsPreferredLanguage;

> Source/WebCore/page/CaptionUserPreferencesMac.mm:620
> +	   return aIsMainContent ? true : false;

return aIsMainContent;

> Source/WebCore/page/CaptionUserPreferencesMac.mm:627
> +	       return aIsMainContent ? true : false;

ditto

> Source/WebCore/page/CaptionUserPreferencesMac.mm:629
> +	       return bIsMainContent ? true : false;

ditto

> Source/WebCore/platform/Language.cpp:160
> -    if (!localeName.isNull() && !localeName.isEmpty())
> -	   return
CFLocaleCopyDisplayNameForPropertyValue(CFLocaleCopyCurrent(),
kCFLocaleIdentifier, localeName.createCFString().get());
> +    if (!localeName.isNull() && !localeName.isEmpty()) {
> +	   RetainPtr<CFLocaleRef> currentLocale(AdoptCF,
CFLocaleCopyCurrent());
> +	   return CFLocaleCopyDisplayNameForPropertyValue(currentLocale.get(),
kCFLocaleIdentifier, localeName.createCFString().get());
> +    }

My bad :(

> Source/WebCore/platform/graphics/InbandTextTrackPrivate.h:59
>      virtual Kind kind() const { return Subtitles; }
> +
>      virtual bool isClosedCaptions() const { return false; }

Super nit: you didn't separate these in InbandTextTrackPrivateAVFObjC.h