Closed Bug 1009587 Opened 10 years ago Closed 10 years ago

Implement image precedence behaviour around suggested thumbnails

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 32

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

Attachments

(6 files)

For instance, we might want to always show the suggested site image instead of thumbnails if possible even on pinned and visited sites.
Ian, Deb, what direction do we want to take here? Should the suggested site images be sticky and always take precedence over the normal thumbnail for a given site? Right now, it's the other way around, once a suggested site is visited, we switch to site thumbnails.
Flags: needinfo?(ibarlow)
Flags: needinfo?(deb)
(In reply to Lucas Rocha (:lucasr) from comment #1)
> Ian, Deb, what direction do we want to take here? Should the suggested site
> images be sticky and always take precedence over the normal thumbnail for a
> given site?

I'd like to start with that. Those tiles look nicer than little thumbnails.
Flags: needinfo?(ibarlow)
Flags: needinfo?(deb)
Depends on: 1016949
Comment on attachment 8430110 [details] [diff] [review]
Part 1: Store suggested sites in a Map instead of a List (r=mfinkle)

We'll need to quickly find suggested sites by URL. Let's use a more appropriate data structure.
Attachment #8430110 - Flags: review?(mark.finkle)
Comment on attachment 8430111 [details] [diff] [review]
Part 2: Add SuggestedSites API to get image URL and bg color (r=mfinkle)

We need an API to query image URLs and background colors from arbitrary URLs so that we can override thumbnails with suggested site images in the UI.
Attachment #8430111 - Flags: review?(mark.finkle)
Comment on attachment 8430112 [details] [diff] [review]
Part 3: Hold a strong reference to cached suggested sites (r=mfinkle)

I originally made it a soft reference as a cautious measure in terms of memory footprint. But it turns our the data structure to cache suggested sites in memory is pretty small. Also, using  a soft reference makes the new APIs for querying image URLs and background colors a bit unreliable as they depend on the in-memory cache. We can't do any blocking I/O in these APIs because they need to be used synchronously in the UI thread.
Attachment #8430112 - Flags: review?(mark.finkle)
Comment on attachment 8430113 [details] [diff] [review]
Part 4: Use suggested thumbnail on top sites whenever it's available (r=mfinkle)

My original design assumed only suggested sites would use the nice default images. This is changing now to apply these images to any type of site (pinned, top, or suggested) in the grid. So, it doesn't make sense anymore to have this data (image url & bg color) in the cursor wrapper itself. This is why the IMAGE_URL and BG_COLOR columns were removed from the top sites 'schema'.

I considered using the SuggestedSites API under the hood directly in TopSitesCursorWrapper but I'd like to avoid coupling them so that I can unit test them more easily. A cursor wrapper only be dealing with cursors anyway.

With this patch, the logic for loading images is all handled in TopSitesPanel. The current image loading code in the top sites grid is overly complex and needs a lot of love. I'm planning to tackle this in bug 997774. But first I need some new APIs in Picasso (https://github.com/square/picasso/pull/512) and in our Favicons framework.
Attachment #8430113 - Flags: review?(mark.finkle)
Comment on attachment 8430114 [details] [diff] [review]
Part 5: Don't try to load thumbnails for blank urls (r=mfinkle)

Facepaln moment: we were trying to load images for sites with empty URLs. And the code was assuming the thumbnails loader would always run, even if there's nothing to load.
Attachment #8430114 - Flags: review?(mark.finkle)
Comment on attachment 8430115 [details] [diff] [review]
Part 6: Don't wait for thumbnails to show blank spots (r=mfinkle)

I've initially done that check to avoid showing the '+' images before the thumbnails were ready to be displayed i.e. it was a way to avoid having images popping up at different times. But the favicon loading logic kinda breaks this behaviour anyway because they are loaded asynchronously and in parallel for each grid item. Let's show real content as soon as possible instead.
Attachment #8430115 - Flags: review?(mark.finkle)
Attachment #8430110 - Flags: review?(mark.finkle) → review+
Attachment #8430111 - Flags: review?(mark.finkle) → review+
Attachment #8430112 - Flags: review?(mark.finkle) → review+
Comment on attachment 8430113 [details] [diff] [review]
Part 4: Use suggested thumbnail on top sites whenever it's available (r=mfinkle)

>diff --git a/mobile/android/base/GeckoApp.java.orig b/mobile/android/base/GeckoApp.java.orig
>new file mode 100644

Whoops. Don't add this file.
Attachment #8430113 - Flags: review?(mark.finkle) → review+
Attachment #8430114 - Flags: review?(mark.finkle) → review+
Attachment #8430115 - Flags: review?(mark.finkle) → review+
(In reply to Mark Finkle (:mfinkle) from comment #16)
> Comment on attachment 8430113 [details] [diff] [review]
> Part 4: Use suggested thumbnail on top sites whenever it's available
> (r=mfinkle)
> 
> >diff --git a/mobile/android/base/GeckoApp.java.orig b/mobile/android/base/GeckoApp.java.orig
> >new file mode 100644
> 
> Whoops. Don't add this file.

Oopsie, removed.
This got in today's nightly an dI can verify the correct behavior using the Marketplace/Mozilla pages. 

OS: Android 4.4
Device: Nexus 4
Status: RESOLVED → VERIFIED
QA Contact: ioana.chiorean
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.