WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
140763
[CSS Grid Layout] Invalid initialization of track sizes with non spanning grid items
https://bugs.webkit.org/show_bug.cgi?id=140763
Summary
[CSS Grid Layout] Invalid initialization of track sizes with non spanning gri...
Sergio Villar Senin
Reported
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).
Attachments
Patch
(23.78 KB, patch)
2015-01-26 06:54 PST
,
Sergio Villar Senin
no flags
Details
Formatted Diff
Diff
Patch
(23.79 KB, patch)
2015-02-09 07:01 PST
,
Sergio Villar Senin
no flags
Details
Formatted Diff
Diff
Patch
(23.79 KB, patch)
2015-02-10 03:26 PST
,
Sergio Villar Senin
koivisto
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Sergio Villar Senin
Comment 1
2015-01-26 06:54:29 PST
Created
attachment 245349
[details]
Patch Not expected to build as it depends on 140540
Sergio Villar Senin
Comment 2
2015-02-09 07:01:01 PST
Created
attachment 246271
[details]
Patch
Csaba Osztrogonác
Comment 3
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.
Sergio Villar Senin
Comment 4
2015-02-10 03:26:48 PST
Created
attachment 246318
[details]
Patch
Antti Koivisto
Comment 5
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.
Antti Koivisto
Comment 6
2015-02-12 06:20:29 PST
> This constructor leaves the cache uninitialised.
Actually it doesn't, it is done in setLength. nm
Sergio Villar Senin
Comment 7
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%.
Sergio Villar Senin
Comment 8
2015-02-12 08:14:29 PST
Committed
r179987
: <
http://trac.webkit.org/changeset/179987
>
Csaba Osztrogonác
Comment 9
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.
WebKit Commit Bot
Comment 10
2015-02-12 11:56:31 PST
Re-opened since this is blocked by
bug 141525
Sergio Villar Senin
Comment 11
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.
Sergio Villar Senin
Comment 12
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.
Sergio Villar Senin
Comment 13
2015-02-16 04:00:34 PST
Committed
r180142
: <
http://trac.webkit.org/changeset/180142
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug