[webkit-reviews] review granted: [Bug 192746] Convert additional String::format clients to alternative approaches : [Attachment 357422] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 192746: Convert additional String::format clients to alternative approaches
https://bugs.webkit.org/show_bug.cgi?id=192746

Attachment 357422: Patch

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




--- Comment #4 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 357422
  --> https://bugs.webkit.org/attachment.cgi?id=357422
Patch

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

Looks good. Some ranty nitpicks below.

> Source/WTF/wtf/JSONValues.cpp:449
> +inline bool appendDoubleQuotedStringEscapedCharacter(StringBuilder& builder,
UChar character)

This function name doesn't quite parse for me. Maybe
appendCharacterEscapingForDoubleQuotedStringIfNeeded?

> Source/WTF/wtf/JSONValues.cpp:489
> +	   // FIXME: This is likely incorrect for surrogate pairs.

Seems worth elaborating on what the action for this FIXME is. "Likely" is too
weak of a statement, as it's obviously never correct to serialize a code unit
into two \u escape sequences.

But I'm not sure if it's observable, maybe callers guarantee that strings here
don't contain surrogate pairs? It just feels unlikely that a bug of this
magnitude could have survived so long in the era of Emoji if it were a real
bug. In this case, I'm not sure what the action would be, maybe to rename the
function so that it wouldn't be misused in the future.

FWIW, I played with JSON.stringify, and couldn't get two \u sequences out of
it. But I didn't check if it takes this code path.

> Source/WTF/wtf/JSONValues.cpp:494
> +	   builder.appendLiteral("\\u");
> +	   builder.append(upperNibbleToASCIIHexDigit(codeUnit >> 8));
> +	   builder.append(lowerNibbleToASCIIHexDigit(codeUnit >> 8));
> +	   builder.append(upperNibbleToASCIIHexDigit(codeUnit));
> +	   builder.append(lowerNibbleToASCIIHexDigit(codeUnit));

I never looked into StringBuilder performance before, but a brief glance over
it doesn't fill me with confidence that appending characters one by one is
performant. This class over-allocates the buffer, but it's still a non-inlined
function call for every character.

Probably no worse than String::format though.

> Source/WebCore/Modules/indexeddb/IDBKeyData.cpp:367
> +	   return makeString("<date> - ",
FormattedNumber::fixedWidth(WTF::get<double>(m_value), 6));

Not new in this patch, but it's sad that we don't have a consistent story for
the WTF namespace. Most of WTF code is designed to be used without an explicit
namespace, and symbols are exported into global namespace right away. But that
kind of thing wouldn't work with "get".

> Source/WebCore/PAL/pal/FileSizeFormatter.cpp:42
> +	   return makeString(FormattedNumber::fixedWidth(size / 1000., 1), "
KB"); // This should be lowercase k.

This lacks "FIXME". May be worth elaborating on what exactly the goal here is.
Obviously, "kilo" prefix is lower case, but using upper case is quite
traditional. Is there action for this FIXME to just make this change locally,
or to switch the whole code base consistently?

> Source/WebCore/html/track/VTTCue.cpp:231
> -	       String::format("translate(-%.2f%%, -%.2f%%)", position.first,
position.second));
> +	       makeString("translate(",
FormattedNumber::fixedWidth(-position.first, 2), "%, ",
FormattedNumber::fixedWidth(-position.second, 2), "%)"));

I think that negating is problem-free for floating point numbers that we can
encounter here, but is it an improvement to do an arithmetic operation here
before converting the number to string?

> Source/WebCore/page/cocoa/ResourceUsageOverlayCocoa.mm:434
> -	   return String::format("%.1f kB", static_cast<double>(number) /
1024);
> +	   return makeString(FormattedNumber::fixedWidth(number / 1024, 1), "
kB");

FIXME: Should be "KiB" (and same for others)? Hate hate hate.

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:106
> +    return makeString("{{",

Hmm, why does StringBuilder need a separate appendLiteral function, but
makeString gets an unadorned literal? Do we need ASCIILiteral here by any
chance?

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:500
> +	       return makeString("U+",

Is there any concern about non-BMP characters here?

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:475
> +    // FIXME: I don't think that formatting with %s and passing an NSObject
from a dictionary will work.

Would you be willing to file a bug for this? I believe that this is new code,
and fixing it should probably be tracked with more than a FIXME.

> Tools/WebKitTestRunner/TestController.cpp:2272
> +    if (auto port = WKSecurityOriginGetPort(origin)) {

This is one of the cases where I think that the type matters, and "auto" takes
away from the reader. I want to know whether it's an unsigned short or
something else when reading this.

> Tools/WebKitTestRunner/TestController.cpp:2274
> +	   // FIXME: Use static_cast<int> to work around makeString's
limitation when formatting numbers that are the same type as UChar.
> +	   return makeString(protocol, "://", host, ':',
static_cast<int>(port));

What is the FIXME action? It says "use", but the code already does that.

I'm guessing that it's about removing static_cast once there is a better way,
but this seems like a strange place to motivate creating that better way.