Finish removing String::format
Created attachment 362588 [details] Patch
Created attachment 362593 [details] Patch
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
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
Created attachment 362610 [details] Patch
Passing all the tests, just need a reviewer.
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)); :|
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.
Committed r242014: <https://trac.webkit.org/changeset/242014>
<rdar://problem/48348149>
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!
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.
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
(lldb) fr va m_padding.underlyingAdapter.m_buffer (const WTF::HexNumberBuffer &) m_padding.underlyingAdapter.m_buffer = 0x00007ffeebc8aa80: { characters = (__elems_ = "0.000") length = 631693656 }
I rolled this out in r242189.
I filed bug 195141 on the crashing.
Thanks, Simon. Sorry for the inconvenience. I’ll re-land this after adding more tests and fixing the crash you ran into.
Committed r242308: <https://trac.webkit.org/changeset/242308>