1234315 - Migrate reading list items to bookmarks

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reading List, defect)

Description

[:Margaret Leibovic]

We should migrate the existing reading list items into a bookmarks folder, and we should make sure adding new reading list items will create bookmarks in this folder.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Items originally in old RL Panel
---
Not RV-able:
 - Just add to Mobile Bookmarks 

Reader View-able: 
 - Also add to new "Reading List"

Comment 2

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234315 - Move readermode tools into their own package r?nalexander

Review commit: https://reviewboard.mozilla.org/r/34827/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34827/

Comment 3

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234315 - Introduce test for url to cache-filename r=nalexander

Review commit: https://reviewboard.mozilla.org/r/34829/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34829/

Comment 4

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234315 - Convert getMobileFolderId into getFolderIDForGuid r?rnewman

We only use this in one location, so there's little advantage to storing our parameters
as static final. Allowing arbitrary guids will allow us to retrieve e.g. the reading
list folder id easily in future.

Review commit: https://reviewboard.mozilla.org/r/34831/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34831/

Comment 5

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234315 - Migrate reading-list table to bookmarks r=rnewman

Review commit: https://reviewboard.mozilla.org/r/34833/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34833/

Comment 6

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234315 - Implement getBookmarkIdForFolderWithParentAndName r?rnewman

Review commit: https://reviewboard.mozilla.org/r/34835/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34835/

Comment 7

[Andrzej Hunt :ahunt]

The commits I've put into review take care of migrating our DB content, however that still puts is in a rather broken state (opening the reading-list panel crashes, you can't add/remove reading-list items, etc). I think some of those issues are covered in other bugs, I'm pretty certain we'll want to wait until we have all of these issues solved before we start reviewing and pushing?

I wonder if we could use a special icon (e.g. the readermode book icon) for the Reading-List folder, just to make it easier to notice? How hard that would be depends on how we actually manage the reading-list folder longer term though (it's probably easy to do if this is a "magical" reading-list folder which we handle specially, it's harder to do if it's just a plain old folder, since we'd need to add general folder-icon management code).

Comment 8

[:Margaret Leibovic]

