Bug 165317

Summary:

Implement the updated port/area-based Scroll Snap Module Level 1 Spec

Product:

WebKit

Reporter:

Wenson Hsieh <wenson_hsieh>

Component:

CSS

Assignee:

Wenson Hsieh <wenson_hsieh>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bdakin, bfulgham, buildbot, commit-queue, dino, jonlee, simon.fraser, thorton, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

134283

Attachments:

Description	Flags
WIP (ChangeLog not finished yet)	none
Archive of layout-test-results from ews101 for mac-yosemite	none
Archive of layout-test-results from ews105 for mac-yosemite-wk2	none
Archive of layout-test-results from ews116 for mac-yosemite	none
Archive of layout-test-results from ews124 for ios-simulator-wk2	none
First pass	none
Patch	none
Patch for landing	commit-queue: commit-queue-
Patch for landing	wenson_hsieh: commit-queue+
Patch for landing	wenson_hsieh: commit-queue+
Patch for landing	none

Wenson Hsieh

Reported 2016-12-02 11:15:01 PST

See http://dev.w3.org/csswg/css-snappoints

Attachments
WIP (ChangeLog not finished yet) (440.71 KB, patch) 2016-12-04 20:03 PST, Wenson Hsieh	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews101 for mac-yosemite (1.09 MB, application/zip) 2016-12-04 21:15 PST, Build Bot	no flags	Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1017.21 KB, application/zip) 2016-12-04 21:18 PST, Build Bot	no flags	Details
Archive of layout-test-results from ews116 for mac-yosemite (1.70 MB, application/zip) 2016-12-04 21:22 PST, Build Bot	no flags	Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (14.70 MB, application/zip) 2016-12-04 21:31 PST, Build Bot	no flags	Details
First pass (412.66 KB, patch) 2016-12-05 10:54 PST, Wenson Hsieh	no flags	Details Formatted Diff Diff
Patch (414.08 KB, patch) 2016-12-06 19:36 PST, Wenson Hsieh	no flags	Details Formatted Diff Diff
Patch for landing (404.70 KB, patch) 2016-12-14 08:08 PST, Wenson Hsieh	commit-queue: commit-queue-	Details Formatted Diff Diff
Patch for landing (404.63 KB, patch) 2016-12-14 08:23 PST, Wenson Hsieh	wenson_hsieh: commit-queue+	Details Formatted Diff Diff
Patch for landing (403.98 KB, patch) 2016-12-19 18:23 PST, Wenson Hsieh	wenson_hsieh: commit-queue+	Details Formatted Diff Diff
Patch for landing (403.98 KB, patch) 2016-12-20 07:21 PST, Wenson Hsieh	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Wenson Hsieh

Comment 1 2016-12-04 18:52:47 PST

<rdar://problem/29490956>

Wenson Hsieh

Comment 2 2016-12-04 20:03:00 PST

Created attachment 296117 [details] WIP (ChangeLog not finished yet)

WebKit Commit Bot

Comment 3 2016-12-04 20:05:20 PST

Attachment 296117 [details] did not pass style-queue: ERROR: Source/WebCore/rendering/style/RenderStyleConstants.h:691: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/rendering/style/RenderStyleConstants.h:692: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 2 in 78 files If any of these errors are false positives, please file a bug against check-webkit-style.

Build Bot

Comment 4 2016-12-04 21:15:09 PST

Comment on attachment 296117 [details] WIP (ChangeLog not finished yet) Attachment 296117 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2624326 New failing tests: css3/scroll-snap/scroll-snap-children-with-transforms.html css3/scroll-snap/scroll-snap-with-scroll-padding.html

Build Bot

Comment 5 2016-12-04 21:15:12 PST

Created attachment 296121 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5

Build Bot

Comment 6 2016-12-04 21:18:47 PST

Comment on attachment 296117 [details] WIP (ChangeLog not finished yet) Attachment 296117 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2624349 New failing tests: css3/scroll-snap/scroll-snap-children-with-transforms.html css3/scroll-snap/scroll-snap-with-scroll-padding.html

Build Bot

Comment 7 2016-12-04 21:18:50 PST

Created attachment 296122 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Build Bot

Comment 8 2016-12-04 21:22:36 PST

Comment on attachment 296117 [details] WIP (ChangeLog not finished yet) Attachment 296117 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2624350 New failing tests: css3/scroll-snap/scroll-snap-children-with-transforms.html css3/scroll-snap/scroll-snap-children-with-scroll-snap-margin.html css3/scroll-snap/scroll-snap-with-scroll-padding.html

Build Bot

Comment 9 2016-12-04 21:22:38 PST

Created attachment 296123 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5

Build Bot

Comment 10 2016-12-04 21:31:40 PST

