Closed Bug 1302888 Opened 7 years ago Closed 6 years ago

Use nsContentUtils::GetContextForContent() to obtain nsPresContext* in dom/animation

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)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file)

Bug 1302888 - Replace GetPresContext() with nsContentUtils::GetContextForContent() to obtain nsPresContext* in dom/animation
59 bytes, text/x-review-board-request
hiro
: review+
Details 
We have nsContentUtils::GetContextForContent[1] which does call GetComposedDoc(), GetShell() and GetPresContext().  We can replace EffectCompositor::GetPresContext() and KeyframeEffectReadOnly::GetPresContext() with it.

http://hg.mozilla.org/mozilla-central/file/8a494adbc5cc/dom/base/nsContentUtils.h#l657
Mentor: hikezoe
Keywords: good-first-bug
I've already built Firefox. Please check my submission at https://reviewboard-hg.mozilla.org/gecko/rev/03710a8a902e as I'm unable to submit a MozReview request even after I run 'hg push review' command.
Flags: needinfo?(hikezoe)
Thanks for taking this!

(In reply to Bharat Raghunathan from comment #1)

> I've already built Firefox. Please check my submission at
> https://reviewboard-hg.mozilla.org/gecko/rev/03710a8a902e as I'm unable to
> submit a MozReview request even after I run 'hg push review' command.

What was the error when you failed to push?
Instead, you can also post the patch here in bugzilla directly.

Anyway your current patch is slightly different from what I expected. What I expected is, for example:

https://hg.mozilla.org/mozilla-central/file/eb2364853477/dom/animation/EffectCompositor.cpp#l886
  nsPresContext* presContext = GetPresContext(aElement);
  if (!presContext) {
    return;
  }

This GetPresContext(aElement) call can be replaced with nsContentUtils::GetContextForContent(aElement).

Another example is:
https://hg.mozilla.org/mozilla-central/file/eb2364853477/dom/animation/KeyframeEffectReadOnly.cpp#l1005
  nsPresContext* presContext = GetPresContext();
  if (presContext && mTarget && mAnimation) {
    presContext->EffectCompositor()->
      RequestRestyle(mTarget->mElement, mTarget->mPseudoType,
                     aRestyleType, mAnimation->CascadeLevel());
  }

We can also replace this GetPresContext() call with:
  if (!mTarget) {
    return;
  }

  nsPresContext* presContext = nsContentUtils::GetContextForContent(mTarget->mElement):
  if (presContext && mAnimation) {
    ...
  }

We have other callers of GetPresContext() in KeyframeEffectReadOnly.cpp:
https://hg.mozilla.org/mozilla-central/file/eb2364853477/dom/animation/KeyframeEffectReadOnly.cpp#l540
https://hg.mozilla.org/mozilla-central/file/eb2364853477/dom/animation/KeyframeEffectReadOnly.cpp#l1695

But please leave them as it is, we will replace them mDocument->IsStyledByServo() in later bug. Or if you can do it in this bug it will be appreciated.

Thanks!
Assignee: nobody → bharatraghunthan9767
Flags: needinfo?(hikezoe)
See Also: → 1344533
I just posted a very similar patch in bug 1344533. I hope the patch helps you.
Thank you,
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)

> We have other callers of GetPresContext() in KeyframeEffectReadOnly.cpp:
> https://hg.mozilla.org/mozilla-central/file/eb2364853477/dom/animation/
> KeyframeEffectReadOnly.cpp#l540
> https://hg.mozilla.org/mozilla-central/file/eb2364853477/dom/animation/
> KeyframeEffectReadOnly.cpp#l1695
> 
> But please leave them as it is, we will replace them
> mDocument->IsStyledByServo() in later bug. Or if you can do it in this bug
> it will be appreciated.

I did post a patch for this in bug 1344598, once that bus is closed, you don't need to worry about these two callers.
Thanks.
Comment on attachment 8846536 [details]
Bug 1302888 - Replace GetPresContext() with nsContentUtils::GetContextForContent() to obtain nsPresContext* in dom/animation

