Clean up nsIPresShell::DidPaint/WillPaint related code

RESOLVED FIXED in mozilla12

Status

()

Core
Layout
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

Trunk
mozilla12
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

nsIPresShell::DidPaint only does something for the root prescontext, so should only be sent to the root viewmanager's presshell.

nsIPresShell::WillPaint should probably be similar.

nsViewManager::Refresh is passing false for aWillSendDidPaint which is wrong.
Created attachment 591693 [details] [diff] [review]
fix
Attachment #591693 - Flags: review?(matspal)
Comment on attachment 591693 [details] [diff] [review]
fix

>Bug 721294. Only call nsIPresShell::WillPaint for the root presshell. Pass aWilLSendDidPaint correctly to nsIPresShell::Paint. Remove aPaintDefaultBackground from nsIPresShell::Paint. r=mats

typo in aWilLSendDidPaint

>layout/base/nsPresShell.cpp
>-  nsIFrame* frame = aPaintDefaultBackground ? nsnull : aViewToPaint->GetFrame();
>+  nsIFrame* frame = aViewToPaint->GetFrame();

'frame' can never be null now, right? so we could remove the null checks...


> PresShell::DidPaint()
> {
>   if (mPaintingSuppressed || !mIsActive || !IsVisible()) {
>     return;
>   }
> 
>   nsRootPresContext* rootPresContext = mPresContext->GetRootPresContext();
>-  if (!rootPresContext) {
>-    return;
>-  }
>+  // This should only be called on root presshells, but maybe if a document
>+  // tree is torn down we might not be a root presshell...
>   if (rootPresContext == mPresContext) {
>     rootPresContext->UpdatePluginGeometry();
>   }
> }

I think it's safe to not bother with GetRootPresContext and just do:
  mPresContext->UpdatePluginGeometry();

and also clean up PresShell::WillPaint in the same way.


>view/src/nsViewManager.cpp
>+nsViewManager::CallDidPaintOnObserver()

You need to change CallWillPaintOnObservers in the same way.
I'm still seeing calls to PresShell::WillPaint for a non-root
prescontext from there...
Attachment #591693 - Flags: review?(matspal) → review-
> I think it's safe to not bother with GetRootPresContext and just do:
>   mPresContext->UpdatePluginGeometry();

It occurred to me that UpdatePluginGeometry() is a method on nsRootPresContext
so your suggested code is probably the best way to do it.  IIRC, we did have a
crash bug when we did cast to nsRootPresContext...  maybe it would be safe now
though with these changes?
(In reply to Mats Palmgren [:mats] from comment #2)
> 'frame' can never be null now, right? so we could remove the null checks...

It's possible to have a presshell that doesn't have a root frame created yet. We might be able to eliminate that, or just return early without painting anything in that case, but I'm not sure; let's treat that separately.

> > PresShell::DidPaint()
> > {
> >   if (mPaintingSuppressed || !mIsActive || !IsVisible()) {
> >     return;
> >   }
> > 
> >   nsRootPresContext* rootPresContext = mPresContext->GetRootPresContext();
> >-  if (!rootPresContext) {
> >-    return;
> >-  }
> >+  // This should only be called on root presshells, but maybe if a document
> >+  // tree is torn down we might not be a root presshell...
> >   if (rootPresContext == mPresContext) {
> >     rootPresContext->UpdatePluginGeometry();
> >   }
> > }
> 
> I think it's safe to not bother with GetRootPresContext and just do:
>   mPresContext->UpdatePluginGeometry();

UpdatePluginGeometry only exists on nsRootPresContext, so we'd have to do some kind of check and cast. I think that's no simpler than my current code.

> >view/src/nsViewManager.cpp
> >+nsViewManager::CallDidPaintOnObserver()
> 
> You need to change CallWillPaintOnObservers in the same way.
> I'm still seeing calls to PresShell::WillPaint for a non-root
> prescontext from there...

Oops. Contrary to the comment I added in nsIPresShell, WillPaint still needs to fire on every visible presshell (I think) to make sure required flushing happens. I'll attach an updated patch.
Created attachment 592297 [details] [diff] [review]
fix comment
Attachment #591693 - Attachment is obsolete: true
Attachment #592297 - Flags: review?(matspal)
> It's possible to have a presshell that doesn't have a root frame created yet.

ok, fair enough, shouldn't painting be suppressed in that case though?
Why is mPaintingSuppressed checked in Will/DidPaint, but not in Paint?

> UpdatePluginGeometry only exists on nsRootPresContext, so we'd have to do
> some kind of check and cast. I think that's no simpler than my current code.

Fwiw, I was aiming for:
  MOZ_ASSERT(mPresContext->IsRoot());
  rootPresContext = static_cast<nsRootPresContext*>(mPresContext)

> Oops. Contrary to the comment I added in nsIPresShell, WillPaint still needs
> to fire on every visible presshell (I think) to make sure required flushing
> happens.

Oh ok, then casting in WillPaint won't work of course...
then again, the caller could pass the root pres shell if we want it, but
we probably don't want to mess with an orphaned context anyway.
Comment on attachment 592297 [details] [diff] [review]
fix comment

I would appreciate a
  MOZ_ASSERT(mPresContext->IsRoot());
as the first line in DidPaint() so we'll get a bug reported on that
Attachment #592297 - Flags: review?(matspal) → review+
(In reply to Mats Palmgren [:mats] from comment #6)
> > It's possible to have a presshell that doesn't have a root frame created yet.
> 
> ok, fair enough, shouldn't painting be suppressed in that case though?
> Why is mPaintingSuppressed checked in Will/DidPaint, but not in Paint?

When painting is suppressed we can still paint the background color of the root. Maybe this doesn't matter anymore now that PresShell::Paint is only called on displayroots (we shouldn't even show the widget for a window while painting is suppressed), but again we shouldn't try to change this all at once.

(In reply to Mats Palmgren [:mats] from comment #7)
> I would appreciate a
>   MOZ_ASSERT(mPresContext->IsRoot());
> as the first line in DidPaint() so we'll get a bug reported on that

I'll make it an NS_ASSERTION; there's no need to abort in that situation.
http://hg.mozilla.org/integration/mozilla-inbound/rev/06d02e7132b2
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/d68b6a9939d8
relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/22986f29b1bd
https://hg.mozilla.org/mozilla-central/rev/22986f29b1bd
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.