1093869 - Migrate (or just remove) old indexedDB reader mode cache

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[:Margaret Leibovic]

Bug 1007409 replaces the indexedDB reader mode cache, with cached JSON files. We should delete the old indexedDB database on app upgrade, and perhaps migrate articles that are stored there. This indexedDB cache had no eviction policy, so there could be a lot of articles in there, but because we don't have any "archive" mechanism for the reading list, it would still match the articles that are actually stored in the user's reading list.

To do this migration, we'd need to do some sort of pref/version check the first time we try to use the cache, which actually happens during the first page load when we're checking to see if we should show the reader icon. This should be pretty low-cost compared to actually parsing the article, but we can do it in an async task to try to avoid impacting page load time.

However, if we just want to clean up the old indexedDB database, and not migrate the data, we could do this at whatever time is most convenient to us.

I just landed the patch for bug 1007409, so Nightly users won't have their cache migrated for them... I wonder if people will complain about that.

Comment 1

[:Margaret Leibovic]

We should try to do something here for 36.

Comment 2

[:Margaret Leibovic]

Attachment: Migrate indexedDB reader mode cache

I decided that we should migrate the indexedDB cache to avoid losing user data. Given the fact that there's no way to archive reading list items, the old cache really should map 1-1 with the items that are in the reading list (except it won't have the ones added from the share overlay).

In bug 1093172, I'm going to add better support for making sure all reading list content is in the cache, but I want to land this first to maintain parity with the old system.

I didn't do any manual testing of the update, but I did make this unit test. However, I can try doing some manual tests before landing this. I'll probably just have to make a build of an old changeset, since too many patches landed after the indexedDB switch to make it easy to back out.

Try push:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bab8a11d3638

Comment 3

[Brian Nicholson (:bnicholson)]

Comment on attachment 8528138 [details] [diff] [review]
Migrate indexedDB reader mode cache

Review of attachment 8528138 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/tests/testReadingListCache.js
@@ +80,5 @@
> +    let request = win.indexedDB.open("about:reader", 1);
> +    request.onerror = e => reject(request.error);
> +
> +    // This will always happen because there is no pre-existing data store.
> +    request.onupgradeneeded = e => {

Nit: Might want to make this "evt" or "event" since "e" is generally used for errors/exceptions (here and elsewhere).

::: mobile/android/chrome/content/Reader.js
@@ +431,5 @@
> +      request.onerror = e => reject(request.error);
> +
> +      // If there is no DB to migrate, don't do anything.
> +      request.onupgradeneeded = e => resolve(null);
> +    }).catch(Cu.reportError);

Drop this catch and bubble to the caller?

Comment 4

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #3)
> Comment on attachment 8528138 [details] [diff] [review]
> Migrate indexedDB reader mode cache
> 
> Review of attachment 8528138 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/tests/testReadingListCache.js
> @@ +80,5 @@
> > +    let request = win.indexedDB.open("about:reader", 1);
> > +    request.onerror = e => reject(request.error);
> > +
> > +    // This will always happen because there is no pre-existing data store.
> > +    request.onupgradeneeded = e => {
> 
> Nit: Might want to make this "evt" or "event" since "e" is generally used
> for errors/exceptions (here and elsewhere).

Will do.

> ::: mobile/android/chrome/content/Reader.js
> @@ +431,5 @@
> > +      request.onerror = e => reject(request.error);
> > +
> > +      // If there is no DB to migrate, don't do anything.
> > +      request.onupgradeneeded = e => resolve(null);
> > +    }).catch(Cu.reportError);
> 
> Drop this catch and bubble to the caller?

Good call. I think originally I thought this would throw if the db doesn't exist, but later learned onupgradeneeded would be called.

Comment 5

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/5b548dd1bdd4

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5b548dd1bdd4