Bug 140763

Summary: [CSS Grid Layout] Invalid initialization of track sizes with non spanning grid items
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: CSSAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, darin, esprehn+autocc, glenn, hyatt, jfernandez, kling, koivisto, kondapallykalyan, ossy, rego, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 140540, 141525, 141562    
Bug Blocks: 60731, 140883    
Attachments:
Description Flags
Patch
none
Patch
none
Patch koivisto: review+

Description Sergio Villar Senin 2015-01-22 03:41:15 PST
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).
Comment 1 Sergio Villar Senin 2015-01-26 06:54:29 PST
Created attachment 245349 [details]
Patch

Not expected to build as it depends on 140540
Comment 2 Sergio Villar Senin 2015-02-09 07:01:01 PST
Created attachment 246271 [details]
Patch
Comment 3 Csaba Osztrogon√°c 2015-02-09 09:22:21 PST
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.
Comment 4 Sergio Villar Senin 2015-02-10 03:26:48 PST
Created attachment 246318 [details]
Patch
Comment 5 Antti Koivisto 2015-02-12 06:16:59 PST
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.
Comment 6 Antti Koivisto 2015-02-12 06:20:29 PST
> This constructor leaves the cache uninitialised.

Actually it doesn't, it is done in setLength. nm
Comment 7 Sergio Villar Senin 2015-02-12 06:37:57 PST
(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%.
Comment 8 Sergio Villar Senin 2015-02-12 08:14:29 PST
Committed r179987: <http://trac.webkit.org/changeset/179987>
Comment 9 Csaba Osztrogon√°c 2015-02-12 10:19:47 PST
(In reply to comment #8)
> Committed r179987: <http://trac.webkit.org/changeset/179987>

It made many tests crash/assert on the debug bots.
Comment 10 WebKit Commit Bot 2015-02-12 11:56:31 PST
Re-opened since this is blocked by bug 141525
Comment 11 Sergio Villar Senin 2015-02-13 04:09:31 PST
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.
Comment 12 Sergio Villar Senin 2015-02-13 07:17:37 PST
(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.
Comment 13 Sergio Villar Senin 2015-02-16 04:00:34 PST
Committed r180142: <http://trac.webkit.org/changeset/180142>