Closed Bug 1302637 Opened 8 years ago Closed 7 years ago

Animation::PostUpdate() should call KeyframeEffectReadOnly::RequestRestyle()

Categories

(Core :: DOM: Animation, defect, P5)

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

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

People

(Reporter: hiro, Assigned: bharatraghunthan9767, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(1 file)

Bug 1302637 - Animation::PostUpdate() calls KeyframeEffectReadonly::RequestRestyle()
59 bytes, text/x-review-board-request
Details 
Now we have KeyframeEffectReadOnly::RequestRestyle() so we should use it in Animation::PostUpdate().  Or create KeyframeEffectReadOnly::PostUpdate()?
Mentor: hikezoe
Keywords: good-first-bug
Should https://dxr.mozilla.org/mozilla-central/source/dom/animation/Animation.cpp#1327-1337 
  nsPresContext* presContext = keyframeEffect->GetPresContext();
  if (!presContext) {
    return;
  }

  presContext->EffectCompositor()
             ->RequestRestyle(target->mElement,
                              target->mPseudoType,
                              EffectCompositor::RestyleType::Layer,
                              CascadeLevel());
}


be replaced with a single function call
KeyframeEffectReadOnly::RequestRestyle(EffectCompositor::RestyleType aRestyleType) ?
Flags: needinfo?(hikezoe)
Also, should https://dxr.mozilla.org/mozilla-central/source/dom/animation/KeyframeEffectReadOnly.cpp#1452-1460
nsPresContext*
KeyframeEffectReadOnly::GetPresContext() const
{
  nsIPresShell* shell = GetPresShell();
  if (!shell) {
    return nullptr;
  }
  return shell->GetPresContext();
}

be dropped/removed?
Comment on attachment 8847182 [details]
Bug 1302637 - Animation::PostUpdate() calls KeyframeEffectReadonly::RequestRestyle()

Are the parameters for KeyframeEffectReadonly::RequestRestyle() correct?
Attachment #8847182 - Flags: review?(hikezoe)
Comment on attachment 8847182 [details]
Bug 1302637 - Animation::PostUpdate() calls KeyframeEffectReadonly::RequestRestyle()

https://reviewboard.mozilla.org/r/120180/#review122206

Not quite right. :-)

RequestRestyle is a member function of KeyframeEffectReadOnly.  We have a pointer of KeyframeEffectReadOnly in Animation::PostUpdate(). So, we can call the function just like:

keyframeEffect->RequestRestyle(EffectCompositor::RestyleType::Layer);

Also, you can drop GetPresContext() from KeyframeEffectReadOnly.h.

I'd recommend you to build firefore with this patch before submitting it.

Thanks!
Attachment #8847182 - Flags: review?(hikezoe)
Assignee: nobody → bharatraghunthan9767
Status: NEW → ASSIGNED
Flags: needinfo?(hikezoe)
Flags: needinfo?(hikezoe)
Attachment #8847182 - Flags: review?(hikezoe)
Comment on attachment 8847182 [details]
Bug 1302637 - Animation::PostUpdate() calls KeyframeEffectReadonly::RequestRestyle()

https://reviewboard.mozilla.org/r/120180/#review122388

Thanks for updating!

::: commit-message-2baef:3
(Diff revision 2)
> +Also removed the instance of GetPresContext() in KeyframeEffectReadOnly.cpp
> +and KeyframeEffectReadOnly.h

GetPresContext() is not an instance.  It's just a function. But yeah 'instance' might have a meaning what I don't know.

::: dom/animation/Animation.cpp:18
(Diff revision 2)
>  #include "mozilla/AutoRestore.h"
>  #include "mozilla/AsyncEventDispatcher.h" // For AsyncEventDispatcher
>  #include "mozilla/Maybe.h" // For Maybe
>  #include "mozilla/AnimationRule.h" // For AnimationRule
>  #include "nsAnimationManager.h" // For CSSAnimation
> +#include "nsContentUtils.h"

We don't need to include nsContentUtils.h.

::: dom/animation/Animation.cpp:1323
(Diff revision 2)
>    KeyframeEffectReadOnly* keyframeEffect = mEffect->AsKeyframeEffect();
>    if (!keyframeEffect) {
>      return;
>    }
>  
>    Maybe<NonOwningAnimationTarget> target = keyframeEffect->GetTarget();

