"Checkerboard" colour is not refreshed until page finishes loading

VERIFIED FIXED in Firefox 15

Status

()

Firefox for Android
General
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Joe Drew (not getting mail), Assigned: Joe Drew (not getting mail))

Tracking

Trunk
Firefox 14
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox15 verified, blocking-fennec1.0 +)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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)

Updated

6 years ago
Assignee: nobody → joe
(Assignee)

Comment 1

6 years ago
Created attachment 615597 [details] [diff] [review]
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.
Attachment #615597 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 2

6 years ago
Created attachment 615598 [details] [diff] [review]
Reset the checkerboard colour when we navigate to a new page

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.
(Assignee)

Comment 4

6 years ago
Created attachment 617121 [details] [diff] [review]
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.
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 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.
Attachment #617121 - Flags: review?(bugmail.mozilla) → review-
(Assignee)

Comment 6

6 years ago
Created attachment 617494 [details] [diff] [review]
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.
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+

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/72ac45a8f8f5
Status: NEW → RESOLVED
Last Resolved: 6 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
Status: RESOLVED → VERIFIED
status-firefox15: --- → verified
You need to log in before you can comment on or make changes to this bug.