1234331 - Enable bookmark star for reader view pages

Status: RESOLVED FIXED

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

Description

[:Margaret Leibovic]

We should allow users to bookmark reader view pages.

Comment 1

[Andrzej Hunt :ahunt]

A few related things that probably need doing for this to work as expected:
- strip about:reader when checking whether a page is bookmarked (in Tab.java)
- strip about:reader when bookmarking (BrowserApp?)
- save to readercache when bookmarking about:reader pages
- maybe save to readercache when opening readermode for a page that is bookmarked?

Comment 2

[Andrzej Hunt :ahunt]

And maybe we should also clear the item from the readercache on bookmark deletion (only if that's the only bookmark with that URL).

Comment 3

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234331 - Allow bookmarking readerview pages r=margaret

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

Comment 4

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234331 - Push article into readercache when bookmarking readerview page r?margaret

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

Comment 5

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234331 - Remove from readercache when unbookmarking page r=margaret

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

Comment 6

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234331 - Push bookmarked item into readercache when entering readermode r?margaret

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

Comment 7

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234331 - Remove readercache item when exiting readermode on bookmarked page r?margaret

A readermoded bookmark that is currently opened will be in the readercache,
if the user exits readermode then we can presume that the user no longer
wants to store the readermode version locally.

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

Comment 8

[Andrzej Hunt :ahunt]

These patches depend directly on those in Bug 1234315, hence adding that dependency.

Comment 9

[Andrzej Hunt :ahunt]

Comment on attachment 8727617 [details]
MozReview Request: Bug 1234331 - Allow bookmarking readerview pages r=margaret

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

Comment 10

[Andrzej Hunt :ahunt]

Comment on attachment 8727618 [details]
MozReview Request: Bug 1234331 - Push article into readercache when bookmarking readerview page r?margaret

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

Comment 11

[Andrzej Hunt :ahunt]

Comment on attachment 8727619 [details]
MozReview Request: Bug 1234331 - Remove from readercache when unbookmarking page r=margaret

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

Comment 12

[Andrzej Hunt :ahunt]

Comment on attachment 8727620 [details]
MozReview Request: Bug 1234331 - Push bookmarked item into readercache when entering readermode r?margaret

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

Comment 13

[Andrzej Hunt :ahunt]

Comment on attachment 8727621 [details]
MozReview Request: Bug 1234331 - Remove readercache item when exiting readermode on bookmarked page r?margaret

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

Comment 14

[:Margaret Leibovic]

Comment on attachment 8727617 [details]
MozReview Request: Bug 1234331 - Allow bookmarking readerview pages r=margaret

https://reviewboard.mozilla.org/r/38549/#review37499

::: mobile/android/base/java/org/mozilla/gecko/Tab.java:566
(Diff revision 2)
>          ThreadUtils.postToBackgroundThread(new Runnable() {
>              @Override
>              public void run() {
> -                String url = getURL();
> +                if (pageUrl == null)
> -                if (url == null)
>                      return;

We could remove this early return outside the runnable, and not even post the runnable in this case.

Not your code, but do we know when this would even happen? Seems like an unexpected case, and there's no feedback for the user.

Comment 15

[:Margaret Leibovic]

Comment on attachment 8727618 [details]
MozReview Request: Bug 1234331 - Push article into readercache when bookmarking readerview page r?margaret

https://reviewboard.mozilla.org/r/38551/#review37625

::: mobile/android/base/java/org/mozilla/gecko/reader/ReadingListHelper.java:61
(Diff revision 2)
>          this.context = context;
>          this.db = profile.getDB();
>          this.readingListAccessor = db.getReadingListAccessor();
>  
>          EventDispatcher.getInstance().registerGeckoThreadListener((NativeEventListener) this,
> -            "Reader:AddToList", "Reader:UpdateList", "Reader:FaviconRequest");
> +            "Reader:AddToList", "Reader:UpdateList", "Reader:FaviconRequest", "Reader:AddToCache");

It's confusing that you're using the same message name from sending messages Java->JS and JS->Java. These messages must mean different things, so let's give them different names.

I'm also confused about what's going on here, since I haven't seen a patch to add ReaderCacheHelper, so I'm not sure what it's doing.

Is the cache still managed by Gecko? If so, why do we need to send a message back to Java after we add the item to the cache?

Comment 16

[:Margaret Leibovic]

Comment on attachment 8727619 [details]
MozReview Request: Bug 1234331 - Remove from readercache when unbookmarking page r=margaret

https://reviewboard.mozilla.org/r/38553/#review37629

::: mobile/android/base/java/org/mozilla/gecko/reader/ReadingListHelper.java:311
(Diff revision 2)
> +
> +        ReaderCacheHelper rch = GeckoProfile.get(context).getReaderCacheHelper();
> +
> +        if (rch.isUrlCached(url)) {
> +            GeckoAppShell.sendEventToGecko(
> +                    GeckoEvent.createBroadcastEvent("Reader:RemoveFromCache", url));

You're going to have to change these after bug 1257319 lands. But you'll probably get a build error that will tell you that as well :)

::: mobile/android/base/java/org/mozilla/gecko/reader/ReadingListHelper.java:316
(Diff revision 2)
> +                    GeckoEvent.createBroadcastEvent("Reader:RemoveFromCache", url));
> +        }
> +
> +        // When removing items from the cache we can probably spare ourselves the async callback
> +        // that we use when adding cached items.
> +        rch.remove(url);

Still not sure what this is doing, but this change seems resonable to me.

Comment 17

[:Margaret Leibovic]

Comment on attachment 8727620 [details]
MozReview Request: Bug 1234331 - Push bookmarked item into readercache when entering readermode r?margaret

https://reviewboard.mozilla.org/r/38555/#review37631

::: mobile/android/base/java/org/mozilla/gecko/Tab.java:757
(Diff revision 2)
>              }
>          }, 500);
> +
> +        // Ensure we store bookmarked pages in the readercache if we open them into readermode
> +        if (success && isBookmark() && AboutPages.isAboutReader(oldURL)) {
> +            ReadingListHelper.cacheReaderItem(ReaderModeUtils.stripAboutReaderUrl(oldURL), mAppContext);

Have you talked to Anthony and Barbara about this? This is a UX change, since it will change what happens when a user taps on a bookmark.

This seems unexpected, I'm not sure that we want to do this (or at least not do it as part of this bug).

Comment 18

[:Margaret Leibovic]

Comment on attachment 8727621 [details]
MozReview Request: Bug 1234331 - Remove readercache item when exiting readermode on bookmarked page r?margaret

https://reviewboard.mozilla.org/r/38557/#review37633

::: mobile/android/base/java/org/mozilla/gecko/Tab.java:759
(Diff revision 2)
>  
>          // Ensure we store bookmarked pages in the readercache if we open them into readermode
>          if (success && isBookmark() && AboutPages.isAboutReader(oldURL)) {
>              ReadingListHelper.cacheReaderItem(ReaderModeUtils.stripAboutReaderUrl(oldURL), mAppContext);
> +        } else if (success && isBookmark() && !AboutPages.isAboutReader(oldURL)) {
> +            ReadingListHelper.removeCachedReaderItem(oldURL, mAppContext);

Same comment applies to this patch. This needs more discussion, I don't think this should happen in this bug.

Comment 19

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/38549/#review37499

> We could remove this early return outside the runnable, and not even post the runnable in this case.
> 
> Not your code, but do we know when this would even happen? Seems like an unexpected case, and there's no feedback for the user.

So I found "may be null if user-entered query hasn't yet been resolved to a URI" for getURL. However I'm not sure how we'd end up in removeBookmark unless the page has previously been bookmarked, which shouldn't be the case if we haven't even resolved the URI yet.

As far as I can tell we fire OnLocationChanged with a null URL as soon as we type in a new URL, so we shouldn't ever have the bookmark star enabled unless we (A) have a fully loaded page and (B) the page is a bookmark, but that doesn't seem to be guaranteed either so we still might need the check?

Comment 20

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/38551/#review37625

> It's confusing that you're using the same message name from sending messages Java->JS and JS->Java. These messages must mean different things, so let's give them different names.
> 
> I'm also confused about what's going on here, since I haven't seen a patch to add ReaderCacheHelper, so I'm not sure what it's doing.
> 
> Is the cache still managed by Gecko? If so, why do we need to send a message back to Java after we add the item to the cache?

ReaderCacheHelper keeps track of the list of items that are actually cached, all that Gecko does is actually create/remove individual readercache items. So when we cache an item we need to track this in Java (we use that to indicate which items are saved offline).

Previously the list of items was maintained by the reading list (which was in Java/SQL) - which we're replacing with ReaderCacheHelper + Bookmarks, Gecko doesn't seem to actually know what items are cached, it can only tell us if a given URL is cached.

It's introduced in (hopefully this link keeps working, but I've had issues with reviewboard confusing commits when I change ordering):
https://reviewboard.mozilla.org/r/38525/

Comment 21

[Andrzej Hunt :ahunt]

Comment on attachment 8727617 [details]
MozReview Request: Bug 1234331 - Allow bookmarking readerview pages r=margaret

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

Comment 22

[Andrzej Hunt :ahunt]

Comment on attachment 8727618 [details]
MozReview Request: Bug 1234331 - Push article into readercache when bookmarking readerview page r?margaret

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

Comment 23

[Andrzej Hunt :ahunt]

Comment on attachment 8727619 [details]
MozReview Request: Bug 1234331 - Remove from readercache when unbookmarking page r=margaret

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

Comment 24

[:Margaret Leibovic]

Comment on attachment 8727618 [details]
MozReview Request: Bug 1234331 - Push article into readercache when bookmarking readerview page r?margaret

https://reviewboard.mozilla.org/r/38551/#review38523

::: mobile/android/chrome/content/Reader.js:87
(Diff revision 3)
>        }
> +
> +      case "Reader:AddToCache": {
> +        // If the article is coming from reader mode, we must have fetched it already.
> +        this._getArticle(aData).then((article) => {
> +          console.log("trying");

Remember to remove debug logging.

::: mobile/android/chrome/content/Reader.js:89
(Diff revision 3)
> +      case "Reader:AddToCache": {
> +        // If the article is coming from reader mode, we must have fetched it already.
> +        this._getArticle(aData).then((article) => {
> +          console.log("trying");
> +          ReaderMode.storeArticleInCache(article).catch(e => Cu.reportError("Error storing article in cache: " + e));
> +        });

It looks like `_getArticle` can throw. We should probably att the catch at the end of the chain.

Comment 26

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/868f7fa5f28581feb1cb725554e74607a872e81c
Bug 1234331 - Allow bookmarking readerview pages r=margaret

https://hg.mozilla.org/integration/fx-team/rev/9602a9e2571319dc724fa520f8d8356a064b434a
Bug 1234331 - Push article into readercache when bookmarking readerview page r=margaret

https://hg.mozilla.org/integration/fx-team/rev/5bb2e0e1df1a7fb4b3f4d6ad75aece5478a0162f
Bug 1234331 - Remove from readercache when unbookmarking page r=margaret

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/868f7fa5f285
https://hg.mozilla.org/mozilla-central/rev/9602a9e25713
https://hg.mozilla.org/mozilla-central/rev/5bb2e0e1df1a

Comment 28

[Teodora Vermesan (:TeoVermesan)]

Reader view pages can be added as bookmarks, so:
Verified as fixed using:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 48.0a1 (2016-04-10)