Closed Bug 938141 Opened 7 years ago Closed 7 years ago

Thumbnail flickering when Home is loaded

Categories

(Firefox for Android :: General, defect)

26 Branch
All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 28
Tracking Status
firefox26 --- ?
firefox27 --- verified
firefox28 --- verified
fennec 27+ ---

People

(Reporter: ibarlow, Assigned: lucasr)

References

Details

Attachments

(1 file)

When tapping the URL bar, we currently show coloured frames with favicons for a split second before loading thumbnails, which creates a janky flicker effect.

The desired behaviour here is to go straight to thumbnails, and avoid that flicker effect.

If a thumbnail does not exist, the "coloured frame + favicon" placeholder should appear at the same time as the other thumbnails in the grid.
Assignee: nobody → lucasr.at.mozilla
Comment on attachment 832943 [details] [diff] [review]
Avoid showing favicons before the thumbnails are loaded (r=rnewman)

The idea here is that we shouldn't try to display/load any image (icon for empty spot, favicon, etc) before the list of thumbnails is loaded.
Attachment #832943 - Flags: review?(rnewman)
Comment on attachment 832943 [details] [diff] [review]
Avoid showing favicons before the thumbnails are loaded (r=rnewman)

>     private boolean mThumbnailSet;
> 
>     // Pinned state.
>     private boolean mIsPinned = false;
> 
>+    // Dirty state.
>+    private boolean mIsDirty = false;
>+
>     // Empty state.
>     private boolean mIsEmpty = true;

An angel lost it wings
Comment on attachment 832943 [details] [diff] [review]
Avoid showing favicons before the thumbnails are loaded (r=rnewman)

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

Let me talk through this for a minute, and you can tell me if I'm misunderstanding anything.


The root cause here is that we have multiple interrelated loads occurring: we have the list and grid, and we have a thumbnails loader that talks to the grid when it has results, and we fire off favicon fetches if needed (which is typically for every item).

This results in a bunch of bindView calls, typically with a cell being displayed before the thumbnail is ready -- and also before we know if there's a thumbnail for that cell.


