Last Comment Bug 680126 - Fix checkerboarding during loading
: Fix checkerboarding during loading
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Benjamin Stover (:stechz)
:
Mentors:
Depends on: 688047
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-18 09:45 PDT by Benjamin Stover (:stechz)
Modified: 2011-09-20 16:02 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Hacky fix: don't paint checkerboards when painting has been suppressed (13.35 KB, patch)
2011-08-19 15:59 PDT, Benjamin Stover (:stechz)
no flags Details | Diff | Review
Fix checkerboarding during loading (4.46 KB, patch)
2011-08-23 09:22 PDT, Benjamin Stover (:stechz)
roc: review+
Details | Diff | Review
WIP: Reduce checkerboarding during load (15.39 KB, patch)
2011-08-23 12:50 PDT, Benjamin Stover (:stechz)
no flags Details | Diff | Review
Reduce checkerboarding during load (14.57 KB, patch)
2011-08-23 15:56 PDT, Benjamin Stover (:stechz)
mbrubeck: review+
Details | Diff | Review

Description Benjamin Stover (:stechz) 2011-08-18 09:45:17 PDT
There are a lot of times we see checkerboard during loading. One thing we could do is ignore the displayport (and thus checkerboarding) if the only thing on the page is a solid color.

Another thing we could do is add a way to remove the displayport, and do that on a location change in Fennec.
Comment 1 Benjamin Stover (:stechz) 2011-08-19 15:49:05 PDT
After playing around, here is one of the biggest sources of checkerboarding during load:
1) something is tapped in the awesomescreen while the keyboard is up
2) browser comes up, keyboard begins to come down
3) document is loading and paints begin to suppress
4) once keyboard is down, screen resizes and browser resizes
5) document would repaint, but paints are suppressed

I'm not quite sure who is at fault here. *Something* should cause a repaint even though paints are suppressed.
Comment 2 Benjamin Stover (:stechz) 2011-08-19 15:59:16 PDT
Created attachment 554571 [details] [diff] [review]
Hacky fix: don't paint checkerboards when painting has been suppressed
Comment 3 Benjamin Stover (:stechz) 2011-08-19 16:02:11 PDT
So the above patch "fixes" the problem by not painting anything while painting has been suppressed in the content process. Like I said above, the right thing to do is to repaint when the window resizes instead of suppressing it.

Roc, do you know what should be in charge of causing a repaint in the content process? And how to do it? UpdateView will ignore the repaint due to presshell suppression.
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-20 05:11:25 PDT
I guess normally what happens in the single-process world is that when you resize a browser window, the whole window is invalid so invalidation is not needed. We ask the content document to paint at its new size and it paints the background color.

Is PuppetWidget::Resize getting called? If so, why isn't it invalidating the area that needs to be repainted?
Comment 5 Benjamin Stover (:stechz) 2011-08-22 10:56:37 PDT
Resize is getting called several times, and I did see an invalidate in there. Hm.
Comment 6 Benjamin Stover (:stechz) 2011-08-22 13:37:18 PDT
To me it looks like there are two problems:

1) http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6145
NS_FRAME_UPDATE_LAYER_TREE isn't set, so we early exit from painting on resize.

2) If we are before the first repaint and A) the new frame isn't present yet or B) the new frame is present. Either way, invalidating the resize difference between the old displayport and the new displayport doesn't really make sense.

I'll play around a little bit.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-22 17:11:19 PDT
We create layers for the entire viewport, not just the displayed portion, right? So the viewport we used when the keyboard was up is not big enough?
Comment 8 Benjamin Stover (:stechz) 2011-08-23 09:22:10 PDT
Created attachment 555116 [details] [diff] [review]
Fix checkerboarding during loading
Comment 9 Benjamin Stover (:stechz) 2011-08-23 09:27:36 PDT
Comment on attachment 555116 [details] [diff] [review]
Fix checkerboarding during loading

This fixes the platform part of our checkerboarding issue by using the prescontext's visible area for the color layer bounds when no frame is present.

I also removed a faulty assertion from nsPresShell that assumes that the root view's dimensions will be the same as the visible area for the presshell, which is simply not true when setCSSViewport has been called. What was the assertion for? If it is for something specific I'd be happy to fix it.
Comment 10 Benjamin Stover (:stechz) 2011-08-23 09:29:29 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> We create layers for the entire viewport, not just the displayed portion,
> right? So the viewport we used when the keyboard was up is not big enough?

