Update the snap points css properties

This patch replaces the snap points css properties for the old spec and add the
properties for the new spec (https://www.w3.org/TR/css-scroll-snap-1/). We also
remove the unit_tests that rely on the old spec.

Properties that are removed:
scroll-snap-points-x, scroll-snap-points-y,
scroll-snap-destination, scroll-snap-coordinate

Properties that are updated:
scroll-snap-type

Properties that are added:
scroll-snap-align, scroll-snap-stop,
scroll-padding, scroll-padding-top, scroll-padding-right,
scroll-padding-bottom,scroll-padding-left, scroll-padding-inline,
scroll-padding-inline-start, scroll-padding-inline-end,
scroll-padding-block, scroll-padding-block-start, scroll-padding-block-end,
scroll-snap-margin, scroll-snap-margin-top, scroll-snap-margin-right,
scroll-snap-margin-bottom, scroll-snap-margin-left, scroll-snap-margin-inline,
scroll-snap-margin-inline-start, scroll-snap-margin-inline-end,
scroll-snap-margin-block, scroll-snap-margin-block-start,
scroll-snap-margin-block-end

BUG=724912
Review-Url: https://codereview.chromium.org/2932593004
Cr-Commit-Position: refs/heads/master@{#479738}
Committed: https://chromium.googlesource.com/chromium/src/+/8a809d14cccfd32e74cacbe33e7249eaa4fea4ad

[sunyunjia, 2017-06-08 20:42:27]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 20:43:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-08 20:47:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-08 20:47:48]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunyunjia, 2017-06-08 22:44:21]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 22:45:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 00:01:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-09 00:01:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunyunjia, 2017-06-09 18:35:08]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 18:35:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 18:54:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-09 18:54:19]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[sunyunjia, 2017-06-09 19:14:04]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 19:14:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-06-09 20:01:11]

Patchset #1 (id:1) has been deleted

[sunyunjia, 2017-06-09 20:01:25]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-06-09 20:23:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-09 20:23:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunyunjia, 2017-06-09 20:44:28]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[sunyunjia, 2017-06-09 20:44:37]

Patchset #1 (id:40001) has been deleted

[sunyunjia, 2017-06-09 20:44:46]

Patchset #1 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-06-09 20:44:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-06-09 20:45:02]

Patchset #1 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-06-09 21:53:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-09 21:53:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunyunjia, 2017-06-09 23:09:02]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 23:09:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-10 00:39:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-10 00:39:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunyunjia, 2017-06-10 17:08:52]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-10 17:08:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-10 18:41:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-10 18:41:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunyunjia, 2017-06-10 18:48:44]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-10 18:48:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-10 20:16:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-10 20:16:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunyunjia, 2017-06-10 21:36:23]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-10 21:36:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-10 23:50:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-10 23:50:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunyunjia, 2017-06-11 00:37:02]

sunyunjia@chromium.org changed reviewers:
+ bokan@chromium.org, flackr@chromium.org, majidvp@chromium.org

[sunyunjia, 2017-06-11 00:37:03]

Hi David,

Here is the patch that updates the css properties of snap points. I'm not a css
expert, so please take a look to see if the changes are appropriate. Thanks!

[sunyunjia, 2017-06-11 00:48:33]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-11 00:48:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-11 02:52:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-11 02:52:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunyunjia, 2017-06-12 13:45:17]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 13:45:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 16:16:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-12 16:16:31]

Dry run: This issue passed the CQ dry run.

[bokan, 2017-06-13 01:30:18]

bokan@chromium.org changed reviewers:
+ meade@chromium.org

[bokan, 2017-06-13 01:30:20]

I took a look, seems reasonable to me but I don't know the style and parsing
bits at all so +meade@ to take a look or add someone appropriate.

Rest of the patch looks fine to me - added some comments - but majidvp@ should
have a look too since I'm not well connected to snap points.

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.cpp:177:
CSSPropertyCaretColor, CSSPropertyScrollSnapType,
The comment above says we shouldn't use this for features behind a runtime flag.

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSValueKeywords.json5 (right):

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSValueKeywords.json5:888: //
image-orientation
Bad rebase?

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/UseCounter.cpp:1135: //
chromium/src/tools/metrics/histograms to update the UMA histogram names.
You'll need changes to histograms.xml by running this script.

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp
(right):

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:108:
EXPECT_EQ(0U, SizeOfSnapAreas(SnapContainer()));
Should intermediate become the snap container then? If so, I would also add an
expectation that SizeOfSnapAreas(intermediate) == 1

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:143:
"style='width:2000px; height:2000px;'></div></div>");
Format this string like so:

"<div style='scroll-snap-align: start;'>"
"  <div style='width:2000px; height:2000px;'></div>"
"</div>"

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:163: "    
  <div id='nested-snap-element' style='scroll-snap-align: "
Don't mix indent size, use either 2 or 4 but not both. If you run out of space
on one line, align the params, e.g.:

"  <div id='nested-snap-element'"
"       style='scroll-snap-align: start;'></div>

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:176:
EXPECT_EQ(2U, SizeOfSnapAreas(GetDocument()));
I'd also check that <body> and <html> have 0 for SizeOfSnapAreas

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.h:1528: /*  //
scroll-snap-stop
Why is this commented out?

[sunyunjia, 2017-06-13 15:42:40]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 15:43:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 15:45:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-13 15:45:32]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunyunjia, 2017-06-13 15:53:12]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 15:53:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 17:00:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-13 17:00:04]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunyunjia, 2017-06-13 17:14:58]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 17:15:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-06-13 17:44:59]

PTAL, thanks!

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.cpp:177:
CSSPropertyCaretColor, CSSPropertyScrollSnapType,
On 2017/06/13 01:30:20, bokan wrote:
> The comment above says we shouldn't use this for features behind a runtime
flag.

Done.

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSValueKeywords.json5 (right):

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSValueKeywords.json5:888: //
image-orientation
On 2017/06/13 01:30:20, bokan wrote:
> Bad rebase?

The old scroll-snap-type has this "from-image", which is also used in
image-orientation. As we retire the old property ion whole, we still want to
keep this value for image-orientation.

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/UseCounter.cpp:1135: //
chromium/src/tools/metrics/histograms to update the UMA histogram names.
On 2017/06/13 01:30:20, bokan wrote:
> You'll need changes to histograms.xml by running this script.

Done.

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp
(right):

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:108:
EXPECT_EQ(0U, SizeOfSnapAreas(SnapContainer()));
On 2017/06/13 01:30:20, bokan wrote:
> Should intermediate become the snap container then? If so, I would also add an
> expectation that SizeOfSnapAreas(intermediate) == 1

Done.

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:143:
"style='width:2000px; height:2000px;'></div></div>");
On 2017/06/13 01:30:20, bokan wrote:
> Format this string like so:
> 
> "<div style='scroll-snap-align: start;'>"
> "  <div style='width:2000px; height:2000px;'></div>"
> "</div>"

Done.

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:163: "    
  <div id='nested-snap-element' style='scroll-snap-align: "
On 2017/06/13 01:30:20, bokan wrote:
> Don't mix indent size, use either 2 or 4 but not both. If you run out of space
> on one line, align the params, e.g.:
> 
> "  <div id='nested-snap-element'"
> "       style='scroll-snap-align: start;'></div>

Done.

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:176:
EXPECT_EQ(2U, SizeOfSnapAreas(GetDocument()));
On 2017/06/13 01:30:20, bokan wrote:
> I'd also check that <body> and <html> have 0 for SizeOfSnapAreas

Done.

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.h:1528: /*  //
scroll-snap-stop
On 2017/06/13 01:30:20, bokan wrote:
> Why is this commented out?

As declared in CSSProperties.json5, This property doesn't need an api_class, so
this code will be automatically generated in ComputedStyleBase.h. So no need to
define it here.

[bokan, 2017-06-13 17:55:39]

Thanks, lgtm but as I mentioned, I'm not authoritative in this code so you'll
need stamps from someone in Style and Majid.

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSValueKeywords.json5 (right):

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSValueKeywords.json5:888: //
image-orientation
On 2017/06/13 17:44:59, sunyunjia wrote:
> On 2017/06/13 01:30:20, bokan wrote:
> > Bad rebase?
> 
> The old scroll-snap-type has this "from-image", which is also used in
> image-orientation. As we retire the old property ion whole, we still want to
> keep this value for image-orientation.

Ah, thanks, I missed that and it looked out of place.

[majidvp, 2017-06-13 19:20:54]

Please list the properties your are adding in this patch and
also list properties you are removing.

I looked at the style values from spec side and have
some comments.

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSValueKeywords.json5 (right):

https://codereview.chromium.org/2932593004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSValueKeywords.json5:888: //
image-orientation
On 2017/06/13 17:44:59, sunyunjia wrote:
> On 2017/06/13 01:30:20, bokan wrote:
> > Bad rebase?
> 
> The old scroll-snap-type has this "from-image", which is also used in
> image-orientation. As we retire the old property ion whole, we still want to
> keep this value for image-orientation.

I do not recall seeing "from-image" in CSS Scroll snap spec.
If it is used for another CSS property then we should just leave it
as is.

https://codereview.chromium.org/2932593004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
(right):

https://codereview.chromium.org/2932593004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:1662:
CSSValueList* list = CSSValueList::CreateSpaceSeparated();
Does this need to be a CSSValueList or can we just use CSSValuePair?
There are only two values in the list.

https://codereview.chromium.org/2932593004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:1671: //
Both 2 properties must be specified.
nit: 2 is redundant.

https://codereview.chromium.org/2932593004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleScrollSnapData.h (right):

https://codereview.chromium.org/2932593004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleScrollSnapData.h:41: :
axis(kSnapAxisNone), strictness(kSnapStrictnessProximity) {}
snap type value grammar is "none | [ x | y | block | inline | both ] [ mandatory
| proximity ]?"

So actually axis does not have a "none" value but the snap type itself has one
and  defaults to it".

It seems we should just not have a is_none bool in the struct rather than
using a non standard axis value to indicate this.

https://codereview.chromium.org/2932593004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleScrollSnapData.h:82: ScrollPadding() :
top(kFixed), right(kFixed), bottom(kFixed), left(kFixed) {}
I don't think you should be initializing this to a "Fixed" size unit.

https://codereview.chromium.org/2932593004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleScrollSnapData.h:108: struct
ScrollSnapMargin {
I think you can factor out a common class for both ScrollPadding
and ScrollSnapMargin which is basically four lengths and typedef
these two. This may be usefull with other similiar properties.

Not sure what is a good name for it e.g., QuadLengthValue, MarginValue.

[majidvp, 2017-06-13 19:27:35]

https://codereview.chromium.org/2932593004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp
(right):

https://codereview.chromium.org/2932593004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:39: "     
  scroll-snap-type: both mandatory"
nit: missing semicolon

[commit-bot: I haz the power, 2017-06-13 20:17:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-13 20:17:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunyunjia, 2017-06-13 21:19:24]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 21:20:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 21:24:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-13 21:24:05]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[meade_UTC10, 2017-06-14 01:24:14]

meade@chromium.org changed reviewers:
+ shend@chromium.org

[meade_UTC10, 2017-06-14 01:24:15]

This lgtm from my end, but I'm also adding shend to have a look since he's been
working on ComputedStyle lately.

https://codereview.chromium.org/2932593004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSValueKeywords.json5 (right):

https://codereview.chromium.org/2932593004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSValueKeywords.json5:1120: // both
Is this intentionally still commented out?

[shend, 2017-06-14 01:37:38]

lgtm with a comment.

https://codereview.chromium.org/2932593004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h (right):

https://codereview.chromium.org/2932593004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h:2798: template <>
(*) You may delete this function.

https://codereview.chromium.org/2932593004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h:2811: template <>
(*) You may delete this function.

https://codereview.chromium.org/2932593004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSProperties.json5 (right):

https://codereview.chromium.org/2932593004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSProperties.json5:1869: field_template:
"keyword",
When you add this line, I believe that the enum mappings in
CSSPrimitiveValueMappings.h for this enum gets automatically generated. So you
can delete those lines (marked as (*) in separate comment).

[sunyunjia, 2017-06-14 18:08:11]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 18:08:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-14 18:14:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-14 18:14:01]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[sunyunjia, 2017-06-14 18:19:42]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 18:20:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-06-14 18:26:13]

Description was changed from

==========
This patch replaces the snap points css properties for the old spec and add the
properties for the new spec (https://www.w3.org/TR/css-scroll-snap-1/). We also
remove the unit_tests that rely on the old spec.

BUG=724912
==========

to

==========
This patch replaces the snap points css properties for the old spec and add the
properties for the new spec (https://www.w3.org/TR/css-scroll-snap-1/). We also
remove the unit_tests that rely on the old spec.

Properties that are removed:
scroll-snap-points-x, scroll-snap-points-y,
scroll-snap-destination, scroll-snap-coordinate

Properties that are updated:
scroll-snap-type

Properties that are added:
scroll-snap-align, scroll-snap-stop,
scroll-padding, scroll-padding-top, scroll-padding-right,
scroll-padding-bottom,scroll-padding-left, scroll-padding-inline,
scroll-padding-inline-start, scroll-padding-inline-end,
scroll-padding-block, scroll-padding-block-start, scroll-padding-block-end,
scroll-snap-margin, scroll-snap-margin-top, scroll-snap-margin-right,
scroll-snap-margin-bottom, scroll-snap-margin-left, scroll-snap-margin-inline,
scroll-snap-margin-inline-start, scroll-snap-margin-inline-end,
scroll-snap-margin-block, scroll-snap-margin-block-start,
scroll-snap-margin-block-end

BUG=724912
==========

[sunyunjia, 2017-06-14 18:36:22]

Hi majid@,

I've updated the patch according to the comments. PTAL, thanks!

https://codereview.chromium.org/2932593004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
(right):

https://codereview.chromium.org/2932593004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:1662:
CSSValueList* list = CSSValueList::CreateSpaceSeparated();
On 2017/06/13 19:20:53, majidvp wrote:
> Does this need to be a CSSValueList or can we just use CSSValuePair?
> There are only two values in the list.

Done.

https://codereview.chromium.org/2932593004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:1671: //
Both 2 properties must be specified.
On 2017/06/13 19:20:53, majidvp wrote:
> nit: 2 is redundant. 

Done.

https://codereview.chromium.org/2932593004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp
(right):

https://codereview.chromium.org/2932593004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:39: "     
  scroll-snap-type: both mandatory"
On 2017/06/13 19:27:34, majidvp wrote:
> nit: missing semicolon

Done.

https://codereview.chromium.org/2932593004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleScrollSnapData.h (right):

https://codereview.chromium.org/2932593004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleScrollSnapData.h:41: :
axis(kSnapAxisNone), strictness(kSnapStrictnessProximity) {}
On 2017/06/13 19:20:54, majidvp wrote:
> snap type value grammar is "none | [ x | y | block | inline | both ] [
mandatory
> | proximity ]?"
> 
> So actually axis does not have a "none" value but the snap type itself has one
> and  defaults to it".
> 
> It seems we should just not have a is_none bool in the struct rather than
> using a non standard axis value to indicate this.

Done.

https://codereview.chromium.org/2932593004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleScrollSnapData.h:82: ScrollPadding() :
top(kFixed), right(kFixed), bottom(kFixed), left(kFixed) {}
On 2017/06/13 19:20:53, majidvp wrote:
> I don't think you should be initializing this to a "Fixed" size unit.
> 

Done.

https://codereview.chromium.org/2932593004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleScrollSnapData.h:108: struct
ScrollSnapMargin {
On 2017/06/13 19:20:54, majidvp wrote:
> I think you can factor out a common class for both ScrollPadding
> and ScrollSnapMargin which is basically four lengths and typedef
> these two. This may be usefull with other similiar properties.
> 
> Not sure what is a good name for it e.g., QuadLengthValue, MarginValue.

Done.

https://codereview.chromium.org/2932593004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h (right):

https://codereview.chromium.org/2932593004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h:2798: template <>
On 2017/06/14 01:37:38, shend wrote:
> (*) You may delete this function.

Done.

https://codereview.chromium.org/2932593004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h:2811: template <>
On 2017/06/14 01:37:38, shend wrote:
> (*) You may delete this function.

Done.

https://codereview.chromium.org/2932593004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSProperties.json5 (right):

https://codereview.chromium.org/2932593004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSProperties.json5:1869: field_template:
"keyword",
On 2017/06/14 01:37:38, shend wrote:
> When you add this line, I believe that the enum mappings in
> CSSPrimitiveValueMappings.h for this enum gets automatically generated. So you
> can delete those lines (marked as (*) in separate comment).

Acknowledged.

https://codereview.chromium.org/2932593004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSValueKeywords.json5 (right):

https://codereview.chromium.org/2932593004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSValueKeywords.json5:1120: // both
On 2017/06/14 01:24:15, meade_UTC10 wrote:
> Is this intentionally still commented out?

I'm just putting these keywords here. I guess the rule is to comment out
keywords that have already been defined in this file?

[commit-bot: I haz the power, 2017-06-14 20:49:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-14 20:49:55]

Dry run: This issue passed the CQ dry run.

[meade_UTC10, 2017-06-15 04:07:25]

meade@chromium.org changed reviewers:
+ bugsnash@chromium.org

[meade_UTC10, 2017-06-15 04:07:29]

I realised also that bugsnash@ will be interested in css/parser and co

[Bugs Nash, 2017-06-15 06:14:52]

bugsnash@chromium.org changed reviewers:
- bugsnash@chromium.org

[majidvp, 2017-06-15 13:57:12]

lgtm with nits.

https://codereview.chromium.org/2932593004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp
(right):

https://codereview.chromium.org/2932593004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:1259: }
else {
nit: my preference is to use a more consistent if/else style 
e.g., remove the else and return similar to what you are doing below.

https://codereview.chromium.org/2932593004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/QuadLengthValue.h (right):

https://codereview.chromium.org/2932593004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/QuadLengthValue.h:11: namespace blink {
nit: typical style is to have a blank line after namespace.

[sunyunjia, 2017-06-15 14:19:41]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 14:19:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 16:41:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-15 16:41:16]

Dry run: This issue passed the CQ dry run.

[sunyunjia, 2017-06-15 16:45:25]

The CQ bit was checked by sunyunjia@chromium.org

[sunyunjia, 2017-06-15 16:45:26]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
shend@chromium.org, meade@chromium.org, majidvp@chromium.org
Link to the patchset: https://codereview.chromium.org/2932593004/#ps220002
(title: "Fix nits")

[commit-bot: I haz the power, 2017-06-15 16:45:50]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 16:52:12]

CQ is committing da patch.

Bot data: {"patchset_id": 220002, "attempt_start_ts": 1497545125846760,
"parent_rev": "5a14542e2df551db37f39c08453074a4225783c3", "commit_rev":
"8a809d14cccfd32e74cacbe33e7249eaa4fea4ad"}

[commit-bot: I haz the power, 2017-06-15 16:52:28]

Description was changed from

==========
This patch replaces the snap points css properties for the old spec and add the
properties for the new spec (https://www.w3.org/TR/css-scroll-snap-1/). We also
remove the unit_tests that rely on the old spec.

Properties that are removed:
scroll-snap-points-x, scroll-snap-points-y,
scroll-snap-destination, scroll-snap-coordinate

Properties that are updated:
scroll-snap-type

Properties that are added:
scroll-snap-align, scroll-snap-stop,
scroll-padding, scroll-padding-top, scroll-padding-right,
scroll-padding-bottom,scroll-padding-left, scroll-padding-inline,
scroll-padding-inline-start, scroll-padding-inline-end,
scroll-padding-block, scroll-padding-block-start, scroll-padding-block-end,
scroll-snap-margin, scroll-snap-margin-top, scroll-snap-margin-right,
scroll-snap-margin-bottom, scroll-snap-margin-left, scroll-snap-margin-inline,
scroll-snap-margin-inline-start, scroll-snap-margin-inline-end,
scroll-snap-margin-block, scroll-snap-margin-block-start,
scroll-snap-margin-block-end

BUG=724912
==========

to

==========
This patch replaces the snap points css properties for the old spec and add the
properties for the new spec (https://www.w3.org/TR/css-scroll-snap-1/). We also
remove the unit_tests that rely on the old spec.

Properties that are removed:
scroll-snap-points-x, scroll-snap-points-y,
scroll-snap-destination, scroll-snap-coordinate

Properties that are updated:
scroll-snap-type

Properties that are added:
scroll-snap-align, scroll-snap-stop,
scroll-padding, scroll-padding-top, scroll-padding-right,
scroll-padding-bottom,scroll-padding-left, scroll-padding-inline,
scroll-padding-inline-start, scroll-padding-inline-end,
scroll-padding-block, scroll-padding-block-start, scroll-padding-block-end,
scroll-snap-margin, scroll-snap-margin-top, scroll-snap-margin-right,
scroll-snap-margin-bottom, scroll-snap-margin-left, scroll-snap-margin-inline,
scroll-snap-margin-inline-start, scroll-snap-margin-inline-end,
scroll-snap-margin-block, scroll-snap-margin-block-start,
scroll-snap-margin-block-end

BUG=724912

Review-Url: https://codereview.chromium.org/2932593004
Cr-Commit-Position: refs/heads/master@{#479738}
Committed:
https://chromium.googlesource.com/chromium/src/+/8a809d14cccfd32e74cacbe33e72...
==========

[commit-bot: I haz the power, 2017-06-15 16:52:30]

Committed patchset #10 (id:220002) as
https://chromium.googlesource.com/chromium/src/+/8a809d14cccfd32e74cacbe33e72...

[Xianzhu, 2017-06-30 20:02:15]

wangxianzhu@chromium.org changed reviewers:
+ wangxianzhu@chromium.org

[Xianzhu, 2017-06-30 20:02:16]

https://codereview.chromium.org/2932593004/diff/220002/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/2932593004/diff/220002/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.h:1363: const Length&
ScrollPaddingBlockStart() const {
Why isn't this the same as other mappings between Top/Left/Bottom/Left and
InlineStart/InlineEnd/BlockStart/BlockEnd like
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/text/...

Can we use the PhysicalToLogical template (shown in the above link) here?