The existing code has logic to avoid a favicon 'winning' over a thumbnail, should it arrive afterwards: 

    public void displayFavicon(Bitmap favicon, String faviconURL) {
        if (mThumbnailSet) {
            // Already showing a thumbnail; do nothing.
            return;
        }

As I understand it, what this patch does is avoid starting a favicon fetch unless the thumbnail loader finished first -- every bindView short-circuits if the mThumbnails isn't set -- and then to force the favicon fetches to occur after the thumbnails are applied through a boolean.

mfinkle already pointed out the visceral reaction to adding a transient boolean member; that's a definite concern for me, albeit small.

But this feels like it has a few edge cases and open questions:

* Isn't this blocking a potentially cached operation (favicon lookup) on a DB-bound operation that could take several seconds (thumbnail fetch)? Most users will have relatively few thumbnails, but lots of favicons.

* What if there are no thumbnails in the DB (resource-constrained device)? Do we still load favicons? If we do, doesn't that mean we're marking the dataset as changed even when it isn't?

* Why aren't we triggering a favicon fetch in the same way we trigger a thumbnail fetch? (Perhaps, indeed, integrated, so that the favicons are all batch loaded and presented via the exact same update as the thumbnails?)
  ** Grab all the URLs, hit the cache, hit the DB in one go for the ones that weren't in the cache...

* Doesn't this result in some view thrashing? We load the rows, then we load the thumbnails, then we trigger all of the favicon hits one by one?


So here is my suggestion.

Presumably we don't fetch thumbnails in that very first top sites cursor fetch because they're big. But we can return whether there is a thumbnail at all, right?

Can we do that, *decide* which items to fetch thumbnails for and which to fetch favicons for, and just do this in one go?
Another little comment: if we really want the thumbnails and favicons to arrive at the same time (per comment 0) we can't do this two-phase is-dirty thing, right?
Status: NEW → ASSIGNED
OS: Mac OS X → Android
Hardware: x86 → All
needinfo for lucasr when he's back from PTO.
Flags: needinfo?(lucasr.at.mozilla)
Comment on attachment 832943 [details] [diff] [review]
Avoid showing favicons before the thumbnails are loaded (r=rnewman)

Clearing review flag for now.
Attachment #832943 - Flags: review?(rnewman)
tracking-fennec: --- → ?
(In reply to Richard Newman [:rnewman] from comment #4)
> Comment on attachment 832943 [details] [diff] [review]
> Avoid showing favicons before the thumbnails are loaded (r=rnewman)
> 
> Review of attachment 832943 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Let me talk through this for a minute, and you can tell me if I'm
> misunderstanding anything.

Sure.

> The root cause here is that we have multiple interrelated loads occurring:
> we have the list and grid, and we have a thumbnails loader that talks to the
> grid when it has results, and we fire off favicon fetches if needed (which
> is typically for every item).

Correct. Just one detail: the list and grid loading is actually done a single operation i.e. they share the same cursor with different offsets.

> This results in a bunch of bindView calls, typically with a cell being
> displayed before the thumbnail is ready -- and also before we know if
> there's a thumbnail for that cell.

Right.

> The existing code has logic to avoid a favicon 'winning' over a thumbnail,
> should it arrive afterwards: 
> 
>     public void displayFavicon(Bitmap favicon, String faviconURL) {
>         if (mThumbnailSet) {
>             // Already showing a thumbnail; do nothing.
>             return;
>         }
> 
> As I understand it, what this patch does is avoid starting a favicon fetch
> unless the thumbnail loader finished first -- every bindView short-circuits
> if the mThumbnails isn't set -- and then to force the favicon fetches to
> occur after the thumbnails are applied through a boolean.

Exactly.

> mfinkle already pointed out the visceral reaction to adding a transient
> boolean member; that's a definite concern for me, albeit small.

Well, the view already has a couple ;-) I don't feel strongly about it though. Alternatives?

> But this feels like it has a few edge cases and open questions:
> 
> * Isn't this blocking a potentially cached operation (favicon lookup) on a
> DB-bound operation that could take several seconds (thumbnail fetch)? Most
> users will have relatively few thumbnails, but lots of favicons.

Yes but does it matter in practice? I mean, we should not show any image in the grid item until we decide which image (thumbnail or favicon) to show anyway. If the favicon is memcached, it will be displayed immediately after the thumbnails are loaded anyway.
 
> * What if there are no thumbnails in the DB (resource-constrained device)?
> Do we still load favicons? If we do, doesn't that mean we're marking the
> dataset as changed even when it isn't?

Good point. I'll update my patch to only mark the items that do not have thumbnails as dirty.

> * Why aren't we triggering a favicon fetch in the same way we trigger a
> thumbnail fetch? (Perhaps, indeed, integrated, so that the favicons are all
> batch loaded and presented via the exact same update as the thumbnails?)
>   ** Grab all the URLs, hit the cache, hit the DB in one go for the ones
> that weren't in the cache...

This exactly what I pointed out before in a few occasions. See:
https://bugzilla.mozilla.org/show_bug.cgi?id=926430#c31
https://bugzilla.mozilla.org/show_bug.cgi?id=926430#c33
https://bugzilla.mozilla.org/show_bug.cgi?id=914296#c62

> * Doesn't this result in some view thrashing? We load the rows, then we load
> the thumbnails, then we trigger all of the favicon hits one by one?

My patch will actually reduce the number of unnecessary operations and view updates:
Without the patch: load the grid items, load favicon, load thumbnail, load favicon for the items without thumbnails
With the patch: load the grid items, load thumbnail, load favicon for the items without thumbnails

The current code does an unnecessary view update for favicons that are discarded just after the thumbnails finish loading. 

> So here is my suggestion.
> 
> Presumably we don't fetch thumbnails in that very first top sites cursor
> fetch because they're big. But we can return whether there is a thumbnail at
> all, right?

This would involve adding a new 'combined_with_thumbnails' view in the db and doing a extra table join which might have an unwanted performance impact on the query itself. One of the main reasons for splitting image loading out of the query was to avoid all these things. 

At first sight, this doesn't seem like a good comprise i.e. we'd making the query itself a bit slower just to show images faster once we load the grid items. Am I missing something?   

> Can we do that, *decide* which items to fetch thumbnails for and which to
> fetch favicons for, and just do this in one go?

I still think the right way to fix this is to load all images (thumbnails and favicons) in a single background operation using batched queries (I assume bug 926139 covers that?). As a bonus, this would greatly simplify the UI code here i.e. a single CursorLoader to load all images in one go.

My intent with this patch is to (at least) have a fix for Fx27. I should have probably been more clear about this when I posted the patch.
Flags: needinfo?(lucasr.at.mozilla)
tracking-fennec: ? → 27+
Comment on attachment 832943 [details] [diff] [review]
Avoid showing favicons before the thumbnails are loaded (r=rnewman)

Re-requesting review after clarifications. I considered only marking the views with URLs without thumbnails but that is not entirely safe as there is not way to know how children views will be recycled and bound to new data.
Attachment #832943 - Flags: review?(rnewman)
Comment on attachment 832943 [details] [diff] [review]
Avoid showing favicons before the thumbnails are loaded (r=rnewman)

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

So long as we don't forget to rework this later, thumb up from me if it fixes the issue.
Attachment #832943 - Flags: review?(rnewman) → review+
Comment on attachment 832943 [details] [diff] [review]
Avoid showing favicons before the thumbnails are loaded (r=rnewman)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 914296 (new favicons API)
User impact if declined: flickering favicons whenever about:home is initially displayed (tapping on URL bar, fennec startup, new tab is added) 
Testing completed (on m-c, etc.): local tests, works fine. Let's bake this in Nightly for a couple of days and then uplift to Fx27.
Risk to taking this patch (and alternatives if risky): Mild, this patch is small enough to be easily backed out if necessary.
String or IDL/UUID changes made by this patch: n/a
Attachment #832943 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/e3aba1d2f23f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Attachment #832943 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
Verified as fixed in builds:
27
28
29
31.0a2 (2014-06-04)
32.0a1 (2014-06-04)

Devices: Asus Transformer Pad TF300T (Android 4.2.1) Motorola Razr (Android 4.0.4)
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.