142635 - Painting suppression doesn't unsuppress correctly

Status: RESOLVED INCOMPLETE

(Core :: Layout, defect, P2)

Description

[Randell Jesup [:jesup] (needinfo me)]

This is a spinoff of bug 110106, hyatt's "Investigate Win32-only paint
suppression weirdness" bug.  Sam Emrick posted a 1-liner patch that makes it
actually unsuppress when it's supposed to.  (discussed in n.p.m.performance)

Patchto be attached.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Sam Emrick's patch (against branch)

Patch from 110106 as a patch, against branch.  r/sr=?  Looks like a pretty
obvious "oops" patch.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hmm.  So, the only thing cv->Show() should do is call mWindow->Show(), which
should make the widgets shown.  Then just below we do set mPaintingSuppressed to
false, and then invalidate the root frame.

So I'm guessing:
 * ShowWindow on Windows is causing a paint, perhaps even synchronously, but
that paint doesn't work if |mPaintingSuppressed| is still set.
 * The Invalidate just below is causing a much later paint (thanks to delayed
paint event prioritization on Windows)
 * This patch seems like it might start causing double-paints, which are a
performance hit.

Maybe we should instead be doing the Invalidate before the cv->Show() ?

Comment 3

[rbs]

Has anybody investigated dbaron's thought-proviking questions?

pros/cons: since 'perception' is more important in this situation (win32-only), 
the (one-off) double paint isn't too critical IMO, compared to the uncertainties 
of the pandora box when trying to change the semantics at the last minute.
Of course doing the analysis is better (emrick?) even if it is just to improve 
the understanding. But to improve the 'perception', I will go for the risk of 
the (one-off) double-paint if the analysis is lacking.


To keep it in context, here is the initial n.p.m.performance post:
Sam Emrick wrote:
> 
> I've investigated a problem that Ivan Eisen is seeing/reporting regarding
> instances of very long times for the initial paint of large/long documents.
> On Windows NT, the problem further seen by lockup of gui responsiveness for
> long periods in document load.
> 
> The source of the problem is such. In layout\html\base\src\nsPresShell.cpp
> implements mPaintSuppressionTimer, with intent to supress initial painting
> of large docuements for a while, default 1200 ms. The timer itself is
> working okay, but the handling of the timer timeout is a bit broken. In
> PresShell::UnsuppressAndInvalidate() is a path leading to cv->Show(); The
> Show() leads to a reflow which should perform the paint. However, it
> doesn't because mPaintingSuppressed = PR_FALSE; hasn't been set yet. The
> mPaintingSuppressed = PR_FALSE needs to be set before the call to Show().
> When changed such, then the initial paint delay time is mush more closely
> honored.
> Sam

Comment 4

[rbs]

> PresShell::UnsuppressAndInvalidate() is a path leading to cv->Show(); The
> Show() leads to a reflow which should perform the paint.

This isn't entirely correct (but it is beside the point). Had a look and there 
is no further reflow at that stage. cv->Show(), i.e. DocumentViewerImpl::Show(), 
leads directly to paint (and although I didn't see it, admittedly there might be 
early returns in the paints because they test for mPaintingSuppressed in the 
presShell and bail out -- but in the adjacent chunk of code to cv->Show(), 
paint-suppression is cleared before doing the Invalidate).

> Maybe we should instead be doing the Invalidate before the cv->Show() ?

Seems to lead to the same effect as emrick's patch since Invalidate() will be 
done after enabling paint.

The current code is doing:
/* as dabron noted, perhaps the paint from cv->Show() is synchronous and might 
fire before the next statement */
  cv->Show();
  mPaintingSuppressed = PR_FALSE;
  rootFrame->Invalidate();

emrick's change is to do:
  mPaintingSuppressed = PR_FALSE;
  cv->Show();
  rootFrame->Invalidate();

and inverting will just do:
  mPaintingSuppressed = PR_FALSE;
  rootFrame->Invalidate();
  cv->Show();

(BTW, what's the rationale of further delaying the paint over there, i.e.,
Invalidate(.../*aImmediate=*/PR_FALSE)?)

Comment 5

[Chris Petersen]

Changing Priority to P2

Comment 6

[Markus Hübner]

Targeting this one 1.1Beta 

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Moving TM to some more realistic target.

Comment 8

[Markus Hübner]

Can we get this in for 1.5alpha ... or even sooner?

Comment 9

[Cees T.]

Is this still needed with Cairo?

And how do patches get noticed, by a keyword?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We don't have a testcase here. I suggest we close this as RESOLVED INCOMPLETE.