Bug 185249 – [LFC] Box invalidation logic should go to dedicated classes.

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

[alan]

The idea here is that when certain style attribute changes, the invalidation logic depends on the type of the formatting context the box lives in.

[alan]

Created attachment 339407 [details]
Patch

[Antti Koivisto]

Comment on attachment 339407 [details]
Patch

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

> Source/WebCore/layout/FormattingState.h:60
> +    enum BaseTypeFlag {
> +        BlockStateFlag     = 1 << 0,
> +        InlineStateFlag    = 1 << 1
> +    };
> +
> +    FormattingState(Ref<FloatingState>&&, BaseTypeFlags);

These two are mutually exclusive so could be replaced with 'enum class Type { Block, Inline }'. But maybe there is going to be a deeper hierarchy of FormattingStates and this will expand?

This could be done with virtual functions but maybe it is a good idea to avoid them entirely.

[alan]

Created attachment 339411 [details]
Patch

[Antti Koivisto]

Comment on attachment 339411 [details]
Patch

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

> Source/WebCore/layout/FormattingState.h:57
> +    enum StateType {
> +        BlockState,
> +        InlineState
> +    };

I would go with

enum class Type { Block, Inline }

This is all scoped to FormattingState so repeating word 'State' is redundant.

> Source/WebCore/layout/LayoutContext.h:62
> +    enum UpdateType {

enum class?

[alan]

Created attachment 339417 [details]
Patch

[alan]

Created attachment 339419 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 339419 [details]
Patch

Clearing flags on attachment: 339419

Committed r231312: <https://trac.webkit.org/changeset/231312>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39944996>

[Antti Koivisto]

Comment on attachment 339419 [details]
Patch

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

> Source/WebCore/layout/LayoutContext.h:69
> +    enum class UpdateType {
> +        Overflow = 1 << 0,
> +        Position = 1 << 1,
> +        Size     = 1 << 2,
> +        All      = Overflow | Position | Size
> +    };
> +    void markNeedsUpdate(const Box&, OptionSet<UpdateType>);

'All' value like this is awkward with OptionSet (it will assert unless used with OptionSet::fromRaw), the enum should contain single bit values only. Defining constant like 

static constexpr OptionSet<UpdateType> UpdateAll = { Overflow, Position, Size };

should work (after 185298).

[alan]

(In reply to Antti Koivisto from comment #10)
> Comment on attachment 339419 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339419&action=review
> 
> > Source/WebCore/layout/LayoutContext.h:69
> > +    enum class UpdateType {
> > +        Overflow = 1 << 0,
> > +        Position = 1 << 1,
> > +        Size     = 1 << 2,
> > +        All      = Overflow | Position | Size
> > +    };
> > +    void markNeedsUpdate(const Box&, OptionSet<UpdateType>);
> 
> 'All' value like this is awkward with OptionSet (it will assert unless used
> with OptionSet::fromRaw), the enum should contain single bit values only.
> Defining constant like 
> 
> static constexpr OptionSet<UpdateType> UpdateAll = { Overflow, Position,
> Size };
> 
> should work (after 185298).
Good point. Will fix it soon.