Created attachment 176148 [details] Proposed change: Added TrackSizingFunction.h, cleaned up how we handle 'none'

Comment on attachment 176148 [details] Proposed change: Added TrackSizingFunction.h, cleaned up how we handle 'none' Attachment 176148 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15000564

Comment on attachment 176148 [details] Proposed change: Added TrackSizingFunction.h, cleaned up how we handle 'none' Attachment 176148 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14985962

Comment on attachment 176148 [details] Proposed change: Added TrackSizingFunction.h, cleaned up how we handle 'none' Attachment 176148 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14988886

Created attachment 176289 [details] Proposed change 2: updated Qt / EFL to build properly.

Comment on attachment 176289 [details] Proposed change 2: updated Qt / EFL to build properly. View in context: https://bugs.webkit.org/attachment.cgi?id=176289&action=review > Source/WebCore/rendering/style/TrackSizingFunction.h:42 > +class TrackSizingFunction { How about just calling this TrackSize? I know the spec calls these functions, but I find that confusing and bulky.

Comment on attachment 176289 [details] Proposed change 2: updated Qt / EFL to build properly. View in context: https://bugs.webkit.org/attachment.cgi?id=176289&action=review >> Source/WebCore/rendering/style/TrackSizingFunction.h:42 >> +class TrackSizingFunction { > > How about just calling this TrackSize? I know the spec calls these functions, but I find that confusing and bulky. I am fine with TrackSize.

Created attachment 176530 [details] Change 2: Renamed the new structure to TrackSize.

Comment on attachment 176530 [details] Change 2: Renamed the new structure to TrackSize. View in context: https://bugs.webkit.org/attachment.cgi?id=176530&action=review I think GridTrackSize would be a bit better than TrackSize, but I guess it's not a big deal. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:996 > +static PassRefPtr<CSSValue> valueForGridTrackList(const Vector<TrackSize>& trackFunctions, const RenderStyle* style) Nit: trackFunctions -> trackSizes > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1000 > - if (trackLengths.size() == 1 && trackLengths[0].isUndefined()) > + if (!trackFunctions.size()) > return cssValuePool().createIdentifierValue(CSSValueNone); Can we add a test case for 0 tracks? > Source/WebCore/css/StyleResolver.cpp:2662 > +static bool createGridTrackBreadth(CSSPrimitiveValue* primitiveValue, StyleResolver* selector, TrackSize& function) Nit: function -> trackSize > Source/WebCore/css/StyleResolver.cpp:2675 > +static bool createGridTrackList(CSSValue* value, Vector<TrackSize>& functions, StyleResolver* selector) Nit: functions -> sizes or trackSizes > Source/WebCore/css/StyleResolver.cpp:2690 > + TrackSize function; > + if (!createGridTrackBreadth(static_cast<CSSPrimitiveValue*>(currValue), selector, function)) Nit: function -> trackSize > Source/WebCore/rendering/style/StyleGridData.h:54 > + Vector<TrackSize> m_gridColumns; > + Vector<TrackSize> m_gridRows; Maybe we should rename this to m_gridColumnTracks or something?

Comment on attachment 176530 [details] Change 2: Renamed the new structure to TrackSize. Attachment 176530 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15027532 New failing tests: fast/css-grid-layout/containing-block-grids.html fast/css-grid-layout/should-not-collapse-anonymous-blocks.html

Comment on attachment 176530 [details] Change 2: Renamed the new structure to TrackSize. View in context: https://bugs.webkit.org/attachment.cgi?id=176530&action=review > I think GridTrackSize would be a bit better than TrackSize, but I guess it's not a big deal. Either are fine by me. As we have prefixed everything with Grid, we may as well continue. >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1000 >> return cssValuePool().createIdentifierValue(CSSValueNone); > > Can we add a test case for 0 tracks? This code path is already covered unless I am mistaking as we are trying to set and get the value ('none') in LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set.html. On top of that, the failing EWS test cases are exercising the empty grid case (though on the layout side).

Comment on attachment 176530 [details] Change 2: Renamed the new structure to TrackSize. Attachment 176530 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15027574 New failing tests: fast/css-grid-layout/should-not-collapse-anonymous-blocks.html fast/css-grid-layout/containing-block-grids.html

Comment on attachment 176530 [details] Change 2: Renamed the new structure to TrackSize. View in context: https://bugs.webkit.org/attachment.cgi?id=176530&action=review >> Source/WebCore/rendering/style/StyleGridData.h:54 >> + Vector<TrackSize> m_gridRows; > > Maybe we should rename this to m_gridColumnTracks or something? The spec renamed those to grid-definition-columns and grid-definition-rows so if we wanted to follow the spec, it would be m_gridDefinitionColumns. I don't mind changing the names but we are still in the early development phase so we will probably find better naming as add more of the grammar and the spec matures. If there is no objection, I would prefer to put a FIXME on those 2 and punt until we have more visibility.

Comment on attachment 176530 [details] Change 2: Renamed the new structure to TrackSize. Attachment 176530 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15036221 New failing tests: fast/css-grid-layout/should-not-collapse-anonymous-blocks.html fast/css-grid-layout/containing-block-grids.html

Created attachment 176603 [details] Patch for landing

Comment on attachment 176603 [details] Patch for landing Rejecting attachment 176603 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: pe Source/WebCore/rendering/style/RenderStyle.h:1632: error: template argument 1 is invalid make: *** [out/Release/obj.target/webcore_dom/Source/WebCore/dom/DocumentMarkerController.o] Error 1 make: *** Waiting for unfinished jobs.... make: *** [out/Release/obj.target/webcore_dom/Source/WebCore/dom/CharacterData.o] Error 1 make: *** [out/Release/obj.target/webcore_dom/Source/WebCore/dom/ContainerNode.o] Error 1 make: *** [out/Release/obj.target/webcore_dom/Source/WebCore/dom/Document.o] Error 1 Full output: http://queues.webkit.org/results/15044065

Created attachment 176625 [details] Patch for landing 2.

Comment on attachment 176625 [details] Patch for landing 2. Clearing flags on attachment: 176625 Committed r136150: <http://trac.webkit.org/changeset/136150>

All reviewed patches have been landed. Closing bug.

Is it intentional that the .gypi references GridTrackSizes.h but GridTrackSize.h was added instead?

(In reply to comment #20) > Is it intentional that the .gypi references GridTrackSizes.h but GridTrackSize.h was added instead? It wasn't and it has been fixed in http://trac.webkit.org/changeset/136166 as it was making Chromium buildbots unhappy. I don't know why it didn't complain locally or on the EWS though.