Bug 205170 – [LFC][IFC] Fix fast/text/whitespace/023.html

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[zalan]

ssia

[Radar WebKit Bug Importer]

<rdar://problem/57881365>

[zalan]

Created attachment 385506 [details]
Patch

[Antti Koivisto]

Comment on attachment 385506 [details]
Patch

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

> Source/WebCore/layout/inlineformatting/InlineLineBuilder.cpp:65
> +    m_collectExpansionOpportunities = textIsAlignJustify && whitespace != WhiteSpace::Pre && whitespace != WhiteSpace::PreWrap && whitespace != WhiteSpace::BreakSpaces;

Doesn't some combination of whitespace != WhiteSpace::Pre && whitespace != WhiteSpace::PreWrap && whitespace != WhiteSpace::BreakSpaces show up in other places too? Maybe there is or should be a helper?

[Antti Koivisto]

Also could factor the into function and initialize from initializer list.

, m_collectExpansionOpportunities(computeCollectExpansionOpportunities(m_initialInlineRun.style(), textIsAlignJustify))

[zalan]

(In reply to Antti Koivisto from comment #3)
> Comment on attachment 385506 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385506&action=review
> 
> > Source/WebCore/layout/inlineformatting/InlineLineBuilder.cpp:65
> > +    m_collectExpansionOpportunities = textIsAlignJustify && whitespace != WhiteSpace::Pre && whitespace != WhiteSpace::PreWrap && whitespace != WhiteSpace::BreakSpaces;
> 
> Doesn't some combination of whitespace != WhiteSpace::Pre && whitespace !=
> WhiteSpace::PreWrap && whitespace != WhiteSpace::BreakSpaces show up in
> other places too? Maybe there is or should be a helper?
Nope, I had the same feeling and did a grep. We use this property for other reasons too like checking whether the new line is preserved () or whether content wrapping is allowed (WhiteSpace::Pre WhiteSpace::NoWrap)

[zalan]

(In reply to zalan from comment #5)
> (In reply to Antti Koivisto from comment #3)
> > Comment on attachment 385506 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=385506&action=review
> > 
> > > Source/WebCore/layout/inlineformatting/InlineLineBuilder.cpp:65
> > > +    m_collectExpansionOpportunities = textIsAlignJustify && whitespace != WhiteSpace::Pre && whitespace != WhiteSpace::PreWrap && whitespace != WhiteSpace::BreakSpaces;
> > 
> > Doesn't some combination of whitespace != WhiteSpace::Pre && whitespace !=
> > WhiteSpace::PreWrap && whitespace != WhiteSpace::BreakSpaces show up in
> > other places too? Maybe there is or should be a helper?
> Nope, I had the same feeling and did a grep. We use this property for other
> reasons too like checking whether the new line is preserved () or whether
> content wrapping is allowed (WhiteSpace::Pre WhiteSpace::NoWrap)
I mean RenderStyle does have a helper.

[Antti Koivisto]

> I mean RenderStyle does have a helper.

Yeah, I was probably thinking that.

[zalan]

Committed r253435: <https://trac.webkit.org/changeset/253435>