Open
Bug 1140194
Opened 10 years ago
Updated 9 years ago
Storage for reading list preview images
Categories
(Firefox Graveyard :: Reading List, defect, P2)
Firefox Graveyard
Reading List
Tracking
(Not tracked)
NEW
People
(Reporter: adw, Unassigned)
References
(Blocks 1 open bug)
Details
(Whiteboard: [readinglist-v2])
We need a way to store preview images for reading list items. Gavin and I talked about doing what the page thumbnail service does: storing individual images on disk. I don't know how we get those images, but whatever gets them would add them to the store I imagine, and then ReadingList(Item) would pull them out.
Comment 1•10 years ago
|
||
(In reply to Drew Willcoxon :adw from comment #0)
> I don't know how we get those images
Ugh, so, see the thread I just started in task-continuity-dev:
https://mail.mozilla.org/pipermail/task-continuity-dev/2015-March/000102.html
I thought this was a synced data value, but it currently isn't. Hopefully we can get that added. Assuming that:
* Bug 1133429 will supply them when a page is added locally. I'll have a URL, but could supply a Blob or something.
* Added via a synced value (string URL). In which case, we'd need to queue it for download. I guess we could do that lazily, if we add a protocol handler similar to moz-anno:favicon, and fetch/cache it when that's hit (based on the item ID) - that'd also solve issues around initial sync and suddenly having 300 images to fetch.
Comment 2•10 years ago
|
||
(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #1)
> I thought this was a synced data value, but it currently isn't. Hopefully we
> can get that added.
That was quick - it's being added:
https://github.com/mozilla-services/readinglist/issues/156
Updated•10 years ago
|
Priority: -- → P1
Updated•10 years ago
|
Whiteboard: [readinglist-v2]
Updated•10 years ago
|
Whiteboard: [readinglist-v2]
Updated•10 years ago
|
Flags: qe-verify?
Flags: firefox-backlog+
Updated•10 years ago
|
Points: --- → 8
Comment 3•10 years ago
|
||
Drew, I think we can close this now as either invalid/wontfix since we are just going with adding the URL to the preview image and letting that get sync'd across devices (bug 1133429/bug 1123518). What do you think?
Flags: needinfo?(adw)
Reporter | ||
Comment 4•10 years ago
|
||
But the image needs to be cached locally, right? Or did we decide not to cache it at all? I don't think we did, but maybe I didn't hear about it.
Flags: needinfo?(adw)
Comment 5•10 years ago
|
||
It was raised as a possibility, but it has some pretty major downsides. Like opening the sidebar causing us to immediately start downloading hundreds of large images, thus swamping your connection. Makes the sidebar itself look slow, as well as whatever else you might be trying to load. I think we very much need to cache the images locally to avoid that.
Comment 6•10 years ago
|
||
I'm feeling like we could ship without this but dolske feels strongly that we should ship with this so keeping as a P1.
Reporter | ||
Comment 7•10 years ago
|
||
(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #6)
> I'm feeling like we could ship without this but dolske feels strongly that
> we should ship with this so keeping as a P1.
Based on our conversation yesterday I think Gavin disagrees with Dolske FWIW. CC'ing him.
Comment 8•10 years ago
|
||
What's the rationale for why this is a requirement for 38.1?
Comment 5's concern (large bandwidth impact) seems like it is mitigated or (mitigate-able) in several ways:
- the normal network cache will help us some (not sure how much in practice)
- only really a problem for people with many reading list entries
- we could change the sidebar code to only load visible images at first (chunking of image download requests)
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(dolske)
Comment 9•10 years ago
|
||
I should also note that users will not see their items shift in size/position as images are loaded since we hard-code the size of the thumbnails and only apply a background image to them, so reflows and repositioning will not occur as the images are loaded.
Comment 10•10 years ago
|
||
Ok, I'll cede on this one. :) We'll be ok for the initial ship, but should really really get back to fix this one for the reasons previously noted. (Or at least mitigate in the ways comment 8 suggests.)
Flags: needinfo?(dolske)
Priority: P1 → P2
Whiteboard: [readinglist-v2]
Assignee | ||
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•