We can also drop this target variable.

::: dom/animation/Animation.cpp:1324
(Diff revision 2)
>    if (!keyframeEffect) {
>      return;
>    }
>  
>    Maybe<NonOwningAnimationTarget> target = keyframeEffect->GetTarget();
> -  if (!target) {
> +  keyframeEffect->RequestRestyle(EffectCompositor::RestyleType::Layer aRestyleTypeLayer);

I think this can't be compiled yet.

keyframeEffect->RequestRestyle(EffectCompositor::RestyleType::Layer);
Attachment #8847182 - Flags: review?(hikezoe)
Bharat, you don't need to set needinfo every time you requested review.
Flags: needinfo?(hikezoe)
Attachment #8847182 - Flags: review?(hikezoe)
Comment on attachment 8847182 [details]
Bug 1302637 - Animation::PostUpdate() calls KeyframeEffectReadonly::RequestRestyle()

https://reviewboard.mozilla.org/r/120180/#review122404

Thanks!  I just pushed a try.

::: dom/animation/Animation.cpp:1321
(Diff revisions 2 - 3)
>  
>    KeyframeEffectReadOnly* keyframeEffect = mEffect->AsKeyframeEffect();
>    if (!keyframeEffect) {
>      return;
>    }
> -
> +  

Nit: extra spaces.
Attachment #8847182 - Flags: review?(hikezoe) → review+
Oops, sorry. I missed KeyframeEffectReadOnly::RequestRestyle is a protected. You need to move the declaration to public section.  I'd definitely recommend you to build firefox with patches.  Thanks!
Comment on attachment 8847182 [details]
Bug 1302637 - Animation::PostUpdate() calls KeyframeEffectReadonly::RequestRestyle()

Any more changes in KeyframeEffectReadOnly.h or in KeyframeEffectReadOnly.cpp?
Flags: needinfo?(hikezoe)
Attachment #8847182 - Flags: review?(hikezoe)
Comment on attachment 8847182 [details]
Bug 1302637 - Animation::PostUpdate() calls KeyframeEffectReadonly::RequestRestyle()

https://reviewboard.mozilla.org/r/120180/#review122656

::: dom/animation/KeyframeEffectReadOnly.h:216
(Diff revision 4)
>      mEffectOptions.GetSpacingAsString(aRetVal);
>    }
>  
>    void NotifyAnimationTimingUpdated();
>  
> +  

Nit: extra spaces.

::: dom/animation/KeyframeEffectReadOnly.h:219
(Diff revision 4)
>    void NotifyAnimationTimingUpdated();
>  
> +  
> +  void RequestRestyle(EffectCompositor::RestyleType aRestyleType);
> +
> +  // Update the associated frame state bits so that, if necessary, a stacking

Please move this comment back to the original position. It's for MaybeUpdateFrameForCompositor().
Attachment #8847182 - Flags: review?(hikezoe) → review+
Attachment #8847182 - Flags: review?(hikezoe)
Comment on attachment 8847182 [details]
Bug 1302637 - Animation::PostUpdate() calls KeyframeEffectReadonly::RequestRestyle()

https://reviewboard.mozilla.org/r/120180/#review122738

::: dom/animation/KeyframeEffectReadOnly.h:215
(Diff revision 5)
>    {
>      mEffectOptions.GetSpacingAsString(aRetVal);
>    }
>  
>    void NotifyAnimationTimingUpdated();
> +  

Could you please drop these extra spaces as well?
Attachment #8847182 - Flags: review?(hikezoe) → review+
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2b5c91cdb7f
Animation::PostUpdate() calls KeyframeEffectReadonly::RequestRestyle() r=hiro
Comment on attachment 8847182 [details]
Bug 1302637 - Animation::PostUpdate() calls KeyframeEffectReadonly::RequestRestyle()

Can you please check if all spaces are fixed, and if so autoland the patch as the try server testing seems to have completed?
Attachment #8847182 - Flags: review?(hikezoe)
Yeah, I just checked it and landed now.
Thanks a lot!
Flags: needinfo?(hikezoe)
Attachment #8847182 - Flags: review?(hikezoe)
https://hg.mozilla.org/mozilla-central/rev/b2b5c91cdb7f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Thanks for helping, :hiro <hikezoe@mozilla.com>!!!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: