While implementing bug 773373 I noticed that the localStorage code is quite complicated by the fact that we have various exceptions to the 5MB quota limit in localStorage. First of all we grant 200MB of data for any page which has offline storage enabled. Second, there was some tie-in with pages which are allowed to use the "persist" feature in *XUL*. This made sense back when localStorage was the only storage mechanism we had. And before we realized the really bad performance problems we have due to the synchronous IO. But now we have IndexedDB as well as more experience with the problems that the synchronous IO is causing. And XUL support has been turned off by default, so definitely no need to worry about that. So let's limit localStorage to 5MB everywhere and remove any integration with offline apps. As an added bonus, this makes the localStorage code significantly simpler.

Comment on attachment 644806 [details] [diff] [review] Patch to fix So, with this patch, when I delete cookies using e.g. the Clear Recent History dialog, will also data stored in localStorage by offline-enabled app (origin) be deleted along with cookies?

Yes. The idea was to completely separate localStorage from offline apps and instead make it behave just like cookies. Right now if you clear data for a specific domain we will clear the cookies for that domain, no matter if offline is enabled for the domain or not. So it seems ok to me to clear localStorage data as well. Long term we should merge the offline storage policies with the indexedDB storage policies. Bug 742822 is laying the foundation for some of that.

Jonas, I'll take a look on the plans prior to review. Until there is a mechanism to preserve localStorage data for offline-enabled domain, I am strongly against your patch. Please also check this blog post, mainly the "(Not only) user’s data saved by apps" section: http://www.janbambas.cz/offline-application-cache-in-firefox/

Why is saving localStorage for offline a prerequisite? We already have indexedDB which won't be deleted when cookie data is deleted. We really want people to use indexedDB instead of localStorage due to the performance issues with localStorage.

Yes, we want, but at the moment there could be applications that use localStorage and store locally important data. And we may delete those data just by deleting cookies. I really cannot accept that.

On the hand, as I thinking of this more, *if this is well communicated*, it really can be a simplification. I'll do the review of the patch tomorrow.

Comment on attachment 644806 [details] [diff] [review] Patch to fix reverting the r flag (actually, r- was accidental)

(In reply to Jonas Sicking (:sicking) from comment #0) > And XUL support has been turned off by default, so definitely no need to > worry about that. In what bug(s)?

Comment on attachment 644806 [details] [diff] [review] Patch to fix Review of attachment 644806 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab. I didn't test my self this time. Sorry for the delay.

Remote xul was disabled in bug 546857 Thanks for the review!!

Forwarding review flag.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd824db5b0df https://hg.mozilla.org/integration/mozilla-inbound/rev/e564016b9de9

https://hg.mozilla.org/mozilla-central/rev/fd824db5b0df https://hg.mozilla.org/mozilla-central/rev/e564016b9de9

You removed clearOfflineApps from the IDL but you didn't remove any of the callers. http://mxr.mozilla.org/comm-central/search?string=.clearOfflineApps

will do that, sorry

(In reply to Jonas Sicking (:sicking) from comment #0) > So let's limit localStorage to 5MB everywhere and remove any integration > with offline apps. > I'll post additional patch to fix remaining offline integration bits.

Since nothing has been backed out, and there are fixes landed on m-i for the remaining problem I'm going to put this back into FIXED state

about:newtab (new tab page) is broken. http://forums.mozillazine.org/viewtopic.php?p=12288671#p12288671

I think the problem is that new tab page stores some data (e.g. pinned links) in local storage and it used to be stored in chromeappsstore.sqlite before this bug. Now it is stored in webappsstore.sqlite. So essentially the data is lost :(

We should probably copy data from chromeappsstore.sqlite to webappsstore.sqlite during LS initialization and then import about:newtab specific data to IDB somewhere in browser initialization. We also need to convert about:newtab to use IDB of course. Sigh.

(In reply to Jan Varga [:janv] from comment #22) > Now it is stored in webappsstore.sqlite. This sounds like a problem. chromeappstore.sqlite was explicitly created so that clearing localstorage data didn't wipe out about: page data (see https://hg.mozilla.org/mozilla-central/rev/737c80aa9134 ). Did this bug regress that?

about: pages shouldn't be using localStorage at all due to it's sync-IO nature. They should migrate to IDB as soon as possible. Of course, it's not ok that we have dataloss, so me and Jan has been talking about various options to fix the current situation.

(In reply to Jonas Sicking (:sicking) from comment #25) > about: pages shouldn't be using localStorage at all due to it's sync-IO > nature. They should migrate to IDB as soon as possible. No objection here, we're already well aware of this problem. But we can't go busting the existing functionality until we've actually done that work.