Bug 191445 - [LFC][IFC] Move some code from InlineFormattingContext::Line to InlineFormattingContext/Geometry
Summary: [LFC][IFC] Move some code from InlineFormattingContext::Line to InlineFormatt...
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: 2018-11-08 16:53 PST by zalan
Modified: 2018-11-09 08:54 PST (History)
5 users (show)

See Also:

Attachments
Patch (21.03 KB, patch)
2018-11-08 18:58 PST, zalan 		no flags Details | Formatted Diff | Diff
Patch (21.30 KB, patch)
2018-11-08 21:21 PST, zalan 		koivisto: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2018-11-08 16:53:24 PST 
The idea here is Line should not need to deal with all the post processing activities like aligning the runs (there will be more)
Comment 1 zalan 2018-11-08 18:58:36 PST 
Created attachment 354302 [details]
Patch
Comment 2 zalan 2018-11-08 21:21:45 PST 
Created attachment 354311 [details]
Patch
Comment 3 Antti Koivisto 2018-11-09 05:22:27 PST 
Comment on attachment 354311 [details]
Patch

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

> Source/WebCore/layout/inlineformatting/InlineFormattingContext.h:87
>          InlineFormattingState& m_formattingState;

Maybe the InlineFormattingState can be passed to the functions that need it? It seems bit silly to have this backpointer in every Line instance.

> Source/WebCore/layout/inlineformatting/InlineFormattingContextGeometry.cpp:111
> +    expansionBehavior ^= AllowTrailingExpansion;
> +    expansionBehavior |= ForbidTrailingExpansion;

Should modernize this to use OptionSet at some point.
Comment 4 Antti Koivisto 2018-11-09 05:23:04 PST 
(neither comment has much to do with this patch)
Comment 5 zalan 2018-11-09 07:29:06 PST 
Committed r238028: <https://trac.webkit.org/changeset/238028>
Comment 6 Radar WebKit Bug Importer 2018-11-09 07:30:24 PST 
<rdar://problem/45943491>
Comment 7 Antti Koivisto 2018-11-09 08:53:19 PST 
Comment on attachment 354311 [details]
Patch

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

>> Source/WebCore/layout/inlineformatting/InlineFormattingContext.h:87
>>          InlineFormattingState& m_formattingState;
> 
> Maybe the InlineFormattingState can be passed to the functions that need it? It seems bit silly to have this backpointer in every Line instance.

Actually Line seems to be a stack object so this comment doesn't make much sense.
Comment 8 zalan 2018-11-09 08:54:29 PST 
(In reply to Antti Koivisto from comment #7)
> Comment on attachment 354311 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354311&action=review
> 
> >> Source/WebCore/layout/inlineformatting/InlineFormattingContext.h:87
> >>          InlineFormattingState& m_formattingState;
> > 
> > Maybe the InlineFormattingState can be passed to the functions that need it? It seems bit silly to have this backpointer in every Line instance.
> 
> Actually Line seems to be a stack object so this comment doesn't make much
> sense.
It is indeed a stack object and we only construct one per formatting context.