We talked about this on IRC, but for context for those following at home: normally we do create layers for the entire viewport, but when there is no frame present we were only creating a layer that covered the invalidated area. This breaks us in the multiprocess case.
Comment 11 Benjamin Stover (:stechz) 2011-08-23 09:30:18 PDT
You might notice I removed some whitespace: there were trailing whitespace characters and the newlines didn't seem necessary anyway, so I just removed them.
Comment 12 Benjamin Stover (:stechz) 2011-08-23 12:50:49 PDT
Created attachment 555163 [details] [diff] [review]
WIP: Reduce checkerboarding during load
Comment 13 Benjamin Stover (:stechz) 2011-08-23 12:58:20 PDT
The WIP does several things:
* remove display port method (probably not a necessary part of the patch)
* only reflow for viewport size once on first paint (we might have to change this? maybe mbrubeck has some cases where this won't work)
* call setWindowSize when keyboard goes away (removes permanent checkerboard issue)
* don't set displayport before first paint (helps for checkerboarding between first paint and viewport reflow, though I'm not exactly sure why).
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-23 14:24:38 PDT
Comment on attachment 555163 [details] [diff] [review]
WIP: Reduce checkerboarding during load

Review of attachment 555163 [details] [diff] [review]:
-----------------------------------------------------------------

Don't forget to rev UUID on nsIDOMWindowUtils.
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-23 15:43:50 PDT
Comment on attachment 555116 [details] [diff] [review]
Fix checkerboarding during loading

Review of attachment 555116 [details] [diff] [review]:
-----------------------------------------------------------------

lovely!
Comment 16 Benjamin Stover (:stechz) 2011-08-23 15:56:21 PDT
Created attachment 555235 [details] [diff] [review]
Reduce checkerboarding during load
Comment 17 Matt Brubeck (:mbrubeck) 2011-08-23 17:42:24 PDT
Comment on attachment 555235 [details] [diff] [review]
Reduce checkerboarding during load

Review of attachment 555235 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/chrome/content/browser.js
@@ -1601,3 @@
>        aTab.updateThumbnail();
>  
> -      browser.messageManager.addMessageListener("MozScrolledAreaChanged", aTab.scrolledAreaChanged);

You can also remove |this.scrolledAreaChanged = this.scrolledAreaChanged.bind(this);| from the Tab constructor.

@@ +2963,5 @@
>  
> +    if (firstPaint) {
> +      // You only get one shot, do not miss your chance to reflow.
> +      this.updateViewportSize();
> +    }

I think you can just remove updateViewportSize() here now that you have removed useFallbackWidth.  See bug 604765 where they were both added.

@@ +3215,5 @@
>      Browser.styles["viewable-width"].width = newWidth + "px";
>      Browser.styles["viewable-width"].maxWidth = newWidth + "px";
>  
>      let startup = !oldHeight && !oldWidth;
> +    if (newHeight > oldHeight || newWidth != oldWidth) {

Please add a comment before the "if" statement, like "Don't update the viewport if the screen it is just shrinking vertically because a keyboard appeared."

Also, you can move "let startup" inside the "if" statement.
Comment 18 Benjamin Stover (:stechz) 2011-08-23 20:48:43 PDT
> You can also remove |this.scrolledAreaChanged =
> this.scrolledAreaChanged.bind(this);| from the Tab constructor.

Done.

> I think you can just remove updateViewportSize() here now that you have
> removed useFallbackWidth.  See bug 604765 where they were both added.

Actually, it's nice to do this after first paint immediately so that the page is sized appropriately during load. Most of the time you don't even notice the size change on my build. Could we keep it? :)

> Please add a comment before the "if" statement, like "Don't update the
> viewport if the screen it is just shrinking vertically because a keyboard
> appeared."
> 
> Also, you can move "let startup" inside the "if" statement.

Done.
Comment 19 Marco Bonardo [::mak] 2011-08-24 01:53:23 PDT
http://hg.mozilla.org/mozilla-central/rev/7e98f39c07e9
Comment 20 Benjamin Stover (:stechz) 2011-08-24 09:07:50 PDT
Frontend patch has not landed.
Comment 21 Matt Brubeck (:mbrubeck) 2011-08-24 11:11:59 PDT
(In reply to Benjamin Stover (:stechz) from comment #18)
> > I think you can just remove updateViewportSize() here now that you have
> > removed useFallbackWidth.  See bug 604765 where they were both added.
> 
> Actually, it's nice to do this after first paint immediately so that the
> page is sized appropriately during load. Most of the time you don't even
> notice the size change on my build. Could we keep it? :)

Sure, we can keep this.
Comment 22 Marco Bonardo [::mak] 2011-08-25 04:32:24 PDT
http://hg.mozilla.org/mozilla-central/rev/b1cab441835f

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