Support creating StyleAnimationValue objects from Servo #13553

birtles · 2016-10-03T07:03:32Z

These are the servo-side changes for bug 1302949. @Manishearth has already reviewed them there.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix bug 1302949
These changes do not require tests because there are existing tests for this in mozilla-central

This change is

highfive · 2016-10-03T07:03:38Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @mbrubeck (or someone else) soon.

highfive · 2016-10-03T07:03:39Z

Heads up! This PR modifies the following files:

@bholley: components/style/gecko_bindings/bindings.rs, components/style/gecko/wrapper.rs

highfive · 2016-10-03T07:03:40Z

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!

Manishearth · 2016-10-03T07:16:29Z

@bors-servo delegate+

feel free to r=me whenever you think it's necessary. Try to get both sides landed at around the same time (if not, no big deal, I can cherry pick when I do the next sync)

bors-servo · 2016-10-03T07:16:30Z

✌️ @birtles can now approve this pull request

birtles · 2016-10-05T04:26:11Z

@bors-servo: try

bors-servo · 2016-10-05T04:26:13Z

⌛ Trying commit 7f94994 with merge 28d76a8...


        Auto merge of #13553 - birtles:animvalues, r=<try>

@Manishearth

Support creating StyleAnimationValue objects from Servo These are the servo-side changes for [bug 1302949](https://bugzilla.mozilla.org/show_bug.cgi?id=1302949#c59). @Manishearth has already reviewed them there. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix [bug 1302949](https://bugzilla.mozilla.org/show_bug.cgi?id=1302949#c59) - [X] These changes do not require tests because there are existing tests for this in mozilla-central  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13553)

bors-servo · 2016-10-05T04:42:50Z

💔 Test failed - linux-dev

Manishearth · 2016-10-05T04:44:35Z

/home/servo/buildbot/slave/linux-dev/build/components/style/gecko/wrapper.rs:75:9: 75:26 error: binary operation `==` cannot be applied to type `core::option::Option<std::sync::Arc<parking_lot::RwLock<properties::PropertyDeclarationBlock>>>` [E0369]
/home/servo/buildbot/slave/linux-dev/build/components/style/gecko/wrapper.rs:75         self.declarations == other.declarations
                                                                                        ^~~~~~~~~~~~~~~~~
/home/servo/buildbot/slave/linux-dev/build/components/style/gecko/wrapper.rs:75:9: 75:26 help: run `rustc --explain E0369` to see a detailed explanation
/home/servo/buildbot/slave/linux-dev/build/components/style/gecko/wrapper.rs:75:9: 75:26 note: an implementation of `std::cmp::PartialEq` might be missing for `core::option::Option<std::sync::Arc<parking_lot::RwLock<properties::PropertyDeclarationBlock>>>`
/home/servo/buildbot/slave/linux-dev/build/components/style/gecko/wrapper.rs:75         self.declarations == other.declarations
                                                                                        ^~~~~~~~~~~~~~~~~
error: aborting due to previous error

birtles · 2016-10-05T04:46:10Z

I'm hitting

/home/servo/buildbot/slave/linux-dev/build/components/style/gecko/wrapper.rs:75:9: 75:26 error: binary operation `==` cannot be applied to type `core::option::Option<std::sync::Arc<parking_lot::RwLock<properties::PropertyDeclarationBlock>>>` [E0369]
/home/servo/buildbot/slave/linux-dev/build/components/style/gecko/wrapper.rs:75         self.declarations == other.declarations
                                                                                        ^~~~~~~~~~~~~~~~~

This used to work. I wonder what changed.

birtles · 2016-10-05T05:07:25Z

Possibly 89a29a7 I guess?

birtles · 2016-10-05T06:08:50Z

@SimonSapin do you have any suggestions how I might fix this build error? Specifically, this line used to work but when I went to land it looks like it has bitrotted because RwLock does do ==:

https://github.com/servo/servo/pull/13553/files#diff-956c49c2a32c332249de4800e44bfcadR74

birtles · 2016-10-05T06:32:39Z

I spoke to @wafflespeanut and @Manishearth on IRC and it looks like I need to use read to get the value and compare it. This is because looking through how this is used in Gecko it looks we do need deep equality. Specifically the operator== where we're using this was added here:

https://hg.mozilla.org/mozilla-central/rev/5db982657c9b2606d9572fced222741cffb136c1

And if that doesn't use deep equality we'll end up doing unnecessary work when we update animations and fail tests that check the number of change notifications we dispatch.

birtles · 2016-10-06T07:33:22Z

I had a look into doing this but quickly remembered I know nothing about rust. I tried doing the following:

impl PartialEq for GeckoDeclarationBlock {
    fn eq(&self, other: &GeckoDeclarationBlock) -> bool {
        let unwrap_declarations = |block: &GeckoDeclarationBlock| {
            match block.declarations {
                None => None,
                Some(ref ptr) => Some(*ptr.read())
            }
        };
        unwrap_declarations(self) == unwrap_declarations(other)
    }
}

But ran into lifetime issues, I think because read() returns a temporary RwLockReadGuard which needs to be kept around long enough for its inner object to be compared. Or something like that. Clearly I'm doing it wrong somehow.

Manishearth · 2016-10-06T07:42:52Z

Just unroll it

match (*self, *other) {
  (None, None) => true,
  (Some(ref s), Some(ref other)) => s.read() == other.read(),
  _ => false,
}

birtles · 2016-10-06T07:52:00Z

Ooh! That's a lot nicer. It didn't work as-is but I tweaked it as follows:

    fn eq(&self, other: &GeckoDeclarationBlock) -> bool {
        match (self.declarations, other.declarations) {
            (None, None) => true,
            (Some(ref s), Some(ref other)) => *s.read() == *other.read(),
            _ => false,
        }
    }

And I still get:

C:\moz\servo\components\style\gecko\wrapper.rs:75:16: 75:20 error: cannot move out of borrowed content [E0507]
C:\moz\servo\components\style\gecko\wrapper.rs:75         match (self.declarations, other.declarations) {
                                                                 ^~~~
C:\moz\servo\components\style\gecko\wrapper.rs:75:16: 75:20 help: run `rustc --explain E0507` to see a detailed explanation
C:\moz\servo\components\style\gecko\wrapper.rs:75:35: 75:40 error: cannot move out of borrowed content [E0507]
C:\moz\servo\components\style\gecko\wrapper.rs:75         match (self.declarations, other.declarations) {
                                                                                    ^~~~~
C:\moz\servo\components\style\gecko\wrapper.rs:75:35: 75:40 help: run `rustc --explain E0507` to see a detailed explanation

birtles · 2016-10-06T07:53:31Z

Oh, and if I don't dereference the read() I get:

C:\moz\servo\components\style\gecko\wrapper.rs:77:47: 77:55 error: binary operation `==` cannot be applied to type `parking_lot::RwLockReadGuard<'_, properties::PropertyDeclarationBlock>` [E0369]
C:\moz\servo\components\style\gecko\wrapper.rs:77             (Some(ref s), Some(ref other)) => s.read() == other.read(),

Manishearth · 2016-10-06T07:55:21Z

Try making it (&self.declaration ,...) and (&None, &None), etc in the matches

birtles · 2016-10-06T07:59:36Z

It works! Thank you!
I now hit a different error but I should be able to fix it in the same way--will update the PR tomorrow.
Thanks again.

birtles · 2016-10-07T01:17:33Z

I finally fixed the all the bitrot. Then I re-pulled and I've been bitrotted again. This time by 540ba90.

birtles · 2016-10-07T01:22:40Z

Oh, actually, it's not bitrot. That changeset just broke building on Windows.

birtles · 2016-10-07T01:28:06Z

@bors-servo: try

birtles · 2016-10-09T12:07:45Z

@bors-servo r=Manishearth

bors-servo · 2016-10-09T12:07:46Z

📌 Commit 7478ac9 has been approved by Manishearth

bors-servo · 2016-10-09T12:20:04Z

⌛ Testing commit 7478ac9 with merge 7a7cf2d...


        Auto merge of #13553 - birtles:animvalues, r=Manishearth

@Manishearth

Support creating StyleAnimationValue objects from Servo These are the servo-side changes for [bug 1302949](https://bugzilla.mozilla.org/show_bug.cgi?id=1302949#c59). @Manishearth has already reviewed them there. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix [bug 1302949](https://bugzilla.mozilla.org/show_bug.cgi?id=1302949#c59) - [X] These changes do not require tests because there are existing tests for this in mozilla-central  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13553)

birtles · 2016-10-09T12:50:39Z

I fixed the merge conflicts but it looks like I've been bitrotted yet again. I don't have a build environment on this computer so this might have to wait a couple more days.

bors-servo · 2016-10-09T12:52:50Z

💔 Test failed - linux-dev

Manishearth · 2016-10-09T12:55:47Z

ports/geckolib/glue.rs

@@ -338,6 +374,54 @@ pub extern "C" fn Servo_StyleSet_Drop(data: RawServoStyleSetOwned) -> () {
    let _ = data.into_box::<PerDocumentStyleData>();
 }

+
+#[no_mangle]
+pub extern "C" fn Servo_ParseProperty(property_bytes: *const u8,


The signature for Servo_ParseProperty doesn't match the generated bindings

It seems like the generated bindings were hand-written here, since the signature in ServoBindingList.h is correct. I'll fix this.

That's just one of a bunch of things are now broken but used to work. I wish bors-servo had got around to merging it a few days ago when it applied cleanly. I'll look into it on Tuesday.

Okay, wfm. That was due to a bors bug I need to fix, had a PR open for it iirc.

bors bug should be fixed by servo/homu#63


        Bug 1302949 - Add a method to parse a property-value pair into a decl…

…aration block; r=Manishearth The property may be a shorthand property in which case the declaration block will contain the expanded longhand properties and values. MozReview-Commit-ID: KxqlYgbIZqL


        Bug 1302949 - Add a method to Gecko bindings for comparing declaratio…

…n blocks; r=Manishearth MozReview-Commit-ID: EtX2oLXdGNm


        Bug 1302949 - Add a method to serialize a declaration block as a sing…

…le value; r=Manishearth MozReview-Commit-ID: 59CCT0P4CBm


        Bug 1302949 - Add a method to restyle with an added declaration; r=Ma…

…nishearth MozReview-Commit-ID: AbX0PCjjEM6

Manishearth · 2016-10-09T13:06:20Z

@bors-servo r+

I think I fixed it

bors-servo · 2016-10-09T13:06:21Z

📌 Commit 45e0b90 has been approved by Manishearth

birtles · 2016-10-09T13:09:11Z

Thanks again!

bors-servo · 2016-10-09T13:53:28Z

⌛ Testing commit 45e0b90 with merge 804317c...


        Auto merge of #13553 - birtles:animvalues, r=Manishearth

@Manishearth

Support creating StyleAnimationValue objects from Servo These are the servo-side changes for [bug 1302949](https://bugzilla.mozilla.org/show_bug.cgi?id=1302949#c59). @Manishearth has already reviewed them there. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix [bug 1302949](https://bugzilla.mozilla.org/show_bug.cgi?id=1302949#c59) - [X] These changes do not require tests because there are existing tests for this in mozilla-central  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13553)

bors-servo · 2016-10-09T14:48:02Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

highfive assigned mbrubeck Oct 3, 2016

highfive added the S-awaiting-review label Oct 3, 2016

mbrubeck assigned Manishearth and unassigned mbrubeck Oct 3, 2016

highfive added the S-tests-failed label Oct 5, 2016

birtles force-pushed the birtles:animvalues branch from 7f94994 to 88a1c2e Oct 7, 2016

highfive removed the S-tests-failed label Oct 7, 2016

highfive added the S-awaiting-review label Oct 9, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Oct 9, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 9, 2016

Manishearth reviewed Oct 9, 2016

View changes

birtles added 4 commits Sep 15, 2016

Bug 1302949 - Add a method to parse a property-value pair into a decl…

56e70ae

…aration block; r=Manishearth The property may be a shorthand property in which case the declaration block will contain the expanded longhand properties and values. MozReview-Commit-ID: KxqlYgbIZqL

Bug 1302949 - Add a method to Gecko bindings for comparing declaratio…

61bcc3a

…n blocks; r=Manishearth MozReview-Commit-ID: EtX2oLXdGNm

Bug 1302949 - Add a method to serialize a declaration block as a sing…

1c78e1c

…le value; r=Manishearth MozReview-Commit-ID: 59CCT0P4CBm

Bug 1302949 - Add a method to restyle with an added declaration; r=Ma…

Loading status checks…

45e0b90

…nishearth MozReview-Commit-ID: AbX0PCjjEM6

Manishearth force-pushed the birtles:animvalues branch from 7478ac9 to 45e0b90 Oct 9, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Oct 9, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 9, 2016

bors-servo merged commit 45e0b90 into servo:master Oct 9, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Oct 9, 2016

servo / servo

Join GitHub today

Support creating StyleAnimationValue objects from Servo #13553

Support creating StyleAnimationValue objects from Servo #13553

Conversation

birtles commented Oct 3, 2016 • edited by larsbergstrom

highfive commented Oct 3, 2016

highfive commented Oct 3, 2016

highfive commented Oct 3, 2016

Manishearth commented Oct 3, 2016

bors-servo commented Oct 3, 2016

birtles commented Oct 5, 2016

bors-servo commented Oct 5, 2016

bors-servo commented Oct 5, 2016

Manishearth commented Oct 5, 2016

birtles commented Oct 5, 2016

birtles commented Oct 5, 2016

birtles commented Oct 5, 2016

birtles commented Oct 5, 2016

birtles commented Oct 6, 2016

Manishearth commented Oct 6, 2016

birtles commented Oct 6, 2016

birtles commented Oct 6, 2016

Manishearth commented Oct 6, 2016

birtles commented Oct 6, 2016

birtles commented Oct 7, 2016

birtles commented Oct 7, 2016

birtles commented Oct 7, 2016

birtles commented Oct 9, 2016

bors-servo commented Oct 9, 2016

bors-servo commented Oct 9, 2016

birtles commented Oct 9, 2016

bors-servo commented Oct 9, 2016

This comment has been minimized.

Manishearth Oct 9, 2016

This comment has been minimized.

Manishearth Oct 9, 2016

This comment has been minimized.

birtles Oct 9, 2016

This comment has been minimized.

birtles Oct 9, 2016

This comment has been minimized.

Manishearth Oct 9, 2016

This comment has been minimized.

Manishearth Oct 9, 2016

Manishearth commented Oct 9, 2016

bors-servo commented Oct 9, 2016

birtles commented Oct 9, 2016

bors-servo commented Oct 9, 2016

bors-servo commented Oct 9, 2016

birtles commented Oct 3, 2016 •

edited by larsbergstrom