stylo: Eliminate passing parent computed values to Servo_GetComputedKeyframeValues

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

(Blocks 2 bugs)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(6 attachments, 1 obsolete attachment)

Assignee

Description

2 years ago
Currently we are passing the computed values of the parent element to Servo_GetComputedKeyframeValues to get inherited values.  This is really cumbersome because we need to get the parent style values, this patters are now scattered around our code (in SMIL).  To avoid this pattern, we should do:

1) FlushPendingNotification(Style)
2) ResolveTransientServoStyle() for the target element
3) Call Servo_GetComputedKeyframeValues with the resolved computed values at 2) and the target element
4) Get the parent computed values in Servo_GetComputedKeyframeValues
  We should check the parent computed values is not stale before getting it

For reference, a log on IRC chat with Cameron:

11:35 AM <hiro> heycam: if we call FlushPendingNotification(Style) and call ResolveTransientServoStyle() for an element, the parent style of the element has been processed too?
11:36 AM <hiro> heycam: I mean the parent style is not stale?
11:36 AM <heycam> hiro: yes, all styles will be up to date.


Note that for CSS animations/transitions, the parent element has also been restyled there.
Assignee

Comment 1

2 years ago
Doh!  I wanted to add bug 1288269 into block list..
Assignee

Comment 2

2 years ago
I just realized that we should fix this prior to bug 1358966 so that we can implement cleaner shorthand handling for SMIL.
Assignee: nobody → hikezoe
Blocks: 1358966
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8873607 [details]
Bug 1367293 - Explicitly cast nsStyleContext* to nullptr.

https://reviewboard.mozilla.org/r/144990/#review148976

(We probably should just fix bug 1339690 sometime soon.)
Attachment #8873607 - Flags: review?(bbirtles) → review+

Comment 11

2 years ago
mozreview-review
Comment on attachment 8873608 [details]
Bug 1367293 - Early return from ValueFromStringHelper() if the target element is not associated with any documents.

https://reviewboard.mozilla.org/r/144992/#review148978
Attachment #8873608 - Flags: review?(bbirtles) → review+

Comment 12

2 years ago
mozreview-review
Comment on attachment 8873609 [details]
Bug 1367293 - Factor out creating context.

https://reviewboard.mozilla.org/r/144994/#review148980

::: servo/ports/geckolib/glue.rs:2442
(Diff revision 1)
>      false
>  }
>  
> +fn create_context<'a>(per_doc_data: &'a PerDocumentStyleDataImpl,
> +                      font_metrics_provider: &'a FontMetricsProvider,
> +                      style: &'a Arc<ComputedValues>,

As discussed, I think this can just be &'a ComputedValues
Attachment #8873609 - Flags: review?(bbirtles) → review+

Comment 13

2 years ago
mozreview-review
Comment on attachment 8873610 [details]
Bug 1367293 - Get parent style from the target element.

https://reviewboard.mozilla.org/r/144996/#review148982

::: commit-message-80a94:16
(Diff revision 1)
> + SequentialTask, that means all elements have been already restyled.
> +
> +Unfortunately we can't assert that the parent style is not stale
> +to check the parent element has no dirty descendant bit since
> +Servo_GetComputedKeyframeValues is called in a SequantialTask
> +that is processed before post travesal, that means elements

Nit: traversal

::: servo/ports/geckolib/glue.rs:2443
(Diff revision 1)
>  }
>  
>  fn create_context<'a>(per_doc_data: &'a PerDocumentStyleDataImpl,
>                        font_metrics_provider: &'a FontMetricsProvider,
>                        style: &'a Arc<ComputedValues>,
> -                      parent_style: &'a Option<&ComputedValues>)
> +                      parent_style: &'a Option<&Arc<ComputedValues>>)

If we can use &'a Option<&ComputedValues> that would seem better but since this is a local function it's probably not a big deal.

Alternatively, would it make sense to just pass the element in and get the parent style from it here?
Attachment #8873610 - Flags: review?(bbirtles) → review+

Comment 14

2 years ago
mozreview-review
Comment on attachment 8873611 [details]
Bug 1367293 - Don't get parent style for ComputeAnimationValue.

https://reviewboard.mozilla.org/r/144998/#review148986

::: layout/style/StyleAnimationValue.cpp:5377
(Diff revision 1)
>  
>      if (!declarations) {
>        return result;
>      }
>  
> -    // We use the current ServoComputeValues and its parent ServoComputeValues
> +    const ServoComputedValues * computedValues =

Nit: * should hug ServoComputedValues (i.e. no space in between)
Attachment #8873611 - Flags: review?(bbirtles) → review+

Comment 15

2 years ago
mozreview-review
Comment on attachment 8873612 [details]
Bug 1367293 - Don't get parent style for GetComputedKeyframeValuesFor.

https://reviewboard.mozilla.org/r/145000/#review148992

This looks great.

I suspect there are a few places where we could switch from using pointers to references and improve code clarity and static checking, but for now this is a great simplification.
Attachment #8873612 - Flags: review?(bbirtles) → review+

Comment 16

2 years ago
mozreview-review
Comment on attachment 8873613 [details]
Bug 1367293 - Drop ServoComputedValuesWithParent entirely.

https://reviewboard.mozilla.org/r/145002/#review148994
Attachment #8873613 - Flags: review?(bbirtles) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8873609 - Attachment is obsolete: true
Assignee

Comment 23

2 years ago
(In reply to Brian Birtles (:birtles) from comment #13)

> ::: servo/ports/geckolib/glue.rs:2443
> (Diff revision 1)
> >  }
> >  
> >  fn create_context<'a>(per_doc_data: &'a PerDocumentStyleDataImpl,
> >                        font_metrics_provider: &'a FontMetricsProvider,
> >                        style: &'a Arc<ComputedValues>,
> > -                      parent_style: &'a Option<&ComputedValues>)
> > +                      parent_style: &'a Option<&Arc<ComputedValues>>)
> 
> If we can use &'a Option<&ComputedValues> that would seem better but since
> this is a local function it's probably not a big deal.

It didn't work. I am not sure we can do it but I am going to leave it as it is.

> Alternatively, would it make sense to just pass the element in and get the
> parent style from it here?

This didn't work either due to lifetime problem. I think there may be a way to do it though.

Comment 25

2 years ago
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d820ab5d75a6
Explicitly cast nsStyleContext* to nullptr. r=birtles
https://hg.mozilla.org/integration/autoland/rev/ebb95a9089b1
Early return from ValueFromStringHelper() if the target element is not associated with any documents. r=birtles
https://hg.mozilla.org/integration/autoland/rev/10c6f7837bbd
Get parent style from the target element. r=birtles
https://hg.mozilla.org/integration/autoland/rev/a3470fcc5cc4
Don't get parent style for ComputeAnimationValue. r=birtles
https://hg.mozilla.org/integration/autoland/rev/c66f819cf5e3
Don't get parent style for GetComputedKeyframeValuesFor. r=birtles
https://hg.mozilla.org/integration/autoland/rev/69e9825432fd
Drop ServoComputedValuesWithParent entirely. r=birtles
You need to log in before you can comment on or make changes to this bug.