Bug 228066 – GetIdentifierStringForPreferredVoiceInListWithLocale() is deprecated in Monterey

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

GetIdentifierStringForPreferredVoiceInListWithLocale() is deprecated in Monterey.

Replacement is CopyIdentifierStringForPreferredVoiceInListWithLocale().

<rdar://problem/80577312>

[David Kilzer (:ddkilzer)]

Created attachment 433759 [details]
Patch v1

[David Kilzer (:ddkilzer)]

Created attachment 433760 [details]
Patch v2

[Alexey Proskuryakov]

Comment on attachment 433760 [details]
Patch v2

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

> Source/WebCore/platform/mac/PlatformSpeechSynthesizerMac.mm:231
> +    adoptNS(CFBridgingRelease(CopyIdentifierStringForPreferredVoiceInListWithLocale(speechSynthesisGetVoiceIdentifiers().get(), (__bridge CFLocaleRef)userLocale)));

return is missing.

We aren't using CFBridgingRelease anywhere in WebCore, not sure how much sense it makes to use it in just one function.

> Source/WebCore/platform/mac/PlatformSpeechSynthesizerMac.mm:248
> +        auto defaultVoiceURI = speechSynthesisGetDefaultVoiceIdentifierForLocale(adoptNS([[NSLocale alloc] initWithLocaleIdentifier:language]).get());

