Bug 205170 - [LFC][IFC] Fix fast/text/whitespace/023.html
Summary: [LFC][IFC] Fix fast/text/whitespace/023.html
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-12-12 09:55 PST by zalan
Modified: 2019-12-12 10:27 PST (History)
5 users (show)

See Also:


Attachments
Patch (3.61 KB, patch)
2019-12-12 09:58 PST, zalan
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2019-12-12 09:55:58 PST
ssia
Comment 1 Radar WebKit Bug Importer 2019-12-12 09:56:18 PST
<rdar://problem/57881365>
Comment 2 zalan 2019-12-12 09:58:40 PST
Created attachment 385506 [details]
Patch
Comment 3 Antti Koivisto 2019-12-12 10:01:18 PST
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?
Comment 4 Antti Koivisto 2019-12-12 10:04:19 PST
Also could factor the into function and initialize from initializer list.

, m_collectExpansionOpportunities(computeCollectExpansionOpportunities(m_initialInlineRun.style(), textIsAlignJustify))
Comment 5 zalan 2019-12-12 10:05:29 PST
(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)
Comment 6 zalan 2019-12-12 10:06:05 PST
(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.
Comment 7 Antti Koivisto 2019-12-12 10:08:18 PST
> I mean RenderStyle does have a helper.

Yeah, I was probably thinking that.
Comment 8 zalan 2019-12-12 10:27:23 PST
Committed r253435: <https://trac.webkit.org/changeset/253435>