Closed
Bug 725428
Opened 11 years ago
Closed 11 years ago
content area goes black and then dark checkerboard before pageload during startup
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Tracking
(firefox14 verified, blocking-fennec1.0 +)
VERIFIED
FIXED
Firefox 14
People
(Reporter: dietrich, Assigned: bnicholson)
References
Details
(Whiteboard: startup-ux)
Attachments
(2 files, 4 obsolete files)
756.94 KB,
video/mp4
|
Details | |
9.67 KB,
patch
|
kats
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
i'm seeing some flashing, then a black content area for up to 3 seconds and then a dark checkerboard and then a white background and finally the page content loads. while there's clearly a performance issue here, this bug is about design: i picked a common page (mobile wikipedia) and loaded it in both Fennec and in the stock browser, cold starting both. the stock browser took longer, but *felt far faster*. here's the timeline of visual events for the stock browser. events that occurred at basically the same time are coalesced into a single timeline event. 1. touch icon 2. app fills screen. all navigation controls are visible. content area is white. title is loaded. favicon is the stop button. 3. progress meter moves from left to right, located between navigation and content area. 4. page content loads. progress meter disappears. favicon replaces stop button. timeline of visual events for fennec: 1. touch icon 2. there's a flash of a black screen (i think - it's really fast) 3. there's a flash of my homescreen 4. app fills screen. navigation controls are visible, except for new-tab "+". content area is black. title is loaded. favicon is progress ring. 5. content area turns into dark checkerboard. new-tab "+" appears. 6. content area turns white. some times the white only covers 2/3 of the content area - it's split vertically, with the other 1/3 being a light grey. 7. page content loads. favicon replaces progress ring. there's a far lower amount of visual activity during the load of the stock browser. the only visual change is the progress meter - a direct visual indication of that the browser is busy doing what you asked. fennec has a high amount of visual activity, almost all of which is superfluous to the requested task. it's feels busy, jumbly and heavy. the net effect is that while fennec finished loading the page faster by the clock, it felt far slower because all this crazy stuff happened on screen while it did so.
dup of bug 716581? if not part of bug 717774?
Updated•11 years ago
|
tracking-fennec: --- → ?
Comment 2•11 years ago
|
||
Dietrich, do you still see this?
Reporter | ||
Comment 3•11 years ago
|
||
Just updated to latest nightly, and did a cold start: 1. content area is solid grey 2. then it's dark checkerboard 3. then it's light checkerboard 4. then content shows up I can do a more detailed breakdown about how data fills into the nav area if it helps.
Comment 4•11 years ago
|
||
Dietrich, can you record what's happening and post it?
Updated•11 years ago
|
blocking-fennec1.0: --- → ?
OS: Mac OS X → Android
Hardware: x86 → ARM
Whiteboard: startup-ux
Updated•11 years ago
|
Assignee: nobody → bnicholson
blocking-fennec1.0: ? → +
Priority: -- → P1
Assignee | ||
Comment 5•11 years ago
|
||
Here's a screencast of it on my phone. The checkerboards flash quickly, but they're there.
Updated•11 years ago
|
tracking-fennec: ? → ---
Assignee | ||
Comment 8•11 years ago
|
||
This patch doesn't draw the background/checkerboard/root layers if the root layer hasn't yet been set. This improves the problem by showing a white screen instead of the background/checkerboard layers before we connect the software layer client. Unfortunately, there's still a quick flash when the software layer client gets hooked up. I don't know what the cause is. Let me know if there's a better way to fix this; I'm not too familiar with any of the gfx classes.
Attachment #605612 -
Flags: feedback?(bugmail.mozilla)
Assignee | ||
Comment 9•11 years ago
|
||
Oops, removed a few unnecessary changes in GeckoApp.
Attachment #605612 -
Attachment is obsolete: true
Attachment #605612 -
Flags: feedback?(bugmail.mozilla)
Attachment #605613 -
Flags: feedback?(bugmail.mozilla)
Comment 10•11 years ago
|
||
Comment on attachment 605613 [details] [diff] [review] WIP patch Review of attachment 605613 [details] [diff] [review]: ----------------------------------------------------------------- In general this looks fairly sane, but the Maple landing tomorrow will change some of this code, so you might as well wait until tomorrow before continuing on this patch. Also it would be a good idea to first decide exactly what we want to be displaying during the period of time in question. By the looks of it you just want white (or the placeholder if it's there), and that's fine - just want to make sure we're clear on that first.
Attachment #605613 -
Flags: feedback?(bugmail.mozilla) → feedback+
Comment 11•11 years ago
|
||
Not seeing the checkboarding currently but I do see light/dark flashing still now that maple has landed, so we should probably revisit this patch.
Assignee | ||
Comment 12•11 years ago
|
||
A somewhat hacky patch that simply sets the screen to white by setting the LayerView background color. Using glClear(), as in the previous patch, still flickered a black screen between setting the view and the glClear() call. With this approach, we must set the LayerView back to a transparent background once we've drawn a page or else the screen will stay white. Setting a flag in setFirstPaintViewport() indicates that we're about to do a first paint; we then set the background back to transparent once we've ended the first draw.
Attachment #605613 -
Attachment is obsolete: true
Attachment #608163 -
Flags: review?(bugmail.mozilla)
Comment 13•11 years ago
|
||
Comment on attachment 608163 [details] [diff] [review] patch Review of attachment 608163 [details] [diff] [review]: ----------------------------------------------------------------- Overall the approach looks fine, but there's a couple of problems as noted below. ::: mobile/android/base/GeckoApp.java @@ +1748,2 @@ > mPlaceholderLayerClient = new PlaceholderLayerClient(mLayerController, mLastViewport); > + if (!mPlaceholderLayerClient.loadScreenshot()) { This is effectively calling loadScreenshot() twice, because it gets called once from the constructor in PlaceholderLayerClient. Might be better to just call it the one time in the constructor, and save the success value in a class variable, and then query that here. ::: mobile/android/base/gfx/GeckoLayerClient.java @@ +366,5 @@ > // sends the request after aborting the animation. The display port request is actually > // a full viewport update, which is fine because if browser.js has somehow moved to > // be out of sync with this first-paint viewport, then we force them back in sync. > mLayerController.abortPanZoomAnimation(); > + mLayerController.getView().setFirstPaint(true); setFirstPaintViewport gets called more than just once during the lifetime of fennec. In fact it will get called every time you switch tabs or load a new page. Setting the firstPaint flag on the view back to true every time this happens means we're going to be doing a lot of unnecessary work (i.e. setting the background back to transparent multiple times). Really you want something like a tri-state flag, so that once you set the background back to transparent you go to a final state and never go back into the first-paint state.
Attachment #608163 -
Flags: review?(bugmail.mozilla) → review-
Assignee | ||
Comment 14•11 years ago
|
||
> ::: mobile/android/base/GeckoApp.java
> @@ +1748,2 @@
> > mPlaceholderLayerClient = new PlaceholderLayerClient(mLayerController, mLastViewport);
> > + if (!mPlaceholderLayerClient.loadScreenshot()) {
>
> This is effectively calling loadScreenshot() twice, because it gets called
> once from the constructor in PlaceholderLayerClient. Might be better to just
> call it the one time in the constructor, and save the success value in a
> class variable, and then query that here.
>
Oops, I intended to remove the loadScreenshot() call from the constructor. Will that also work?
Comment 15•11 years ago
|
||
(In reply to Brian Nicholson (:bnicholson) from comment #14) > Oops, I intended to remove the loadScreenshot() call from the constructor. > Will that also work? Sure, that works too.
Assignee | ||
Comment 16•11 years ago
|
||
Addressed above comments.
Attachment #608163 -
Attachment is obsolete: true
Attachment #608460 -
Flags: review?(bugmail.mozilla)
Comment 17•11 years ago
|
||
Comment on attachment 608460 [details] [diff] [review] patch v2 Review of attachment 608460 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/gfx/GeckoLayerClient.java @@ +366,5 @@ > // sends the request after aborting the animation. The display port request is actually > // a full viewport update, which is fine because if browser.js has somehow moved to > // be out of sync with this first-paint viewport, then we force them back in sync. > mLayerController.abortPanZoomAnimation(); > + mLayerController.getView().setPaintState(LayerView.PAINT_BEFORE_FIRST); This still has the same problem as before; the state will keep bouncing between PAINT_BEFORE_FIRST and PAINT_AFTER_FIRST and you'll keep setting the color back to transparent unnecessarily. At some point there has to be a guard such that once you're in AFTER_FIRST you can't go back to BEFORE_FIRST. I'd prefer the guard to be in the LayerView class since that's more encapsulated. ::: mobile/android/base/gfx/LayerView.java @@ +244,5 @@ > public LayerRenderer getLayerRenderer() { > return mRenderer; > } > + > + public void setPaintState(int paintState) { Add a comment here saying the parameter must be one of the PAINT_xxx constants. Or use an enum instead of ints. Either is fine by me. @@ +248,5 @@ > + public void setPaintState(int paintState) { > + mPaintState = paintState; > + } > + > + public int getPaintState() { Add a comment here saying the return value is one of the PAINT_xxx constants. Alternatively, use an enum instead of ints. Either is fine by me.
Attachment #608460 -
Flags: review?(bugmail.mozilla) → review-
Assignee | ||
Comment 18•11 years ago
|
||
Fixed problems noted in comments.
Attachment #608460 -
Attachment is obsolete: true
Attachment #608487 -
Flags: review?(bugmail.mozilla)
Comment 19•11 years ago
|
||
Comment on attachment 608487 [details] [diff] [review] patch v3 Review of attachment 608487 [details] [diff] [review]: ----------------------------------------------------------------- Nice, r=me with nit addressed ::: mobile/android/base/gfx/LayerView.java @@ +250,5 @@ > + /* paintState must be a PAINT_xxx constant. The state will only be changed > + * if paintState represents a state that occurs after the current state. */ > + public void setPaintState(int paintState) { > + if (paintState > mPaintState) > + mPaintState = paintState; nit: add braces around the if body.
Attachment #608487 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 20•11 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/017f6dd98dc0
Assignee | ||
Comment 21•11 years ago
|
||
Backed out for robocop failures. http://hg.mozilla.org/integration/mozilla-inbound/rev/4595ab8f1cd0
Not sure if you knew robocop had an issue if the test was extending PixelBase : bug 737411 This should be fixed in inbound, though.
Assignee | ||
Comment 23•11 years ago
|
||
(In reply to Naoki Hirata :nhirata from comment #22) > Not sure if you knew robocop had an issue if the test was extending > PixelBase : bug 737411 > > This should be fixed in inbound, though. Nope, I didn't realize that. It looks like the other rc failures were from another bug, so relanding: http://hg.mozilla.org/integration/mozilla-inbound/rev/3d3aff127856
Comment 24•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3d3aff127856
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Assignee | ||
Comment 25•11 years ago
|
||
Comment on attachment 608487 [details] [diff] [review] patch v3 [Approval Request Comment] Prevents flashing of black screen at startup.
Attachment #608487 -
Flags: approval-mozilla-aurora?
Comment 26•11 years ago
|
||
This will need to wait for maple to be uplifted before it can land on aurora.
Comment 27•11 years ago
|
||
Comment on attachment 608487 [details] [diff] [review] patch v3 [Triage Comment] Mobile only - approved for Aurora 13.
Attachment #608487 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 28•11 years ago
|
||
Verified/fixed on: Nightly Fennec 14.0a1 (2012-03-29) Device: Samsung Nexus S OS: Android 2.3.6
Status: RESOLVED → VERIFIED
status-firefox14:
--- → verified
Updated•2 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•