Closed Bug 1343751 Opened 7 years ago Closed 7 years ago

stylo: Store transition properties into nsStyleDisplay::mTransitions

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(2 files, 7 obsolete files)

Bug 1343751 - ensure_len() for nsStyleAutoArray<StyleTransition>.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1343751 - Update mochitest expectation for transition tests.
59 bytes, text/x-review-board-request
emilio
: review+
Details 
This is a sibling of bug 1328786.
Gosh! ensure_len() is only for nsStyleAutoArray<StyleAnimation>...
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.
Emilio, would you mind reviewing these? I was going to ask Cameron to review them but he is still away.
Thanks!
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.
Attachment #8843062 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8843063 [details]
Bug 1343751 - ensure_len() for nsStyleAutoArray<StyleTransition>.

https://reviewboard.mozilla.org/r/116818/#review118466
Attachment #8843063 - Flags: review?(emilio+bugs) → review+
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 ;)
Attachment #8843064 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8843065 [details]
Bug 1343751 - Pass transition-delay and transition-duration into gecko struct.

https://reviewboard.mozilla.org/r/116822/#review118472
Attachment #8843065 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8843066 [details]
Bug 1343751 - Make a macro for timing function.

https://reviewboard.mozilla.org/r/116824/#review118474
Attachment #8843066 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8843067 [details]
Bug 1343751 - Pass transition-timing-function into gecko struct.

https://reviewboard.mozilla.org/r/116826/#review118476
Attachment #8843067 - Flags: review?(emilio+bugs) → review+
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)
Attachment #8843068 - Flags: review?(emilio+bugs) → review+
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
Attachment #8843069 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8843070 [details]
Bug 1343751 - Update mochitest expectation for transition tests.

https://reviewboard.mozilla.org/r/116832/#review118486
Attachment #8843070 - Flags: review?(emilio+bugs) → review+
Thank you Emilio for quick and reviewing!
Attachment #8843062 - Attachment is obsolete: true
Attachment #8843064 - Attachment is obsolete: true
Attachment #8843065 - Attachment is obsolete: true
Attachment #8843066 - Attachment is obsolete: true
Attachment #8843067 - Attachment is obsolete: true
Attachment #8843068 - Attachment is obsolete: true
Attachment #8843069 - Attachment is obsolete: true
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
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
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.
Pushed by hikezoe@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/a88ced8a00c1
https://hg.mozilla.org/mozilla-central/rev/cf95c2d49e1c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: