Bug 165317

Summary: Implement the updated port/area-based Scroll Snap Module Level 1 Spec
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: CSSAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, bfulgham, buildbot, commit-queue, dino, jonlee, simon.fraser, thorton, webkit-bug-importer
Priority: P2 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

Description Wenson Hsieh 2016-12-02 11:15:01 PST
See http://dev.w3.org/csswg/css-snappoints
Comment 1 Wenson Hsieh 2016-12-04 18:52:47 PST
<rdar://problem/29490956>
Comment 2 Wenson Hsieh 2016-12-04 20:03:00 PST
Created attachment 296117 [details]
WIP (ChangeLog not finished yet)
Comment 3 WebKit Commit Bot 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.
Comment 4 Build Bot 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
Comment 5 Build Bot 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
Comment 6 Build Bot 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
Comment 7 Build Bot 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
Comment 8 Build Bot 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
Comment 9 Build Bot 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
Comment 10 Build Bot 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
Comment 11 Build Bot 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
Comment 12 Wenson Hsieh 2016-12-05 10:54:48 PST
Created attachment 296160 [details]
First pass
Comment 13 Wenson Hsieh 2016-12-06 19:36:05 PST
Created attachment 296367 [details]
Patch
Comment 14 Dean Jackson 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?
Comment 15 Wenson Hsieh 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.
Comment 16 Wenson Hsieh 2016-12-14 08:08:28 PST
Created attachment 297088 [details]
Patch for landing
Comment 17 WebKit Commit Bot 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
Comment 18 Wenson Hsieh 2016-12-14 08:23:39 PST
Created attachment 297090 [details]
Patch for landing
Comment 19 Wenson Hsieh 2016-12-19 18:23:21 PST
Created attachment 297497 [details]
Patch for landing
Comment 20 Wenson Hsieh 2016-12-20 07:21:04 PST
Created attachment 297520 [details]
Patch for landing
Comment 21 WebKit Commit Bot 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>
Comment 22 WebKit Commit Bot 2016-12-20 10:45:27 PST
All reviewed patches have been landed.  Closing bug.