This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop)
For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Before we approve merge to M63, please provide following details:
Which change you're requesting merge for?
Is this M63 regression and critical to merge?
Is the change well baked/verified in Canary and safe to merge?
Please provide any other details to justify the merge. Thank you.
Please note M63 is already promoted to Beta so merge bar is very high.
Before we approve merge to M63, please provide following details:
Which change you're requesting merge for?
The patch in #13 i.e., https://chromium.googlesource.com/chromium/src.git/+/24c13771fb5a3f5d72bcd69ef1980d23ee030cf2%5E%21/
It renames the name of a new CSS feature that is being shipped in M63 from "scroll-boundary-
behavior" to "overscroll-behavior". The change reflects a recent change in the specification [1].
So we are renaming the CSS property to match the spec before shipping it.
Is this M63 regression and critical to merge?
This is not a regression. It is a new feature we are shipping and we need to match the specification.
So our options are to either remove the feature or match the specification change with this patch.
Is the change well baked/verified in Canary and safe to merge?
The change has been in Canary for a day but I can wait for longer if necessary. (e.g., I can merge
on Monday Oct 30th)
We have taken the following steps to make sure this is a safe and well-tested change:
- The patch is the minimum change necessary to just rename the publicly exposed CSS property.
In particular, we have opted to not change Blink or Content internal implementation at all.
So the change is only in CSS parsing machinery and quite straightforward. This was to ensure
the smallest change needed.
- The change is tested in particular by layout tests that ensure validity of web exposed API:
- LayoutTests/webexposed/css-properties-as-js-properties-expected.txt
- LayoutTests/webexposed/css-property-listing-expected.txt
There are 6 other tests in that CL that ensure functionality has not changed.
We have also done manual checks on Canary to verify this as well (2 different persons).
- There is only a single patch to merge. This should minimize risk of mistakes when merging with
the branch.
- The patch is fairly trivial i.e., no additional branch or loop logic is introduces. It replaces
one string with another.
Please provide any other details to justify the merge. Thank you.
Given that this is a fairly safe change we think it is better to make the change than
completely disable the feature for M63. I have also reached out to API owners and have made sure
they don't object to this API change [2, 3].
[1] https://github.com/WICG/scroll-boundary-behavior/issues/24
[2] https://groups.google.com/a/chromium.org/d/msg/blink-dev/OqBNF2efmFA/HreeIDaXBgAJ
[3] https://groups.google.com/a/chromium.org/d/msg/blink-api-owners-discuss/4Ma4YwSWfzA/-qk6ivOXAgAJ
Please note M63 is already promoted to Beta so merge bar is very high.
Comment 1 by majidvp@chromium.org
, Oct 25 2017Owner: majidvp@chromium.org
Status: Started (was: Assigned)