Please check if modifying these two function calls are enough or if there are more instances that need to be changed
Flags: needinfo?(hikezoe)
Attachment #8846536 - Flags: review?(hikezoe)
Can you please tell me if  in                                                            https://hg.mozilla.org/mozilla-central/file/eb2364853477/dom/animation/KeyframeEffectReadOnly.cpp#l1442:
nsPresContext*
KeyframeEffectReadOnly::GetPresContext() const
{
  nsIPresShell* shell = GetPresShell();
  if (!shell) {
    return nullptr;
  }
  return shell->GetPresContext();
}

and in
https://hg.mozilla.org/mozilla-central/file/eb2364853477/dom/animation/EffectCompositor.cpp#l915:
/* static */ nsPresContext*
EffectCompositor::GetPresContext(Element* aElement)
{
  MOZ_ASSERT(aElement);
  nsIPresShell* shell = nsComputedDOMStyle::GetPresShellForContent(aElement);
  if (!shell) {
    return nullptr;
  }
  return shell->GetPresContext();
}
need to be modified?
Thanks.
Comment on attachment 8846536 [details]
Bug 1302888 - Replace GetPresContext() with nsContentUtils::GetContextForContent() to obtain nsPresContext* in dom/animation

https://reviewboard.mozilla.org/r/119596/#review121456

Thanks for doing this!

I think we can also drop EffectCompositor::GetPresContext().  Would you mind update this patch with dropping the function?

