1140194 - Storage for reading list preview images

Status: RESOLVED INCOMPLETE

(Firefox Graveyard :: Reading List, defect, P2)

Description

[Drew Willcoxon :adw]

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

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 4

[Drew Willcoxon :adw]

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.

Comment 5

[Justin Dolske [:Dolske]]

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

[Bryan Clark (DevTools PM) [:clarkbw]]

I'm feeling like we could ship without this but dolske feels strongly that we should ship with this so keeping as a P1.

Comment 7

[Drew Willcoxon :adw]

(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

[:Gavin Sharp [email: [e-mail]]]

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)

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

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

[Justin Dolske [:Dolske]]

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