:-(

[David Kilzer (:ddkilzer)]

Created attachment 433761 [details]
Patch v3

[David Kilzer (:ddkilzer)]

Comment on attachment 433760 [details]
Patch v2

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

>> Source/WebCore/platform/mac/PlatformSpeechSynthesizerMac.mm:231
>> +    adoptNS(CFBridgingRelease(CopyIdentifierStringForPreferredVoiceInListWithLocale(speechSynthesisGetVoiceIdentifiers().get(), (__bridge CFLocaleRef)userLocale)));
> 
> return is missing.
> 
> We aren't using CFBridgingRelease anywhere in WebCore, not sure how much sense it makes to use it in just one function.

Thanks!  Will fix return statement in v4.  Will also change to use __bridge_transfer cast instead.

>> Source/WebCore/platform/mac/PlatformSpeechSynthesizerMac.mm:248
>> +        auto defaultVoiceURI = speechSynthesisGetDefaultVoiceIdentifierForLocale(adoptNS([[NSLocale alloc] initWithLocaleIdentifier:language]).get());
> 
> :-(

Not sure why the frowny face here.  Not a fan of `auto`?  In this case, the function returning the value is in the same source file, so it's not as much of a mystery.

[David Kilzer (:ddkilzer)]

Created attachment 433763 [details]
Patch v4

[David Kilzer (:ddkilzer)]

Created attachment 433764 [details]
Patch v5

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #6)
> Created attachment 433763 [details]
> Patch v4

Part of another patch was included here because I didn't use "-g HEAD" with prepare-ChangeLog.  That was weird.

[Alexey Proskuryakov]

Comment on attachment 433764 [details]
Patch v5

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

> Source/WebCore/platform/mac/PlatformSpeechSynthesizerMac.mm:231
> +    return adoptNS((__bridge_transfer NString *)CopyIdentifierStringForPreferredVoiceInListWithLocale(speechSynthesisGetVoiceIdentifiers().get(), (__bridge CFLocaleRef)userLocale));

We aren't using __bridge_transfer anywhere in WebCore/WebKit either.

> Source/WebCore/platform/mac/PlatformSpeechSynthesizerMac.mm:248
> -        NSString *defaultVoiceURI = speechSynthesisGetDefaultVoiceIdentifierForLocale(adoptNS([[NSLocale alloc] initWithLocaleIdentifier:language]).get());
> +        auto defaultVoiceURI = speechSynthesisGetDefaultVoiceIdentifierForLocale(adoptNS([[NSLocale alloc] initWithLocaleIdentifier:language]).get());

Not as much of a mystery as when it's in another file, but strictly worse than before, in my opinion.

[David Kilzer (:ddkilzer)]

Comment on attachment 433764 [details]
Patch v5

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

>> Source/WebCore/platform/mac/PlatformSpeechSynthesizerMac.mm:231
>> +    return adoptNS((__bridge_transfer NString *)CopyIdentifierStringForPreferredVoiceInListWithLocale(speechSynthesisGetVoiceIdentifiers().get(), (__bridge CFLocaleRef)userLocale));
> 
> We aren't using __bridge_transfer anywhere in WebCore/WebKit either.

Previously I changed CFBridgingTranfer() to a (__bridge_transfer) cast, but you made the same comment both times without explaining what your specific concern is.

Can you state your specific concern, and perhaps propose a solution?

BTW, this cast operator is used elsewhere in the project:

$ grep -l -r __bridge_transfer Source/*
Source/ThirdParty/libwebrtc/Source/webrtc/test/mac_capturer.mm
Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitDecoder.mm
Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitEncoder.mm
Source/WTF/wtf/BlockPtr.h
Source/WTF/wtf/cocoa/URLCocoa.mm
Source/WTF/wtf/text/cocoa/StringCocoa.mm

>> Source/WebCore/platform/mac/PlatformSpeechSynthesizerMac.mm:248
>> +        auto defaultVoiceURI = speechSynthesisGetDefaultVoiceIdentifierForLocale(adoptNS([[NSLocale alloc] initWithLocaleIdentifier:language]).get());
> 
> Not as much of a mystery as when it's in another file, but strictly worse than before, in my opinion.

Changing the static method to return an NSString * type would require autoreleasing the +1 retained CFStringRef on Monterey, which puts the object in an autoreleasePool, which should be avoided if possible for performance reasaons.

Perhaps the static method should return a RetainPtr<CFStringRef>, and then we can cast it via (_bridge NSString *) where it's used below?

Would that address both of your concerns?

[Alexey Proskuryakov]

As you used a language facility for the first time in WebCore/WebKit without any explanation, I think that there needs to be an explanation of why you did it.

If it's in preparation for eventual ARC adoption, I think that there are some unanswered questions:

- Will we be doing such adoption ever?
- What is the value of starting with one function, that just seems to add complexity for no benefit.
- What is the value of doing it in a line that has adoptNS anyway, and thus would need to change again.

None of that is a big deal, but it feels wrong to me to overload a build fix with novel and unrelated changes.

As for auto, the culture in this project is that people you like it just ignore opinions of others who think that it makes code worse in most cases, so I don't think that it's worth discussing further.

[David Kilzer (:ddkilzer)]

(In reply to Alexey Proskuryakov from comment #11)
> As you used a language facility for the first time in WebCore/WebKit without
> any explanation, I think that there needs to be an explanation of why you
> did it.
> 
> If it's in preparation for eventual ARC adoption, I think that there are
> some unanswered questions:
> 
> - Will we be doing such adoption ever?
> - What is the value of starting with one function, that just seems to add
> complexity for no benefit.
> - What is the value of doing it in a line that has adoptNS anyway, and thus
> would need to change again.
> 
> None of that is a big deal, but it feels wrong to me to overload a build fix
> with novel and unrelated changes.

The patch was written (a) to avoid putting any objects in an autoreleasePool, (b) to make speechSynthesisGetDefaultVoiceIdentifierForLocale() return RetainPtr<NSString> to match other variables in the calling function (which makes the `auto` type easier to infer), and (c) put all the casts in the speechSynthesisGetDefaultVoiceIdentifierForLocale() helper method.

But none of this is a big deal since I can use a return value of RetainPtr<CFStringRef> instead and resolve these objections to reviewing the patch.

> As for auto, the culture in this project is that people you like it just
> ignore opinions of others who think that it makes code worse in most cases,
> so I don't think that it's worth discussing further.

Okay.

[David Kilzer (:ddkilzer)]

Created attachment 433918 [details]
Patch v6

[EWS]

Committed r280129 (239839@main): <https://commits.webkit.org/239839@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433918 [details].

[Myles C. Maxfield]

I believe this broke the internal build:

Ld WebCore
Undefined symbols for architecture x86_64:
  "_CopyIdentifierStringForPreferredVoiceInListWithLocale", referenced from:
      __ZN7WebCoreL49speechSynthesisGetDefaultVoiceIdentifierForLocaleEP8NSLocale in UnifiedSource36-mm.o
ld: symbol(s) not found for architecture x86_64

[Chris Dumez]

Reverted r280129 for reason:

Broke the internal Monterey build bots

Committed r280153 (239850@main): <https://commits.webkit.org/239850@main>

[David Kilzer (:ddkilzer)]

(In reply to Chris Dumez from comment #16)
> Reverted r280129 for reason:
> 
> Broke the internal Monterey build bots
> 
> Committed r280153 (239850@main): <https://commits.webkit.org/239850@main>

Thanks for rolling out.  I can’t work on this any longer, so assigning back to the default assignee.

[Kate Cheney]

Created attachment 434462 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 434462 [details]
Patch

r=me to re-land my patch.

[David Kilzer (:ddkilzer)]

Comment on attachment 434462 [details]
Patch

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

> Source/WebCore/platform/mac/PlatformSpeechSynthesizerMac.mm:230
> +#if HAVE(SPEECHSYNTHESIS_MONTEREY_SPI)

We might want to add “&& HAVE(INTERNAL_SDK)”—or whatever the correct macro is—here so Open Source contributors can still build WebKit until this SPI is available in a Monterey Seed build.

[Kate Cheney]

Created attachment 434552 [details]
Patch for landing

[Kate Cheney]

(In reply to David Kilzer (:ddkilzer) from comment #20)
> Comment on attachment 434462 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434462&action=review
> 
> > Source/WebCore/platform/mac/PlatformSpeechSynthesizerMac.mm:230
> > +#if HAVE(SPEECHSYNTHESIS_MONTEREY_SPI)
> 
> We might want to add “&& HAVE(INTERNAL_SDK)”—or whatever the correct macro
> is—here so Open Source contributors can still build WebKit until this SPI is
> available in a Monterey Seed build.

Added in patch for landing. Thanks!

[EWS]

Committed r280441 (240079@main): <https://commits.webkit.org/240079@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434552 [details].