[webkit-reviews] review granted: [Bug 227031] Update Media Accessibility preferences in the WebContent process from the UI process : [Attachment 431447] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Per Arne Vollan
<pvollan at apple.com>'s request for review:
Bug 227031: Update Media Accessibility preferences in the WebContent process
from the UI process
https://bugs.webkit.org/show_bug.cgi?id=227031

Attachment 431447: Patch

https://bugs.webkit.org/attachment.cgi?id=431447&action=review




--- Comment #3 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 431447
  --> https://bugs.webkit.org/attachment.cgi?id=431447
Patch

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

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:222
> +    case kMACaptionAppearanceDisplayTypeForcedOnly:
> +	   return ForcedOnly;
> +
> +    case kMACaptionAppearanceDisplayTypeAutomatic:
> +	   return Automatic;
> +
> +    case kMACaptionAppearanceDisplayTypeAlwaysOn:
> +	   return AlwaysOn;
> +    }

I think it is a mistake to have the one method sometimes return the cached
value and sometimes call the system because the caller always knows if it to do
one thing or the other, so I would split this into two methods.

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:527
> +    if (cachedPreferredLanguages().has_value())
> +	   return cachedPreferredLanguages().value();
> +
> +    auto captionLanguages =
adoptCF(MACaptionAppearanceCopySelectedLanguages(kMACaptionAppearanceDomainUser
));
> +    CFIndex captionLanguagesCount = captionLanguages ?
CFArrayGetCount(captionLanguages.get()) : 0;
> +
> +    Vector<String> preferredLanguages;
> +    preferredLanguages.reserveInitialCapacity(captionLanguagesCount);
> +    for (CFIndex i = 0; i < captionLanguagesCount; i++)
> +	  
preferredLanguages.uncheckedAppend(static_cast<CFStringRef>(CFArrayGetValueAtIn
dex(captionLanguages.get(), i)));
> +
> +    return preferredLanguages;

Ditto.