680126 - Fix checkerboarding during loading

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benjamin Stover (:stechz)]

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)]

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)]

Attachment: Hacky fix: don't paint checkerboards when painting has been suppressed

Comment 3

[Benjamin Stover (:stechz)]

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) (email my personal email if necessary)]

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)]

Resize is getting called several times, and I did see an invalidate in there. Hm.

Comment 6

[Benjamin Stover (:stechz)]

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) (email my personal email if necessary)]

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)]

Attachment: Fix checkerboarding during loading

Comment 9

[Benjamin Stover (:stechz)]

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)]

(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)]

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)]

Attachment: WIP: Reduce checkerboarding during load

Comment 13

[Benjamin Stover (:stechz)]

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) (email my personal email if necessary)]

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) (email my personal email if necessary)]

Comment on attachment 555116 [details] [diff] [review]
Fix checkerboarding during loading

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

lovely!

Comment 16

[Benjamin Stover (:stechz)]

Attachment: Reduce checkerboarding during load

Comment 17

[Matt Brubeck (:mbrubeck)]

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)]

> 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]]

http://hg.mozilla.org/mozilla-central/rev/7e98f39c07e9

Comment 20

[Benjamin Stover (:stechz)]

Frontend patch has not landed.

Comment 21

[Matt Brubeck (:mbrubeck)]

(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]]

http://hg.mozilla.org/mozilla-central/rev/b1cab441835f