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)
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 32
People
(Reporter: lucasr, Assigned: lucasr)
References
Details
Attachments
(6 files)
5.41 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
3.89 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
5.77 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
124.64 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
3.63 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
1.38 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
For instance, we might want to always show the suggested site image instead of thumbnails if possible even on pinned and visited sites.
Assignee | ||
Comment 1•10 years ago
|
||
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)
Comment 2•10 years ago
|
||
(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)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(deb)
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Comment 10•10 years ago
|
||
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)
Assignee | ||
Comment 11•10 years ago
|
||
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)
Assignee | ||
Comment 12•10 years ago
|
||
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)
Assignee | ||
Comment 13•10 years ago
|
||
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)
Assignee | ||
Comment 14•10 years ago
|
||
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)
Assignee | ||
Comment 15•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8430110 -
Flags: review?(mark.finkle) → review+
Updated•10 years ago
|
Attachment #8430111 -
Flags: review?(mark.finkle) → review+
Updated•10 years ago
|
Attachment #8430112 -
Flags: review?(mark.finkle) → review+
Comment 16•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8430114 -
Flags: review?(mark.finkle) → review+
Updated•10 years ago
|
Attachment #8430115 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 17•10 years ago
|
||
(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.
Assignee | ||
Comment 18•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/4237886831c4 https://hg.mozilla.org/integration/fx-team/rev/1fba768ddb01 https://hg.mozilla.org/integration/fx-team/rev/c02f55857863 https://hg.mozilla.org/integration/fx-team/rev/1df799baaad1 https://hg.mozilla.org/integration/fx-team/rev/a88116d0474b https://hg.mozilla.org/integration/fx-team/rev/cf301870f3be
https://hg.mozilla.org/mozilla-central/rev/4237886831c4 https://hg.mozilla.org/mozilla-central/rev/1fba768ddb01 https://hg.mozilla.org/mozilla-central/rev/c02f55857863 https://hg.mozilla.org/mozilla-central/rev/1df799baaad1 https://hg.mozilla.org/mozilla-central/rev/a88116d0474b https://hg.mozilla.org/mozilla-central/rev/cf301870f3be
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Comment 20•10 years ago
|
||
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
Updated•10 years ago
|
QA Contact: ioana.chiorean
Updated•3 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
•