Bug 232686 – Compiler should be able to check localized format strings for consistency

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Kilzer (:ddkilzer)]

Make macros in LocalizableStrings.h check format strings for consistency.

Currently none of the macros or functions check localized format strings on either Gtk or on Apple platforms.

[Radar WebKit Bug Importer]

<rdar://problem/84994345>

[David Kilzer (:ddkilzer)]

Created attachment 443248 [details]
Patch v1

[David Kilzer (:ddkilzer)]

I think I can get rid of the CFAutorelease() (fixing Windows build and eliminating autoreleased objects in this code path) by using CF_RETURNS_RETAINED on localizedCFString() and CF_RELEASES_ARGUMENT on formatLocalizedString().

I may have to stop using localizedCFString() with WEB_UI_CFSTRING(), though.

And WEB_UI_NSSTRING() may also need an -autorelease call.

[David Kilzer (:ddkilzer)]

Created attachment 443417 [details]
Patch v2

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #4)
> Created attachment 443417 [details]
> Patch v2

So much goodness here:
- Allows compiler to check format strings for consistency (types and counts of args vs. placeholders) on all platforms.
- Eliminates autoreleased NS/CFString objects, except when calling localizedNSString(), which is required based on how it's used.  These NSString objects will stop being autoreleased after switching to ARC.
- Eliminates runtime const char* -> CFStringRef conversions (except in WebKitLegacy, which can be done in a follow-up if desired).
- Eliminates CFStringRef -> WTF::String -> CFStringRef conversions for format strings.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #4)
> Created attachment 443417 [details]
> Patch v2

The Apple Windows port is crashing a lot on this patch:

    <https://ews-build.webkit.org/#/builders/10/builds/112721>

It turns out that Source/WebKitLegacy/win/WebPreferences.cpp had one use of WEB_UI_STRING() in WebKit!WebPreferences::initializeDefaultSettings():

    String defaultDefaultEncoding(WEB_UI_STRING("ISO-8859-1", "The default, default character encoding on Windows"));
    CFDictionaryAddValue(defaults.get(), CFSTR(WebKitDefaultTextEncodingNamePreferenceKey), defaultDefaultEncoding.createCFString().get());

And due to the way WEB_UI_STRING() worked before this patch, this code was basically just using WEB_UI_STRING() to convert the const char* into a WTF::String, and then into a CFStringRef.  :|

