Bug 194893 – Finish removing String::format

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Darin Adler]

Finish removing String::format

[Darin Adler]

Created attachment 362588 [details]
Patch

[Darin Adler]

Created attachment 362593 [details]
Patch

[EWS Watchlist]

Comment on attachment 362593 [details]
Patch

Attachment 362593 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11228925

New failing tests:
fast/viewport/ios/device-width-viewport-after-changing-view-scale.html

[EWS Watchlist]

Created attachment 362597 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Darin Adler]

Created attachment 362610 [details]
Patch

[Darin Adler]

Passing all the tests, just need a reviewer.

[Daniel Bates]

Comment on attachment 362610 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        (WebCore::cpuUsageString): Use makeString, FormatttedNumber, and pad.

FormatttedNumber => FormattedNumber (remove one of the t's)

> Source/WTF/wtf/Assertions.cpp:109
> +        return String();

Ok as-is. return { }?

> Source/WTF/wtf/Assertions.cpp:113
> +    unsigned len = result;

OK as-is. length?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1327
> +    logString.append(makeString(pad(' ', 12 + depth * 2, hex(reinterpret_cast<uintptr_t>(&layer))), " id ", backing->graphicsLayer()->primaryLayerID(), " (", FormattedNumber::fixedWidth(absoluteBounds.x().toFloat(), 3), ',', FormattedNumber::fixedWidth(absoluteBounds.y().toFloat(), 3), '-', FormattedNumber::fixedWidth(absoluteBounds.maxX().toFloat(), 3), ',', FormattedNumber::fixedWidth(absoluteBounds.maxY().toFloat(), 3), ") ", FormattedNumber::fixedWidth(backing->backingStoreMemoryEstimate() / 1024, 2), "KB"));

I'm starting to remember why I like String::format() more :| Hoping this will grow on me.

> Source/WebCore/rendering/RenderTheme.cpp:579
> +        return makeString((time < 0 ? "-" : ""), hours, ':', pad('0', 2, minutes), ':', pad('0', 2, seconds));
> +    return makeString((time < 0 ? "-" : ""), pad('0', 2, minutes), ':', pad('0', 2, seconds));

:|

[Darin Adler]

Comment on attachment 362610 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:1327
>> +    logString.append(makeString(pad(' ', 12 + depth * 2, hex(reinterpret_cast<uintptr_t>(&layer))), " id ", backing->graphicsLayer()->primaryLayerID(), " (", FormattedNumber::fixedWidth(absoluteBounds.x().toFloat(), 3), ',', FormattedNumber::fixedWidth(absoluteBounds.y().toFloat(), 3), '-', FormattedNumber::fixedWidth(absoluteBounds.maxX().toFloat(), 3), ',', FormattedNumber::fixedWidth(absoluteBounds.maxY().toFloat(), 3), ") ", FormattedNumber::fixedWidth(backing->backingStoreMemoryEstimate() / 1024, 2), "KB"));
> 
> I'm starting to remember why I like String::format() more :| Hoping this will grow on me.

I don’t consider this "preferred style", just the easiest way to convert one of the most complicated call sites, which therefore is in the very last patch of this sequence. If there were lots of call sites like this one I might feel the loss of format strings more acutely. Note that this one call site could be made a lot prettier and more elegant in multiple ways, but my goal wasn’t that; it was to get rid of the last few callers in the simplest way.

[Darin Adler]

Committed r242014: <https://trac.webkit.org/changeset/242014>

[Radar WebKit Bug Importer]

<rdar://problem/48348149>

[Darin Adler]

Just realized that actually removing String::format may again cause problems with the web inspector like it did back when we tried to fix bug 188191. I hope not!

[Simon Fraser (smfr)]

I'm getting crashes with compositing logging enabled, under:

    logString.append(makeString(pad(' ', 12 + depth * 2, hex(reinterpret_cast<uintptr_t>(&layer))), " id ", backing->graphicsLayer()->primaryLayerID(), " (", FormattedNumber::fixedWidth(absoluteBounds.x().toFloat(), 3), ',', FormattedNumber::fixedWidth(absoluteBounds.y().toFloat(), 3), '-', FormattedNumber::fixedWidth(absoluteBounds.maxX().toFloat(), 3), ',', FormattedNumber::fixedWidth(absoluteBounds.maxY().toFloat(), 3), ") ", FormattedNumber::fixedWidth(backing->backingStoreMemoryEstimate() / 1024, 2), "KB"));

Not sure why yet. I just just crash in memcpy tho.

[Simon Fraser (smfr)]

    frame #0: 0x000000010d004a69 libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell + 41
  * frame #1: 0x00000005ff656697 WebCore`void WTF::StringImpl::copyCharacters<unsigned char>(destination="", source="", numCharacters=631693656) at StringImpl.h:1088:5
    frame #2: 0x00000005ff810044 WebCore`void WTF::StringTypeAdapter<WTF::HexNumberBuffer, void>::writeTo<unsigned char>(this=0x00007ffeebc8a4d0, destination="") const at HexNumber.h:104:87
    frame #3: 0x00000006027f40be WebCore`void WTF::StringTypeAdapter<WTF::PaddingSpecification<WTF::StringTypeAdapter<WTF::HexNumberBuffer, void> >, void>::writeTo<unsigned char>(this=0x00007ffeebc89fa0, destination="") const at StringConcatenate.h:229:37
    frame #4: 0x00000006027f2de4 WebCore`void WTF::makeStringAccumulator<unsigned char, WTF::StringTypeAdapter<WTF::PaddingSpecification<WTF::StringTypeAdapter<WTF::HexNumberBuffer, void> >, void>, WTF::StringTypeAdapter<char const*, void>, WTF::StringTypeAdapter<unsigned long long, void>, WTF::StringTypeAdapter<char const*, void>, WTF::StringTypeAdapter<WTF::FormattedNumber, void>, WTF::StringTypeAdapter<char, void>, WTF::StringTypeAdapter<WTF::FormattedNumber, void>, WTF::StringTypeAdapter<char, void>, WTF::StringTypeAdapter<WTF::FormattedNumber, void>, WTF::StringTypeAdapter<char, void>, WTF::StringTypeAdapter<WTF::FormattedNumber, void>, WTF::StringTypeAdapter<char const*, void>, WTF::StringTypeAdapter<WTF::FormattedNumber, void>, WTF::StringTypeAdapter<char const*, void> >(result="", adapter=StringTypeAdapter<WTF::PaddingSpecification<WTF::StringTypeAdapter<WTF::HexNumberBuffer> >, void> @ 0x00007ffeebc89fa0, adapters=StringTypeAdapter<const char *, void> @ 0x00007ffeebc89f90, adapters=(m_number = 39), adapters=StringTypeAdapter<const char *, void> @ 0x00007ffeebc89fe0, adapters=StringTypeAdapter<WTF::FormattedNumber, void> @ 0x00007ffeebc89f80, adapters=(m_character = ','), adapters=StringTypeAdapter<WTF::FormattedNumber, void> @ 0x00007ffeebc89f70, adapters=(m_character = '-'), adapters=StringTypeAdapter<WTF::FormattedNumber, void> @ 0x00007ffeebc89f60, adapters=(m_character = ','), adapters=StringTypeAdapter<WTF::FormattedNumber, void> @ 0x00007ffeebc89f50, adapters=StringTypeAdapter<const char *, void> @ 0x00007ffeebc8a020, adapters=StringTypeAdapter<WTF::FormattedNumber, void> @ 0x00007ffeebc89f48, adapters=StringTypeAdapter<const char *, void> @ 0x00007ffeebc8a038) at StringConcatenate.h:257:13
    frame #5: 0x00000006027f26d5 WebCore`WTF::String WTF::tryMakeStringFromAdapters<WTF::StringTypeAdapter<WTF::PaddingSpecification<WTF::StringTypeAdapter<WTF::HexNumberBuffer, void> >, void>, WTF::StringTypeAdapter<char const*, void>, WTF::StringTypeAdapter<unsigned long long, void>, WTF::StringTypeAdapter<char const*, void>, WTF::StringTypeAdapter<WTF::FormattedNumber, void>, WTF::StringTypeAdapter<char, void>, WTF::StringTypeAdapter<WTF::FormattedNumber, void>, WTF::StringTypeAdapter<char, void>, WTF::StringTypeAdapter<WTF::FormattedNumber, void>, WTF::StringTypeAdapter<char, void>, WTF::StringTypeAdapter<WTF::FormattedNumber, void>, WTF::StringTypeAdapter<char const*, void>, WTF::StringTypeAdapter<WTF::FormattedNumber, void>, WTF::StringTypeAdapter<char const*, void> >(adapter=StringTypeAdapter<WTF::PaddingSpecification<WTF::StringTypeAdapter<WTF::HexNumberBuffer> >, void> @ 0x00007ffeebc8a310, adapters=StringTypeAdapter<const char *, void> @ 0x00007ffeebc8a300, adapters=(m_number = 39), adapters=StringTypeAdapter<const char *, void> @ 0x00007ffeebc8a350, adapters=StringTypeAdapter<WTF::FormattedNumber, void> @ 0x00007ffeebc8a2f0, adapters=(m_character = ','), adapters=StringTypeAdapter<WTF::FormattedNumber, void> @ 0x00007ffeebc8a2e0, adapters=(m_character = '-'), adapters=StringTypeAdapter<WTF::FormattedNumber, void> @ 0x00007ffeebc8a2d0, adapters=(m_character = ','), adapters=StringTypeAdapter<WTF::FormattedNumber, void> @ 0x00007ffeebc8a2c0, adapters=StringTypeAdapter<const char *, void> @ 0x00007ffeebc8a390, adapters=StringTypeAdapter<WTF::FormattedNumber, void> @ 0x00007ffeebc8a2b8, adapters=StringTypeAdapter<const char *, void> @ 0x00007ffeebc8a3a8) at StringConcatenate.h:277:9
    frame #6: 0x00000006027f2018 WebCore`WTF::String WTF::tryMakeString<WTF::PaddingSpecification<WTF::StringTypeAdapter<WTF::HexNumberBuffer, void> >, char const*, unsigned long long, char const*, WTF::FormattedNumber, char, WTF::FormattedNumber, char, WTF::FormattedNumber, char, WTF::FormattedNumber, char const*, WTF::FormattedNumber, char const*>(strings=PaddingSpecification<WTF::StringTypeAdapter<WTF::HexNumberBuffer> > @ 0x00007ffeebc8a4c8, strings=" id ", strings=39, strings=" (", strings=(m_buffer = "0.000", m_length = 5), strings=',', strings=(m_buffer = "622.000", m_length = 7), strings='-', strings=(m_buffer = "414.000", m_length = 7), strings=',', strings=(m_buffer = "670.000", m_length = 7), strings=") ", strings=(m_buffer = "0.00", m_length = 4), strings="KB") at StringConcatenate.h:295:12
    frame #7: 0x00000006027cdca1 WebCore`WTF::String WTF::makeString<WTF::PaddingSpecification<WTF::StringTypeAdapter<WTF::HexNumberBuffer, void> >, char const*, unsigned long long, char const*, WTF::FormattedNumber, char, WTF::FormattedNumber, char, WTF::FormattedNumber, char, WTF::FormattedNumber, char const*, WTF::FormattedNumber, char const*>(strings=PaddingSpecification<WTF::StringTypeAdapter<WTF::HexNumberBuffer> > @ 0x00007ffeebc8a828, strings=" id ", strings=39, strings=" (", strings=(m_buffer = "0.000", m_length = 5), strings=',', strings=(m_buffer = "622.000", m_length = 7), strings='-', strings=(m_buffer = "414.000", m_length = 7), strings=',', strings=(m_buffer = "670.000", m_length = 7), strings=") ", strings=(m_buffer = "0.00", m_length = 4), strings="KB") at StringConcatenate.h:301:21
    frame #8: 0x00000006027cd253 WebCore`WebCore::RenderLayerCompositor::logLayerInfo(this=0x0000000625a6e158, layer=0x0000000633593540, phase="updateBackingAndHierarchy", depth=2) at RenderLayerCompositor.cpp:1327:22

[Simon Fraser (smfr)]

(lldb) fr va m_padding.underlyingAdapter.m_buffer
(const WTF::HexNumberBuffer &) m_padding.underlyingAdapter.m_buffer = 0x00007ffeebc8aa80: {
  characters = (__elems_ = "0.000")
  length = 631693656
}

[Simon Fraser (smfr)]

I rolled this out in r242189.

[Simon Fraser (smfr)]

I filed bug 195141 on the crashing.

[Darin Adler]

Thanks, Simon. Sorry for the inconvenience. I’ll re-land this after adding more tests and fixing the crash you ran into.

[Darin Adler]

Committed r242308: <https://trac.webkit.org/changeset/242308>