Migrate line breaking code from ints to Optional<unsigned>s
Created attachment 286080 [details] WIP
Created attachment 286090 [details] Patch
Created attachment 286125 [details] Patch
Comment on attachment 286125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286125&action=review Make sure to do performance testing if you haven’t already. This is all inlined because it's so hot, so maybe the separate booleans in the Optional will slow things down a bit. > Source/WebCore/rendering/BreakLines.h:23 > +#ifndef BreakLines_h > +#define BreakLines_h Should use #pragma once > Source/WebCore/rendering/BreakLines.h:35 > +static const UChar asciiLineBreakTableFirstChar = '!'; > +static const UChar asciiLineBreakTableLastChar = 127; > +static const unsigned asciiLineBreakTableColumnCount = (asciiLineBreakTableLastChar - asciiLineBreakTableFirstChar) / 8 + 1; No need for "static" on any of these. Not sure the names here need to say ASCII. Given the first character is '!' and the last is 127, then, yes, it's only ASCII characters, but the code doesn't seem to rely on that so why include that acronym in the names. Also, why not write out the word "Character" instead of "Char"? > Source/WebCore/rendering/BreakLines.h:42 > +enum class NBSPBehavior { > + IgnoreNBSP, > + TreatNBSPAsBreak, > +}; Do we need to use NBSP here instead of writing out NonBreakingSpace? The acronym seems odious. > Source/WebCore/rendering/BreakLines.h:186 > + String string = lazyBreakIterator.string(); We could make this faster by making the function return const String& and use auto& here to avoid unnecessary reference count churning. Or even use StringView. > Source/WebCore/rendering/BreakLines.h:252 > + if (keepAllWords) { > + if (breakNBSP) > + nextBreakable = nextBreakablePositionKeepingAllWords(lazyBreakIterator, startPosition); > + else > + nextBreakable = nextBreakablePositionKeepingAllWordsIgnoringNBSP(lazyBreakIterator, startPosition); > + } else if (isLooseMode) { > + if (breakNBSP) > + nextBreakable = nextBreakablePositionLoose(lazyBreakIterator, startPosition); > + else > + nextBreakable = nextBreakablePositionIgnoringNBSPLoose(lazyBreakIterator, startPosition); > + } else { > + if (breakNBSP) > + nextBreakable = nextBreakablePosition(lazyBreakIterator, startPosition); > + else > + nextBreakable = nextBreakablePositionIgnoringNBSP(lazyBreakIterator, startPosition); > + } Seems kind of messy with all these inline functions that are just boilerplate calling the 8 bit and 16 bit versions of things. I wonder if there is a cleaner way to organize this so we don’t actually have to write everything out. > Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.h:27 > #ifndef SimpleLineLayoutTextFragmentIterator_h > #define SimpleLineLayoutTextFragmentIterator_h #pragma once > Source/WebCore/rendering/line/BreakingContext.h:26 > #ifndef BreakingContext_h > #define BreakingContext_h #pragma once > Source/WebKit/ios/Misc/WebUIKitSupport.mm:94 > LazyLineBreakIterator breakIterator(String(characters, length)); I think this shows that we should change LazyLineBreakIterator to operate on a StringView rather than a String. If there is no lifetime problem with the string we are doing the line breaks on, then it seems valuable to make that change. > Source/WebKit/ios/Misc/WebUIKitSupport.mm:97 > - while ((breakPos = nextBreakablePosition(breakIterator, breakPos)) < position) > + while (static_cast<int>(breakPos = nextBreakablePosition(breakIterator, breakPos)) < position) > lastBreakPos = breakPos++; > - return lastBreakPos < position ? (NSUInteger)lastBreakPos : INT_MAX; > + return static_cast<int>(lastBreakPos) < position ? lastBreakPos : INT_MAX; These casts aren’t obviously safe if the numbers happen to be huge. But perhaps we have a guarantee they won’t be.
Created attachment 286162 [details] Patch for committing
(In reply to comment #4) > Comment on attachment 286125 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=286125&action=review > > Make sure to do performance testing if you haven’t already. This is all > inlined because it's so hot, so maybe the separate booleans in the Optional > will slow things down a bit. PLT shows no regression.
Comment on attachment 286162 [details] Patch for committing Clearing flags on attachment: 286162 Committed r204531: <http://trac.webkit.org/changeset/204531>
(In reply to comment #6) > (In reply to comment #4) > > Comment on attachment 286125 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=286125&action=review > > > > Make sure to do performance testing if you haven’t already. This is all > > inlined because it's so hot, so maybe the separate booleans in the Optional > > will slow things down a bit. > > PLT shows no regression. Running PerformanceTests/Layout/line-layout.html also shows no regression.