::: dom/animation/KeyframeEffectReadOnly.cpp:1001
(Diff revision 1)
>    EffectCompositor::RestyleType aRestyleType)
>  {
> -  nsPresContext* presContext = GetPresContext();
> -  if (presContext && mTarget && mAnimation) {
> +   if (!mTarget) {
> +    return;
> +  }
> +  

nit: Please drop these extra spaces.
Attachment #8846536 - Flags: review?(hikezoe) → review+
(In reply to Bharat Raghunathan from comment #7)
> https://hg.mozilla.org/mozilla-central/file/eb2364853477/dom/animation/
> EffectCompositor.cpp#l915:
> /* static */ nsPresContext*
> EffectCompositor::GetPresContext(Element* aElement)
> {
>   MOZ_ASSERT(aElement);
>   nsIPresShell* shell = nsComputedDOMStyle::GetPresShellForContent(aElement);
>   if (!shell) {
>     return nullptr;
>   }
>   return shell->GetPresContext();
> }
> need to be modified?

Yes, exactly. We don't need this any more.  We can drop this.

> Can you please tell me if  in                                               
> https://hg.mozilla.org/mozilla-central/file/eb2364853477/dom/animation/
> KeyframeEffectReadOnly.cpp#l1442:
> nsPresContext*
> KeyframeEffectReadOnly::GetPresContext() const
> {
>   nsIPresShell* shell = GetPresShell();
>   if (!shell) {
>     return nullptr;
>   }
>   return shell->GetPresContext();
> }

As for this, we can drop this in bug 1302637. If you will take bug 1302637, it would be much appreciated. Thank you!
Flags: needinfo?(hikezoe)
Comment on attachment 8846536 [details]
Bug 1302888 - Replace GetPresContext() with nsContentUtils::GetContextForContent() to obtain nsPresContext* in dom/animation

We should wait for this bug to be fixed before working on Bug 1302637 maybe (because the same file KeyframeEffectReadonly.cpp is getting modified there also? )
Also, why didn't lint check show the spacing problem(which you mentioned as a nit)?
Flags: needinfo?(hikezoe)
Attachment #8846536 - Flags: review?(hikezoe)
Comment on attachment 8846536 [details]
Bug 1302888 - Replace GetPresContext() with nsContentUtils::GetContextForContent() to obtain nsPresContext* in dom/animation

https://reviewboard.mozilla.org/r/119596/#review121670

Thanks for updating.  Once you got r+, you don't usually need to ask review again.
Attachment #8846536 - Flags: review?(hikezoe) → review+
(In reply to Bharat Raghunathan from comment #11)
> Comment on attachment 8846536 [details]
> Bug 1302888 - Replace GetPresContext() with
> nsContentUtils::GetContextForContent() to obtain nsPresContext* in
> dom/animation
> 
> We should wait for this bug to be fixed before working on Bug 1302637 maybe
> (because the same file KeyframeEffectReadonly.cpp is getting modified there
> also? )
> Also, why didn't lint check show the spacing problem(which you mentioned as
> a nit)?

Which lint tool?  As you can see MozReview shows the spaces. See https://reviewboard.mozilla.org/r/119596/diff/1/ .
Flags: needinfo?(hikezoe)
Comment on attachment 8846536 [details]
Bug 1302888 - Replace GetPresContext() with nsContentUtils::GetContextForContent() to obtain nsPresContext* in dom/animation

https://reviewboard.mozilla.org/r/119596/#review121704

::: dom/animation/KeyframeEffectReadOnly.cpp:1000
(Diff revision 2)
>  KeyframeEffectReadOnly::RequestRestyle(
> -  EffectCompositor::RestyleType aRestyleType)
> -{
> -  nsPresContext* presContext = GetPresContext();
> -  if (presContext && mTarget && mAnimation) {
> +  EffectCompositor::RestyleType aRestyleType) {
> +   if (!mTarget) {
> +    return;
> +  }
> +  nsPresContext* presContext = nsContentUtils::GetContextForContent(mTarget->mElement):

Oh! I missed the last character was ':' instead of ';'.   Fix this, please.
Thanks.
Can you push this to the try server please?
Flags: needinfo?(hikezoe)
Comment on attachment 8846536 [details]
Bug 1302888 - Replace GetPresContext() with nsContentUtils::GetContextForContent() to obtain nsPresContext* in dom/animation

Setting review flag again because r+ got cancelled.
Attachment #8846536 - Flags: review?(hikezoe)
Comment on attachment 8846536 [details]
Bug 1302888 - Replace GetPresContext() with nsContentUtils::GetContextForContent() to obtain nsPresContext* in dom/animation

https://reviewboard.mozilla.org/r/119596/#review121800

::: dom/animation/KeyframeEffectReadOnly.cpp:996
(Diff revision 3)
>    }
>  }
>  
>  void
>  KeyframeEffectReadOnly::RequestRestyle(
> -  EffectCompositor::RestyleType aRestyleType)
> +  EffectCompositor::RestyleType aRestyleType) {

Please put this brace back to the original position.
Attachment #8846536 - Flags: review?(hikezoe) → review+
(In reply to Bharat Raghunathan from comment #16)
> Can you push this to the try server please?

Here it is:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfeeef2c3f90600ecaa6b703bc7964978c114e71
Flags: needinfo?(hikezoe)
Comment on attachment 8846536 [details]
Bug 1302888 - Replace GetPresContext() with nsContentUtils::GetContextForContent() to obtain nsPresContext* in dom/animation

Fixed position of opening brace in ::dom/animation/KeyframeEffectReadOnly.cpp:996
./mach eslint <filename> is not giving me any errors, the spaces are shown only in MozReview.
And if the try server tests pass, this can be autolanded right?
Flags: needinfo?(hikezoe)
Attachment #8846536 - Flags: review?(hikezoe)
Comment on attachment 8846536 [details]
Bug 1302888 - Replace GetPresContext() with nsContentUtils::GetContextForContent() to obtain nsPresContext* in dom/animation

https://reviewboard.mozilla.org/r/119596/#review121820

Thank you Bharat.　I am curious why MozReview alwasy clears r+..
I will land this patch once the try finished.
Attachment #8846536 - Flags: review?(hikezoe) → review+
Anything else left to be fixed or will that be known only after try and autoland testing is over?
Nothing.  I will land the patch after I confirmed that try looks good. Thank you!
Flags: needinfo?(hikezoe)
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5d0ce2805e5b
Replace GetPresContext() with nsContentUtils::GetContextForContent() to obtain nsPresContext* in dom/animation r=hiro
https://hg.mozilla.org/mozilla-central/rev/5d0ce2805e5b
Status: NEW → RESOLVED
Closed: 6 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.