Bug 115362 - [CSS Grid Layout] Implement support for <flex>
Summary: [CSS Grid Layout] Implement support for <flex>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords: BlinkMergeCandidate
Depends on:
Blocks: 60731
  Show dependency treegraph
 
Reported: 2013-04-29 09:26 PDT by Julien Chaffraix
Modified: 2013-10-14 10:39 PDT (History)
17 users (show)

See Also:


Attachments
Patch (82.62 KB, patch)
2013-09-16 06:33 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (89.83 KB, patch)
2013-09-16 08:24 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (89.83 KB, patch)
2013-09-16 08:58 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (89.83 KB, patch)
2013-09-16 09:31 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (89.83 KB, patch)
2013-09-16 10:11 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (89.38 KB, patch)
2013-09-17 02:59 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (89.42 KB, patch)
2013-09-17 11:20 PDT, Sergio Villar Senin
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2013-04-29 09:26:22 PDT
<flex> is a non-negative dimension with the unit "fr". See http://dev.w3.org/csswg/css-grid/#fr-unit for the details.

In order to support <flex>, we need to add a layer of indirection to the code that assumes that grid-{rows|columns} are Length. This will prevent spreading the knowledge of <flex> all over the code for no good reason (it's grid specific).
Comment 1 Sergio Villar Senin 2013-09-12 01:17:29 PDT
<flex> support was added to Blink in r149134 , r149480, r149532, r150287 and r156127
Comment 2 Sergio Villar Senin 2013-09-13 07:18:39 PDT
I'll also integrate the changes for this bug I found https://codereview.chromium.org/23537037/, once it gets reviewed in Blink.
Comment 3 Sergio Villar Senin 2013-09-16 06:33:03 PDT
Created attachment 211774 [details]
Patch
Comment 4 WebKit Commit Bot 2013-09-16 06:34:25 PDT
Attachment 211774 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-columns-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-columns.html', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-rows-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-rows.html', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-columns-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-columns.html', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-rows-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-rows.html', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-multiple-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-multiple.html', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set.html', u'LayoutTests/fast/css-grid-layout/grid-dynamic-updates-relayout-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-dynamic-updates-relayout.html', u'LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set-multiple.js', u'LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set.js', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSGrammar.y.in', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParserValues.cpp', u'Source/WebCore/css/CSSParserValues.h', u'Source/WebCore/css/CSSPrimitiveValue.cpp', u'Source/WebCore/css/CSSPrimitiveValue.h', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/rendering/RenderGrid.cpp', u'Source/WebCore/rendering/RenderGrid.h', u'Source/WebCore/rendering/style/GridLength.h', u'Source/WebCore/rendering/style/GridTrackSize.h']" exit_code: 1
Source/WebCore/css/CSSPrimitiveValue.h:104:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Build Bot 2013-09-16 06:58:44 PDT
Comment on attachment 211774 [details]
Patch

Attachment 211774 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1914429
Comment 6 Build Bot 2013-09-16 07:26:01 PDT
Comment on attachment 211774 [details]
Patch

Attachment 211774 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1816139
Comment 7 Sergio Villar Senin 2013-09-16 08:24:59 PDT
Created attachment 211785 [details]
Patch
Comment 8 WebKit Commit Bot 2013-09-16 08:28:02 PDT
Attachment 211785 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-columns-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-columns.html', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-rows-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-rows.html', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-columns-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-columns.html', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-rows-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-rows.html', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-multiple-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-multiple.html', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set.html', u'LayoutTests/fast/css-grid-layout/grid-dynamic-updates-relayout-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-dynamic-updates-relayout.html', u'LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set-multiple.js', u'LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set.js', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSGrammar.y.in', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParserValues.cpp', u'Source/WebCore/css/CSSParserValues.h', u'Source/WebCore/css/CSSPrimitiveValue.cpp', u'Source/WebCore/css/CSSPrimitiveValue.h', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/rendering/RenderGrid.cpp', u'Source/WebCore/rendering/RenderGrid.h', u'Source/WebCore/rendering/style/GridLength.h', u'Source/WebCore/rendering/style/GridTrackSize.h']" exit_code: 1
Source/WebCore/css/CSSPrimitiveValue.h:104:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Build Bot 2013-09-16 08:49:22 PDT
Comment on attachment 211785 [details]
Patch

Attachment 211785 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1883303
Comment 10 Build Bot 2013-09-16 08:50:38 PDT
Comment on attachment 211785 [details]
Patch

Attachment 211785 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1878785
Comment 11 Sergio Villar Senin 2013-09-16 08:58:01 PDT
Created attachment 211796 [details]
Patch

Another build fix
Comment 12 WebKit Commit Bot 2013-09-16 09:03:57 PDT
Attachment 211796 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-columns-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-columns.html', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-rows-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-rows.html', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-columns-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-columns.html', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-rows-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-rows.html', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-multiple-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-multiple.html', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set.html', u'LayoutTests/fast/css-grid-layout/grid-dynamic-updates-relayout-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-dynamic-updates-relayout.html', u'LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set-multiple.js', u'LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set.js', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSGrammar.y.in', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParserValues.cpp', u'Source/WebCore/css/CSSParserValues.h', u'Source/WebCore/css/CSSPrimitiveValue.cpp', u'Source/WebCore/css/CSSPrimitiveValue.h', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/rendering/RenderGrid.cpp', u'Source/WebCore/rendering/RenderGrid.h', u'Source/WebCore/rendering/style/GridLength.h', u'Source/WebCore/rendering/style/GridTrackSize.h']" exit_code: 1
Source/WebCore/css/CSSPrimitiveValue.h:104:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Build Bot 2013-09-16 09:17:30 PDT
Comment on attachment 211796 [details]
Patch

Attachment 211796 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1810261
Comment 14 Sergio Villar Senin 2013-09-16 09:31:09 PDT
Created attachment 211801 [details]
Patch
Comment 15 WebKit Commit Bot 2013-09-16 09:33:14 PDT
Attachment 211801 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-columns-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-columns.html', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-rows-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-rows.html', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-columns-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-columns.html', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-rows-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-rows.html', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-multiple-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-multiple.html', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set.html', u'LayoutTests/fast/css-grid-layout/grid-dynamic-updates-relayout-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-dynamic-updates-relayout.html', u'LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set-multiple.js', u'LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set.js', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSGrammar.y.in', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParserValues.cpp', u'Source/WebCore/css/CSSParserValues.h', u'Source/WebCore/css/CSSPrimitiveValue.cpp', u'Source/WebCore/css/CSSPrimitiveValue.h', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/rendering/RenderGrid.cpp', u'Source/WebCore/rendering/RenderGrid.h', u'Source/WebCore/rendering/style/GridLength.h', u'Source/WebCore/rendering/style/GridTrackSize.h']" exit_code: 1
Source/WebCore/css/CSSPrimitiveValue.h:104:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Build Bot 2013-09-16 09:55:29 PDT
Comment on attachment 211801 [details]
Patch

Attachment 211801 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1812102
Comment 17 Build Bot 2013-09-16 09:55:36 PDT
Comment on attachment 211801 [details]
Patch

Attachment 211801 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1839155
Comment 18 Sergio Villar Senin 2013-09-16 10:11:52 PDT
Created attachment 211807 [details]
Patch
Comment 19 WebKit Commit Bot 2013-09-16 10:14:34 PDT
Attachment 211807 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-columns-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-columns.html', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-rows-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-rows.html', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-columns-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-columns.html', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-rows-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-rows.html', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-multiple-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-multiple.html', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set.html', u'LayoutTests/fast/css-grid-layout/grid-dynamic-updates-relayout-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-dynamic-updates-relayout.html', u'LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set-multiple.js', u'LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set.js', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSGrammar.y.in', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParserValues.cpp', u'Source/WebCore/css/CSSParserValues.h', u'Source/WebCore/css/CSSPrimitiveValue.cpp', u'Source/WebCore/css/CSSPrimitiveValue.h', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/rendering/RenderGrid.cpp', u'Source/WebCore/rendering/RenderGrid.h', u'Source/WebCore/rendering/style/GridLength.h', u'Source/WebCore/rendering/style/GridTrackSize.h']" exit_code: 1
Source/WebCore/css/CSSPrimitiveValue.h:104:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Build Bot 2013-09-16 10:37:46 PDT
Comment on attachment 211807 [details]
Patch

Attachment 211807 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1824067
Comment 21 Sergio Villar Senin 2013-09-17 02:59:25 PDT
Created attachment 211878 [details]
Patch
Comment 22 WebKit Commit Bot 2013-09-17 03:01:18 PDT
Attachment 211878 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-columns-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-columns.html', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-rows-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-rows.html', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-columns-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-columns.html', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-rows-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-rows.html', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-multiple-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-multiple.html', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set.html', u'LayoutTests/fast/css-grid-layout/grid-dynamic-updates-relayout-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-dynamic-updates-relayout.html', u'LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set-multiple.js', u'LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set.js', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSGrammar.y.in', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParserValues.cpp', u'Source/WebCore/css/CSSParserValues.h', u'Source/WebCore/css/CSSPrimitiveValue.cpp', u'Source/WebCore/css/CSSPrimitiveValue.h', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/rendering/RenderGrid.cpp', u'Source/WebCore/rendering/RenderGrid.h', u'Source/WebCore/rendering/style/GridLength.h', u'Source/WebCore/rendering/style/GridTrackSize.h']" exit_code: 1
Source/WebCore/css/CSSPrimitiveValue.h:104:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Build Bot 2013-09-17 03:28:41 PDT
Comment on attachment 211878 [details]
Patch

Attachment 211878 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1869364
Comment 24 Build Bot 2013-09-17 03:52:36 PDT
Comment on attachment 211878 [details]
Patch

Attachment 211878 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1812266
Comment 25 Sergio Villar Senin 2013-09-17 11:20:31 PDT
Created attachment 211926 [details]
Patch

Crossing fingers
Comment 26 WebKit Commit Bot 2013-09-17 11:22:03 PDT
Attachment 211926 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-columns-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-columns.html', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-rows-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-rows.html', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-columns-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-columns.html', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-rows-expected.txt', u'LayoutTests/fast/css-grid-layout/flex-content-resolution-rows.html', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-multiple-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-multiple.html', u'LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set.html', u'LayoutTests/fast/css-grid-layout/grid-dynamic-updates-relayout-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-dynamic-updates-relayout.html', u'LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set-multiple.js', u'LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set.js', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSGrammar.y.in', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParserValues.cpp', u'Source/WebCore/css/CSSParserValues.h', u'Source/WebCore/css/CSSPrimitiveValue.cpp', u'Source/WebCore/css/CSSPrimitiveValue.h', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/rendering/RenderGrid.cpp', u'Source/WebCore/rendering/RenderGrid.h', u'Source/WebCore/rendering/style/GridLength.h', u'Source/WebCore/rendering/style/GridTrackSize.h']" exit_code: 1
Source/WebCore/css/CSSPrimitiveValue.h:104:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 Sergio Villar Senin 2013-10-10 01:40:21 PDT
The style issue is a false positive.