The fix is to remove WEB_UI_STRING() and just use CFSTR("ISO-8859-1") directly.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #6)
> (In reply to David Kilzer (:ddkilzer) from comment #4)
> > Created attachment 443417 [details]
> > Patch v2
> 
> The Apple Windows port is crashing a lot on this patch:
> 
>     <https://ews-build.webkit.org/#/builders/10/builds/112721>
> 
> It turns out that Source/WebKitLegacy/win/WebPreferences.cpp had one use of
> WEB_UI_STRING() in WebKit!WebPreferences::initializeDefaultSettings():
> 
>     String defaultDefaultEncoding(WEB_UI_STRING("ISO-8859-1", "The default,
> default character encoding on Windows"));
>     CFDictionaryAddValue(defaults.get(),
> CFSTR(WebKitDefaultTextEncodingNamePreferenceKey),
> defaultDefaultEncoding.createCFString().get());
> 
> And due to the way WEB_UI_STRING() worked before this patch, this code was
> basically just using WEB_UI_STRING() to convert the const char* into a
> WTF::String, and then into a CFStringRef.  :|
> 
> The fix is to remove WEB_UI_STRING() and just use CFSTR("ISO-8859-1")
> directly.

Actually, this could be changed in Localizable.strings!  So we just need to use WEB_UI_CFSTRING() here.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #7)
> (In reply to David Kilzer (:ddkilzer) from comment #6)
> > (In reply to David Kilzer (:ddkilzer) from comment #4)
> > > Created attachment 443417 [details]
> > > Patch v2
> > 
> > The Apple Windows port is crashing a lot on this patch:
> > 
> >     <https://ews-build.webkit.org/#/builders/10/builds/112721>
> > 
> > It turns out that Source/WebKitLegacy/win/WebPreferences.cpp had one use of
> > WEB_UI_STRING() in WebKit!WebPreferences::initializeDefaultSettings():
> > 
> >     String defaultDefaultEncoding(WEB_UI_STRING("ISO-8859-1", "The default,
> > default character encoding on Windows"));
> >     CFDictionaryAddValue(defaults.get(),
> > CFSTR(WebKitDefaultTextEncodingNamePreferenceKey),
> > defaultDefaultEncoding.createCFString().get());
> > 
> > And due to the way WEB_UI_STRING() worked before this patch, this code was
> > basically just using WEB_UI_STRING() to convert the const char* into a
> > WTF::String, and then into a CFStringRef.  :|
> > 
> > The fix is to remove WEB_UI_STRING() and just use CFSTR("ISO-8859-1")
> > directly.
> 
> Actually, this could be changed in Localizable.strings!  So we just need to
> use WEB_UI_CFSTRING() here.

Actually, Windows had its own copy of WebCore::localizedString(const char*) in Source/WebCore/platform/win/LocalizedStringsWin.cpp, and that's no longer being used, and the new code in WebCore::localizedString(CFStringRef) won't work on Windows.

So that's the real bug here.

[David Kilzer (:ddkilzer)]

Created attachment 443432 [details]
Patch v3

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #9)
> Created attachment 443432 [details]
> Patch v3

Try to fix Windows and WPE builds.

[David Kilzer (:ddkilzer)]

Created attachment 443434 [details]
Patch v4

[David Kilzer (:ddkilzer)]

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

Try to fix WPE build.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #12)
> (In reply to David Kilzer (:ddkilzer) from comment #11)
> > Created attachment 443434 [details]
> > Patch v4
> 
> Try to fix WPE build.

Windows test failure looks related to the patch:

-PASS f.contentDocument.title is "green-24x24.jpg 24×24 pixels"
-PASS f.contentDocument.head.getElementsByTagName('title')[0].textContent is "green-24x24.jpg 24×24 pixels"
+FAIL f.contentDocument.title should be green-24x24.jpg 24×24 pixels. Was localized string not found.
+FAIL f.contentDocument.head.getElementsByTagName('title')[0].textContent should be green-24x24.jpg 24×24 pixels. Was localized string not found.

<https://ews-build.webkit.org/#/builders/10/builds/112739>

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #13)
> (In reply to David Kilzer (:ddkilzer) from comment #12)
> > (In reply to David Kilzer (:ddkilzer) from comment #11)
> > > Created attachment 443434 [details]
> > > Patch v4
> > 
> > Try to fix WPE build.
> 
> Windows test failure looks related to the patch:
> 
> -PASS f.contentDocument.title is "green-24x24.jpg 24×24 pixels"
> -PASS f.contentDocument.head.getElementsByTagName('title')[0].textContent is
> "green-24x24.jpg 24×24 pixels"
> +FAIL f.contentDocument.title should be green-24x24.jpg 24×24 pixels. Was
> localized string not found.
> +FAIL f.contentDocument.head.getElementsByTagName('title')[0].textContent
> should be green-24x24.jpg 24×24 pixels. Was localized string not found.
> 
> <https://ews-build.webkit.org/#/builders/10/builds/112739>

And...I didn't faithfully merge WebCore::localizedString() from LocalizedStringsWin.cpp.

Patch v5 coming up.

[David Kilzer (:ddkilzer)]

Created attachment 443454 [details]
Patch v5

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #15)
> Created attachment 443454 [details]
> Patch v5

Try to fix Windows layout test failure from Patch v4.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #16)
> (In reply to David Kilzer (:ddkilzer) from comment #15)
> > Created attachment 443454 [details]
> > Patch v5
> 
> Try to fix Windows layout test failure from Patch v4.

So the same test is still failing on Windows:

    fast/images/imageDocument-title.html
    <https://ews-build.webkit.org/#/builders/10/builds/112770>

The last thing I can think of is that a CFStringRef of the key was built like this before the patch:

    const char* key = "%@ %@×%@ pixels";
    CFStringRef cfKey = CFStringCreateWithCStringNoCopy(NULL, key, kCFStringEncodingUTF8, kCFAllocatorNull);

But after this patch, the code would create the same CFStringRef this way:

    CFStringRef cfKey = CFSTR("%@ %@×%@ pixels");

Could those result in different, unequal CFStringRef objects, but only on Windows?

This apparently only affects Windows since the same test works fine on iOS and macOS.

[David Kilzer (:ddkilzer)]

Created attachment 443465 [details]
Patch v5 ALT

Same as Patch v5, but replaced multiplication sign ("×") with lower-case "x" in the localized string to see if that made a difference on Windows.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #18)
> Created attachment 443465 [details]
> Patch v5 ALT
> 
> Same as Patch v5, but replaced multiplication sign ("×") with lower-case "x"
> in the localized string to see if that made a difference on Windows.

This test should fail on all platforms with Patch v5 AALT (because the expectation is wrong):

    fast/images/imageDocument-title.html

[David Kilzer (:ddkilzer)]

Created attachment 443468 [details]
Patch v5 RELEASE ASSERT

Same as Patch v5, but changes debug assert to release assert when a localized string is not found.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #20)
> Created attachment 443468 [details]
> Patch v5 RELEASE ASSERT
> 
> Same as Patch v5, but changes debug assert to release assert when a
> localized string is not found.

This has to be some kind of text encoding issue with MSVC on Windows.  The layout test failures on this patch actually have this stderr output:

DumpRenderTree.exe[5844:142c] WARNING: CFSTR("%@ %@\37777777703\37777777627%@ pixels") has non-7 bit chars, interpreting using MacOS Roman encoding for now, but this will change. Please eliminate usages of non-7 bit chars (including escaped characters above \177 octal) in CFSTR().

Does the file need to be marked as using UTF-8 encoding for MSVC?

[David Kilzer (:ddkilzer)]

Created attachment 443486 [details]
Patch v6

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #22)
> Created attachment 443486 [details]
> Patch v6

This patch attempts to fix Apple's Windows port by introducing separate macros that don't use CFSTR().

This may be more complex than it needs to be--might be better to make Apple's non-Windows ports use const char* arguments like Windows if there isn't a huge performance hit to create CFStringRef objects at runtime.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #23)
> (In reply to David Kilzer (:ddkilzer) from comment #22)
> > Created attachment 443486 [details]
> > Patch v6
> 
> This patch attempts to fix Apple's Windows port by introducing separate
> macros that don't use CFSTR().
> 
> This may be more complex than it needs to be--might be better to make
> Apple's non-Windows ports use const char* arguments like Windows if there
> isn't a huge performance hit to create CFStringRef objects at runtime.

Yay!  This fixed the AppleWin issue.  Will clean up the patch and post a "final" one for review.

[David Kilzer (:ddkilzer)]

Created attachment 443494 [details]
Patch v7

[David Kilzer (:ddkilzer)]

Comment on attachment 443494 [details]
Patch v7

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

Need to rebase this patch already.

> Source/WebCore/ChangeLog:23
> +        when using WEB_UI_NSString()/localizedNSString() directly (the

Fix capitalization of WEB_UI_NSSTRING() macro.

> Source/WebCore/ChangeLog:110
> +          localizedFormatString().

Should be localizedString().

> Source/WebCore/Scripts/extract-localizable-strings.pl:248
> +                if (($token =~ /(WEB_)?UI_(CF)?STRING(_KEY)?(_INTERNAL)?$/) || ($token =~ /WEB_UI_FORMAT_(CF)?STRING(_KEY)?$/) || ($token =~ /WEB_UI_NSSTRING$/) || ($token =~ /WEB_UI_STRING_WITH_MNEMONIC$/)) {

Combine new regex with existing regex as above.

[David Kilzer (:ddkilzer)]

Created attachment 443511 [details]
Patch v8

[Alex Christensen]

Comment on attachment 443511 [details]
Patch v8

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

> Source/WebCore/platform/LocalizedStrings.cpp:58
> +    CFRelease(format);

Where is the corresponding CFRetain?

[Darin Adler]

Comment on attachment 443511 [details]
Patch v8

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

>> Source/WebCore/platform/LocalizedStrings.cpp:58
>> +    CFRelease(format);
> 
> Where is the corresponding CFRetain?

In the localizedString function. That’s why this does CF_RELEASES_ARGUMENT.

> Source/WebCore/platform/LocalizedStrings.h:408
> +    CFStringRef localizedFormatString(const char* key) CF_FORMAT_ARGUMENT(1) CF_RETURNS_RETAINED;
> +    WEBCORE_EXPORT CFStringRef localizedString(CFStringRef key) CF_RETURNS_RETAINED;

Why not add "copy" to the names of these functions?

Why not have these return RetainPtr<CFStringRef>?

> Source/WebCore/platform/LocalizedStrings.h:414
> +    String formatLocalizedString(CFStringRef CF_RELEASES_ARGUMENT format, ...) CF_FORMAT_FUNCTION(1, 2);

This function is kind of tricky to use correctly because you have to pass a retained format string.

I guess we can’t make it take a RetainPtr<CFStringRef>&&, but maybe it could just take a normal CFStringRef and not release its argument, and maybe write .get() at the call sites?

[David Kilzer (:ddkilzer)]

Comment on attachment 443511 [details]
Patch v8

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

>>> Source/WebCore/platform/LocalizedStrings.cpp:58
>>> +    CFRelease(format);
>> 
>> Where is the corresponding CFRetain?
> 
> In the localizedString function. That’s why this does CF_RELEASES_ARGUMENT.

Correct.

>> Source/WebCore/platform/LocalizedStrings.h:408
>> +    WEBCORE_EXPORT CFStringRef localizedString(CFStringRef key) CF_RETURNS_RETAINED;
> 
> Why not add "copy" to the names of these functions?
> 
> Why not have these return RetainPtr<CFStringRef>?

> Why not add "copy" to the names of these functions?

So we don't have to use CF_RETURNS_RETAINED?  I didn't think of "looking up a localized string" as a "copy" operation, but I'm not opposed to changing the name to include "copy".

> Why not have these return RetainPtr<CFStringRef>?

I covered this in the ChangeLog, but (a) I know it was a lot to read, and (b) I'm open to better ways to document it:

        * platform/LocalizedStrings.cpp:
        (WebCore::formatLocalizedString):
        [...]
        - Note that `format` is marked as CF_RELEASES_ARGUMENT on Apple
          platforms because localizedFormatString() can't use a return
          type of RetainPtr<> and also use CF_FORMAT_ARGUMENT(), and we
          want to avoid autoreleasing the CFStringRef returned.
        [...]

Basically, it's to avoid auto-releasing the CFStringRef and allowing the compiler to check the format string.

The compiler complains that `RetainPtr<CFStringRef>` isn't a "string type" when you change the function signature.  :(

>> Source/WebCore/platform/LocalizedStrings.h:414
>> +    String formatLocalizedString(CFStringRef CF_RELEASES_ARGUMENT format, ...) CF_FORMAT_FUNCTION(1, 2);
> 
> This function is kind of tricky to use correctly because you have to pass a retained format string.
> 
> I guess we can’t make it take a RetainPtr<CFStringRef>&&, but maybe it could just take a normal CFStringRef and not release its argument, and maybe write .get() at the call sites?

Nope.  You can't have the compiler check the format string and use RetainPtr<>, or you must autorelease the CFStringRef returned.

Oh, and `CFAutorelease()` apparently isn't available on AppleWin, so that port would have to go back to having a different code path.

Also, if you weren't reading my patch updates, CFSTR() on Windows uses MacRoman encoding (not UTF-8), so any C-string with an ASCII character over 127(?) would result in an incorrectly-encoded CFStringRef that would not match the Localized.strings entry.  (If Windows didn't have that bug, I could construct a CFStringRef at compile time using CFSTR() on the arguments, but instead we have localizedFormatString() and localizedString().)

As long as folks use the macros as intended, it all just works.  (If they don't use the macros, that breaks Tools/Scripts/update-webkit-localizable-strings, too.)

[Darin Adler]

(In reply to David Kilzer (:ddkilzer) from comment #30)
> > Why not add "copy" to the names of these functions?
> 
> So we don't have to use CF_RETURNS_RETAINED?  I didn't think of "looking up
> a localized string" as a "copy" operation, but I'm not opposed to changing
> the name to include "copy".

Yes, I think the name copyLocalizedString would be pretty natural.

> > Why not have these return RetainPtr<CFStringRef>?

> Basically, it's to avoid auto-releasing the CFStringRef and allowing the
> compiler to check the format string.

That sounds like you are talking about the argument to formatLocalizedString, not the return value from localizedString. I was thinking the macros could have the "get()" inside them.

> Also, if you weren't reading my patch updates, CFSTR() on Windows uses
> MacRoman encoding (not UTF-8), so any C-string with an ASCII character over
> 127(?) would result in an incorrectly-encoded CFStringRef that would not
> match the Localized.strings entry.  (If Windows didn't have that bug, I
> could construct a CFStringRef at compile time using CFSTR() on the
> arguments, but instead we have localizedFormatString() and
> localizedString().)

That’s really frustrating. But I think the days of the Windows/CF port are numbered, so this is not forever.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #31)
> (In reply to David Kilzer (:ddkilzer) from comment #30)
> > > Why not add "copy" to the names of these functions?
> > 
> > So we don't have to use CF_RETURNS_RETAINED?  I didn't think of "looking up
> > a localized string" as a "copy" operation, but I'm not opposed to changing
> > the name to include "copy".
> 
> Yes, I think the name copyLocalizedString would be pretty natural.

Okay, will do.

> > > Why not have these return RetainPtr<CFStringRef>?
> 
> > Basically, it's to avoid auto-releasing the CFStringRef and allowing the
> > compiler to check the format string.
> 
> That sounds like you are talking about the argument to
> formatLocalizedString, not the return value from localizedString. I was
> thinking the macros could have the "get()" inside them.

I'm not sure what you're saying here.  I can probably figure this out tomorrow, but a quick pseudo-code example would really help here.  (I can't visualize how having a .get() inside a function would not result in an over-release--or really where the .get() would even go.)

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #32)
> (In reply to Darin Adler from comment #31)
> > (In reply to David Kilzer (:ddkilzer) from comment #30)
> > > > Why not have these return RetainPtr<CFStringRef>?
> > 
> > > Basically, it's to avoid auto-releasing the CFStringRef and allowing the
> > > compiler to check the format string.
> > 
> > That sounds like you are talking about the argument to
> > formatLocalizedString, not the return value from localizedString. I was
> > thinking the macros could have the "get()" inside them.
> 
> I'm not sure what you're saying here.  I can probably figure this out
> tomorrow, but a quick pseudo-code example would really help here.  (I can't
> visualize how having a .get() inside a function would not result in an
> over-release--or really where the .get() would even go.)

Was just tired last night.  I think you're suggesting this:

-#define WEB_UI_FORMAT_CFSTRING(string, description) WebCore::localizedFormatString(string)
+#define WEB_UI_FORMAT_CFSTRING(string, description) adoptCF(WebCore::localizedFormatString(string)).get()

But that results in a compiler error because the compiler can't reason about the format string to check its placeholder count/types:

In file included from WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource250.cpp:7:
./platform/LocalizedStrings.cpp:289:34: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
    return formatLocalizedString(WEB_UI_FORMAT_CFSTRING("Look Up “%@”", "Look Up context menu item with selected word"), selectedCFString.get());
                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource250.cpp:7:
In file included from ./platform/LocalizedStrings.cpp:28:
./platform/LocalizedStrings.h:405:53: note: expanded from macro 'WEB_UI_FORMAT_CFSTRING'
#define WEB_UI_FORMAT_CFSTRING(string, description) adoptCF(WebCore::localizedFormatString(string)).get()
                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[Darin Adler]

Comment on attachment 443511 [details]
Patch v8

Seems fine as long as we get an error if I pass a CFStringRef that does not come from WEB_UI_FORMAT_CFSTRING to formatLocalizedString.

I’m also not sure why we name the macro WEB_UI_FORMAT_CFSTRING instead of just WEB_USE_FORMAT_STRING. It’s not like any one port supports both.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #34)
> Comment on attachment 443511 [details]
> Patch v8
> 
> Seems fine as long as we get an error if I pass a CFStringRef that does not
> come from WEB_UI_FORMAT_CFSTRING to formatLocalizedString.

I think I can invert the `formatLocalizedString(WEB_UI_FORMAT_CFSTRING(...))` pattern to just `WEB_UI_FORMAT_CFSTRING(...)` and make formatLocalizedString() an implementation detail of the macro.

Not sure how I would make this an error other than adding a check-webkit-style check for it.

> I’m also not sure why we name the macro WEB_UI_FORMAT_CFSTRING instead of
> just WEB_USE_FORMAT_STRING. It’s not like any one port supports both.

I created WEB_UI_FORMAT_CFSTRING() with WEB_UI_FORMAT_STRING() due to this comment that I removed from Source/WebCore/platform/LocalizedStrings.h about WEB_UI_CFSTRING():

-// This is exactly as WEB_UI_STRING, but renamed to ensure the string is not scanned by non-CF ports.

But as far as I can tell, it's actually Apple's ports that have extra localized strings because there is no WEB_UI_FORMAT_GLIB() to use in certain places.  I didn't know the history here, so I didn't want to make too many changes.  (This patch has already grown much bigger than I imagined it would originally.)

Whatever port was parsing the WEB_UI_STRING() macros at the time of the comment either doesn't have the tool in WebKit's source tree, or the port was removed years ago.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #35)
> -// This is exactly as WEB_UI_STRING, but renamed to ensure the string is
> not scanned by non-CF ports.

Added for Bug 169672 in r214244 by Carlos Garcia Campos (CCed here).

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #35)
> (In reply to Darin Adler from comment #34)
> > Comment on attachment 443511 [details]
> > Patch v8
> > 
> > Seems fine as long as we get an error if I pass a CFStringRef that does not
> > come from WEB_UI_FORMAT_CFSTRING to formatLocalizedString.
> 
> I think I can invert the
> `formatLocalizedString(WEB_UI_FORMAT_CFSTRING(...))` pattern to just
> `WEB_UI_FORMAT_CFSTRING(...)` and make formatLocalizedString() an
> implementation detail of the macro.

I remember why we can't do this now--CF_FORMAT_FUNCTION() won't work with a const char* parameter type:

Source/WebCore/platform/LocalizedStrings.h:413:59: error: format argument not a CFString
    String formatLocalizedString(const char* format, ...) CF_FORMAT_FUNCTION(1, 2);
                                 ~~~~~~~~~~~~~~~~~~       ^                  ~

We can do this once we can use CFSTR() in the macro because we're blocked by Windows defaulting to Mac Roman encoding for CFSTR() at compile time (unless I can find a way to work around this Windows behavior by getting the raw bytes and re-encoding them.)

> Not sure how I would make this an error other than adding a
> check-webkit-style check for it.

Will have to add a check-webkit-style check (unless I can work around the Windows behavior).

[Carlos Garcia Campos]

(In reply to David Kilzer (:ddkilzer) from comment #35)
> (In reply to Darin Adler from comment #34)
> > Comment on attachment 443511 [details]
> > Patch v8
> > 
> > Seems fine as long as we get an error if I pass a CFStringRef that does not
> > come from WEB_UI_FORMAT_CFSTRING to formatLocalizedString.
> 
> I think I can invert the
> `formatLocalizedString(WEB_UI_FORMAT_CFSTRING(...))` pattern to just
> `WEB_UI_FORMAT_CFSTRING(...)` and make formatLocalizedString() an
> implementation detail of the macro.
> 
> Not sure how I would make this an error other than adding a
> check-webkit-style check for it.
> 
> > I’m also not sure why we name the macro WEB_UI_FORMAT_CFSTRING instead of
> > just WEB_USE_FORMAT_STRING. It’s not like any one port supports both.
> 
> I created WEB_UI_FORMAT_CFSTRING() with WEB_UI_FORMAT_STRING() due to this
> comment that I removed from Source/WebCore/platform/LocalizedStrings.h about
> WEB_UI_CFSTRING():
> 
> -// This is exactly as WEB_UI_STRING, but renamed to ensure the string is
> not scanned by non-CF ports.
> 
> But as far as I can tell, it's actually Apple's ports that have extra
> localized strings because there is no WEB_UI_FORMAT_GLIB() to use in certain
> places.  I didn't know the history here, so I didn't want to make too many
> changes.  (This patch has already grown much bigger than I imagined it would
> originally.)
> 
> Whatever port was parsing the WEB_UI_STRING() macros at the time of the
> comment either doesn't have the tool in WebKit's source tree, or the port
> was removed years ago.

It's here https://github.com/WebKit/WebKit/blob/main/Source/WebCore/platform/gtk/po/CMakeLists.txt#L28

[David Kilzer (:ddkilzer)]

Created attachment 445106 [details]
Patch v9 for landing

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #39)
> Created attachment 445106 [details]
> Patch v9 for landing

- Renames localizedString(CFStringRef) to copyLocalizedString() and changed return type to RetainPtr<CFStringRef>.  (A new localizedString(CFStringRef) helper function is added that calls copyLocalizedString().)
- Removes CF_RELEASES_ARGUMENT for formatLocalizedString(), and inverts the WEB_UI_FORMAT_[CF]STRING() macro so that formatLocalizedString() is an implementation detail.
- Attempts to fix GLIB localized string processing for new WEB_UI_FORMAT_STRING() macro.
- Adds back CFSTR() optimization for non-Apple-Windows USE(CF) ports so that a CFStringRef doesn't need to be created at runtime from a C-string.  The Apple Windows port still creates a no-copy CFStringRef at runtime from the C-string since its default encoding for CFSTR() is Mac Roman, which causes incorrect encoding. 


The only change this patch doesn't make (that I noticed while working on this) is to introduce a WEB_UI_GLIB_STRING() (and maybe WEB_UI_FORMAT_GLIB_STRING()) macro to avoid the need to localize strings for Apple ports that are never used.

[David Kilzer (:ddkilzer)]

Created attachment 445110 [details]
Patch v10 for landing

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #41)
> Created attachment 445110 [details]
> Patch v10 for landing

- Fix WEB_UI_FORMAT_STRING() macro definitions for gtk and wpe ports.

[EWS]

Committed r286156 (244540@main): <https://commits.webkit.org/244540@main>

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