744439 - "Checkerboard" colour is not refreshed until page finishes loading

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Joe Drew (not getting mail)]

If you load one page, then load another, the checkerboard colour remains the background colour of the first page until the second page finishes loading.

Comment 1

[Joe Drew (not getting mail)]

Attachment: Reset the checkerboard colour when we navigate to a new page

I would have preferred to do this on DOMContentUnloaded, except that event doesn't exist. :(

This simply resets the checkerboard colour to white when the location changes to a new page (ignoring anchor changes). It can then be set correctly when DOMContentLoaded is fired.

Comment 2

[Joe Drew (not getting mail)]

Attachment: Reset the checkerboard colour when we navigate to a new page

that's not even close to the right patch

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 615598 [details] [diff] [review]
Reset the checkerboard colour when we navigate to a new page

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

I assume you meant to ask me for review on this updated patch..

So I don't think that this will work perfectly. Specifically, there is a period of time between the location change event and when the page is actually drawn to the screen, and we could still be drawing the old page during that time. If we flip the checkerboard color back to white while we are still showing the old page to the user, that seems wrong. Also something related but perhaps not specific to this bug: what happens when we switch tabs between two pages that have different background colours? Do we update the checkerboard colour properly?

I think a more correct fix is to update the checkerboard colour when GeckoLayerClient.setFirstPaintViewport gets called, because that will only get called when we start displaying a new page (either a new page load, or on tab switch). We should either pass in the checkerboard colour through there, or store it on the Tab object and read it during that call. We should be doing something similar with the mWaitForTouchListeners boolean in TouchEventHandler for it to be more correct, and also with the user-scalable flag for bug 707571.

Comment 4

[Joe Drew (not getting mail)]

Attachment: Add checkerboard colour to Tab

This patch adds a checkerboard colour field to Tab, resets it on navigation, and uses it on setFirstPaintViewport as requested.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 617121 [details] [diff] [review]
Add checkerboard colour to Tab

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

r- for the background tab problem, it should be straightforward enough to fix though. Also another nit that splinter won't show properly: put back the blank line you took out of Tab.java.

::: mobile/android/base/GeckoApp.java
@@ +835,5 @@
>                          getLayerController().setCheckerboardColor(backgroundColor);
>                      } else {
>                          // Default to white if no color is given
>                          getLayerController().setCheckerboardColor(Color.WHITE);
>                      }

Just realized this code is busted. It will set the checkerboard colour on the foreground tab when a background tab gets a DOMContentLoaded on a background tab. This block should be removed; it should just save the colour in the tab, and then only set it on the layer controller in setFirstPaintViewport. The string parsing in LayerController can be moved into Tab.java.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +57,5 @@
>  import android.util.Log;
>  import android.view.View;
>  import java.util.Map;
>  import java.util.HashMap;
> +import org.mozilla.gecko.Tab;

Don't need to import Tab here.

Comment 6

[Joe Drew (not getting mail)]

Attachment: Set Tab checkerboard colour on DOMContentLoaded

This does what Kats asked for, but also syncs up the LayerController checkerboard colour in our DOMContentLoaded handler, because we can't rely on what order DOMContentLoaded and setFirstPaintViewport arrive in.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 617494 [details] [diff] [review]
Set Tab checkerboard colour on DOMContentLoaded

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

Looks good, thanks.

Comment 8

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72ac45a8f8f5

Comment 9

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/72ac45a8f8f5

Comment 10

[Cristian Nicolae (:xti)]

Verified fixed on:

Firefox 15.0a1 (2012-05-27)
Device: Galaxy Nexus
OS: Android 4.0.2