akling, dino mind taking a look at the patch?
Comment 28 Andreas Kling 2013-10-14 02:02:48 PDT
Comment on attachment 211926 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=211926&action=review

r=me

> Source/WebCore/rendering/RenderGrid.cpp:78
> +    // Required by std::sort.
> +    GridTrackForNormalization operator=(const GridTrackForNormalization& o)

Huh. I thought the compiler would generate this.

> Source/WebCore/rendering/RenderGrid.cpp:392
> +    std::sort(tracksForNormalization.begin(), tracksForNormalization.end(), sortByGridNormalizedFlexValue);

I would use a lambda instead of a separate function here:

std::sort(tracksForNormalization.begin(), tracksForNormalization.end(), [](const GridTrackForNormalization& a, const GridTrackForNormalization& b) {
    return a.m_normalizedFlexValue < b.m_normalizedFlexValue;
});

> Source/WebCore/rendering/style/GridLength.h:72
> +        return m_length == o.m_length && m_flex == o.m_flex && m_type == o.m_type;

It would be better to do these comparisons in "cheapness" order:
m_type -> m_flex -> m_length
Comment 29 Sergio Villar Senin 2013-10-14 03:35:16 PDT
Committed r157393: <http://trac.webkit.org/changeset/157393>
Comment 30 Darin Adler 2013-10-14 10:14:21 PDT
Comment on attachment 211926 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=211926&action=review