(In reply to Andrzej Hunt :ahunt from comment #7)
> The commits I've put into review take care of migrating our DB content,
> however that still puts is in a rather broken state (opening the
> reading-list panel crashes, you can't add/remove reading-list items, etc). I
> think some of those issues are covered in other bugs, I'm pretty certain
> we'll want to wait until we have all of these issues solved before we start
> reviewing and pushing?

Yeah, we're going to have to think carefully about our plan for landing all these changes. Let's make sure we don't end up in a busted state for 47 if we need to decide to hold back shipping these changes. I think we should keep this migration logic behind a Nightly-only flag until we're confident that we want to let this feature ride the trains.

I'm okay with a somewhat busted state in Nightly, but we should probably remove all the "Add to reading list" UI at the same time that we land this migration, since none of that UI will work. Or if we want to be prepared to undo those changes, we could also hide those pieces of UI on Nightly before we remove them in the future. As much as I'm okay with a busted Nightly, we need to have a contingency plan for it to work well after the merge to Aurora.

We should avoid trying to maintain a large local patch series on your machine, since that becomes a pain to review and maintain, and any large commit series is going to be guaranteed to require follow-up fixes anyway. If we really think a lot of bugs need to all land at once for this migration, maybe we should consider working from a project branch. 

> I wonder if we could use a special icon (e.g. the readermode book icon) for
> the Reading-List folder, just to make it easier to notice? How hard that
> would be depends on how we actually manage the reading-list folder longer
> term though (it's probably easy to do if this is a "magical" reading-list
> folder which we handle specially, it's harder to do if it's just a plain old
> folder, since we'd need to add general folder-icon management code).

After some more discussion last week, product/UX decided that instead of migrating items to a newly-created reading list folder, we should just migrate them all to the root mobile bookmarks folder, and then create a "smart" reading list folder that lists all items for which we have saved reader view content.

Barbara is planning to send out an email about this, but I wanted to raise that here before you spend too much time worrying about this.

Comment 9

[Andrzej Hunt :ahunt]

Ooops, I just realised my first draft migration commit still results in us creating a new reading-list table on database creation (i.e. new install), that's the only change in the new set of patches going onto reviewboard...

Comment 10

[Andrzej Hunt :ahunt]

Comment on attachment 8718985 [details]
MozReview Request: Bug 1234315 - Move readermode tools into their own package r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34827/diff/1-2/

Comment 11

[Andrzej Hunt :ahunt]

Comment on attachment 8718986 [details]
MozReview Request: Bug 1234315 - Introduce test for url to cache-filename r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34829/diff/1-2/

Comment 12

[Andrzej Hunt :ahunt]

Comment on attachment 8718987 [details]
MozReview Request: Bug 1234315 - Convert getMobileFolderId into getFolderIDForGuid r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34831/diff/1-2/

Comment 13

[Andrzej Hunt :ahunt]

Comment on attachment 8718988 [details]
MozReview Request: Bug 1234315 - Migrate reading-list table to bookmarks r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34833/diff/1-2/

Comment 14

[Andrzej Hunt :ahunt]

Comment on attachment 8718989 [details]
MozReview Request: Bug 1234315 - Implement getBookmarkIdForFolderWithParentAndName r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34835/diff/1-2/

Comment 15

[Andrzej Hunt :ahunt]

Notes for (mainly) myself:

We'll need to be able to quickly determine which bookmarks are also readerview items (i.e. available in the readercache), in order to show a readerview icon for those bookmarks. Hence we'd need to ideally do readercache bookkeeping in the DB. We should create this table (probably just mapping URL->cache-file-name) as part of the same DB migration. We can then continue using one SQL query (with a join against the readercache list) to generate the bookmarks list.

Comment 16

[Andrzej Hunt :ahunt]

Comment on attachment 8718986 [details]
MozReview Request: Bug 1234315 - Introduce test for url to cache-filename r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34829/diff/2-3/

Comment 17

[Andrzej Hunt :ahunt]

Comment on attachment 8718985 [details]
MozReview Request: Bug 1234315 - Move readermode tools into their own package r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34827/diff/2-3/

Comment 18

[Andrzej Hunt :ahunt]

Comment on attachment 8718987 [details]
MozReview Request: Bug 1234315 - Convert getMobileFolderId into getFolderIDForGuid r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34831/diff/2-3/

Comment 19

[Andrzej Hunt :ahunt]

Comment on attachment 8718989 [details]
MozReview Request: Bug 1234315 - Implement getBookmarkIdForFolderWithParentAndName r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34835/diff/2-3/

Comment 20

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234315 - Introduce SavedReaderViewHelper to track cached reader view items r?nalexander

Review commit: https://reviewboard.mozilla.org/r/38525/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38525/

Comment 21

[Andrzej Hunt :ahunt]

Comment on attachment 8718988 [details]
MozReview Request: Bug 1234315 - Migrate reading-list table to bookmarks r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34833/diff/2-3/

Comment 22

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/38525/#review35161

::: mobile/android/base/java/org/mozilla/gecko/GeckoProfile.java:479
(Diff revision 1)
> +    public synchronized  ReaderCacheHelper getReaderCacheHelper() {

nit: spaces.

::: mobile/android/base/java/org/mozilla/gecko/reader/ReaderCacheHelper.java:21
(Diff revision 1)
> + * we have.

Your model is that you have a single instance, which then manages its own locking.  That's a reasonable approach, but you should spell it out here.

::: mobile/android/base/java/org/mozilla/gecko/reader/ReaderCacheHelper.java:42
(Diff revision 1)
> +            items = new JSONObject();

Log.  A little more chatty throughout, I think.

::: mobile/android/base/java/org/mozilla/gecko/reader/ReaderCacheHelper.java:47
(Diff revision 1)
> +    private JSONObject makeItem(String path, long size) throws JSONException {

Null check path?  Perhaps annotate them.

::: mobile/android/base/java/org/mozilla/gecko/reader/ReaderCacheHelper.java:77
(Diff revision 1)
> +        commit();

With the inline `commit`, you can't call this safely from the main UI thread.  Is that what you want?  (You can't with the `insertAnnotations`, either, of course.)

::: mobile/android/base/java/org/mozilla/gecko/reader/ReaderCacheHelper.java:89
(Diff revision 1)
> +    public int size() {

Are you confident that `JSONObject` is itself thread-safe, so that this is safe?  My understanding is that it isn't, so this should be synchronized as well.

Comment 23

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8727596 [details]
MozReview Request: Bug 1234315 - Introduce SavedReaderViewHelper to track cached reader view items r?nalexander

Clearing flag.

Comment 24

[Andrzej Hunt :ahunt]

Comment on attachment 8718986 [details]
MozReview Request: Bug 1234315 - Introduce test for url to cache-filename r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34829/diff/3-4/

Comment 25

[Andrzej Hunt :ahunt]

Comment on attachment 8718985 [details]
MozReview Request: Bug 1234315 - Move readermode tools into their own package r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34827/diff/3-4/

Comment 26

[Andrzej Hunt :ahunt]

Comment on attachment 8718987 [details]
MozReview Request: Bug 1234315 - Convert getMobileFolderId into getFolderIDForGuid r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34831/diff/3-4/

Comment 27

[Andrzej Hunt :ahunt]

Comment on attachment 8718989 [details]
MozReview Request: Bug 1234315 - Implement getBookmarkIdForFolderWithParentAndName r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34835/diff/3-4/

Comment 28

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234315 - Add reader view to UrlAnnotations r=sebastian

Review commit: https://reviewboard.mozilla.org/r/40605/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40605/

Comment 29

[Andrzej Hunt :ahunt]

Comment on attachment 8727596 [details]
MozReview Request: Bug 1234315 - Introduce SavedReaderViewHelper to track cached reader view items r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38525/diff/1-2/

Comment 30

[Andrzej Hunt :ahunt]

Comment on attachment 8718988 [details]
MozReview Request: Bug 1234315 - Migrate reading-list table to bookmarks r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34833/diff/3-4/

Comment 31

[Richard Newman [:rnewman]]

I agree with Margaret that this either needs to land in a project branch (or somewhere that it can be rebased, but with builds produced), or needs a very careful plan for landing that allows for flags to be flipped to preserve functionality while minimizing risk. I'd like to see lots more lists written down somewhere…

Comment 32

[Richard Newman [:rnewman]]

Comment on attachment 8718986 [details]
MozReview Request: Bug 1234315 - Introduce test for url to cache-filename r=nalexander

https://reviewboard.mozilla.org/r/34829/#review37203

Comment 33

[Richard Newman [:rnewman]]

Comment on attachment 8718985 [details]
MozReview Request: Bug 1234315 - Move readermode tools into their own package r?nalexander

https://reviewboard.mozilla.org/r/34827/#review37205

I encourage you to split out the refactors/renames/etc. -- anything you can land now without breaking any functionality -- into a separate bug and land them immediately. No sense making this stack any deeper than it already is.

Comment 34

[Richard Newman [:rnewman]]

Comment on attachment 8718987 [details]
MozReview Request: Bug 1234315 - Convert getMobileFolderId into getFolderIDForGuid r?rnewman

https://reviewboard.mozilla.org/r/34831/#review37207

1. Why would you need to do this if we're not subsequently adding items into that folder?
2. Why would you need to do this if we're not migrating items into a folder at all?

Comment 35

[Richard Newman [:rnewman]]

Comment on attachment 8718989 [details]
MozReview Request: Bug 1234315 - Implement getBookmarkIdForFolderWithParentAndName r?rnewman

https://reviewboard.mozilla.org/r/34835/#review37209

Same comment.

Note that Mobile Bookmarks is special, because it's a root -- protected, never moved, never deleted.

Comment 36

[Richard Newman [:rnewman]]

https://reviewboard.mozilla.org/r/38525/#review37211

I think this needs a little more thought, and for those thoughts to be turned into documentation. What is this class *for*? What does it actually guarantee?

::: mobile/android/base/java/org/mozilla/gecko/GeckoProfile.java:91
(Diff revision 2)
>      private final File mMozillaDir;
>      private final Context mApplicationContext;
>  
>      private final BrowserDB mDB;
>  
> +    private ReaderCacheHelper mReaderCacheHelper = null;

Not being `final` is a sign that you're going to hit concurrency or lifecycle issues at some point. Comment this here, noting that you should only access this -- even internally -- through the accessor.

::: mobile/android/base/java/org/mozilla/gecko/reader/ReaderCacheHelper.java:25
(Diff revision 2)
> +import java.io.IOException;
> +
> +/**
> + * Helper to keep track of items that are stored in the readermode cache. This is intended to allow
> + * us to quickly determine whether a given url is in the cache, and also how many cached items
> + * we have.

But it's wrong, though, isn't it? You're adding READERMODE annotations for a URL when you add, and you're removing READERMODE annotations when you remove, but what happens when Android (or the user, or an add-on, or your own bug) removes files from your cache directory without updating this bookkeeping?

(If you're using the Android cache dir for reader items, the OS itself can delete things. If you're not, you need to give strong thought to expiration.)

What you're doing here only works if READERMODE means "I would like this thing to be cached", but you still need some way of knowing at point of use if the thing you want to show is _actually_ in the cache.

Think carefully about what a URL metadata entry for "reader view" really means -- is it existence bookkeeping (in which case does it reflect user intent?), or is it intent tracking (in which case does it reflect the capabilities we have when we're offline?)?

::: mobile/android/base/java/org/mozilla/gecko/reader/ReaderCacheHelper.java:131
(Diff revision 2)
> +
> +        profile.writeFile(FILE_PATH, mItems.toString());
> +    }
> +
> +    /**
> +     * Return the ReaderMode url for a given url if it is contained in the cache. Returns the

s/url/URL/g throughout.

::: mobile/android/base/java/org/mozilla/gecko/reader/ReaderCacheHelper.java:137
(Diff revision 2)
> +     * plain url if the page is not cached.
> +     */
> +    public static String getReaderUrlIfCached(final Context context, @NonNull final String url) {
> +        ReaderCacheHelper rch = GeckoProfile.get(context).getReaderCacheHelper();
> +
> +        if (rch.isUrlCached(url)) {

s/Url/URL/g throughout.

Comment 37

[Richard Newman [:rnewman]]

Comment on attachment 8718988 [details]
MozReview Request: Bug 1234315 - Migrate reading-list table to bookmarks r=rnewman

https://reviewboard.mozilla.org/r/34833/#review37213

Comments regardless of more directional changes.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1191
(Diff revision 4)
> +                                     null,
> +                                     null,
> +                                     null,
> +                                     ReadingListItems.ADDED_ON + " DESC");
> +
> +        if (readingListCursor.getCount() > 0) {

Always use the boolean result from `moveToFirst` instead of `getCount`, and prefer early returns.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1234
(Diff revision 4)
> +                        new String[] { String.valueOf(mobileBookmarksID) } );
> +
> +                readingListFolderID = getBookmarkIdForFolderWithParentAndName(db, mobileBookmarksID, migratedReadingListTitle);
> +            }
> +
> +            // Reuse the ContentValues outside the loop to prevent allocating unnecessary additional new ContentValues.

I don't think this is strictly safe -- ContentProviders kinda own the passed-in CV.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1256
(Diff revision 4)
> +            while (readingListCursor.moveToNext()) {
> +                final String url = readingListCursor.getString(readingListCursor.getColumnIndexOrThrow(ReadingListItems.URL));
> +
> +                readingListItemValues.put(Bookmarks.GUID, Utils.generateGuid());
> +                readingListItemValues.put(Bookmarks.URL, url);
> +                readingListItemValues.put(Bookmarks.TITLE, readingListCursor.getString(readingListCursor.getColumnIndexOrThrow(ReadingListItems.TITLE)));

Compute the column indices once, not on each step through the loop.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1285
(Diff revision 4)
> +                        final String path = cacheFile.getAbsolutePath();
> +                        long size = cacheFile.length();
> +
> +                        readerCacheHelper.put(url, path, size);
> +                    } else {
> +                        // This should never happen, but we don't actually know whether or not orphaned

Telemetry?

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1294
(Diff revision 4)
> +                }
> +            }
> +        }
> +
> +        db.execSQL("DROP TABLE " + TABLE_READING_LIST);
> +        readingListCursor.close();

This should be in a `finally` block, and it should probably happen before you drop the table it refers to!

Comment 38

[Andrzej Hunt :ahunt]

(In reply to Richard Newman [:rnewman] from comment #34)
> Comment on attachment 8718987 [details]
> MozReview Request: Bug 1234315 - Convert getMobileFolderId into
> getFolderIDForGuid r?rnewman
> 
> https://reviewboard.mozilla.org/r/34831/#review37207
> 
> 1. Why would you need to do this if we're not subsequently adding items into
> that folder?
> 2. Why would you need to do this if we're not migrating items into a folder
> at all?

My mental to-do list failed here, I'll remove this patch - I intially thought the migrated reading list folder would have a special GUID (which was in my intial implementation), and then forgot to revert this when I undid that.

Comment 39

[Andrzej Hunt :ahunt]

Comment on attachment 8718985 [details]
MozReview Request: Bug 1234315 - Move readermode tools into their own package r?nalexander

Will be landed earlier in Bug 1257555 to minimise the number of patches landing simultaneously.

Comment 40

[Andrzej Hunt :ahunt]

Comment on attachment 8718986 [details]
MozReview Request: Bug 1234315 - Introduce test for url to cache-filename r=nalexander

Will also be landed earlier in Bug 1257555 to minimise the number of patches landing simultaneously.

Comment 41

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/34833/#review37325

Clearing review.  I have no significant concerns here, modulo following our established `Cursor` practice, like rnewman pointed out.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1148
(Diff revision 4)
>  
> +    // Get the cache path for a URL, based on the storage format in place during the 27to28 transition.
> +    // This is a reimplementation of _toHashedPath from ReaderMode.jsm - given that we're likely
> +    // to migrate the ReaderCache implementation at some point, it seems safest to have a local
> +    // implementation here - moreover this is probably faster than calling into JS.
> +    private String getReaderCacheFileNameForURL(String url) {

A little unit test for this, asserting a few things computed in JS and vice-versa, would be nice.

In general, can we not migrate this cache forward at all and just try to re-cache items?  Or wait until the user re-visits an item?

I think most caches will be very small (since few users use the feature heavily), and it's finicky to get this migration -- which touches file storage produced by Gecko! -- just right.  Can we skip it?

Comment 42

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8727596 [details]
MozReview Request: Bug 1234315 - Introduce SavedReaderViewHelper to track cached reader view items r?nalexander

https://reviewboard.mozilla.org/r/38525/#review37329

Clearing review pending your cycle with rnewman.

Comment 43

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/38525/#review37331

Clearing review pending your cycle with rnewman.

Comment 44

[Andrzej Hunt :ahunt]

> :::
> mobile/android/base/java/org/mozilla/gecko/reader/ReaderCacheHelper.java:25
> (Diff revision 2)
> > +import java.io.IOException;
> > +
> > +/**
> > + * Helper to keep track of items that are stored in the readermode cache. This is intended to allow
> > + * us to quickly determine whether a given url is in the cache, and also how many cached items
> > + * we have.
> 
> But it's wrong, though, isn't it? You're adding READERMODE annotations for a
> URL when you add, and you're removing READERMODE annotations when you
> remove, but what happens when Android (or the user, or an add-on, or your
> own bug) removes files from your cache directory without updating this
> bookkeeping?
> 
> (If you're using the Android cache dir for reader items, the OS itself can
> delete things. If you're not, you need to give strong thought to expiration.)
> 
> What you're doing here only works if READERMODE means "I would like this
> thing to be cached", but you still need some way of knowing at point of use
> if the thing you want to show is _actually_ in the cache.
> 
> Think carefully about what a URL metadata entry for "reader view" really
> means -- is it existence bookkeeping (in which case does it reflect user
> intent?), or is it intent tracking (in which case does it reflect the
> capabilities we have when we're offline?)?
> 

In the initial implementation, the ReaderCacheHelper will be a 1:1 copy of the readermode url-annotations. We use our own (non-cache) directory, so the "cache" part of the name is misleading.

I agree that we should move to a true (Android managed) cache in future, but that's really a separate issue from this migration. The url-annotations table is unnecessary at this stage, but it's helpful to have to (A) avoid another DB migration in future and (B) allow simpler implementation of the smartfolder, which is more future-proof.

If/when we move to a true cache (this will also be required if/when we sync annotations too), we'll need to add another state (currently a bookmark is either a normal bookmark, or an offline readermode bookmark), i.e. we'll have normal bookmark, readermode bookmark, readermode+offline bookmark . Showing just the readermode+offline state is simple with the in-memory cache helper, however showing DB data (i.e. the annotations) consistently across all our awesomescreen lists is hard to keep consistent, and we'd need to do more work there for that to happen. The issue is we have 3+ different DB queries for bookmarks, topsites, and search results, with 3 different adapters, so passing in the readermode state via SQL is difficult (we probably should simplify that into one implementation), but we use the same view to display a bookmark in all 3 adapters, hence with the ReaderCache we just need to check for the offline state in that view (i.e. in TwoLinePageRow).

(The ideal future state is that the url-annotations will be the canonical record of readermode items, but right now we don't do cache cleanup and we don't sync, so we don't need to care about that immediately.)

Comment 45

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8731448 [details]
MozReview Request: Bug 1234315 - Add reader view to UrlAnnotations r=sebastian

https://reviewboard.mozilla.org/r/40605/#review37585

Comment 46

[Richard Newman [:rnewman]]

(In reply to Andrzej Hunt :ahunt from comment #44)

> The issue is we have 3+ different DB
> queries for bookmarks, topsites, and search results, with 3 different
> adapters, so passing in the readermode state via SQL is difficult (we
> probably should simplify that into one implementation), but we use the same
> view to display a bookmark in all 3 adapters, hence with the ReaderCache we
> just need to check for the offline state in that view (i.e. in
> TwoLinePageRow).

I think this might be a difference of perspective. My view has/had been to not include "is this page in the reader cache?" in SQL at all -- that quick in-memory lookup can be done at point of display. That doesn't help if you absolutely need a list of cached items to be able to run SQL queries (e.g., frecency input), of course.

Perhaps your point about the three states helps clarify this: the URL annotation is "readermode", the offline part is present-in-cache, and right now those are equivalent, right?

I'd like to see these commits explain/comment/etc. what's going on and why, and document what the assumptions are.

Comment 47

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/38525/#review37211

> Not being `final` is a sign that you're going to hit concurrency or lifecycle issues at some point. Comment this here, noting that you should only access this -- even internally -- through the accessor.

Is adding a wrapper/accessor object to guarantee safety here overkill?

Something like:

    // We can't initialise the ReaderCacheHelper while constructing GeckoProfile since we might not have a profile
    // dir at that point. (ReaderCacheHelper construction calls back into GeckoProfile in order
    // to get the profileDir, which we shouldn't do during GeckoProfile construction.)
    // Hence the lazy construction.
    private class ReaderCacheHelperAccessor {
        private ReaderCacheHelper mReaderCacheHelper = null;

        public synchronized ReaderCacheHelper getReaderCacheHelper() {
            if (mReaderCacheHelper == null) {
                mReaderCacheHelper = new ReaderCacheHelper(mApplicationContext);
            }

            return mReaderCacheHelper;
        }
    }


    private final ReaderCacheHelperAccessor mReaderCacheHelperAccessor = new ReaderCacheHelperAccessor();
    
    public ReaderCacheHelper getReaderCacheHelper() {
        return mReaderCacheHelperAccessor.getReaderCacheHelper();
    }

Comment 48

[Andrzej Hunt :ahunt]

Comment on attachment 8731448 [details]
MozReview Request: Bug 1234315 - Add reader view to UrlAnnotations r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40605/diff/1-2/

Comment 49

[Andrzej Hunt :ahunt]

Comment on attachment 8727596 [details]
MozReview Request: Bug 1234315 - Introduce SavedReaderViewHelper to track cached reader view items r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38525/diff/2-3/

Comment 50

[Andrzej Hunt :ahunt]

Comment on attachment 8718988 [details]
MozReview Request: Bug 1234315 - Migrate reading-list table to bookmarks r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34833/diff/4-5/

Comment 51

[Andrzej Hunt :ahunt]

Comment on attachment 8718986 [details]
MozReview Request: Bug 1234315 - Introduce test for url to cache-filename r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34829/diff/4-5/

Comment 52

[Andrzej Hunt :ahunt]

Comment on attachment 8718986 [details]
MozReview Request: Bug 1234315 - Introduce test for url to cache-filename r=nalexander

Cancelling an inexistant r+ - reviewboard seems to have gotten confused by patches being reordered / rearranged / landed separately?

Comment 53

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8718986 [details]
MozReview Request: Bug 1234315 - Introduce test for url to cache-filename r=nalexander

https://reviewboard.mozilla.org/r/34829/#review38601

There's nothing wrong with this, but it's complex.  I think I would have prefered a little JUnit 4 unit test that just asserted Java(foo) == value, and then a browser-chrome JS unit test that just assert JS(foo) == value, and hard-code the values on all sides.  Next time!

Comment 54

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8727596 [details]
MozReview Request: Bug 1234315 - Introduce SavedReaderViewHelper to track cached reader view items r?nalexander

https://reviewboard.mozilla.org/r/38525/#review38607

I have some questions.  I will come back to this ticket to try to understand more, since I feel like there's a lot of tricky machinations that don't add up to me quite yet.

::: mobile/android/base/java/org/mozilla/gecko/GeckoProfile.java:91
(Diff revision 3)
>      private final File mMozillaDir;
>      private final Context mApplicationContext;
>  
>      private final BrowserDB mDB;
>  
> +    // We can't initialise the ReaderCacheHelper while constructing GeckoProfile since we might not

This is tortured.  If you want lazy initialization, why not do it in `getReaderCache()` below?

::: mobile/android/base/java/org/mozilla/gecko/reader/ReaderCacheHelper.java:33
(Diff revision 3)
> + * to cached items. This is _not_ a true cache, we never purge/cleanup items here - we only remove
> + * items when we un-readermode/bookmark them. This is an acceptable model while we can guarantee the
> + * 1:1 correspondence.
> + *
> + * It isn't strictly necessary to mirror cached items in SQL at this stage, however it seems sensible
> + * to to maintain URL anotations to avoid additional DB migrations in future.

nit: to to.

::: mobile/android/base/java/org/mozilla/gecko/reader/ReaderCacheHelper.java:49
(Diff revision 3)
> + * task to perform at least the cache cleanup / monitoring.
> + *
> + * We have one instance per GeckoProfile, which manages its own locking. You should always retrieve
> + * ReaderCacheHelper using GeckoProfile.getReaderCacheHelper().
> + */
> +public class ReaderCacheHelper {

Why `ReaderCacheHelper`?  Why not just `ReaderCache`?

::: mobile/android/base/java/org/mozilla/gecko/reader/ReaderCacheHelper.java:59
(Diff revision 3)
> +
> +    private static final String DIRECTORY = "readercache";
> +    private static final String FILE_NAME = "items.json";
> +    private static final String FILE_PATH = DIRECTORY + "/" + FILE_NAME;
> +
> +    private final JSONObject mItems;

This is expensive!  Why do we feel that we must keep the cache in memory, even before it's used?

::: mobile/android/base/java/org/mozilla/gecko/reader/ReaderCacheHelper.java:119
(Diff revision 3)
> +        mItems.remove(URL);
> +
> +        ThreadUtils.postToBackgroundThread(new Runnable() {
> +            @Override
> +            public void run() {
> +//                URLAnnotations annotations = GeckoProfile.get(mContext).getDB().getUrlAnnotations();

What's going on here?  The comment ("remove") and the commented code don't add up.

::: mobile/android/base/java/org/mozilla/gecko/reader/ReaderCacheHelper.java:154
(Diff revision 3)
> +    /**
> +     * Return the ReaderMode URL for a given URL if it is contained in the cache. Returns the
> +     * plain URL if the page is not cached.
> +     */
> +    public static String getReaderURLIfCached(final Context context, @NonNull final String pageURL) {
> +        ReaderCacheHelper rch = GeckoProfile.get(context).getReaderCacheHelper();

Why vector through the profile in this way at all?

We can only have one GeckoProfile per process, ever.  We can just have a singleton that knows the one Gecko profile it cares about.

Comment 55

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/38525/#review38607

> This is tortured.  If you want lazy initialization, why not do it in `getReaderCache()` below?

That's what I did originally, but we could _potentially_ run into concurrency issues if another method decides to access mReaderCacheHelper directly instead of going through getReaderCache(). I wanted to guarantee that we can't access the ReaderCache incorrectly within GeckoProfile, which seems difficult without this ReaderCacheHelperAccessor. (I implemented this in response to https://reviewboard.mozilla.org/r/38525/#comment48005 - maybe I'm being too paranoid, but I wasn't a huge fan of using a comment to try to ensure correct usage?)

> Why `ReaderCacheHelper`?  Why not just `ReaderCache`?

No strong reason. Currently cache management is shared across JS and Java (JS determines file-names, and also creates the actual cache data), hence naming this ReaderCache might incorrectly suggest that this class alone handles the caching, but we're wanting to move in the direction of having all management being done in Java at some point.

> This is expensive!  Why do we feel that we must keep the cache in memory, even before it's used?

We're going to access the cache as soon as we show any of topsites, bookmarks, search results. I.e. in a lot of cases we will want to have the data immediately. However I've just realised that restoring to a previously opened page is much more likely than restoring to about:home, in which case lazy loading makes more sense (since we're slowing down restoring here). I'll try to think of a better solution.

(This will affect decorating cached bookmarks, since we currently assume we can query ReaderCacheHelper instantaneously.)

> What's going on here?  The comment ("remove") and the commented code don't add up.

Not sure if I'm addressing the right concerns here: URLAnnotations is just a helper object which lets us access annotations in DB. I.e. getAnnotations just retrieves an accessor which lets us manipulate annotations that are in tehe DB. (URLAnnotations then offers get/insert methods, and sebastian is adding the remove method in his patches.)

Comment 56

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/38525/#review38607

> We're going to access the cache as soon as we show any of topsites, bookmarks, search results. I.e. in a lot of cases we will want to have the data immediately. However I've just realised that restoring to a previously opened page is much more likely than restoring to about:home, in which case lazy loading makes more sense (since we're slowing down restoring here). I'll try to think of a better solution.
> 
> (This will affect decorating cached bookmarks, since we currently assume we can query ReaderCacheHelper instantaneously.)

I've done some measurements - my N4 usually take 4-6ms to load the cache, for 0-20 items in the cache (compared to an average of 50ms for the topsites cursor query, with a fairly small database which doesn't have much more than 40 history items, although the bookmarks/search queries might be faster). So, loading this early wouldn't really be noticeable compared to the other work we're already doing when displaying the cache data. After that queries should be instantaneous, which is the main reason we wanted the cache (and since we query the cache for most awesomescreen items, we'll want to keep it in memory forever).


As far as I can tell we care about the loading time during app startup, where we'll have one of the two scenarios:
(A) Open/restore to homescreen/homepanels - in this case we need to load the cache immediately, so we don't really care where exactly we load the cache.
(B) Restore to previously opened sites. In this case we don't need the cache immediately, and could wait until the first time we show homepanels.


Currently we load the cache on the UI thread (i.e. the first time we check whether a URL is cached, we load the file), which is probably a bad idea. I'm not sure how to solve this while effectively dealing with scenario (B).

The options I can see are:
1. Load the file during BrowserDatabaseHelper.onLoad (this is called the first time we access the DB, and all DB access is on the background thread already, hence we won't block the UI thread - since loading the cache is fast the impact on DB load time should be negligible).
For (A) this means we at least don't block the UI thread, for (B) we'd load the cache after startup, but in the background (we check whether a page is a bookmark after loading, i.e. we load the DB at that point - however AFAICS that doesn't block restoring opened pages).
2. Load at some other point during startup (I don't know enough about what other options there are...).


We could use the background thread every time we want to show a cache decoration, but that seems like overkill given we'll have the cache in memory for all but the first load. Moreover I'm guessing that's a more noticeable performance hit if we're having to pass through UI->background->UI for every single topsite/bookmark/search result?

Comment 57

[Richard Newman [:rnewman]]

Comment on attachment 8718988 [details]
MozReview Request: Bug 1234315 - Migrate reading-list table to bookmarks r=rnewman

https://reviewboard.mozilla.org/r/34833/#review39281

I'm really not happy about the naming here.

It seems we have three entities:

* Persistent disk storage for Reader View versions of pages.
* Persistent in-database annotations of URLs for which we've saved RV versions. _These are not necessarily bookmarks_.
* In-memory cache of those annotations.

I'd like to see a little more time spent being precise about what's a cache and what isn't. Folding this in with the previous part might be necessary to make this truly clear.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1154
(Diff revision 5)
> +    // This is a reimplementation of _toHashedPath from ReaderMode.jsm - given that we're likely
> +    // to migrate the ReaderCache implementation at some point, it seems safest to have a local
> +    // implementation here - moreover this is probably faster than calling into JS.
> +    private String getReaderCacheFileNameForURL(String url) {
> +        try {
> +            byte[] utf8 = url.getBytes("UTF8");

`url.getBytes(java.nio.charset.StandardCharsets.UTF_8)` won't throw `UnsupportedEncodingException`.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1179
(Diff revision 5)
> +    private void upgradeDatabaseFrom30to31(SQLiteDatabase db) {
> +        // We only need to do the migration if reading-list items already exist. We could do a query of count(*) on
> +        // TABLE_READING_LIST, however if we are doing the migration, we'll need to query all items in the reading-list,
> +        // hence we might as well just query all items, and proceed with the migration if cursor.count > 0.
> +
> +        // We try and retain the original ordering below. Our LocalReadingListAccessor actually coalesced

We try to…

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1211
(Diff revision 5)
> +
> +            final long now = System.currentTimeMillis();
> +
> +            // Reuse the ContentValues outside the loop to prevent allocating unnecessary additional new ContentValues.
> +
> +            // We try to retain the same order as the reading-list would show. We should hopefully be reading the

s/reading-list/reading list

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1254
(Diff revision 5)
> +
> +            final File profileDir = GeckoProfile.get(mContext).getDir();
> +            final File cacheDir = new File(profileDir, "readercache");
> +
> +
> +            // At the time of this migration the ReaderCache is a 1:1 mirror of readermode

Throughout: readermode -> reader view.

This block comment is absolutely crucial, particularly because the naming of these entities is _so wrong_.

A 'cache' has a particular set of properties:

* It's not the canonical store of data.
* It's typically not complete.
* It can be discarded, partially or entirely.


Our offline reader view storage isn't a cache, unless you take the perspective that it's a cache of the web -- the stuff stored in the 'cache' might not be recoverable at all (e.g., saved order confirmations).

Your `ReaderCacheHelper` here is a little bit of both: it's a cache _of the contents of the database_, but it also fronts the disk storage, which (if you squint) acts like a cache of potential RV items.

RCH's block comment reads:

```
Helper to keep track of items that are stored in the readermode cache.
```

which I don't think is quite correct now, right?

I would suggest doing some renaming here to clarify that this isn't a cache of reader view items (that storage is persistent and managed), but we _do_ have an in-memory cache -- it's the intermediary between the metadata here in the database and the RV HTML on disk in storage.

Naming these things clearly and correctly is important.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1284
(Diff revision 5)
> +
> +                        readerCacheHelper.put(url, path, size);
> +                    } else {
> +                        // This should never happen, but we don't actually know whether or not orphaned
> +                        // items happened in the wild.
> +                        cacheFile.delete();

I'd wrap this in a `try..catch` to make sure it doesn't interrupt migration and leave the user in a bad state.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1292
(Diff revision 5)
> +                }
> +
> +                Telemetry.addToHistogram("FENNEC_READING_LIST_MIGRATION_ORPHANED_ITEMS", orphanedItems);
> +            }
> +        } finally {
> +            readingListCursor.close();

Close this before you populate the cache.

Comment 58

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: [temporary] Bug 1234315 - Load SavedReaderViewHelper during DB onLoad instead of on UI thread r?nalexander

We'll want to squash this into the previous commit, assuming this is the approach we want.

Review commit: https://reviewboard.mozilla.org/r/43247/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43247/

Comment 59

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: [temporary] (Bug 1234315) Create Bookmarks+Annotations view as part of rl-migration r?rnewman

This commit will be squashed into the reading-list migration commit, but is separate
for now to simplify review.

We need a bookmarks+annotations view to generate the reading-list smart folder. This
doesn't have to be part of the initial rl-migration, however it makes sense to generate
this as part of that migration to avoid an additional migration just to add the new view.
The reading-list smarfolder will be created in Bug 1257345.

Review commit: https://reviewboard.mozilla.org/r/43249/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43249/

Comment 60

[Andrzej Hunt :ahunt]

Comment on attachment 8731448 [details]
MozReview Request: Bug 1234315 - Add reader view to UrlAnnotations r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40605/diff/2-3/

Comment 61

[Andrzej Hunt :ahunt]

Comment on attachment 8727596 [details]
MozReview Request: Bug 1234315 - Introduce SavedReaderViewHelper to track cached reader view items r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38525/diff/3-4/

Comment 62

[Andrzej Hunt :ahunt]

Comment on attachment 8718988 [details]
MozReview Request: Bug 1234315 - Migrate reading-list table to bookmarks r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34833/diff/5-6/

Comment 63

[Andrzej Hunt :ahunt]

Comment on attachment 8718986 [details]
MozReview Request: Bug 1234315 - Introduce test for url to cache-filename r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34829/diff/5-6/

Comment 64

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/34833/#review39281

> I'd wrap this in a `try..catch` to make sure it doesn't interrupt migration and leave the user in a bad state.

It turns out File.delete() doesn't throw, but it returns a boolean flag (which I've used for a warning).

Comment 65

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/43247/#review39871

::: mobile/android/base/java/org/mozilla/gecko/reader/SavedReaderViewHelper.java:135
(Diff revision 1)
>  
>          ThreadUtils.postToBackgroundThread(new Runnable() {
>              @Override
>              public void run() {
>                  UrlAnnotations annotations = GeckoProfile.get(mContext).getDB().getUrlAnnotations();
> -                annotations.insertReaderviewUrl(mContext.getContentResolver(), pageURL);
> +                annotations.insertReadermodeUrl(mContext.getContentResolver(), pageURL);

Ooops, histedit screwup by me here... I'll mark this as an issue to avoid the spam of repushing all 6 parts again.

::: mobile/android/base/java/org/mozilla/gecko/reader/SavedReaderViewHelper.java:151
(Diff revision 1)
>  
>          ThreadUtils.postToBackgroundThread(new Runnable() {
>              @Override
>              public void run() {
>                  UrlAnnotations annotations = GeckoProfile.get(mContext).getDB().getUrlAnnotations();
> -                annotations.deleteReaderviewUrl(mContext.getContentResolver(), pageURL);
> +                annotations.deleteReadermodeUrl(mContext.getContentResolver(), pageURL);

Same here.

Comment 66

[Richard Newman [:rnewman]]

Comment on attachment 8718988 [details]
MozReview Request: Bug 1234315 - Migrate reading-list table to bookmarks r=rnewman

https://reviewboard.mozilla.org/r/34833/#review40695

Can you write tests for this migration?

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:480
(Diff revision 6)
>  
>          db.execSQL("CREATE INDEX idx_search_history_last_visited ON " +
> -                SearchHistory.TABLE_NAME + "(" + SearchHistory.DATE_LAST_VISITED + ")");
> +                   SearchHistory.TABLE_NAME + "(" + SearchHistory.DATE_LAST_VISITED + ")");
>      }
>  
>      private void createReadingListTable(final SQLiteDatabase db, final String tableName) {

You should kill these two, too.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1152
(Diff revision 6)
>  
> +    // Get the cache path for a URL, based on the storage format in place during the 27to28 transition.
> +    // This is a reimplementation of _toHashedPath from ReaderMode.jsm - given that we're likely
> +    // to migrate the SavedReaderViewHelper implementation at some point, it seems safest to have a local
> +    // implementation here - moreover this is probably faster than calling into JS.
> +    private String getReaderCacheFileNameForURL(String url) {

`static`

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1202
(Diff revision 6)
> +        // We'll want to walk the cache directory, so that we can (A) bookkeep readercache items
> +        // that we want and (B) delete unneeded readercache items. (B) shouldn't actually happen, but
> +        // is possible if there were bugs in our reader-caching code.
> +        // We need to construct this here since we populate this map while walking the DB cursor,
> +        // and use the map later when walking the cache.
> +        final Map<String, String> fileToUrlMap = new HashMap<>();

s/Url/URL

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1219
(Diff revision 6)
> +                throw new IllegalStateException("mobile bookmarks folder must already exist");
> +            }
> +
> +            final long now = System.currentTimeMillis();
> +
> +            // Reuse the ContentValues outside the loop to prevent allocating unnecessary additional new ContentValues.

Kill this line: you don't do that!

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1240
(Diff revision 6)
> +                final String url = readingListCursor.getString(readingListCursor.getColumnIndexOrThrow(ReadingListItems.URL));
> +
> +                readingListItemValues.put(Bookmarks.PARENT, mobileBookmarksID);
> +                readingListItemValues.put(Bookmarks.GUID, Utils.generateGuid());
> +                readingListItemValues.put(Bookmarks.URL, url);
> +                readingListItemValues.put(Bookmarks.TITLE, readingListCursor.getString(titleColumnID));

Hilariously, Android doesn't specify what happens when the column is null-valued and you call `getString`.

Furthermore, you can't put `null` into `ContentValues` like this; you would have to do `putNull`.

You should check `isNull` here and do the right thing, or have the query return a coalesced value like

```
COALESCE(title, '')
```

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1291
(Diff revision 6)
> +                    // This should never happen, but we don't actually know whether or not orphaned
> +                    // items happened in the wild.
> +                    boolean deleted = cacheFile.delete();
> +
> +                    if (!deleted) {
> +                        Log.w(LOGTAG, "Failed to delete orphaned saved reader view file: " + cacheFile.getPath());

Don't log the path.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1297
(Diff revision 6)
> +                    }
> +                }
> +            }
> +        }
> +
> +        db.execSQL("DROP TABLE " + TABLE_READING_LIST);

Paranoia says `DROP TABLE IF EXISTS` is a prudent choice.

Comment 67

[Richard Newman [:rnewman]]

Comment on attachment 8736396 [details]
MozReview Request: [temporary] (Bug 1234315) Create Bookmarks+Annotations view as part of rl-migration r?rnewman

As mentioned on IRC, this is wrong: we shouldn't be left-outer-joining if we're only going to filter on annotation.

The downside of views is that it's easy to lose sight of all the stuff that's happening down below.

Comment 68

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/34833/#review40695

> You should kill these two, too.

Filed as Bug 1262178. (I'm not hugely keen on modifying existing migrations given the potential for breakage, and we'd probably want to have extensive testing before introducing any changes here?)

Comment 69

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234315 - Add test for reading-list to bookmarks (30->31) migration

Review commit: https://reviewboard.mozilla.org/r/44373/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44373/

Comment 70

[Andrzej Hunt :ahunt]

Comment on attachment 8736395 [details]
MozReview Request: [temporary] Bug 1234315 - Load SavedReaderViewHelper during DB onLoad instead of on UI thread r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43247/diff/1-2/

Comment 71

[Andrzej Hunt :ahunt]

Comment on attachment 8718988 [details]
MozReview Request: Bug 1234315 - Migrate reading-list table to bookmarks r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34833/diff/6-7/

Comment 72

[Andrzej Hunt :ahunt]

Comment on attachment 8736396 [details]
MozReview Request: [temporary] (Bug 1234315) Create Bookmarks+Annotations view as part of rl-migration r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43249/diff/1-2/

Comment 73

[Andrzej Hunt :ahunt]

Comment on attachment 8718986 [details]
MozReview Request: Bug 1234315 - Introduce test for url to cache-filename r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34829/diff/6-7/

Comment 74

[Richard Newman [:rnewman]]

Comment on attachment 8736396 [details]
MozReview Request: [temporary] (Bug 1234315) Create Bookmarks+Annotations view as part of rl-migration r?rnewman

https://reviewboard.mozilla.org/r/43249/#review41091

This is fine, but I don't feel like this really buys you anything when implementing UX for annotated bookmarks -- you'll still need to handle the case of URLs being present twice (multiple bookmarks), multiplied by URLs being annotated with the same annotation key more than once, and so on.

Comment 75

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8736395 [details]
MozReview Request: [temporary] Bug 1234315 - Load SavedReaderViewHelper during DB onLoad instead of on UI thread r?nalexander

https://reviewboard.mozilla.org/r/43247/#review41101

I'm fine with this approach.

::: mobile/android/base/java/org/mozilla/gecko/reader/SavedReaderViewHelper.java:85
(Diff revision 2)
> +        }
> +
> +        return instance;
> +    }
> +
> +    public synchronized void loadItems() {

nit: Note thread restrictions in comment, please.

::: mobile/android/base/java/org/mozilla/gecko/reader/SavedReaderViewHelper.java:100
(Diff revision 2)
> -            Log.i(LOG_TAG, "Couldn't read readercache file, initialising empty readercache instead", e);
> -            items = new JSONObject();
> +            Log.i(LOG_TAG, "Couldn't read saved reader view list from file, initialising empty list instead", e);
> +            mItems = new JSONObject();
> +        }
> +    }
> +
> +    private void assertItemsLoaded() {

You are consuming this in `synchronized` member methods, but it's worth making this `synchronized` too.

Comment 76

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8727596 [details]
MozReview Request: Bug 1234315 - Introduce SavedReaderViewHelper to track cached reader view items r?nalexander

https://reviewboard.mozilla.org/r/38525/#review41095

nits and one real question: why aren't you returning anything from the item cached in the cache?

I can't find consuming code for this (in this series) so it's hard to figure out what's really happening here.  However, I'm happy to see these helpers land and get re-worked as they get used.  Forward!

::: mobile/android/base/java/org/mozilla/gecko/db/LocalUrlAnnotations.java:211
(Diff revision 4)
>      public void insertScreenshot(final ContentResolver cr, final String pageUrl, final String screenshotPath) {
>          insertAnnotation(cr, pageUrl, Key.SCREENSHOT.getDbValue(), screenshotPath);
>      }
> +
> +    @Override
> +    public void insertReaderviewUrl(final ContentResolver cr, final String pageUrl) {

nit: if it's `READER_VIEW`, these should follow suit and be `ReaderView`.

::: mobile/android/base/java/org/mozilla/gecko/reader/SavedReaderViewHelper.java:21
(Diff revision 4)
> +/**
> + * Helper to keep track of items that are stored in the reader view cache. This is an in-memory list
> + * of the reader view items that are cached on disk. It is intended to allow quickly determining whether
> + * a given URL is in the cache, and also how many cached items there are.
> + *
> + * Currently we have 1:1 correspondence of reader view items (in the URL-annotations table)
> + * to cached items. This is _not_ a true cache, we never purge/cleanup items here - we only remove
> + * items when we un-reader-view/bookmark them. This is an acceptable model while we can guarantee the
> + * 1:1 correspondence.
> + *
> + * It isn't strictly necessary to mirror cached items in SQL at this stage, however it seems sensible
> + * to maintain URL anotations to avoid additional DB migrations in future.
> + * It is also simpler to implement the reading list smart-folder using the annotations (even if we do
> + * all other decoration from our in-memory cache record), as that is what we will need when
> + * we move away from the 1:1 correspondence.
> + *
> + * Bookmarks can be in one of two states - plain bookmark, or reader view bookmark that is also saved
> + * offline. We're hoping to introduce real cache management / cleanup in future, in which case a
> + * third user-visible state (reader view bookmark without a cache entry) will be added. However that logic is
> + * much more complicated and requires substantial changes in how we decorate reader view bookmarks.
> + * With the current 1:1 correspondence we can use this in-memory helper  to quickly decorate
> + * bookmarks (in all the various lists and panels that are used), whereas supporting
> + * the third state requires significant changes in order to allow joining with the
> + * url-annotations table wherever bookmarks might be retrieved (i.e. multiple homepanels, each with
> + * their own loaders and adapter).
> + *
> + * If/when cache cleanup and sync are implemented, url annotations will be the canonical record of
> + * user intent, and the cache will no longer represent all reader view bookmarks. We will have (A)
> + * cached items that are not a bookmark, or bookmarks without the reader view annotation (both of
> + * these would need purging), and (B) bookmarks with a reader view annotation, but not stored in
> + * the cache (which we might want to download in the background). Supporting (B) is currently difficult,
> + * see previous paragraph.
> + */

Nice overview!

::: mobile/android/base/java/org/mozilla/gecko/reader/SavedReaderViewHelper.java:41
(Diff revision 4)
> + *
> + * Bookmarks can be in one of two states - plain bookmark, or reader view bookmark that is also saved
> + * offline. We're hoping to introduce real cache management / cleanup in future, in which case a
> + * third user-visible state (reader view bookmark without a cache entry) will be added. However that logic is
> + * much more complicated and requires substantial changes in how we decorate reader view bookmarks.
> + * With the current 1:1 correspondence we can use this in-memory helper  to quickly decorate

nit: extra spaces.

::: mobile/android/base/java/org/mozilla/gecko/reader/SavedReaderViewHelper.java:47
(Diff revision 4)
> + * bookmarks (in all the various lists and panels that are used), whereas supporting
> + * the third state requires significant changes in order to allow joining with the
> + * url-annotations table wherever bookmarks might be retrieved (i.e. multiple homepanels, each with
> + * their own loaders and adapter).
> + *
> + * If/when cache cleanup and sync are implemented, url annotations will be the canonical record of

nit: URL annotations (consistent capitalization).

::: mobile/android/base/java/org/mozilla/gecko/reader/SavedReaderViewHelper.java:75
(Diff revision 4)
> +
> +        JSONObject items;
> +        try {
> +            items = GeckoProfile.get(mContext).readJSONObjectFromFile(FILE_PATH);
> +        } catch (IOException e) {
> +            Log.i(LOG_TAG, "Couldn't read readercache file, initialising empty readercache instead", e);

I'm leaning towards not logging the exception.  It's not actionable, since you immediately recover.

::: mobile/android/base/java/org/mozilla/gecko/reader/SavedReaderViewHelper.java:94
(Diff revision 4)
> +
> +    public synchronized boolean isURLCached(@NonNull final String URL) {
> +        return mItems.has(URL);
> +    }
> +
> +    public synchronized void put(@NonNull final String pageURL, @NonNull final String path, final long size) {

Looks like there are no thread restrictions here.  Consider saying as much.

::: mobile/android/base/java/org/mozilla/gecko/reader/SavedReaderViewHelper.java:140
(Diff revision 4)
> +        GeckoProfile profile = GeckoProfile.get(mContext);
> +        File cacheDir = new File(profile.getDir(), DIRECTORY);
> +
> +        if (!cacheDir.exists()) {
> +            Log.i(LOG_TAG, "No preexisting cache directory, creating now");
> +            cacheDir.mkdir();

This can fail, I imagine.  Perhaps catch and throw `IllegalStateException`.

::: mobile/android/base/java/org/mozilla/gecko/reader/SavedReaderViewHelper.java:143
(Diff revision 4)
> +        if (!cacheDir.exists()) {
> +            Log.i(LOG_TAG, "No preexisting cache directory, creating now");
> +            cacheDir.mkdir();
> +        }
> +
> +        profile.writeFile(FILE_PATH, mItems.toString());

Man, this API is not flexible.  But we can live with it, I guess.  A failed write should be silently ignored, I guess; and we'll purge the cache on the equally failed read.

::: mobile/android/base/java/org/mozilla/gecko/reader/SavedReaderViewHelper.java:146
(Diff revision 4)
> +        }
> +
> +        profile.writeFile(FILE_PATH, mItems.toString());
> +    }
> +
> +    private static SavedReaderViewHelper ourSavedReaderViewHelper = null;

`our` is very unusual.  If you must prefix, `s` is well-known to mean `static`.

If you are hoping to convey something about how this is shared with the choice of our, I don't get it and recommend saying more.

::: mobile/android/base/java/org/mozilla/gecko/reader/SavedReaderViewHelper.java:162
(Diff revision 4)
> +    /**
> +     * Return the Reader View URL for a given URL if it is contained in the cache. Returns the
> +     * plain URL if the page is not cached.
> +     */
> +    public static String getReaderURLIfCached(final Context context, @NonNull final String pageURL) {
> +        SavedReaderViewHelper rch = getSavedReaderViewHelper(context);

nit: `rch` is no longer a reasonable name.

::: mobile/android/base/java/org/mozilla/gecko/reader/SavedReaderViewHelper.java:165
(Diff revision 4)
> +     */
> +    public static String getReaderURLIfCached(final Context context, @NonNull final String pageURL) {
> +        SavedReaderViewHelper rch = getSavedReaderViewHelper(context);
> +
> +        if (rch.isURLCached(pageURL)) {
> +            return ReaderModeUtils.getAboutReaderForUrl(pageURL);

Why isn't this returning the `path` from the cache?

Comment 77

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/38525/#review41095

Currently we only need to know cache-state, hence we only use isURLCached() (and getReaderURLIfCached is a utility for this: in most cases we want to open the readercached version of a page if available). We don't actually need to track the file-path or size right now, but we'll need the paths for when we move cache-management into Java, and we wanted file sizes for telemetry.

Comment 78

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8738196 [details]
MozReview Request: Bug 1234315 - Add test for reading-list to bookmarks (30->31) migration

https://reviewboard.mozilla.org/r/44373/#review41627

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testReadingListToBookmarksMigration.java:33
(Diff revision 1)
> +import static org.mozilla.gecko.db.BrowserContract.*;
> +import static org.mozilla.gecko.tests.helpers.AssertionHelper.fAssertEquals;
> +import static org.mozilla.gecko.tests.helpers.AssertionHelper.fAssertNotNull;
> +import static org.mozilla.gecko.tests.helpers.AssertionHelper.fAssertTrue;
> +
> +// TODO: Move to junit 3 tests, once those run in automation. There is no ui testing to do so it's a better fit.

nit: file a bug, include a bug #

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testReadingListToBookmarksMigration.java:46
(Diff revision 1)
> +        add("DWUP3U4ERC6TKJVSYXKJLHHEFY.json");
> +        add("KWNV7PXD3JFOJBQJVFXI3CQKNE.json");

Could we name these more reasonably? Or load them dynamically (make sure to assert length > 0)?

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testReadingListToBookmarksMigration.java:80
(Diff revision 1)
> +
> +        final Cursor c = db.rawQuery("SELECT * FROM " + ReadingListItems.TABLE_NAME, null);
> +
> +        fAssertNotNull("Cursor cannot be null", c);
> +        try {
> +            if (c.moveToFirst()) {

fAssertTrue(c.moveToFirst)?

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testReadingListToBookmarksMigration.java:102
(Diff revision 1)
> +        final Cursor c = db.rawQuery("SELECT * FROM " +
> +                                     Bookmarks.TABLE_NAME + " JOIN " + UrlAnnotations.TABLE_NAME +
> +                                     " ON " + DBUtils.qualifyColumn(Bookmarks.TABLE_NAME, Bookmarks.URL) + " = " +
> +                                     DBUtils.qualifyColumn(UrlAnnotations.TABLE_NAME, UrlAnnotations.URL) +
> +                                     " WHERE " + DBUtils.qualifyColumn(UrlAnnotations.TABLE_NAME, UrlAnnotations.KEY) + " = ?",
> +                new String[]{

nit: `new String[] {`

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testReadingListToBookmarksMigration.java:108
(Diff revision 1)
> +                        UrlAnnotations.Key.READER_VIEW.getDbValue()
> +                });
> +
> +        fAssertNotNull("Cursor cannot be null", c);
> +        try {
> +            if (c.moveToFirst()) {

nit: `fAssertTrue(c.moveToFirst`)

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testReadingListToBookmarksMigration.java:125
(Diff revision 1)
> +    }
> +
> +    /**
> +     * @throws IOException if the database cannot be copied.
> +     */
> +    public void test() throws IOException {

nit: for us, these usually mirror the class name (maybe it provides better output when the test fails?)

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testReadingListToBookmarksMigration.java:175
(Diff revision 1)
> +     * Copies assets into the desired locations. We need to copy our DB into a temporary file,
> +     * and readercache items into the profile directory.
> +     *
> +     * @throws IOException if reading the existing files or writing the temporary files fails
> +     */
> +    private String copyAssets() throws IOException {

We should probably separate this functionality into a separate class at some point.

Comment 79

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/27b876dbafbb4a53558016b62473084be2e105da
Bug 1234315 - Add reader view to UrlAnnotations r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/1ec317239f7c01fa916bc2c799e92be99e51c413
Bug 1234315 - Introduce SavedReaderViewHelper to track cached reader view items r=nalexander

https://hg.mozilla.org/integration/fx-team/rev/f47114deff561ac36d06d3df5558765a8aca0681
Bug 1234315 - Migrate reading-list table to bookmarks r=rnewman

https://hg.mozilla.org/integration/fx-team/rev/9a16db43e8cf1e44bea9da8411aed6a8f4fc9f7e
Bug 1234315 - Add test for reading-list to bookmarks (30->31) migration r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/2c81d5f8a352e3877b6a7f5c41801bcf36c6466e
Bug 1234315 - Introduce test for url to cache-filename r=nalexander

Comment 80

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/27b876dbafbb
https://hg.mozilla.org/mozilla-central/rev/1ec317239f7c
https://hg.mozilla.org/mozilla-central/rev/f47114deff56
https://hg.mozilla.org/mozilla-central/rev/9a16db43e8cf
https://hg.mozilla.org/mozilla-central/rev/2c81d5f8a352