Bug 228066 - GetIdentifierStringForPreferredVoiceInListWithLocale() is deprecated in Monterey
Summary: GetIdentifierStringForPreferredVoiceInListWithLocale() is deprecated in Monterey
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-18 14:56 PDT by David Kilzer (:ddkilzer)
Modified: 2021-07-29 12:54 PDT (History)
12 users (show)

See Also:


Attachments
Patch v1 (7.78 KB, patch)
2021-07-18 15:09 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (8.00 KB, patch)
2021-07-18 15:15 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v3 (9.21 KB, patch)
2021-07-18 15:45 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v4 (9.22 KB, patch)
2021-07-18 16:35 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v5 (6.65 KB, patch)
2021-07-18 16:36 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v6 (7.05 KB, patch)
2021-07-20 20:59 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch (7.16 KB, patch)
2021-07-28 14:04 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch for landing (7.17 KB, patch)
2021-07-29 11:57 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2021-07-18 14:56:39 PDT
GetIdentifierStringForPreferredVoiceInListWithLocale() is deprecated in Monterey.

Replacement is CopyIdentifierStringForPreferredVoiceInListWithLocale().

<rdar://problem/80577312>
Comment 1 David Kilzer (:ddkilzer) 2021-07-18 15:09:07 PDT
Created attachment 433759 [details]
Patch v1
Comment 2 David Kilzer (:ddkilzer) 2021-07-18 15:15:56 PDT
Created attachment 433760 [details]
Patch v2
Comment 3 Alexey Proskuryakov 2021-07-18 15:30:18 PDT
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());

:-(
Comment 4 David Kilzer (:ddkilzer) 2021-07-18 15:45:09 PDT
Created attachment 433761 [details]
Patch v3
Comment 5 David Kilzer (:ddkilzer) 2021-07-18 16:32:34 PDT
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.
Comment 6 David Kilzer (:ddkilzer) 2021-07-18 16:35:22 PDT
Created attachment 433763 [details]
Patch v4
Comment 7 David Kilzer (:ddkilzer) 2021-07-18 16:36:54 PDT
Created attachment 433764 [details]
Patch v5
Comment 8 David Kilzer (:ddkilzer) 2021-07-18 16:37:29 PDT
(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.
Comment 9 Alexey Proskuryakov 2021-07-18 19:25:29 PDT
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.
Comment 10 David Kilzer (:ddkilzer) 2021-07-18 21:03:12 PDT
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?
Comment 11 Alexey Proskuryakov 2021-07-19 09:14:12 PDT
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.
Comment 12 David Kilzer (:ddkilzer) 2021-07-20 20:58:08 PDT
(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.
Comment 13 David Kilzer (:ddkilzer) 2021-07-20 20:59:17 PDT
Created attachment 433918 [details]
Patch v6
Comment 14 EWS 2021-07-21 05:57:29 PDT
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].
Comment 15 Myles C. Maxfield 2021-07-21 11:23:31 PDT
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
Comment 16 Chris Dumez 2021-07-21 12:27:01 PDT
Reverted r280129 for reason:

Broke the internal Monterey build bots

Committed r280153 (239850@main): <https://commits.webkit.org/239850@main>
Comment 17 David Kilzer (:ddkilzer) 2021-07-21 12:53:44 PDT
(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.
Comment 18 Kate Cheney 2021-07-28 14:04:12 PDT
Created attachment 434462 [details]
Patch
Comment 19 David Kilzer (:ddkilzer) 2021-07-29 04:17:41 PDT
Comment on attachment 434462 [details]
Patch

r=me to re-land my patch.
Comment 20 David Kilzer (:ddkilzer) 2021-07-29 04:21:16 PDT
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.
Comment 21 Kate Cheney 2021-07-29 11:57:45 PDT
Created attachment 434552 [details]
Patch for landing
Comment 22 Kate Cheney 2021-07-29 11:58:16 PDT
(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!
Comment 23 EWS 2021-07-29 12:54:36 PDT
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].