Support discrete animations for more Gecko things #17544

dadaa · 2017-06-28T12:05:24Z

This PR fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1371115

./mach build -d does not report any errors
./mach test-tidy does not report any errors

There are tests for these changes. Add some tests into dom/animation/test/ of m-c in patch 14. Also, remove test fail annotation from meta in wpt as patch 15.

This change is

highfive · 2017-06-28T12:05:29Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/longhand/position.mako.rs, components/style/properties/longhand/background.mako.rs, components/style/properties/longhand/text.mako.rs, components/style/gecko/conversions.rs, components/style/properties/longhand/svg.mako.rs and 17 more
@emilio: components/style/properties/longhand/position.mako.rs, components/style/properties/longhand/background.mako.rs, components/style/properties/longhand/text.mako.rs, components/style/gecko/conversions.rs, components/style/properties/longhand/svg.mako.rs and 17 more

highfive · 2017-06-28T12:05:30Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify style code, but no tests are modified. Please consider adding a test!

dadaa · 2017-06-28T12:10:54Z

These patches have been reviewed by @hiikezoe

nox · 2017-06-28T12:36:58Z

components/style/gecko/conversions.rs

+            },
+            nsStyleImageType::eStyleImageType_Image => {
+                let url = self.get_image_url();
+                match self.mCropRect.mPtr.is_null() {


Use if instead of match.

nox · 2017-06-28T12:39:28Z

components/style/gecko/conversions.rs

+                        let pos_y = LengthOrPercentage::from_gecko_style_coord(&gecko_gradient.mBgPosY)
+                            .expect("mBgPosY has valid data");
+                        let horizontal_direction = match pos_x {
+                            LengthOrPercentage::Percentage(Percentage(0.0)) => X::Left,


Matching against float literals is a warning in nightly, please don't pattern match them if you have time.

nox · 2017-06-28T12:40:26Z

components/style/gecko/conversions.rs

+                        NS_STYLE_GRADIENT_SIZE_CLOSEST_CORNER => ShapeExtent::ClosestCorner,
+                        NS_STYLE_GRADIENT_SIZE_FARTHEST_CORNER => ShapeExtent::FarthestCorner,
+                        // We don't choose ShapeExtent::Contain and ShapeExtent::Cover
+                        // since those 'contain' and 'cover' keywords are handled as synonyms of the standard


Aren't they synonyms only in semantics, but must be preserved?

I don't think we need to preserve since the contain and cover already dropped from spec[1]. Also, MDN[2] said "Early drafts included other keywords (cover and contain) as synonyms of the standard farthest-corner and closest-side respectively. ".
And Gecko does not preserve both contain and cover keywords.[3]

[1] https://drafts.csswg.org/css-images-3/#typedef-size
[2] https://developer.mozilla.org/en-US/docs/Web/CSS/radial-gradient
[3] https://searchfox.org/mozilla-central/source/layout/style/nsCSSProps.cpp#1917

This is not the spec you should be looking at. These keywords indeed were present only in an previous version of the spec, but that previous version is the one we are supposed to implement to support -webkit prefixed gradients.

Safari parses and preserves these two keywords with the prefixed gradients.

<div style="background-image: -webkit-radial-gradient(center, ellipse contain, yellow 0%, green 100%);"></div> <pre> <script> var div = document.getElementsByTagName("div")[0]; document.writeln(div.style.backgroundImage); document.writeln(getComputedStyle(div).backgroundImage); </script>

https://www.w3.org/TR/2011/WD-css3-images-20110217/#radial-gradients

Thank you very much, please let me check.

Please let me fix this after fixing https://bugzilla.mozilla.org/show_bug.cgi?id=1217664
I'll write a memo in that source code.

nox · 2017-06-28T12:40:35Z

components/style/gecko/conversions.rs

+                            NS_STYLE_GRADIENT_SIZE_EXPLICIT_SIZE => {
+                                let radius = Length::from_gecko_style_coord(&gecko_gradient.mRadiusX)
+                                    .expect("mRadiusX has valid data");
+                                debug_assert!(radius ==


Nit: debug_assert_eq.

nox · 2017-06-28T12:41:36Z

components/style/gecko/conversions.rs

+                            NS_STYLE_GRADIENT_SIZE_EXPLICIT_SIZE => {
+                                Ellipse::Radii(
+                                    LengthOrPercentage::from_gecko_style_coord(&gecko_gradient.mRadiusX)
+                                        .expect("mRadiusX has valid data"),


Nit: AFAIK expect should be given the description of the error, not the description of what valid things are expected.

thanks, I'll fix all messages that are in patches in this time.

nox · 2017-06-28T12:42:53Z

components/style/properties/gecko.mako.rs

+            use values::specified::url::SpecifiedUrl;
+            use values::None_;
+
+            match self.gecko.${gecko_ffi_name}.mRawPtr.is_null() {


nox · 2017-06-28T12:43:40Z

components/style/properties/gecko.mako.rs

+        longhands::_moz_border_${side.ident}_colors::computed_value::T(
+            if gecko_colors.is_null() {
+                None
+            } else {


Nit: Short-circuiting would be better here:

if gecko_colors.is_null() { return None; }

nox · 2017-06-28T12:44:22Z

components/style/properties/gecko.mako.rs

+                if self.gecko.${value.gecko}.mInteger == 0 {
+                    None
+                } else {
+                    debug_assert!(nsStyleGridLine_kMinLine <= self.gecko.${value.gecko}.mInteger &&


Nit: two assertions would be better, given it would print which particular test failed.

I'm sorry forgot to fix it.
I'll do soon.

nox · 2017-06-28T12:44:56Z

components/style/properties/gecko.mako.rs

+        use gecko_string_cache::Atom;
+        use values::CustomIdent;
+
+        match self.gecko.mWillChange.mBuffer.len() {


nox · 2017-06-28T12:46:00Z

components/style/properties/longhand/border.mako.rs

+
+#[cfg(feature = "gecko")] use values::computed::{LengthOrNumber, NumberOrPercentage};
+#[cfg(feature = "gecko")] impl_rect_conversions!(LengthOrNumber);
+#[cfg(feature = "gecko")] impl_rect_conversions!(NumberOrPercentage);


Couldn't all of this be in gecko.mako.rs with the rest? Feels quite out of place here.

I'll move to related code to style/gecko/conversions.rs
Thanks

bors-servo · 2017-06-28T20:02:53Z

☔ The latest upstream changes (presumably #17541) made this pull request unmergeable. Please resolve the merge conflicts.

dadaa · 2017-06-30T16:34:19Z

r? @nox
Could you review again?
I'll fix -webkit-radial-gradient in another bug.

nox · 2017-07-01T08:27:58Z

@bors-servo r+

bors-servo · 2017-07-01T08:27:58Z

📌 Commit 66562a5 has been approved by nox

bors-servo · 2017-07-01T08:28:03Z

⌛ Testing commit 66562a5 with merge e6eb90e...

Support discrete animations for more Gecko things  This PR fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1371115 ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors  - [X] There are tests for these changes. Add some tests into dom/animation/test/ of m-c in patch 14. Also, remove test fail annotation from meta in wpt as patch 15.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17544)

bors-servo · 2017-07-01T08:45:01Z

💔 Test failed - linux-dev

bors-servo · 2017-07-04T01:13:00Z

☔ The latest upstream changes (presumably #17268) made this pull request unmergeable. Please resolve the merge conflicts.

…_conversions

dadaa · 2017-07-05T04:27:53Z

These patches have been reviewed by @hiikezoe and @birtles

birtles · 2017-07-05T05:16:43Z

@bors-servo try

bors-servo · 2017-07-05T05:16:47Z

⌛ Trying commit 1b28d5e with merge 42e551f...

Support discrete animations for more Gecko things  This PR fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1371115 ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors  - [X] There are tests for these changes. Add some tests into dom/animation/test/ of m-c in patch 14. Also, remove test fail annotation from meta in wpt as patch 15.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17544)

hiikezoe · 2017-07-05T05:54:05Z

@bors-servo delegate+

bors-servo · 2017-07-05T05:54:05Z

✌️ @dadaa can now approve this pull request

dadaa · 2017-07-05T05:56:41Z

@bors-servo r=hiro p=10

bors-servo · 2017-07-05T05:56:41Z

📌 Commit 1b28d5e has been approved by hiro

bors-servo · 2017-07-05T05:56:43Z

⚡ Previous build results for arm32, linux-rel-wpt, mac-dev-unit, windows-msvc-dev are reusable. Rebuilding only android, arm64, linux-dev, linux-rel-css, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4...

bors-servo · 2017-07-05T06:37:49Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: hiro
Pushing 42e551f to master...

highfive assigned asajeffrey Jun 28, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 28, 2017

nox changed the title ~~Bug1371115~~ Support discrete animations for more Gecko things Jun 28, 2017

nox suggested changes Jun 28, 2017

View reviewed changes

highfive added the S-needs-rebase There are merge conflict errors. label Jun 28, 2017

dadaa force-pushed the bug1371115 branch 2 times, most recently from 7ec8f0b to 66562a5 Compare June 29, 2017 17:51

highfive assigned nox and unassigned asajeffrey Jun 30, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Jul 1, 2017

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jul 1, 2017

highfive added the S-needs-rebase There are merge conflict errors. label Jul 4, 2017

dadaa added 4 commits July 5, 2017 12:54

implements nsStyleCoord type properties animatable

983361e

implements nsStyleGridLine type properties animatable

3498068

implements nsStyleSides type properties animatable

a155a63

implements nsStyleCounterData type properties animatable

6316b7a

dadaa added 10 commits July 5, 2017 12:59

implements nsStyleTextOverflow type properties animatable

7d1374e

implements nsStyleQuoteValues type properties animatable

0555977

implements nsStyleBorder type properties animatable

699ec02

implements nsFont type properties animatable

961bdee

rename macro from impl_gecko_keyword_from_trait to impl_gecko_keyword…

0107b3e

…_conversions

implements u8 and array type properties animatable

b3def4a

implements URLValue type properties animatable

4924a48

implements nsStyleImage type properties animatable

d885747

implements nsStyleImageRequest type properties animatable

d8dcc52

update auto generated files

1b28d5e

dadaa force-pushed the bug1371115 branch from 66562a5 to 1b28d5e Compare July 5, 2017 04:27

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jul 5, 2017

highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Jul 5, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-needs-rebase There are merge conflict errors. labels Jul 5, 2017

bors-servo merged commit 1b28d5e into servo:master Jul 5, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jul 5, 2017

Support discrete animations for more Gecko things #17544

Support discrete animations for more Gecko things #17544

Conversation

dadaa commented Jun 28, 2017 • edited by larsbergstrom Loading

highfive commented Jun 28, 2017

highfive commented Jun 28, 2017

dadaa commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadaa Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors-servo commented Jun 28, 2017

dadaa commented Jun 30, 2017

nox commented Jul 1, 2017

bors-servo commented Jul 1, 2017

bors-servo commented Jul 1, 2017

bors-servo commented Jul 1, 2017

bors-servo commented Jul 4, 2017

dadaa commented Jul 5, 2017

birtles commented Jul 5, 2017

bors-servo commented Jul 5, 2017

hiikezoe commented Jul 5, 2017

bors-servo commented Jul 5, 2017

dadaa commented Jul 5, 2017

bors-servo commented Jul 5, 2017

bors-servo commented Jul 5, 2017

bors-servo commented Jul 5, 2017

dadaa commented Jun 28, 2017 •

edited by larsbergstrom

Loading

dadaa Jun 28, 2017 •

edited

Loading