Last Comment Bug 721294 - Clean up nsIPresShell::DidPaint/WillPaint related code
: Clean up nsIPresShell::DidPaint/WillPaint related code
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla12
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-25 21:04 PST by Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
Modified: 2012-01-28 18:56 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (13.12 KB, patch)
2012-01-25 21:27 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
mats: review-
Details | Diff | Splinter Review
fix comment (13.14 KB, patch)
2012-01-27 15:15 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review

Description Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-25 21:04:51 PST
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.
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-25 21:27:20 PST
Created attachment 591693 [details] [diff] [review]
fix
Comment 2 Mats Palmgren (:mats) 2012-01-27 03:23:18 PST
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...
Comment 3 Mats Palmgren (:mats) 2012-01-27 12:19:28 PST
> 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?
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-27 15:13:19 PST
(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.
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-27 15:15:55 PST
Created attachment 592297 [details] [diff] [review]
fix comment
Comment 6 Mats Palmgren (:mats) 2012-01-27 17:29:26 PST
> 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 7 Mats Palmgren (:mats) 2012-01-27 17:31:06 PST
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
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-27 19:36:02 PST
(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.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-27 19:40:23 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/06d02e7132b2
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-27 20:50:18 PST
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/d68b6a9939d8
relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/22986f29b1bd
Comment 11 Joe Drew (not getting mail) 2012-01-28 18:56:18 PST
https://hg.mozilla.org/mozilla-central/rev/22986f29b1bd

Note You need to log in before you can comment on or make changes to this bug.