Closed Bug 1349004 Opened 7 years ago Closed 6 years ago

stylo: Call UpdateProperties with ServoComputedValues (for target element and parent's one) instead of nsStyleContext

Categories

(Core :: CSS Parsing and Computation, enhancement, P4)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED WONTFIX
Tracking Flags:
Tracking Status
firefox57 --- wontfix

People

(Reporter: hiro, Assigned: mantaroh)

References

Details

Attachments

(5 files, 4 obsolete files)

bug1349004.patch
8.47 KB, patch
Details | Diff | Splinter Review
Bug 1349004 part 1 - Make DoSetKeyframe template function and rename to SetKeyframe.
59 bytes, text/x-review-board-request
Details
Bug 1349004 part 2 - Call UpdateProperties with ServoComputedValuesWithParent.
59 bytes, text/x-review-board-request
Details
Refactor SetKeyframes()
8.79 KB, patch
Details | Diff | Splinter Review
Bug 1349004 part 3 - Remove unnecessary UpdateProperties.
59 bytes, text/x-review-board-request
Details 
In current gecko, UpdateProperties is called from two different cases, one is called from script thread when a script animation is created, one is called from nsStyleSet::GetContext when style context for the target element is changed.

Whereas in stylo, UpdateProperties is called from only for the former case. We need to call UpdateProperties in a SequentialTask for the latter case in stylo too.  In doing so, we should call UpdateProperties with servo's computed values even in the case where script animation is generated.
Summary: stylo: Call UpdateProperties with ServoComputedValues (for target element and parent's one) → stylo: Call UpdateProperties with ServoComputedValues (for target element and parent's one) instead of nsStyleContext
P3 as a placeholder priority, feel free to change.
Priority: -- → P3
In bug 1350754 we will call UpdateEffectProperties() with servo's computed values for stylo.
In this bug we need to use ServoStyleSet::ResolveServoStyle in KeyframeEffectReadOnly::SetKeyframes() [1], and drop nsStyleContext::GetParentAllowServo() in KeyframeEffectReadOnly::UpdateProperties().

[1] https://hg.mozilla.org/mozilla-central/file/cc53710589fb/dom/animation/KeyframeEffectReadOnly.cpp#l185
Mantaroh will work on this.
Assignee: nobody → mantaroh
I missed we call UpdateProperties() with nsStyleContext in KeyframeEffect.cpp. We need to fix them too.
Attached patch WIP_bug1349004.patch (obsolete) — — Splinter Review 
This is too rough patch, and my first servo related patch.
So this might be wrong my understand.

hiro, 
Could you please point out where I'm wrong.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> In bug 1350754 we will call UpdateEffectProperties() with servo's computed
> values for stylo.
> In this bug we need to use ServoStyleSet::ResolveServoStyle in
> KeyframeEffectReadOnly::SetKeyframes() [1], and drop
> nsStyleContext::GetParentAllowServo() in
> KeyframeEffectReadOnly::UpdateProperties().
> 
> [1]
> https://hg.mozilla.org/mozilla-central/file/cc53710589fb/dom/animation/
> KeyframeEffectReadOnly.cpp#l185

The parameter of KeyframeEffectReadOnly::DoSetKeyframes will be StyleType is nsStyleContext or ServoComputedValuesWithParent. So I think that we will need to get the parent's servo computed values.
Attachment #8852291 - Flags: feedback?(hikezoe)
Comment on attachment 8852291 [details] [diff] [review]
WIP_bug1349004.patch

Unfortunately this is totally wrong.
We need to get ServoComputedValues by *ResolveServoStyle()* instead of nsStyleContext*.

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #5)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> > In bug 1350754 we will call UpdateEffectProperties() with servo's computed
> > values for stylo.
> > In this bug we need to use ServoStyleSet::ResolveServoStyle in
> > KeyframeEffectReadOnly::SetKeyframes() [1], and drop
> > nsStyleContext::GetParentAllowServo() in
> > KeyframeEffectReadOnly::UpdateProperties().
> > 
> > [1]
> > https://hg.mozilla.org/mozilla-central/file/cc53710589fb/dom/animation/
> > KeyframeEffectReadOnly.cpp#l185
> 
> The parameter of KeyframeEffectReadOnly::DoSetKeyframes will be StyleType is
> nsStyleContext or ServoComputedValuesWithParent. So I think that we will
> need to get the parent's servo computed values.

Yes, right.
Attachment #8852291 - Flags: feedback?(hikezoe) → feedback-
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> Comment on attachment 8852291 [details] [diff] [review]
> WIP_bug1349004.patch
> 
> Unfortunately this is totally wrong.
> We need to get ServoComputedValues by *ResolveServoStyle()* instead of
> nsStyleContext*.

Oops. ServoStyleSet::ResolveServoStyle() does not support pseudo element. We should use ServoStyleSet::ResolveStyleLazily() instead.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> Comment on attachment 8852291 [details] [diff] [review]
> WIP_bug1349004.patch
> 
> Unfortunately this is totally wrong.
> We need to get ServoComputedValues by *ResolveServoStyle()* instead of
> nsStyleContext*.
> 
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #5)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> > > In bug 1350754 we will call UpdateEffectProperties() with servo's computed
> > > values for stylo.
> > > In this bug we need to use ServoStyleSet::ResolveServoStyle in
> > > KeyframeEffectReadOnly::SetKeyframes() [1], and drop
> > > nsStyleContext::GetParentAllowServo() in
> > > KeyframeEffectReadOnly::UpdateProperties().
> > > 
> > > [1]
> > > https://hg.mozilla.org/mozilla-central/file/cc53710589fb/dom/animation/
> > > KeyframeEffectReadOnly.cpp#l185
> > 
> > The parameter of KeyframeEffectReadOnly::DoSetKeyframes will be StyleType is
> > nsStyleContext or ServoComputedValuesWithParent. So I think that we will
> > need to get the parent's servo computed values.
> 
> Yes, right.

Thank you for your feedback.
I misled the nsStyleContext and ServoStyleSet, and I'm going to rewrite this patch.

First try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2682212b43783233d6accc7bc6157390ee56c49
That is on the right track. A terrible mistake is there is that nobody hold the reference of ServoComputedValues returned by GetServoComputedValuesWithParent().  Also we don't need to call GetStyleContext() in case of stylo.
Thanks, hiro,

(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> That is on the right track. A terrible mistake is there is that nobody hold
> the reference of ServoComputedValues returned by
> GetServoComputedValuesWithParent().  Also we don't need to call
> GetStyleContext() in case of stylo.

Second try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6267a5a5a67fcb8c5e164396db6cba987e6cadb
ServoComputedValuesWithParent has just const pointer [1].  

So we will need to something like this:

 RefPtr<ServoComputedValues> computedValues = 
   ResolveStyleLazily();
 RefPtr<ServoComputedValues> parentComputedValues = 
   ResolveStyleLazily();
 ServoComputedValuesWithParent computedValuesWithParent = { computedValues, parentComputedValues };
 UpdateProperties(pairs);

[1] https://hg.mozilla.org/mozilla-central/file/60d7a0496a36/dom/animation/KeyframeEffectReadOnly.h#l149

Also in case of pseudo element, mTarget->mElement is the parent element of the pseudo element, please see the comment in OwningAnimationTarget.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> ServoComputedValuesWithParent has just const pointer [1].  
> 
> So we will need to something like this:
> 
>  RefPtr<ServoComputedValues> computedValues = 
>    ResolveStyleLazily();
>  RefPtr<ServoComputedValues> parentComputedValues = 
>    ResolveStyleLazily();
>  ServoComputedValuesWithParent computedValuesWithParent = { computedValues,
> parentComputedValues };
>  UpdateProperties(pairs);

Oops, sorry. UpdateProperties(computedValuesWithParent);
Attached patch bug1349004.patch — — Splinter Review 
Thanks hiro,

(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> > ServoComputedValuesWithParent has just const pointer [1].  
> > 
> > So we will need to something like this:
> > 
> >  RefPtr<ServoComputedValues> computedValues = 
> >    ResolveStyleLazily();
> >  RefPtr<ServoComputedValues> parentComputedValues = 
> >    ResolveStyleLazily();
> >  ServoComputedValuesWithParent computedValuesWithParent = { computedValues,
> > parentComputedValues };
> >  UpdateProperties(pairs);
> 
> Oops, sorry. UpdateProperties(computedValuesWithParent);

I wrote this changes.
But I don't have a much knowledge of pseudo implementation. In my understand, pseudo element hasn't content node, Is it right?
Attachment #8852291 - Attachment is obsolete: true
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #13)

> I wrote this changes.
> But I don't have a much knowledge of pseudo implementation. In my
> understand, pseudo element hasn't content node, Is it right?

We have generated content for pseudo element.
Comment on attachment 8853217 [details] [diff] [review]
bug1349004.patch

Review of attachment 8853217 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/KeyframeEffect.cpp
@@ +183,5 @@
> +    } else {
> +      RefPtr<nsStyleContext> styleContext = GetTargetStyleContext();
> +      if (styleContext) {
> +        UpdateProperties(styleContext);
> +      }

Could you please add a helper function to do these process rather than repeating here and there.
Note that I did talk with Cameron on IRC about using ResolveStyleLazily().  He told me that ResolveStyleLazily() does not call PreTraverseSync().  We will need a new method similar to ResolveStyleLazily() but calling PreTraverseSync() if there is a case that we need to call PreTraverseSync().
Thanks, I'm going update the patch.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #16)
> Note that I did talk with Cameron on IRC about using ResolveStyleLazily(). 
> He told me that ResolveStyleLazily() does not call PreTraverseSync().  We
> will need a new method similar to ResolveStyleLazily() but calling
> PreTraverseSync() if there is a case that we need to call PreTraverseSync().

I faced the crash that mTarget->mElement->GetComposeDocument() is null on crashtest(1239889-1.html).
This test is creating the keyframe with created element, this element doesn't belong with any document.

I think that we will need to fix KeyframeUtils::GetComputedKeyframeValues() too. We don't call this function from current code. [1]
https://dxr.mozilla.org/mozilla-central/rev/3364cc17988c013c36f2a8123315db2855393011/dom/animation/KeyframeUtils.cpp#595

In this function, we get presShell from element of parameter. However this element doesn't have a document in this case. So I think that we will need to use presShell which getting from mDocument of KeyframeEffectReadOnly or others.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #17)
> Thanks, I'm going update the patch.
> 
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #16)
> > Note that I did talk with Cameron on IRC about using ResolveStyleLazily(). 
> > He told me that ResolveStyleLazily() does not call PreTraverseSync().  We
> > will need a new method similar to ResolveStyleLazily() but calling
> > PreTraverseSync() if there is a case that we need to call PreTraverseSync().
> 
> I faced the crash that mTarget->mElement->GetComposeDocument() is null on
> crashtest(1239889-1.html).
> This test is creating the keyframe with created element, this element
> doesn't belong with any document.
> 
> I think that we will need to fix KeyframeUtils::GetComputedKeyframeValues()
> too. We don't call this function from current code. [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 3364cc17988c013c36f2a8123315db2855393011/dom/animation/KeyframeUtils.cpp#595
> 
> In this function, we get presShell from element of parameter. However this
> element doesn't have a document in this case. So I think that we will need
> to use presShell which getting from mDocument of KeyframeEffectReadOnly or
> others.

This is not a right thing to do. We should check returned value by GetCurrentServoComputedValues(), the function returns nullptr when the target element is not associated with any documents.  This is the same condition as we do KeyframeEffectReadOnly::GetTargetContext().
Please split adding the new method for ServoStyleSet as a separate patch.
Also I think we can drop the call of ApplyDistributeSpacing() there because paced spacing will be removed.  I think it's for the case where the target element is not associated with documents, but I guess we don't have such test cases in our tree.
Comment on attachment 8855211 [details]
Bug 1349004 part 2 - Add ResolveServoStyle for get resolved servo's computed values.

https://reviewboard.mozilla.org/r/127080/#review129858

Is there any reason why we don't call ResolveStyleLazily() inside this function?
Comment on attachment 8855212 [details]
Bug 1349004 part 3 - Use UpdateProperties with ServoComputedValues.

https://reviewboard.mozilla.org/r/127082/#review129860

::: dom/animation/KeyframeEffect.cpp:113
(Diff revision 1)
> -    RefPtr<nsStyleContext> styleContext = GetTargetStyleContext();
> -    if (styleContext) {
> +    if (!UpdatePropertiesFromCurrentStyle() &&
> +        mEffectOptions.mSpacingMode == SpacingMode::paced) {

As we discussed last week, I think we don't need to check SpacingMode::paced here.  Did you find test cases rely on this? If so, let's remove the test cases first.

::: dom/animation/KeyframeEffectReadOnly.cpp:1840
(Diff revision 1)
> +  RefPtr<ServoComputedValues> computedValues =
> +    styleSet->ResolveServoStyleWithPreTraverseSync(mTarget->mElement, pseudo);
> +
> +  RefPtr<ServoComputedValues> parentComputedValues =
> +    styleSet->ResolveServoStyleWithPreTraverseSync(parent, pseudo);
> +

As I commented in comment 9, nobody holds these computed values. We should hold the reference until we finished calling UpdateProperties().
Comment on attachment 8855212 [details]
Bug 1349004 part 3 - Use UpdateProperties with ServoComputedValues.

https://reviewboard.mozilla.org/r/127082/#review129860

> As we discussed last week, I think we don't need to check SpacingMode::paced here.  Did you find test cases rely on this? If so, let's remove the test cases first.

No. There aren't these tests. So I drop this check and calling ApplyDistributeSpacing.
Comment on attachment 8855211 [details]
Bug 1349004 part 2 - Add ResolveServoStyle for get resolved servo's computed values.

https://reviewboard.mozilla.org/r/127080/#review130204

::: commit-message-5182b:1
(Diff revision 2)
> +Bug 1349004 part 1 - Add ResolveServoStyleWithPreTraverseSync for updating properties with stylo. r=hiro

This commit message is not quite right.
The new function is for get resolved servo's computed values without generating nsStyleContext.

::: layout/style/ServoStyleSet.h:237
(Diff revision 2)
>    already_AddRefed<ServoComputedValues> ResolveServoStyle(dom::Element* aElement);
>  

I am guessing we should rename this existent ResolveServoStyle() to ResolveServoStyleWithoutSyncUpData, or... It seems to me that ResolveServoStyle() just returns already resolved style, so ResolvedServoStyle()?

And the new function can be named ResolveServoStyle()?

::: layout/style/ServoStyleSet.h:240
(Diff revision 2)
>     * ServoComputedValues, not an nsStyleContext.
>     */
>    already_AddRefed<ServoComputedValues> ResolveServoStyle(dom::Element* aElement);
>  
> +  /**
> +   * Resolve style for the given element and pseudo information,

for the given (pseudo-)element.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
> ::: layout/style/ServoStyleSet.h:237
> (Diff revision 2)
> >    already_AddRefed<ServoComputedValues> ResolveServoStyle(dom::Element* aElement);
> >  
> 
> I am guessing we should rename this existent ResolveServoStyle() to
> ResolveServoStyleWithoutSyncUpData, or... It seems to me that
> ResolveServoStyle() just returns already resolved style, so
> ResolvedServoStyle()?
> 
> And the new function can be named ResolveServoStyle()?

Cameron, this new function is what we talked about last week.
We'd like to use this to get up-to-date servo's computed style for script animations.
What do you think about it name?
Flags: needinfo?(cam)
Comment on attachment 8855212 [details]
Bug 1349004 part 3 - Use UpdateProperties with ServoComputedValues.

https://reviewboard.mozilla.org/r/127082/#review130200

::: dom/animation/KeyframeEffectReadOnly.cpp:12
(Diff revision 2)
>  #include "mozilla/dom/KeyframeEffectReadOnly.h"
>  
>  #include "mozilla/dom/KeyframeAnimationOptionsBinding.h"
>    // For UnrestrictedDoubleOrKeyframeAnimationOptions;
>  #include "mozilla/dom/CSSPseudoElement.h"
> +#include "mozilla/dom/ElementInlines.h"

Is this really necessary?

::: dom/animation/KeyframeEffectReadOnly.cpp:1843
(Diff revision 2)
> +  return styleSet->ResolveServoStyleWithPreTraverseSync(mTarget->mElement, pseudo);
> +}
> +
> +already_AddRefed<ServoComputedValues>
> +KeyframeEffectReadOnly::GetParentServoComputedValues()
> +{

I don't prefer these two functions. They have duplicate codes.

I am guessing KeyframeEffect::UpdatePropertiesFromCurrentStyle() can be a template function.

template<typename Process>
void
KeyframeEffectReadOnly::ProcessXX(Process&& aProcess)
{
  if (mDocument->IsStyleByServo()) {
    nsPresContext* presContext = nsContentUtils::GetContextForElement(mTarget->mElement);
    // We might need to return early if presContext is nullptr, but I am not sure it really happens.
    
    ServoStyleSet* styleSet = presContext->StyleSet()->AsServo();
    MOZ_ASSERT(styleSet);
    
    nsIAtom* pseudo = GetPseudo;
    RefPtr<ServoComputedValues> currrentValues =
      styleSet->ResolveServoStyleXX(mTarget->mElement, pseudo);
    Element* parent = pseudo
                    ? mTarget->mElement.get()
                    : mtarget->mElement->GetFlatten...();
    RefPtr<ServoComputedValues> parentValues;
    if (parent) {
      parentValues =
        styleSet->ResolveServoStyleXX(mTarget->mElement, nullptr); // We don't need to pass pseudo for the parent.
    }
    const ServoComputedValuesWithParent v = { .., ..};
    aProcess(servoComputedValuesWithParent);
  } else {
    RefPtr<nsStyleContext> styleContext = GetTargetContext();
    aProcess(styleContext);
  }
}

'Process' is actually a bad name, so we should name proper one.

::: dom/animation/KeyframeEffectReadOnly.cpp:1875
(Diff revision 2)
> +KeyframeEffectReadOnly::GetPseudoFromAnimationTarget()
> +{
> +  return mTarget->mPseudoType < CSSPseudoElementType::Count
> +                  ? nsCSSPseudoElements::GetPseudoAtom(mTarget->mPseudoType)
> +                  : nullptr;
> +}

I don't think this utility is necessary. nsCSSPseudoElement::GetPsuedoAtom() should return nullptr if the given pseudo type is not pseudo, I think.  See bug 1353238.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #30)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
> > ::: layout/style/ServoStyleSet.h:237
> > (Diff revision 2)
> > >    already_AddRefed<ServoComputedValues> ResolveServoStyle(dom::Element* aElement);
> > >  
> > 
> > I am guessing we should rename this existent ResolveServoStyle() to
> > ResolveServoStyleWithoutSyncUpData, or... It seems to me that
> > ResolveServoStyle() just returns already resolved style, so
> > ResolvedServoStyle()?
> > 
> > And the new function can be named ResolveServoStyle()?
> 
> Cameron, this new function is what we talked about last week.
> We'd like to use this to get up-to-date servo's computed style for script
> animations.
> What do you think about it name?

After discussion with Cameron about this, we should call FlushPendingNotifications(Style) and ServoStyleSet::Resolve(dom::Element*).

I am doing it in bug 1324700.
Flags: needinfo?(cam)
Comment on attachment 8855212 [details]
Bug 1349004 part 3 - Use UpdateProperties with ServoComputedValues.

https://reviewboard.mozilla.org/r/127082/#review130200

> Is this really necessary?

I used the Element::GetFlattenedTreeParentElementForStyle(), this inline function defined in ElementInlines.h.
When building, clang warn that 'inline function ...GetFlattenedTreeParentElementFroStyle() const used but never deinfed.', if we remove this include line.

> I don't prefer these two functions. They have duplicate codes.
> 
> I am guessing KeyframeEffect::UpdatePropertiesFromCurrentStyle() can be a template function.
> 
> template<typename Process>
> void
> KeyframeEffectReadOnly::ProcessXX(Process&& aProcess)
> {
>   if (mDocument->IsStyleByServo()) {
>     nsPresContext* presContext = nsContentUtils::GetContextForElement(mTarget->mElement);
>     // We might need to return early if presContext is nullptr, but I am not sure it really happens.
>     
>     ServoStyleSet* styleSet = presContext->StyleSet()->AsServo();
>     MOZ_ASSERT(styleSet);
>     
>     nsIAtom* pseudo = GetPseudo;
>     RefPtr<ServoComputedValues> currrentValues =
>       styleSet->ResolveServoStyleXX(mTarget->mElement, pseudo);
>     Element* parent = pseudo
>                     ? mTarget->mElement.get()
>                     : mtarget->mElement->GetFlatten...();
>     RefPtr<ServoComputedValues> parentValues;
>     if (parent) {
>       parentValues =
>         styleSet->ResolveServoStyleXX(mTarget->mElement, nullptr); // We don't need to pass pseudo for the parent.
>     }
>     const ServoComputedValuesWithParent v = { .., ..};
>     aProcess(servoComputedValuesWithParent);
>   } else {
>     RefPtr<nsStyleContext> styleContext = GetTargetContext();
>     aProcess(styleContext);
>   }
> }
> 
> 'Process' is actually a bad name, so we should name proper one.

In this bug, I thinkt that aProcess is UpdateProperties. There are two function of this which parameter is different. So I think that we can call this function without template function.
Comment on attachment 8855211 [details]
Bug 1349004 part 2 - Add ResolveServoStyle for get resolved servo's computed values.

https://reviewboard.mozilla.org/r/127080/#review133144

Clearing review request since this will not need once bug 1324700 landed.
Attachment #8855211 - Flags: review?(hikezoe)
Comment on attachment 8856344 [details]
Bug 1349004 part 1  - Rename ResolveServoStyle to ResolveServoStyleWithoutSyncUpData

https://reviewboard.mozilla.org/r/128256/#review133146

This is not needed either.
Attachment #8856344 - Flags: review?(hikezoe)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
We still need the part 3 patch here.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Comment on attachment 8855212 [details]
Bug 1349004 part 3 - Use UpdateProperties with ServoComputedValues.

https://reviewboard.mozilla.org/r/127082/#review134224

::: dom/animation/KeyframeEffectReadOnly.cpp:188
(Diff revision 4)
>      KeyframeUtils::GetKeyframesFromObject(aContext, mDocument, aKeyframes, aRv);
>    if (aRv.Failed()) {
>      return;
>    }
>  
> -  RefPtr<nsStyleContext> styleContext = GetTargetStyleContext();
> +  SetKeyframes(Move(keyframes), nullptr);

I don't think it's a good idea to change the function behavior when passing nullptr. It's not easy to know what the nullptr exactly means from caller side just like passing a boolean.

So, instead, how about adding SetKeyframes(nsTArray<Keyframe>&& aKeyframes) ?
Anyway, this patch should be revised since we have now ResolveTransientServoStyle() and can use it in this patch.
Attachment #8855212 - Flags: review?(hikezoe)
Attachment #8856344 - Attachment is obsolete: true
Attachment #8855211 - Attachment is obsolete: true
Attachment #8855212 - Attachment is obsolete: true
Comment on attachment 8860316 [details]
Bug 1349004 part 1 - Make DoSetKeyframe template function and rename to SetKeyframe.

https://reviewboard.mozilla.org/r/132344/#review135974

I am reluctant to add another SetKeyframes function here.  We should drop SetKeyframes(nsTArray<Keyframes>&&, nsStyleContext*) and SetKeyframes(nsTArray<Keyframe>&&, const ServoComptuedValuesWithParent&), and make 'template<typename StyleType> DoSetKeyframes(nsTArray<Keyframe>&&, StyleType&& aStyleType)' public (and rename it to SetKeyframes).

I know there is a caller of SetKeyframes(nsTArray<Keyframe>&&, nsStyleContext*) in nsTransitionManager.cpp, so we end up having three different SetKeyframes(nsTArray<Keyframe>&&, StyleType&& aStyleType) in binary but I think it's acceptable.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #45)
> Comment on attachment 8860316 [details]
> Bug 1349004 part1 - Separate KeyframeEffectReadOnly::DoSetKeyframes in order
> to adding UpdateProperty function which is not need style context.
> 
> https://reviewboard.mozilla.org/r/132344/#review135974
> 
> I am reluctant to add another SetKeyframes function here.  We should drop
> SetKeyframes(nsTArray<Keyframes>&&, nsStyleContext*) and
> SetKeyframes(nsTArray<Keyframe>&&, const ServoComptuedValuesWithParent&),
> and make 'template<typename StyleType> DoSetKeyframes(nsTArray<Keyframe>&&,
> StyleType&& aStyleType)' public (and rename it to SetKeyframes).
> 
> I know there is a caller of SetKeyframes(nsTArray<Keyframe>&&,
> nsStyleContext*) in nsTransitionManager.cpp, so we end up having three
> different SetKeyframes(nsTArray<Keyframe>&&, StyleType&& aStyleType) in
> binary but I think it's acceptable.

To be clear, I don't object add 'SetKeyframes(nsTArray<Keyframe>&& aKeyframes)', I do propose to add 'SetKeyframes(nsTArray<Keyframe>&& aKeyframes)' and drop existent SetKeyframes(nsTArray<Keyframes>&&, nsStyleContext*) and SetKeyframes(nsTArray<Keyframes>&&, const ServoComputedValuesWithParent&).
Mantaroh, what is the current status?  Other people refer to this original implementation (using GetParentAllowServo) (bug 1355348 and bug 1346052). I am now thinking we should fix this bug as far as possible.
For your reference, this is what I was thinking in comment 42. I haven't run any tests with this patch, just confirmed build success.
Thanks hiro,

(In reply to Hiroyuki Ikezoe (:hiro) from comment #48)
> Created attachment 8862205 [details] [diff] [review]
> Refactor SetKeyframes()
> 
> For your reference, this is what I was thinking in comment 42. I haven't run
> any tests with this patch, just confirmed build success.

Sorry for late reply.
I re-wrote the patch, So could you please confirm again?

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9f799a854d93f22cd3d5ce84f3c44c83a6aba30
Comment on attachment 8860317 [details]
Bug 1349004 part 2 - Call UpdateProperties with ServoComputedValuesWithParent.

https://reviewboard.mozilla.org/r/132346/#review141452

Thanks! Looks much better now. But still needs some works.

1. We need to call FlushPendingNotifications() as I commented in comment 32 because nsComputedDOMStyle::GetStyleContext() does so.
   Also, FlushPendingNotifications() might be destroy pres shell, so we need to hold a reference of the pres shell. For this reason, I don't think it's a good idea to have two separate functions (GetCurrentServoComputedValues and GetParentServoComputedValues) because we need to call FlushPendingNotifications in both functions.  (You may think that we can hold the reference outside the functions and then call two functions respectively, but it's error-prone. Actually we don't currently hold a reference of nsPresContext there).

::: dom/animation/KeyframeEffectReadOnly.cpp:197
(Diff revision 3)
> +      nsStyleContext* value = nullptr;
> +      SetKeyframes(Move(keyframes), value);

2.  This is really horrible. We need to figure out avoid this.  Should we factor out most of part of DoSetKeyframes() other than UpdateProperties() and MaybeUpdateFrameForCompositor(), and then call it when the targe element is not associated with document?
    Also, This pres context check can be applied for gecko's case, we can check it outside 'if (mDocument->IsStyledByServo())'.
See Also: → 1367293
Comment on attachment 8860316 [details]
Bug 1349004 part 1 - Make DoSetKeyframe template function and rename to SetKeyframe.

https://reviewboard.mozilla.org/r/132344/#review145892
Attachment #8860316 - Flags: review?(hikezoe)
Comment on attachment 8860317 [details]
Bug 1349004 part 2 - Call UpdateProperties with ServoComputedValuesWithParent.

https://reviewboard.mozilla.org/r/132346/#review145894
Attachment #8860317 - Flags: review?(hikezoe)
Comment on attachment 8866650 [details]
Bug 1349004 part 3 - Remove unnecessary UpdateProperties.

https://reviewboard.mozilla.org/r/138254/#review145896
Attachment #8866650 - Flags: review?(hikezoe)
Priority: P3 → --
Priority: -- → P4
status-firefox57=wontfix unless someone thinks this bug should block 57
status-firefox57: --- → wontfix
I think this is no longer worth doing.  We will drop gecko part soon and we no longer pass parent style for servo (bug 1367293).
Status: REOPENED → RESOLVED
Closed: 7 years ago → 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: