1343751 - stylo: Store transition properties into nsStyleDisplay::mTransitions

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Hiroyuki Ikezoe (:hiro)]

This is a sibling of bug 1328786.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6264f22428ccebf004cc1655d916a975f3bf93c9

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Gosh! ensure_len() is only for nsStyleAutoArray<StyleAnimation>...

Comment 3

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=de70f8e5dcfaf05f171f4b9470359fbb71110425

It turns out that we should skip start_transitions_if_applicable() in cascade_primary_or_pseudo before landing this. Otherwise servo side code tries to trigger transition.  Actually I did skip it in a patch for bug 1341985.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1343751 - Replace assert!(len() > 0) with debug_assert!(is_empty()).

Addressed this comment.
https://github.com/servo/servo/pull/15793#discussion_r103906186

Review commit: https://reviewboard.mozilla.org/r/116816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116816/

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1343751 - ensure_len() for nsStyleAutoArray<StyleTransition>.

Review commit: https://reviewboard.mozilla.org/r/116818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116818/

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1343751 - Modify some macro for animations to be able to use for transition properties as well.

Review commit: https://reviewboard.mozilla.org/r/116820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116820/

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1343751 - Pass transition-delay and transition-duration into gecko struct.

Review commit: https://reviewboard.mozilla.org/r/116822/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116822/

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1343751 - Make a macro for timing function.

Review commit: https://reviewboard.mozilla.org/r/116824/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116824/

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1343751 - Pass transition-timing-function into gecko struct.

Review commit: https://reviewboard.mozilla.org/r/116826/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116826/

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1343751 - Pass transition-property into gecko's struct.

Review commit: https://reviewboard.mozilla.org/r/116828/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116828/

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1343751 - Skip animation and transition process in servo side.

Otherwise they trigger their animations and transitions.
We will introduce gecko's own process in bug 1341985 (animations) and
bug 1341372 (transitions).

Review commit: https://reviewboard.mozilla.org/r/116830/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116830/

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1343751 - Update mochitest expectation for transition tests.

Review commit: https://reviewboard.mozilla.org/r/116832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116832/

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Emilio, would you mind reviewing these? I was going to ask Cameron to review them but he is still away.
Thanks!

Comment 14

[Hiroyuki Ikezoe (:hiro)]

A try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c6690a68070ac3fbe3b78222c36255b3a2c7779

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8843062 [details]
Bug 1343751 - Replace assert!(len() > 0) with debug_assert!(is_empty()).

https://reviewboard.mozilla.org/r/116816/#review118464

r=me, thanks!

::: commit-message-c54c6:1
(Diff revision 1)
> +Bug 1343751 - Replace assert!(len() > 0) with debug_assert!(is_empty()). r?emilio

nit: debug_assert!(!is_empty()) in the commit message.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8843063 [details]
Bug 1343751 - ensure_len() for nsStyleAutoArray<StyleTransition>.

https://reviewboard.mozilla.org/r/116818/#review118466

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8843064 [details]
Bug 1343751 - Modify some macro for animations to be able to use for transition properties as well.

https://reviewboard.mozilla.org/r/116820/#review118468

Looks fine! Thanks for not duplicating all over the place, which was the easy thing to do ;)

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8843065 [details]
Bug 1343751 - Pass transition-delay and transition-duration into gecko struct.

https://reviewboard.mozilla.org/r/116822/#review118472

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8843066 [details]
Bug 1343751 - Make a macro for timing function.

https://reviewboard.mozilla.org/r/116824/#review118474

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8843067 [details]
Bug 1343751 - Pass transition-timing-function into gecko struct.

https://reviewboard.mozilla.org/r/116826/#review118476

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8843068 [details]
Bug 1343751 - Pass transition-property into gecko's struct.

https://reviewboard.mozilla.org/r/116828/#review118480

::: servo/components/style/properties/gecko.mako.rs:1779
(Diff revision 1)
>      ${impl_transition_timing_function()}
>  
> +    pub fn set_transition_property(&mut self, v: longhands::transition_property::computed_value::T) {
> +        use gecko_bindings::structs::nsCSSPropertyID_eCSSPropertyExtra_no_properties;
> +
> +        unsafe { self.gecko.mTransitions.ensure_len(v.0.len()) };

We can move this ensure_len() call into the not empty case, right?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:142
(Diff revision 1)
> +                    ${helpers.to_nscsspropertyid(prop.ident)}
> +                        => TransitionProperty::${prop.camel_case},
> +                % endif
> +            % endfor
> +            nsCSSPropertyID::eCSSPropertyExtra_all_properties => TransitionProperty::All,
> +            _ => panic!(),

please add a descriptive message here, such as:

panic!("Unsupported Servo transition property: {:?}", property)

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8843069 [details]
Bug 1343751 - Skip animation and transition process in servo side.

https://reviewboard.mozilla.org/r/116830/#review118484

::: servo/components/style/matching.rs:579
(Diff revision 1)
>          let mut new_values = self.cascade_internal(context, primary_style,
>                                                     &mut pseudo_style, &booleans);
>  
>          // Handle animations.
>          if booleans.animate {
> +            if cfg!(feature = "servo") {

nit: `if booleans.animate && cfg!(feature = "servo")` instead

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8843070 [details]
Bug 1343751 - Update mochitest expectation for transition tests.

https://reviewboard.mozilla.org/r/116832/#review118486

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Thank you Emilio for quick and reviewing!

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8843063 [details]
Bug 1343751 - ensure_len() for nsStyleAutoArray<StyleTransition>.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116818/diff/1-2/

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8843070 [details]
Bug 1343751 - Update mochitest expectation for transition tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116832/diff/1-2/

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Split off servo side changes.
I will send a PR to servo after Xidorn's work[1] gets merged.
[1] https://github.com/servo/servo/pull/15793

Comment 28

[Hiroyuki Ikezoe (:hiro)]

https://github.com/servo/servo/pull/15848

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Final try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a95d402ac98b30e4f4b6a33e96b022034871976
The failure numbers on test_value_storage.html increased.  I will modify mochitest expectations before landing.

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8843063 [details]
Bug 1343751 - ensure_len() for nsStyleAutoArray<StyleTransition>.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116818/diff/2-3/

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8843070 [details]
Bug 1343751 - Update mochitest expectation for transition tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116832/diff/2-3/

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a88ced8a00c1
ensure_len() for nsStyleAutoArray<StyleTransition>. r=emilio
https://hg.mozilla.org/integration/autoland/rev/cf95c2d49e1c
Update mochitest expectation for transition tests. r=emilio

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a88ced8a00c1
https://hg.mozilla.org/mozilla-central/rev/cf95c2d49e1c