>> Source/WebCore/rendering/RenderGrid.cpp:78
>> +    GridTrackForNormalization operator=(const GridTrackForNormalization& o)
> 
> Huh. I thought the compiler would generate this.

Yes, the compiler should generate this. Please take this out or add a comment in the bug send mail to webkit-dev explaining why it doesn’t work.

My guess is that an earlier version of this patch tried to use a reference for m_track, and that's why std::sort didn't work.

> Source/WebCore/rendering/RenderGrid.cpp:386
> +    Vector<GridTrackForNormalization> tracksForNormalization;
> +    for (size_t i = 0; i < tracks.size(); ++i) {
> +        const GridTrackSize& trackSize = gridTrackSize(direction, i);
> +        if (!trackSize.maxTrackBreadth().isFlex())
> +            continue;
> +
> +        tracksForNormalization.append(GridTrackForNormalization(tracks[i], trackSize.maxTrackBreadth().flex()));
> +    }

Should we be using inline capacity or reserveCapacity for better speed?

>> Source/WebCore/rendering/RenderGrid.cpp:392
>> +    std::sort(tracksForNormalization.begin(), tracksForNormalization.end(), sortByGridNormalizedFlexValue);
> 
> I would use a lambda instead of a separate function here:
> 
> std::sort(tracksForNormalization.begin(), tracksForNormalization.end(), [](const GridTrackForNormalization& a, const GridTrackForNormalization& b) {
>     return a.m_normalizedFlexValue < b.m_normalizedFlexValue;
> });

