Simple line layout: Skip 16bit specific checks on 8bit content.

[zalan]

They are redundant.

[Radar WebKit Bug Importer]

<rdar://problem/30361948>

[zalan]

Created attachment 300601 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 300601 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=300601&action=review

> Source/WebCore/rendering/SimpleLineLayout.cpp:-139
> -template <typename CharacterType>

Historically, we've gotten some speedups by using the template to generate a completely new loop for 8-bit code rather than doing it at runtime. We might not want to lose this benefit.

> Source/WebCore/rendering/SimpleLineLayout.cpp:160
> +        // 16bit checks start here.

Redundant with the line above. Should remove.

> Source/WebCore/rendering/SimpleLineLayout.cpp:163
>              bool isLatinIncludingExtendedB = character <= 0x01FF;

What does this have to do with justification?

> Source/WebCore/rendering/SimpleLineLayout.cpp:169
> +        if (lineHeightConstraint && primaryFont.boundsForGlyph(glyphData.glyph).height() > *lineHeightConstraint)

Why doesn't this need to be done in the 8-bit case? Any glyph can have giant bounds (see Zapfino)

> Source/WebCore/rendering/SimpleLineLayout.cpp:195
> +        // FIXME: Do not return until after checking all children.

Why?

> Source/WebCore/rendering/SimpleLineLayout.cpp:196
> +        if (!textRenderer.textLength())

Why don't you just detect this case early and ASSERT() here? It's really easy to measure 0 length runs.

[zalan]

> > Source/WebCore/rendering/SimpleLineLayout.cpp:160
> > +        // 16bit checks start here.
> 
> Redundant with the line above. Should remove.
Ok. I just wanted to emphasize that this is for 16bit only.

> 
> > Source/WebCore/rendering/SimpleLineLayout.cpp:163
> >              bool isLatinIncludingExtendedB = character <= 0x01FF;
> 
> What does this have to do with justification?
This is a hack for iBooks content (extend justified text support to Latin-B + punctuation. see r210948 -we talked about this a week ago.) 
> 
> > Source/WebCore/rendering/SimpleLineLayout.cpp:169
> > +        if (lineHeightConstraint && primaryFont.boundsForGlyph(glyphData.glyph).height() > *lineHeightConstraint)
> 
> Why doesn't this need to be done in the 8-bit case? Any glyph can have giant
> bounds (see Zapfino)
Yes, I missed this. Thanks.

> 
> > Source/WebCore/rendering/SimpleLineLayout.cpp:195
> > +        // FIXME: Do not return until after checking all children.
> 
> Why?
Because we could still do SLL on a block that has multiple text children even when one of them is empty.

> 
> > Source/WebCore/rendering/SimpleLineLayout.cpp:196
> > +        if (!textRenderer.textLength())
> 
> Why don't you just detect this case early and ASSERT() here? It's really
> easy to measure 0 length runs.
This is the part where we start looking into the content (as opposed to css properties). We can move checks around, but the current grouping is as follows:
basic tree structure checks, css properties, content.

[zalan]

Created attachment 300623 [details]
Patch

[Antti Koivisto]

Comment on attachment 300623 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=300623&action=review

> Source/WebCore/rendering/SimpleLineLayout.cpp:141
> +template <typename CharacterType>
> +static AvoidanceReasonFlags canUseForCharacter(CharacterType character, bool textIsJustified, IncludeReasons includeReasons)

It might be nicer to to first declare the template and then provide both specializations separately:

template <typename CharacterType> AvoidanceReasonFlags canUseForCharacter(CharacterType, bool textIsJustified, IncludeReasons);

template<> AvoidanceReasonFlags canUseForCharacter(UChar character, bool textIsJustified, IncludeReasons includeReasons) 
{
   ...

I don't think 'static' is needed.

[Antti Koivisto]

Comment on attachment 300623 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=300623&action=review

> Source/WebCore/rendering/SimpleLineLayout.cpp:189
> +        canUseForCharacter(character, textIsJustified, includeReasons);

Should we do something with the return value?

[zalan]

Created attachment 300624 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 300624 [details]
Patch

Clearing flags on attachment: 300624

Committed r211671: <http://trac.webkit.org/changeset/211671>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.