516013 - Aggressive caching of the current lightweight theme

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[Dão Gottwald [:dao]]

Right now the images for lightweight themes use the web cache, but we should store them more robustly for users who clear the cache on shutdown or servers that send bad caching headers, and maybe other cases.

Comment 1

[Johnathan Nightingale [:johnath]]

Dao, I believe you said this was on your plate, not Dave's, so I'm assigning it to you to clear out the "blockers assigned to nobody@" list. If I'm wrong, please feel free to reassign. I don't have to tell you how important it is that we get this piece working.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Another common case is users who establish a network connection after starting their browser (indeed, often because they start their browser, and its first request initiates the network connection attempt).  In that case, we won't be able to load the images from the network on startup, and perhaps not from the web cache either (even when the images are in it).

Comment 3

[Dão Gottwald [:dao]]

Attachment: WIP

This works, I'm not sure that it's sane...

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 403499 [details] [diff] [review]
WIP

I think dumping unknown files in the profile folder could be dangerous, but maybe it's not. Dave, what do you think?

Comment 5

[Dave Townsend [:mossop]]

I don't think storing the files in the profile is a particular problem, after all the images are probably getting stored in the cache in the profile right now anyway. I wonder whether we should consider caching the files for all recent themes though. This would solve the problem with trying to change to a different theme when offline.

Comment 6

[Dão Gottwald [:dao]]

I was trying to avoid inventing a file structure, e.g. theme -> file name mappings (just using the IDs as is doesn't seem right, as they aren't sanitized), and keeping track of these files. And while it's unfortunate that you can't switch themes in offline mode, I don't think that's all that important. As a quick and dirty fix, the Add-ons manager could probably hide any lightweight themes except for the current one in offline mode.

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 403499 [details] [diff] [review]
WIP

I think we need to verify that aStatus is a success and that the http status was a successful one or we'll end up caching an error page.

I'm also wondering if there is an easy way we can kick off persisting the images in the event that the user exits before it is done.

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #7)
> (From update of attachment 403499 [details] [diff] [review])
> I think we need to verify that aStatus is a success and that the http status
> was a successful one or we'll end up caching an error page.

requestSucceeded seems to translate to status==200.

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 403499 [details] [diff] [review]
WIP

Missed that in my read through, this is fine.

Comment 10

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/5f03363ae12d

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Looks like this is causing orange:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1255117441.1255119464.1727.gz

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Backed out:
http://hg.mozilla.org/mozilla-central/rev/505a2a075e11
http://hg.mozilla.org/mozilla-central/rev/52fb6780a698

Comment 13

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/f02075968efe

Comment 14

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/09e009ff6ef9

Comment 15

[Tony Chung [:tchung]]

dao, how can i test this?

Comment 16

[Dão Gottwald [:dao]]

Cut your network connection, clear the cache and restart Firefox.

Comment 17

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091202 Minefield/3.7a1pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2b5pre) Gecko/20091202 Namoroka/3.6b5pre