Bug 103799

Created attachment 177283 [details] Proposed change.

Comment on attachment 177283 [details] Proposed change. View in context: https://bugs.webkit.org/attachment.cgi?id=177283&action=review Please add some more test cases before landing. > Source/WebCore/css/CSSFunctionValue.h:54 > + explicit CSSFunctionValue(String, PassRefPtr<CSSValueList>); Nit: No explicit. > Source/WebCore/css/CSSParser.cpp:4538 > + // We only accept the following grammar: minmax( <track-breadth> , <track-breadth> ) It's not clear to me what 'we' refers to here. I would probably say, "The spec defines the following grammar:". > Source/WebCore/rendering/style/GridTrackSize.h:64 > + m_minTrackBreadth = length; > + m_maxTrackBreadth = length; I see, you set both min and max so you can simplify operator==? I would make operator== more complex instead. > LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt:18 > +PASS getComputedStyle(gridWithMinMax, '').getPropertyValue('-webkit-grid-columns') is 'minmax(10%, 15px)' > +PASS getComputedStyle(gridWithMinMax, '').getPropertyValue('-webkit-grid-rows') is 'minmax(20px, 50%)' Can you add some test cases with 'auto', '-webkit-min-content' and '-webkit-calc' as arguments to minmax?.

Comment on attachment 177283 [details] Proposed change. View in context: https://bugs.webkit.org/attachment.cgi?id=177283&action=review >> Source/WebCore/css/CSSFunctionValue.h:54 >> + explicit CSSFunctionValue(String, PassRefPtr<CSSValueList>); > > Nit: No explicit. I would prefer to keep it but it's a personal preference. >> Source/WebCore/css/CSSParser.cpp:4538 >> + // We only accept the following grammar: minmax( <track-breadth> , <track-breadth> ) > > It's not clear to me what 'we' refers to here. I would probably say, "The spec defines the following grammar:". Sounds very good. >> Source/WebCore/rendering/style/GridTrackSize.h:64 >> + m_maxTrackBreadth = length; > > I see, you set both min and max so you can simplify operator==? I would make operator== more complex instead. It's not only for operator==. The track sizing algorithm in the specification (section 9.2) doesn't special case the single length case but always checks both min and max track sizing functions. We could special case it for performance but for now, I am taking the approach of following the algorithm to the letter. >> LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt:18 >> +PASS getComputedStyle(gridWithMinMax, '').getPropertyValue('-webkit-grid-rows') is 'minmax(20px, 50%)' > > Can you add some test cases with 'auto', '-webkit-min-content' and '-webkit-calc' as arguments to minmax?. More testing is always better! However there are 2 issues here: * -webkit-min-content is not allowed yet (we don't handle it in StyleResolver and probably also CSSParser). * -webkit-calc is something we can't get back from getComputedStyle properly. See bug 103761 and the discussion in bug 103335.

Created attachment 177566 [details] Rebaselined patch with extra testing, fixed a tiny coding style violation (no else after return).

Comment on attachment 177566 [details] Rebaselined patch with extra testing, fixed a tiny coding style violation (no else after return). View in context: https://bugs.webkit.org/attachment.cgi?id=177566&action=review > Source/WebCore/rendering/RenderGrid.cpp:147 > + // FIXME: we stil need to support calc() here (bug 103761) Nit: Add a period to the end of this sentence and capitalize the first letter.

Committed r136588: <http://trac.webkit.org/changeset/136588>