Is an unstable sort OK here? It means that tracks could sort in an unpredictable order. Do we have test cases that cover cases where the normalized flex values are equal?
Comment 31 Sergio Villar Senin 2013-10-14 10:26:18 PDT
(In reply to comment #30)
> (From update of attachment 211926 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211926&action=review
> 
> >> Source/WebCore/rendering/RenderGrid.cpp:78
> >> +    GridTrackForNormalization operator=(const GridTrackForNormalization& o)
> > 
> > Huh. I thought the compiler would generate this.
> 
> Yes, the compiler should generate this. Please take this out or add a comment in the bug send mail to webkit-dev explaining why it doesn’t work.
> 
> My guess is that an earlier version of this patch tried to use a reference for m_track, and that's why std::sort didn't work.

It was indeed generated by the compiler, I removed it in the patch that landed.

> > Source/WebCore/rendering/RenderGrid.cpp:386
> > +    Vector<GridTrackForNormalization> tracksForNormalization;
> > +    for (size_t i = 0; i < tracks.size(); ++i) {
> > +        const GridTrackSize& trackSize = gridTrackSize(direction, i);
> > +        if (!trackSize.maxTrackBreadth().isFlex())
> > +            continue;
> > +
> > +        tracksForNormalization.append(GridTrackForNormalization(tracks[i], trackSize.maxTrackBreadth().flex()));
> > +    }
> 
> Should we be using inline capacity or reserveCapacity for better speed?

Using tracks.size()? Yeah I guess we could, I'm normally skeptic about those microoptimizations without data to back them. I guess we still don't have enough users to know whether or not web authors will frequently use flexible sizes or not. Maybe a FIXME there was needed in order not to forget about it.
 
> >> Source/WebCore/rendering/RenderGrid.cpp:392
> >> +    std::sort(tracksForNormalization.begin(), tracksForNormalization.end(), sortByGridNormalizedFlexValue);
> > 
> > I would use a lambda instead of a separate function here:
> > 
> > std::sort(tracksForNormalization.begin(), tracksForNormalization.end(), [](const GridTrackForNormalization& a, const GridTrackForNormalization& b) {
> >     return a.m_normalizedFlexValue < b.m_normalizedFlexValue;
> > });
> 
> Is an unstable sort OK here? It means that tracks could sort in an unpredictable order. Do we have test cases that cover cases where the normalized flex values are equal?

Why do you say "unstable sort"?
Comment 32 Darin Adler 2013-10-14 10:39:00 PDT
Comment on attachment 211926 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=211926&action=review

>>>> Source/WebCore/rendering/RenderGrid.cpp:392
>>>> +    std::sort(tracksForNormalization.begin(), tracksForNormalization.end(), sortByGridNormalizedFlexValue);
>>> 
>>> I would use a lambda instead of a separate function here:
>>> 
>>> std::sort(tracksForNormalization.begin(), tracksForNormalization.end(), [](const GridTrackForNormalization& a, const GridTrackForNormalization& b) {
>>>     return a.m_normalizedFlexValue < b.m_normalizedFlexValue;
>>> });
>> 
>> Is an unstable sort OK here? It means that tracks could sort in an unpredictable order. Do we have test cases that cover cases where the normalized flex values are equal?
> 
> Why do you say "unstable sort"?

The std::sort function offers an unstable sort. There are also stable sorts in the standard library, such as std::stable_sort. An unstable sort does not guarantee the ordering of equal values after sorting, whereas a stable sort guarantees that if there are equal values their relative ordering will be preserved.

Whether a sort is stable or not does not matter if there are no equal elements in the collection to be sorted. If there are equal elements, then you have to consider whether the ordering of those elements matters, or if it’s OK to perturb their ordering arbitrarily as a side effect of the sorting.