WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
115362
[CSS Grid Layout] Implement support for <flex>
https://bugs.webkit.org/show_bug.cgi?id=115362
Summary
[CSS Grid Layout] Implement support for <flex>
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.
Top of Page
Format For Printing
XML
Clone This Bug