Remove BufferForAppendingHyphen
Created attachment 233954 [details] Patch
Comment on attachment 233954 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=233954&action=review > Source/WebCore/rendering/InlineTextBox.cpp:4 > + * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2014 Apple Inc. All rights reserved. We can use year ranges, so this should be 2004-2011, 2014, or maybe even 2004-2014 if we made changes in 2012 and 2013. > Source/WebCore/rendering/InlineTextBox.cpp:257 > + if (!string.isEmpty()) { Why are you adding this check? The old code doesn’t check for this case. If you are fixing a bug, then should we add a test case for that bug? > Source/WebCore/rendering/InlineTextBox.cpp:259 > + string = string + hyphenString; Why not use string.append(hyphenString) instead of string = string + hyphenString? Irritating to have to do this memory allocation. > Source/WebCore/rendering/InlineTextBox.h:-29 > -#include <wtf/text/StringBuilder.h> Looks like TextIterator.cpp, and maybe other files, now needs to include StringBuilder.h.
(In reply to comment #2) > > Source/WebCore/rendering/InlineTextBox.cpp:259 > > + string = string + hyphenString; > > Why not use string.append(hyphenString) instead of string = string + hyphenString? I'll explain to you IRL. It is not critical, I can cut it.
Created attachment 233964 [details] Patch
Comment on attachment 233964 [details] Patch Attachment 233964 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5224078520614912 New failing tests: fast/text/hyphen-min-preferred-width.html fast/text/hyphenate-first-word-after-skipped-space.html
Created attachment 233971 [details] Archive of layout-test-results from webkit-ews-03 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 233964 [details] Patch Attachment 233964 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4719324543582208 New failing tests: fast/text/hyphen-min-preferred-width.html fast/text/hyphenate-first-word-after-skipped-space.html
Created attachment 233972 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 233973 [details] Patch
Comment on attachment 233973 [details] Patch I don’t understand this new patch that uses a String*. Why is that better than the version where we took in a String&?
(In reply to comment #10) > (From update of attachment 233973 [details]) > I don’t understand this new patch that uses a String*. Why is that better than the version where we took in a String&? The problem is StringBuilder is pure overhead in the old code. The problems I have are: 1) The initial reserveCapacity(256) spend time in malloc in all cases, we never need that buffer. 2) On the call sites of constructTextRun, we have too much overhead due to StringBuilder (2 deref + 2 destructor + more memory manipulation. 3) When we have an hyphen, we have a second "reserveCapacity()" that allocates a string of the wrong type in many cases. 4) The final append() have extra overhead of using StringBuilder. I noticed the issue because (1) and (2) are hurting us on ARMv7.
Comment on attachment 233973 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=233973&action=review > Source/WebCore/ChangeLog:10 > + The problem is that StringBuilder's memory was also implicitely used to keep Typo "implicitly".
Committed r170561: <http://trac.webkit.org/changeset/170561>