Closed Bug 1234331 Opened 4 years ago Closed 4 years ago

Enable bookmark star for reader view pages

Categories

(Firefox for Android :: Reader View, defect)

35 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- verified

People

(Reporter: Margaret, Assigned: ahunt)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

We should allow users to bookmark reader view pages.
Blocks: 1234328
Assignee: nobody → 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?
And maybe we should also clear the item from the readercache on bookmark deletion (only if that's the only bookmark with that URL).
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/
These patches depend directly on those in Bug 1234315, hence adding that dependency.
Depends on: 1234315
Attachment #8727617 - Attachment description: MozReview Request: Bug 1234331 - Allow bookmarking readerview pages → MozReview Request: Bug 1234331 - Allow bookmarking readerview pages r?margaret
Attachment #8727617 - Flags: review?(margaret.leibovic)
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 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/
Attachment #8727618 - Attachment description: MozReview Request: Bug 1234331 - Push article into readercache when bookmarking readerview page → MozReview Request: Bug 1234331 - Push article into readercache when bookmarking readerview page r?margaret
Attachment #8727618 - Flags: review?(margaret.leibovic)
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/
Attachment #8727619 - Attachment description: MozReview Request: Bug 1234331 - Remove from readercache when unbookmarking page → MozReview Request: Bug 1234331 - Remove from readercache when unbookmarking page r?margaret
Attachment #8727619 - Flags: review?(margaret.leibovic)
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/
Attachment #8727620 - Attachment description: MozReview Request: Bug 1234331 - Push bookmarked item into readercache when entering readermode → MozReview Request: Bug 1234331 - Push bookmarked item into readercache when entering readermode r?margaret
Attachment #8727620 - Flags: review?(margaret.leibovic)
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/
Attachment #8727621 - Attachment description: MozReview Request: Bug 1234331 - Remove readercache item when exiting readermode on bookmarked page → MozReview Request: Bug 1234331 - Remove readercache item when exiting readermode on bookmarked page r?margaret
Attachment #8727621 - Flags: review?(margaret.leibovic)
Attachment #8727617 - Flags: review?(margaret.leibovic) → review+
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 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?
Attachment #8727618 - Flags: review?(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.
Attachment #8727619 - Flags: review?(margaret.leibovic) → review+
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).
Attachment #8727620 - Flags: review?(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.
Attachment #8727621 - Flags: review?(margaret.leibovic)
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?
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 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/
Attachment #8727617 - Attachment description: MozReview Request: Bug 1234331 - Allow bookmarking readerview pages r?margaret → MozReview Request: Bug 1234331 - Allow bookmarking readerview pages r=margaret
Attachment #8727618 - Flags: review?(margaret.leibovic)
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 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/
Attachment #8727619 - Attachment description: MozReview Request: Bug 1234331 - Remove from readercache when unbookmarking page r?margaret → MozReview Request: Bug 1234331 - Remove from readercache when unbookmarking page r=margaret
Attachment #8727620 - Attachment is obsolete: true
Attachment #8727621 - Attachment is obsolete: true
Attachment #8727618 - Flags: review?(margaret.leibovic) → review+
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.
Duplicate of this bug: 1262365
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
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)
You need to log in before you can comment on or make changes to this bug.