Closed Bug 756912 Opened 10 years ago Closed 7 years ago

Some thumbnails missing from Firefox Beta home screen

Categories

(Firefox for Android Graveyard :: General, defect, P5)

ARM
Android
defect

Tracking

(fennec+)

RESOLVED INCOMPLETE
Tracking Status
fennec + ---

People

(Reporter: davidwboswell, Unassigned, Mentored)

References

Details

(Keywords: uiwanted, Whiteboard: [Engagement])

Attachments

(2 files, 2 obsolete files)

Web page or screen you were on when you saw the issue: 

Steps to reproduce:
1. Launch Firefox beta
2. Look at thumbnails on home screen
3. Some top sites have screenshot thumbnails and some don't.  The links for the missing top sites thumbnails are

http://www.mozilla.org/en-US/mobile/14.0/releasenotes/

http://www.mozilla.org/en-US/mobile/beta/faq/

What you expected: 
I expected all top sites on home page to have screenshot thumbnails.

Crash report ID (if applicable):
blocking-fennec1.0: --- → ?
OS: Mac OS X → Android
Hardware: x86 → ARM
tracking-fennec: --- → 15+
blocking-fennec1.0: ? → soft
Assignee: nobody → gpascutto
Also we shouldn't store 404's in the history, and they certainly shouldn't end up in top sites.
Blassey noted that these might be default bookmarks. Probably the top sites logic should verify we have thumbnails before displaying anything?
Attachment #631955 - Flags: review?(lucasr.at.mozilla)
Attachment #631958 - Flags: review?(lucasr.at.mozilla)
>Also we shouldn't store 404's in the history, and they certainly shouldn't end up 
>in top sites.

The tricky part here is that we add stuff to the history very early in the pageload, and there's no way to undo that the way we store history now.

Does "handlePageShow" (Content:PageShow) trigger for every normal page load? Maybe we can move the history add in there?
Comment on attachment 631958 [details] [diff] [review]
Patch 2. Filter thumbnail-less sites from topSites

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

What happens on about:home when you run Fennec for the first time with this patch? Empty list of top sites? Is that the intended UX? I have too many questions to r+ this patch :-)

