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

RESOLVED FIXED in Firefox 55

Status

()

P5
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: hiro, Assigned: bharatraghunthan9767, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla55
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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
(Reporter)

Updated

2 years ago
Mentor: hikezoe
Keywords: good-first-bug
(Assignee)

Comment 1

2 years ago
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)
(Reporter)

Comment 2

2 years ago
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)
(Reporter)

Updated

2 years ago
See Also: → bug 1344533
(Reporter)

Comment 3

2 years ago
I just posted a very similar patch in bug 1344533. I hope the patch helps you.
Thank you,
(Reporter)

Comment 4

2 years ago
(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 hidden (mozreview-request)
(Assignee)

Comment 6

2 years ago
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)
(Assignee)

Comment 7

2 years ago
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.
(Reporter)

Comment 8

2 years ago
mozreview-review
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+
(Reporter)

Comment 9

2 years ago
(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 hidden (mozreview-request)
(Assignee)

Comment 11

2 years ago
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)
(Reporter)

Comment 12

2 years ago
mozreview-review
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+
(Reporter)

Comment 13

2 years ago
(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)
(Reporter)

Comment 14

2 years ago
mozreview-review
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.
Comment hidden (mozreview-request)
(Assignee)

Comment 16

2 years ago
Can you push this to the try server please?
Flags: needinfo?(hikezoe)
(Assignee)

Comment 17

2 years ago
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)
(Reporter)

Comment 18

2 years ago
mozreview-review
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+
(Reporter)

Comment 19

2 years ago
(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 hidden (mozreview-request)
(Assignee)

Comment 21

2 years ago
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)
(Reporter)

Comment 22

2 years ago
mozreview-review
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+
(Assignee)

Comment 23

2 years ago
Anything else left to be fixed or will that be known only after try and autoland testing is over?
(Reporter)

Comment 24

2 years ago
Nothing.  I will land the patch after I confirmed that try looks good. Thank you!
Flags: needinfo?(hikezoe)

Comment 25

2 years ago
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

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5d0ce2805e5b
Status: NEW → RESOLVED
Last Resolved: 2 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.