Bug 140763 – [CSS Grid Layout] Invalid initialization of track sizes with non spanning grid items

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sergio Villar Senin]

The track sizing algorithm describes in section "11.5. Resolve Intrinsic Track Sizes" how to size content sized functions. The first step consists of sizing the tracks to fit non-spanning grid items, i.e, items with span == 1 on the given direction (rows or columns).

We instead, were not treating those non-spanning grid items differently. Consequently track sizes were not properly computed in some scenarios (mainly because infinite growth limits were not resolved to absolute lengths before using the spanning items to size the tracks).

[Sergio Villar Senin]

Created attachment 245349 [details]
Patch

Not expected to build as it depends on 140540

[Sergio Villar Senin]

Created attachment 246271 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 246271 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:587
> +#if ENABLE(ASSERT)

ENABLE(ASSERT) -> !ASSERT_DISABLED

There isn't ENABLE_ASSERT defined, but ASSERT_DISABLED.

[Sergio Villar Senin]

Created attachment 246318 [details]
Patch

[Antti Koivisto]

Comment on attachment 246318 [details]
Patch

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

> Source/WebCore/rendering/style/GridTrackSize.h:51
>      GridTrackSize(LengthType type = Undefined)
> -        : m_type(LengthTrackSizing)
> -        , m_minTrackBreadth(type)
> -        , m_maxTrackBreadth(type)
>      {
> +        setLength(GridLength(Length(type)));
>      }

This constructor leaves the cache uninitialised.

> Source/WebCore/rendering/style/GridTrackSize.h:123
> +    void cacheMinMaxTrackBreadthTypes()
> +    {
> +        m_minTrackBreadthIsMinContent = minTrackBreadth().isLength() && minTrackBreadth().length().isMinContent();
> +        m_minTrackBreadthIsMaxContent = minTrackBreadth().isLength() && minTrackBreadth().length().isMaxContent();
> +        m_maxTrackBreadthIsMaxContent = maxTrackBreadth().isLength() && maxTrackBreadth().length().isMaxContent();
> +        m_maxTrackBreadthIsMinContent = maxTrackBreadth().isLength() && maxTrackBreadth().length().isMinContent();
> +    }

Is this cache really valuable? Those looks like cheap checks.

[Antti Koivisto]

> This constructor leaves the cache uninitialised.

Actually it doesn't, it is done in setLength. nm

[Sergio Villar Senin]

(In reply to comment #5)
> Comment on attachment 246318 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246318&action=review
> 
> > Source/WebCore/rendering/style/GridTrackSize.h:51
> >      GridTrackSize(LengthType type = Undefined)
> > -        : m_type(LengthTrackSizing)
> > -        , m_minTrackBreadth(type)
> > -        , m_maxTrackBreadth(type)
> >      {
> > +        setLength(GridLength(Length(type)));
> >      }
> 
> This constructor leaves the cache uninitialised.
> 
> > Source/WebCore/rendering/style/GridTrackSize.h:123
> > +    void cacheMinMaxTrackBreadthTypes()
> > +    {
> > +        m_minTrackBreadthIsMinContent = minTrackBreadth().isLength() && minTrackBreadth().length().isMinContent();
> > +        m_minTrackBreadthIsMaxContent = minTrackBreadth().isLength() && minTrackBreadth().length().isMaxContent();
> > +        m_maxTrackBreadthIsMaxContent = maxTrackBreadth().isLength() && maxTrackBreadth().length().isMaxContent();
> > +        m_maxTrackBreadthIsMinContent = maxTrackBreadth().isLength() && maxTrackBreadth().length().isMinContent();
> > +    }
> 
> Is this cache really valuable? Those looks like cheap checks.

As I mentioned it looks like it is. I was also a bit surprised due to the nature of the checks, but as I mention in the ChangeLog their impact in the total amount of time goes down from 1% to 0.3%.

[Sergio Villar Senin]

Committed r179987: <http://trac.webkit.org/changeset/179987>

[Csaba Osztrogonác]

(In reply to comment #8)
> Committed r179987: <http://trac.webkit.org/changeset/179987>

It made many tests crash/assert on the debug bots.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 141525

[Sergio Villar Senin]

I noticed that I missed to include in the patch lo land this condition:

!minTrackBreadth().length().isUndefined() (for min track breadths)

!maxTrackBreadth().length().isUndefined() (for max track breadths)

in chacheMinMaxTrackBreadthTypes(). I'll reland with it.

[Sergio Villar Senin]

(In reply to comment #11)
> I noticed that I missed to include in the patch lo land this condition:
> 
> !minTrackBreadth().length().isUndefined() (for min track breadths)
> 
> !maxTrackBreadth().length().isUndefined() (for max track breadths)
> 
> in chacheMinMaxTrackBreadthTypes(). I'll reland with it.

Actually I think it'd be better to completely remove the undefined case, and call always the constructor with the valid length. I'll do it in a different patch.

[Sergio Villar Senin]

Committed r180142: <http://trac.webkit.org/changeset/180142>