style: Add an iterator for transition properties #26200

mrobinson · 2020-04-16T12:00:55Z

This simplifies the code a bit and also will allow us to more easily
make improvements to servo's animation implementation in the future.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #___ (GitHub issue number if applicable)

There are tests for these changes OR
These changes do not require tests because they should not change behavior.

highfive · 2020-04-16T12:01:05Z

Heads up! This PR modifies the following files:

@emilio: components/style/gecko/wrapper.rs, components/style/animation.rs, components/style/properties/helpers/animated_properties.mako.rs, components/style/properties/properties.mako.rs

highfive · 2020-04-16T12:01:07Z

Warning

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

mrobinson · 2020-04-16T12:02:08Z

@emilio, if you have some free moments do you mind looking at this change? I'm not entirely certain what the right procedure is for making changes to the Gecko-specific code. I've verified that the changes build, but I don't think I have try access any longer.

emilio

Thanks, and sorry for the lag. This looks good with a few nits to me.

I can push it to try, let me know if you want to recover access, I think we're pretty liberal reenabling accounts :)

emilio · 2020-04-16T21:31:12Z

components/style/properties/helpers/animated_properties.mako.rs

+            });
+        }
+
+        let index = match self.index_range.next() {


nit: You can use .next()?.

Oh, nice. Fixed.

emilio · 2020-04-16T21:31:12Z

components/style/properties/helpers/animated_properties.mako.rs

+        };
+
+        match self.style.get_box().transition_property_at(index) {
+            TransitionProperty::Custom(..) | TransitionProperty::Unsupported(..) => self.next(),


May be better to make this a loop { } instead of recursive. I don't think it should be much harder.

Sounds good. I've changed this too a loop and added a comment explaining what is going on.

emilio · 2020-04-16T21:31:12Z

components/style/properties/properties.mako.rs

+    pub fn transition_properties<'a>(
+        &'a self
+    ) -> animated_properties::TransitionPropertyIterator<'a>
+    {


{ should go to the previous line I think.

rustfmt probably disagrees, since CI is passing.

It seems like rustfmt might not be processing Mako templates? In any case, I've formatted this to look like the other methods in the file.

Yeah, we don't rustfmt this file.

emilio

r=me with the nits fixed.

mrobinson · 2020-04-17T08:04:47Z

Thanks, and sorry for the lag. This looks good with a few nits to me.

I can push it to try, let me know if you want to recover access, I think we're pretty liberal reenabling accounts :)

Thanks for the review! I've fixed your nits and will ask bors to merge this. Do you mind pushing this one to try for me? I think once I make one or two more changes to the gecko code, I might need try access again. I'd hate to ask people to do a bunch of work and then never use it.

@bors-servo r=emilio

bors-servo · 2020-04-17T08:04:48Z

📌 Commit 18ddf64 has been approved by emilio

bors-servo · 2020-04-17T08:04:53Z

⌛ Testing commit 18ddf64 with merge 0675e1e...


        Auto merge of #26200 - mrobinson:animation-iterator, r=emilio

style: Add an iterator for transition properties This simplifies the code a bit and also will allow us to more easily make improvements to servo's animation implementation in the future.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [x] These changes do not require tests because they should not change behavior.

bors-servo · 2020-04-17T08:39:52Z

💔 Test failed - status-taskcluster

mrobinson · 2020-04-17T08:43:06Z

@bors-servo retry

#21067

bors-servo · 2020-04-17T08:43:10Z

⌛ Testing commit 18ddf64 with merge ef7a473...


        Auto merge of #26200 - mrobinson:animation-iterator, r=emilio

style: Add an iterator for transition properties This simplifies the code a bit and also will allow us to more easily make improvements to servo's animation implementation in the future.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [x] These changes do not require tests because they should not change behavior.

bors-servo · 2020-04-17T09:02:33Z

💔 Test failed - status-taskcluster


        Add an iterator for transition properties

This simplifies the code a bit and also will allow us to more easily make improvements to servo's animation implementation in the future.

mrobinson · 2020-04-17T13:12:59Z

@bors-servo r=emilio

bors-servo · 2020-04-17T13:13:00Z

📌 Commit fad79a7 has been approved by emilio

bors-servo · 2020-04-17T13:23:54Z

⌛ Testing commit fad79a7 with merge fb45a63...


        Auto merge of #26200 - mrobinson:animation-iterator, r=emilio

style: Add an iterator for transition properties This simplifies the code a bit and also will allow us to more easily make improvements to servo's animation implementation in the future.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [x] These changes do not require tests because they should not change behavior.

bors-servo · 2020-04-17T14:22:13Z

💔 Test failed - status-taskcluster

mrobinson · 2020-04-17T14:57:07Z

@bors-servo retry

#25233

bors-servo · 2020-04-17T15:36:17Z

⌛ Testing commit fad79a7 with merge 8349b4f...


        Auto merge of #26200 - mrobinson:animation-iterator, r=emilio

style: Add an iterator for transition properties This simplifies the code a bit and also will allow us to more easily make improvements to servo's animation implementation in the future.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [x] These changes do not require tests because they should not change behavior.

bors-servo · 2020-04-17T16:31:56Z

💔 Test failed - status-taskcluster

jdm · 2020-04-17T16:47:44Z

@bors-servo retry

#25513

bors-servo · 2020-04-17T17:42:48Z

⌛ Testing commit fad79a7 with merge 71940ff...

bors-servo · 2020-04-17T17:55:52Z

☀️ Test successful - status-taskcluster
Approved by: emilio
Pushing 71940ff to master...

mrobinson requested a review from emilio Apr 16, 2020

highfive assigned paulrouget Apr 16, 2020

highfive added the S-awaiting-review label Apr 16, 2020

mrobinson added the A-stylo label Apr 16, 2020

mrobinson changed the title ~~Add an iterator for transition properties~~ style: Add an iterator for transition properties Apr 16, 2020

emilio reviewed Apr 16, 2020

View changes

emilio approved these changes Apr 16, 2020

View changes

mrobinson force-pushed the mrobinson:animation-iterator branch from a176159 to 18ddf64 Apr 17, 2020

highfive assigned emilio and unassigned paulrouget Apr 17, 2020

highfive added S-awaiting-merge and removed S-awaiting-review labels Apr 17, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Apr 17, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Apr 17, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Apr 17, 2020

Add an iterator for transition properties

Loading status checks…

fad79a7

This simplifies the code a bit and also will allow us to more easily make improvements to servo's animation implementation in the future.

mrobinson force-pushed the mrobinson:animation-iterator branch from e0ac731 to fad79a7 Apr 17, 2020

highfive added S-awaiting-review and removed S-tests-failed labels Apr 17, 2020

highfive added S-awaiting-merge and removed S-awaiting-review labels Apr 17, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Apr 17, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Apr 17, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Apr 17, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Apr 17, 2020

bors-servo merged commit 71940ff into servo:master Apr 17, 2020
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Apr 17, 2020

mrobinson deleted the mrobinson:animation-iterator branch Apr 17, 2020

servo / servo

style: Add an iterator for transition properties #26200

style: Add an iterator for transition properties #26200

mrobinson commented Apr 16, 2020

highfive commented Apr 16, 2020

highfive commented Apr 16, 2020

mrobinson commented Apr 16, 2020

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio left a comment

mrobinson commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020

mrobinson commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020

mrobinson commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020

mrobinson commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020

jdm commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020

servo / servo

Join GitHub today

style: Add an iterator for transition properties #26200

style: Add an iterator for transition properties #26200

Conversation

mrobinson commented Apr 16, 2020

highfive commented Apr 16, 2020

highfive commented Apr 16, 2020

mrobinson commented Apr 16, 2020

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio left a comment

mrobinson commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020

mrobinson commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020

mrobinson commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020

mrobinson commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020

jdm commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020