Storage for reading list preview images

NEW
Unassigned

Status

defect
P2
normal
4 years ago
3 years ago

People

(Reporter: adw, Unassigned)

Tracking

(Blocks 1 bug)

Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [readinglist-v2])

Reporter

Description

4 years ago
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.
(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.
(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
Priority: -- → P1
Whiteboard: [readinglist-v2]
Whiteboard: [readinglist-v2]
Flags: qe-verify?
Flags: firefox-backlog+
Blocks: 1132074
Points: --- → 8
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

4 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)
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.
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

4 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.
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)
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.
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]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.