::: mobile/android/base/db/LocalBrowserDB.java
@@ +191,5 @@
>      }
>  
>      public Cursor getTopSites(ContentResolver cr, int limit) {
> +        String selection = Combined.THUMBNAIL + " IS NOT NULL";
> +        String[] selectionArgs = null;

Is appendFilter still used anywhere after this patch? Is your assumption here that about pages don't have screenshots? Why is the url filter not needed anymore?
Comment on attachment 631955 [details] [diff] [review]
Patch 1. Refactor getTopSites filtering

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

::: mobile/android/base/db/LocalBrowserDB.java
@@ +121,5 @@
> +            selectionArgs = aSelectionArgs;
> +        }
> +    }
> +
> +    private SelectionPair appendFilter(String selection,

appendUrlFilter is probably more explicit.
Attachment #631955 - Flags: review?(lucasr.at.mozilla) → review+
>What happens on about:home when you run Fennec for the first time with this patch? 
>Empty list of top sites? 

Yes.

>Is that the intended UX? 

What's the alternative? If we don't have anything we can't show anything. Rework the UI to show a list of URL's if there's no thumbnails? Pre-load thumbnails for the add-on sites into the DB? (Blergh!)

>Is appendFilter still used anywhere after this patch? 

Nope, it could go.

>Is your assumption here that about pages don't have screenshots? Why is the url 
>filter not needed anymore?

I couldn't find any reason for adding it in the first place in bug 709708 which introduced the filter. The best I could come up with myself is that the about: pages can't get thumbnails and so we don't want them in AboutTopSites. So in that case this filter is no longer needed.
Keywords: uiwanted
(In reply to Gian-Carlo Pascutto (:gcp) from comment #8)
> >What happens on about:home when you run Fennec for the first time with this patch? 
> >Empty list of top sites? 
> 
> Yes.
> 
> >Is that the intended UX? 
> 
> What's the alternative? If we don't have anything we can't show anything.
> Rework the UI to show a list of URL's if there's no thumbnails? Pre-load
> thumbnails for the add-on sites into the DB? (Blergh!)

The pre-loaded images don't necessarily have to be a thumbnail. They could be more like a stylish banner for the page or something. Maybe check with UX? Just think that showing some top sites in about:home on first launch is good thing.

> >Is appendFilter still used anywhere after this patch? 
> 
> Nope, it could go.
> 
> >Is your assumption here that about pages don't have screenshots? Why is the url 
> >filter not needed anymore?
> 
> I couldn't find any reason for adding it in the first place in bug 709708
> which introduced the filter. The best I could come up with myself is that
> the about: pages can't get thumbnails and so we don't want them in
> AboutTopSites. So in that case this filter is no longer needed.

We use this filter to avoid any about: page to show up as a top site. They might have thumbnails so I'd probably keep the filter.
Summary for UX:

On the first Fennec startup, the user will not have visited any sites yet. We will have added some default bookmarks for them, but as these pages have never actually rendered we do not have thumbnails for them. 

Question 1: What should we do/show the user? Currently we show "empty" thumbnails.

We add some bookmarks such as about:firefox and about:home.

Question 2: Should we allow these special bookmarks to show in the Top Sites pane? (Currently we do not)
Reassigning based on a Brad's request. Note the need to eventually deal with 404's in comment 1.
Assignee: gpascutto → lucasr.at.mozilla
I am having the same issue.  Popular sites like Facebook, ABC News, and Yahoo! do not have a homescreeen thumbnail (empty) where as other sites do.  I believe this used to work at one time but now it does not.
Ian, Madhava, need your input here. Should we filter top sites so that we only show the ones with thumbnails in about:home? If we do that, we won't show any top sites on first launch, for example. What do you think?
I would be happy with not showing any top site thumbnails on first launch. 

If I recall correctly, that was actually the intended design some time ago. http://www.flickr.com/photos/61892693@N03/7003045121/in/photostream
tracking-fennec: 15+ → 17+
FWIW, i don't see any thumbnails anymore in Firefox 14.0.1 on 64-bit Windows 7 Enterprise. Maybe there's a network share issue, but everything else works.
(In reply to Cees T. from comment #15)
> FWIW, i don't see any thumbnails anymore in Firefox 14.0.1 on 64-bit Windows
> 7 Enterprise. Maybe there's a network share issue, but everything else works.
Wrong product; but thank's for commenting.
tracking-fennec: 17+ → ?
Keywords: qawanted
What is the status of this patch.
Flags: needinfo?(lucasr.at.mozilla)
Keywords: qawanted
Given that we now load thumbnails asynchronously in about:home after the query returns, it will be a bit tricky to skip top sites without thumbnails. The patches attached here will need to rewritten to account for the async thumbnail loading somehow.
Flags: needinfo?(lucasr.at.mozilla)
Attachment #631958 - Flags: review?(lucasr.at.mozilla) → review-
gcp, need new patches
tracking-fennec: ? → +
Flags: needinfo?(gpascutto)
With our delayed thumbnail loading, it's impossible to avoid the empty sites from showing up - we have no idea yet whether they have thumbnails or not the moment we show them.

So this is the best I could find; it will cause the empty thumbnails to "snap" away and be replaced by the next best sites, if necessary. I thought this would be potentially annoying if it snaps something from under the users finger, but in my testing it happens very quickly, so it not that bad. And not having thumbnails is pretty much a temporary state.

So we go through 3 states: sites without thumbnails, same sites with some thumbnails, and then a snap where the "holes" are filled by bubbling up sites.

I don't think we can do better without regressing about:home performance again.
Assignee: lucasr.at.mozilla → gpascutto
Attachment #631955 - Attachment is obsolete: true
Attachment #631958 - Attachment is obsolete: true
Attachment #682476 - Flags: review?(lucasr.at.mozilla)
Flags: needinfo?(gpascutto)
tracking-fennec: + → ---
blocking-fennec1.0: soft → ---
tracking-fennec: --- → +
Alternative approach: add a "has_thumbnail" field to Combined, default to FALSE, set if we have a thumbnail. Probably prettier than this,
(In reply to Gian-Carlo Pascutto (:gcp) from comment #21)
> Alternative approach: add a "has_thumbnail" field to Combined, default to
> FALSE, set if we have a thumbnail. Probably prettier than this,

This would probably degrade the performance of the frecency query a bit. We are not joining the images table (favicons, thumbnails) anymore.
Comment on attachment 682476 [details] [diff] [review]
Patch. Filter thumbnail-less URLs from topsites

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

I'd prefer to wait for the UI costumization (bug 783312) bits to land before pushing something like this. Cancelling the review for now.
Attachment #682476 - Flags: review?(lucasr.at.mozilla)
>This would probably degrade the performance of the frecency query a bit. We are 
>not joining the images table (favicons, thumbnails) anymore.

Why would it degrade the performance? The point of adding that field (not a computed field!) is exactly because we no longer join. Given the frecency calculation then has to run over fewer records, it might even be faster, and you can even take it into account when eventually joining the images table, which makes that query faster again.

>I'd prefer to wait for the UI costumization (bug 783312) bits to land before pushing something 
>like this.

OK. I think it makes more sense if the person coping with that bugs fixes this one; part of the patch may be salvageable or you can try Brad's approach.
Assignee: gpascutto → nobody
Depends on: 783312
I'm trying to triage some of these thumbnail bugs and would like some clarification.  The attachment shows a 404 for a couple of the thumbnails (implying an attempt *was* made to visit the site and have a thumbnail captured, but that attempt failed), but most of the discussion is talking about a first-run experience (where we'd just expect a missing thumbnail for the default bookmarks, not a 404).

Is the 404 in the attachment just a side-effect of a missing thumbnail on Fennec?  Or maybe this bug is conflating a few different issues (eg, I believe some comments are just reflecting bug 756881).  Or maybe there is something else I'm missing?
This bug is about what to do for top sites that have no thumbnail. No more no less.

There are a number of reasons why you could have that situation: first-run, some 404 situations and the situation that bug 756881 describes.
Whiteboard: [Engagement] → [Engagement][mentor=gcp]
Mentor: gpascutto
Whiteboard: [Engagement][mentor=gcp] → [Engagement]
filter on [mass-p5]
Priority: -- → P5
This is no longer relevant for first run: we have suggested sites instead.

We also have mitigations, like the tileImage work we added recently.

Various other bugs cover the case of a desktop page never getting a thumbnail -- bug 934702, for example.

And implementation-wise, we don't have a combined view now.

Closing.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.