Closed Bug 1367293 Opened 7 years ago Closed 7 years ago

stylo: Eliminate passing parent computed values to Servo_GetComputedKeyframeValues

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

(Blocks 2 open bugs)

Details

Attachments

(6 files, 1 obsolete file)

Bug 1367293 - Explicitly cast nsStyleContext* to nullptr.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1367293 - Early return from ValueFromStringHelper() if the target element is not associated with any documents.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1367293 - Get parent style from the target element.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1367293 - Don't get parent style for ComputeAnimationValue.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1367293 - Don't get parent style for GetComputedKeyframeValuesFor.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1367293 - Drop ServoComputedValuesWithParent entirely.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
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.
Doh!  I wanted to add bug 1288269 into block list..
Blocks: stylo-css-animations
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 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 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 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 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 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 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 on attachment 8873613 [details]
Bug 1367293 - Drop ServoComputedValuesWithParent entirely.

https://reviewboard.mozilla.org/r/145002/#review148994
Attachment #8873613 - Flags: review?(bbirtles) → review+
Attachment #8873609 - Attachment is obsolete: true
(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.
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.

Attachment

General

Creator:
Created:
Updated:
Size: