[webkit-reviews] review denied: [Bug 135756] Elements whose contents start with an astral Unicode symbol disappear when CSS `::first-letter` is applied to them : [Attachment 236395] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 135756: Elements whose contents start with an astral Unicode symbol
disappear when CSS `::first-letter` is applied to them
https://bugs.webkit.org/show_bug.cgi?id=135756

Attachment 236395: Patch
https://bugs.webkit.org/attachment.cgi?id=236395&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=236395&action=review


We can do better.

> Source/WebCore/rendering/RenderBlock.cpp:-3589
> -	   length++;

You should just write something like this:

    length +=
numCharactersInGraphemeClusters(StringView(oldText).substring(length), 1);

You’ll have to change the numCharactersInGraphemeClusters to take a StringView
rather than a String, but otherwise this should do the job.

Maybe you’ll need to performance test to see that’s fast enough, but it will do
the right thing.

> Source/WebCore/rendering/RenderBlock.cpp:3589
> +	   // FIXME: The surrogate might come from another <span>

It would be more appropriate to say another text node, not <span>:

    // FIXME: The trail surrogate might be in a subsequent text node.

But it seems strange to put the FIXME here when the same issue applies to all
the rest of the string processing in this function. It’s not specific to this
one part of the algorithm.

> Source/WebCore/rendering/RenderBlock.cpp:3590
> +	   // FIXME: Presumably, combining marks should be a part of this, too

I would word this completely differently, but it’s not needed if you take my
advice above.

> Source/WebCore/rendering/RenderBlock.cpp:3593
> +	       UChar32 codepoint = 0;
> +	       U16_NEXT(oldText.characters16(), length, oldText.length(),
codepoint);

No need to initialize this to zero. Also no reason to smash two words together:
“codepoint”. I would just call this local variable “character”. But you don’t
need this code at all.