[webkit-reviews] review granted: [Bug 226038] [Modern Media Controls] REGRESSION(r254389) media controls needs the full list of language preferences for ordering tracks : [Attachment 429616] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 226038: [Modern Media Controls] REGRESSION(r254389) media controls needs
the full list of language preferences for ordering tracks
https://bugs.webkit.org/show_bug.cgi?id=226038

Attachment 429616: Patch

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




--- Comment #21 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 429616
  --> https://bugs.webkit.org/attachment.cgi?id=429616
Patch

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

I can't review the media and subtitle specific code, but the language code
stuff looks good.

> Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig:35
> +// Needed by `Source/WTF/wtf/cocoa/LanguageCocoa.mm`.
> +WK_INTERNATIONAL_SUPPORT_LDFLAGS = -framework InternationalSupport;
> +
> +OTHER_LDFLAGS_BASE = $(OTHER_LDFLAGS_HIDE_SYMBOLS) -iframework
$(SDK_DIR)$(SYSTEM_LIBRARY_DIR)/PrivateFrameworks -force_load
"$(BUILT_PRODUCTS_DIR)/DerivedSources/JavaScriptCore/libWTF.a"
$(WK_INTERNATIONAL_SUPPORT_LDFLAGS);

Why is this necessary? The only change to LanguageCocoa.mm is a comment.

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:486
> +    if (!testingMode() && MediaAccessibilityLibrary()) {
> +	   auto captionLanguages =
adoptCF(MACaptionAppearanceCopySelectedLanguages(kMACaptionAppearanceDomainUser
));
> +	   CFIndex captionLanguagesCount = captionLanguages ?
CFArrayGetCount(captionLanguages.get()) : 0;
> +	   if (captionLanguagesCount) {
> +	       Vector<String> temp;
> +	       temp.reserveInitialCapacity(captionLanguagesCount +
preferredLanguages.size());
> +	       for (CFIndex i = 0; i < captionLanguagesCount; i++)
> +		  
temp.uncheckedAppend(static_cast<CFStringRef>(CFArrayGetValueAtIndex(captionLan
guages.get(), i)));
> +	       temp.appendVector(WTFMove(preferredLanguages));
> +	       preferredLanguages = WTFMove(temp);
>	   }
>      }

I think WebKit prefers early return style like the red code below this, rather
than the triangle style here.

>
LayoutTests/media/modern-media-controls/tracks-support/sorted-by-user-preferred
-languages.html:36
> +    shouldBecomeDifferent("shadowRoot.querySelector('button.tracks')",
"null", () => {

This is a big scary. Does this only work because we're inside a test?

>
WebKitLibraries/WebKitPrivateFrameworkStubs/Mac/101500/InternationalSupport.fra
mework/InternationalSupport.tbd:7
> +--- !tapi-tbd-v3
> +archs:	    [ x86_64 ]
> +install-name:   
'/System/Library/PrivateFrameworks/InternationalSupport.framework/International
Support'
> +platform: zippered
> +exports:
> +  - archs:		[ x86_64 ]
> +...

Are you sure you need these .tbd files? I don't think we're calling anything
additional from InternationalSupport in this patch.