Closed Bug 744439 Opened 11 years ago Closed 11 years ago

"Checkerboard" colour is not refreshed until page finishes loading


(Firefox for Android Graveyard :: General, defect)

Not set


(firefox15 verified, blocking-fennec1.0 +)

Firefox 14
Tracking Status
firefox15 --- verified
blocking-fennec1.0 --- +


(Reporter: joe, Assigned: joe)



(1 file, 3 obsolete files)

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.
blocking-fennec1.0: ? → +
Assignee: nobody → joe
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.
Attachment #615597 - Flags: review?(bugmail.mozilla)
that's not even close to the right patch
Attachment #615597 - Attachment is obsolete: true
Attachment #615597 - Flags: review?(bugmail.mozilla)
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.
Attached patch Add checkerboard colour to Tab (obsolete) — Splinter Review
This patch adds a checkerboard colour field to Tab, resets it on navigation, and uses it on setFirstPaintViewport as requested.
Attachment #615598 - Attachment is obsolete: true
Attachment #617121 - Flags: review?(bugmail.mozilla)
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

::: mobile/android/base/
@@ +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

::: mobile/android/base/gfx/
@@ +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.
Attachment #617121 - Flags: review?(bugmail.mozilla) → review-
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.
Attachment #617121 - Attachment is obsolete: true
Attachment #617494 - Flags: review?(bugmail.mozilla)
Comment on attachment 617494 [details] [diff] [review]
Set Tab checkerboard colour on DOMContentLoaded

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

Looks good, thanks.
Attachment #617494 - Flags: review?(bugmail.mozilla) → review+
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Verified fixed on:

Firefox 15.0a1 (2012-05-27)
Device: Galaxy Nexus
OS: Android 4.0.2
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.