Summary: | [CSS Grid Layout] Invalid initialization of track sizes with non spanning grid items | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sergio Villar Senin <svillar> | ||||||||
Component: | CSS | Assignee: | 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
Sergio Villar Senin
2015-01-22 03:41:15 PST
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> |