Comment on attachment 296117 [details] WIP (ChangeLog not finished yet) Attachment 296117 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2624363 New failing tests: css3/scroll-snap/scroll-snap-children-with-transforms.html css3/scroll-snap/scroll-snap-with-scroll-padding.html css3/scroll-snap/scroll-snap-children-with-scroll-snap-margin.html css3/scroll-snap/scroll-snap-positions.html

Build Bot

Comment 11 2016-12-04 21:31:43 PST

Created attachment 296124 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Wenson Hsieh

Comment 12 2016-12-05 10:54:48 PST

Created attachment 296160 [details] First pass

Wenson Hsieh

Comment 13 2016-12-06 19:36:05 PST

Created attachment 296367 [details] Patch

Dean Jackson

Comment 14 2016-12-13 14:25:15 PST

Comment on attachment 296367 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=296367&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1219 > + return WTFMove(value); I don't think you need to WTFMove here, assuming value is a Ref<CSSValue>. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1227 > + return WTFMove(value); Ditto. > Source/WebCore/css/CSSPrimitiveValueMappings.h:5374 > -template<> inline CSSPrimitiveValue::CSSPrimitiveValue(ScrollSnapType e) > +template<> inline CSSPrimitiveValue::CSSPrimitiveValue(ScrollSnapStrictness e) > : CSSValue(PrimitiveClass) > { > m_primitiveUnitType = CSS_VALUE_ID; > switch (e) { Use a nicer variable name. > Source/WebCore/css/CSSPrimitiveValueMappings.h:5407 > +template<> inline CSSPrimitiveValue::CSSPrimitiveValue(ScrollSnapAxis e) > + : CSSValue(PrimitiveClass) > +{ > + m_primitiveUnitType = CSS_VALUE_ID; > + switch (e) { Use a nicer variable name. > Source/WebCore/css/CSSPrimitiveValueMappings.h:5450 > +template<> inline CSSPrimitiveValue::CSSPrimitiveValue(ScrollSnapAxisAlignType e) > + : CSSValue(PrimitiveClass) > +{ > + m_primitiveUnitType = CSS_VALUE_ID; > + switch (e) { Ditto. > Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:33 > +bool operator==(const ScrollSnapType& a, const ScrollSnapType& b) Any reason why this isn't inline in the .h? > Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:38 > +bool operator==(const ScrollSnapAlign& a, const ScrollSnapAlign& b) Ditto? > Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:54 > + : scrollPadding(0, 0, 0, 0) Can you do this in the class definition?

Wenson Hsieh

Comment 15 2016-12-14 07:36:51 PST

Comment on attachment 296367 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=296367&action=review >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1219 >> + return WTFMove(value); > > I don't think you need to WTFMove here, assuming value is a Ref<CSSValue>. Good point -- fixed. >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1227 >> + return WTFMove(value); > > Ditto. Fixed! >> Source/WebCore/css/CSSPrimitiveValueMappings.h:5374 >> switch (e) { > > Use a nicer variable name. Done! >> Source/WebCore/css/CSSPrimitiveValueMappings.h:5407 >> + switch (e) { > > Use a nicer variable name. Done! >> Source/WebCore/css/CSSPrimitiveValueMappings.h:5450 >> + switch (e) { > > Ditto. Done! >> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:33 >> +bool operator==(const ScrollSnapType& a, const ScrollSnapType& b) > > Any reason why this isn't inline in the .h? No reason! Moved to the .h (and made it inline) >> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:38 >> +bool operator==(const ScrollSnapAlign& a, const ScrollSnapAlign& b) > > Ditto? Done. >> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:54 >> + : scrollPadding(0, 0, 0, 0) > > Can you do this in the class definition? Yes -- moved to the class definition.

Wenson Hsieh

Comment 16 2016-12-14 08:08:28 PST

Created attachment 297088 [details] Patch for landing

WebKit Commit Bot

Comment 17 2016-12-14 08:10:27 PST

Comment on attachment 297088 [details] Patch for landing Rejecting attachment 297088 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 297088, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: tTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-overflow.html patching file LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-padding.html patching file LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-rotated.html patching file LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-scrolling-jumps-to-top.html Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.webkit.org/results/2721159

Wenson Hsieh

Comment 18 2016-12-14 08:23:39 PST

Created attachment 297090 [details] Patch for landing

Wenson Hsieh

Comment 19 2016-12-19 18:23:21 PST

Created attachment 297497 [details] Patch for landing

Wenson Hsieh

Comment 20 2016-12-20 07:21:04 PST

Created attachment 297520 [details] Patch for landing

WebKit Commit Bot

Comment 21 2016-12-20 10:45:22 PST

Comment on attachment 297520 [details] Patch for landing Clearing flags on attachment: 297520 Committed r210024: <http://trac.webkit.org/changeset/210024>

WebKit Commit Bot

Comment 22 2016-12-20 10:45:27 PST

All reviewed patches have been landed. Closing bug.

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