Bug 160859 – Migrate line breaking code from ints to Optional<unsigned>s

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Migrate line breaking code from ints to Optional<unsigned>s

[Myles C. Maxfield]

Created attachment 286080 [details]
WIP

[Myles C. Maxfield]

Created attachment 286090 [details]
Patch

[Myles C. Maxfield]

Created attachment 286125 [details]
Patch

[Darin Adler]

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.

[Myles C. Maxfield]

Created attachment 286162 [details]
Patch for committing

[Myles C. Maxfield]

(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.

[WebKit Commit Bot]

Comment on attachment 286162 [details]
Patch for committing

Clearing flags on attachment: 286162

Committed r204531: <http://trac.webkit.org/changeset/204531>

[Myles C. Maxfield]

(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.