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).
Created attachment 245349 [details] Patch Not expected to build as it depends on 140540
Created attachment 246271 [details] Patch
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.
Created attachment 246318 [details] Patch
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.
> This constructor leaves the cache uninitialised. Actually it doesn't, it is done in setLength. nm
(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%.
Committed r179987: <http://trac.webkit.org/changeset/179987>
(In reply to comment #8) > Committed r179987: <http://trac.webkit.org/changeset/179987> It made many tests crash/assert on the debug bots.
Re-opened since this is blocked by bug 141525
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.
(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.
Committed r180142: <http://trac.webkit.org/changeset/180142>