Bug 115362

Summary:

[CSS Grid Layout] Implement support for <flex>

Product:

WebKit

Reporter:

Julien Chaffraix <jchaffraix>

Component:

Layout and Rendering

Assignee:

Sergio Villar Senin <svillar>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aestes, allan.jensen, buildbot, commit-queue, darin, dbates, dino, esprehn+autocc, glenn, kling, koivisto, kondapallykalyan, macpherson, menard, rniwa, svillar, syoichi

Priority:

Keywords:

BlinkMergeCandidate

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

60731

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	kling: review+

Julien Chaffraix

Reported 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).

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
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Sergio Villar Senin

Comment 1 2013-09-12 01:17:29 PDT

<flex> support was added to Blink in r149134 , r149480, r149532, r150287 and r156127

Sergio Villar Senin

Comment 2 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.

Sergio Villar Senin

Comment 3 2013-09-16 06:33:03 PDT

Created attachment 211774 [details] Patch

WebKit Commit Bot

Comment 4 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.

Build Bot

Comment 5 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

Build Bot

Comment 6 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

Sergio Villar Senin

Comment 7 2013-09-16 08:24:59 PDT

Created attachment 211785 [details] Patch

WebKit Commit Bot

Comment 8 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.

Build Bot

Comment 9 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

Build Bot

Comment 10 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

Sergio Villar Senin

Comment 11 2013-09-16 08:58:01 PDT

Created attachment 211796 [details] Patch Another build fix

WebKit Commit Bot

Comment 12 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.

Build Bot

Comment 13 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

Sergio Villar Senin

Comment 14 2013-09-16 09:31:09 PDT

Created attachment 211801 [details] Patch

WebKit Commit Bot

Comment 15 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.

Build Bot

Comment 16 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

Build Bot

Comment 17 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

Sergio Villar Senin

Comment 18 2013-09-16 10:11:52 PDT

Created attachment 211807 [details] Patch

WebKit Commit Bot

Comment 19 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.

Build Bot

Comment 20 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

Sergio Villar Senin

Comment 21 2013-09-17 02:59:25 PDT

Created attachment 211878 [details] Patch

WebKit Commit Bot

Comment 22 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.

Build Bot

Comment 23 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

Build Bot

Comment 24 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

Sergio Villar Senin

Comment 25 2013-09-17 11:20:31 PDT

Created attachment 211926 [details] Patch Crossing fingers

WebKit Commit Bot

Comment 26 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.

Sergio Villar Senin

Comment 27 2013-10-10 01:40:21 PDT

The style issue is a false positive. akling, dino mind taking a look at the patch?

Andreas Kling

Comment 28 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

Sergio Villar Senin

Comment 29 2013-10-14 03:35:16 PDT

Committed r157393: <http://trac.webkit.org/changeset/157393>

Darin Adler

Comment 30 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?

Sergio Villar Senin

Comment 31 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"?

Darin Adler

Comment 32 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.

Note You need to log in before you can comment on or make changes to this bug.