1267467 - Opening a reading list/reader mode icon should bump its history

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

Opening a reading list item or reader mode item does not update its position in history.

STR:
1. Have an item in the reading list.
2. Open a bunch of other websites.
3. Open reading list item in a tab.
4. Go to History panel.

Expected:
Item is at the top of history because it was just opened.

Actual:
Item is not at the top of history because it was opened from cache and no "page" was loaded.

We should update the history count and lastVisited.

Comment 1

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

Talking to Margaret about this too, this is probably something for 49. 

But given the direction we want to take "History" and how it's for "recalling", we should make sure it also shows up in there.

Comment 2

[Andrzej Hunt :ahunt]

I have a suspicion about: pages are filtered out on platform side, specifically if we treat about: pages as typeChrome (as opposed to typeContent) then we'd never pass the URI to our history manager at:
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#12809

(I haven't actually debugged this yet, so I can't be certain that this is what's happening.)


Similarly going directly to an about:reader url doesn't result in a history entry on desktop. E.g. try to open the following URL -> no history entry
about:reader?url=https://support.mozilla.org/en-US/kb/save-web-pages-your-reading-list-firefox-android

Comment 4

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1267467 - Don't filter out reader view pages when storing history r?jchen

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

Comment 5

[Andrzej Hunt :ahunt]

This patch lets us save about:reader URLs. However we probably also want to strip the about:reader part of the URL (perhaps on the Java side) so that we don't have separate about:reader and plain versions of the site in history?

Comment 6

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1267467 - Strip about:reader when storing URIs in history r?margaret

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

Comment 7

[Andrzej Hunt :ahunt]

Comment on attachment 8749815 [details]
MozReview Request: Bug 1267467 - Don't filter out reader view pages when storing history r?jchen

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

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8749815 [details]
MozReview Request: Bug 1267467 - Don't filter out reader view pages when storing history r?jchen

https://reviewboard.mozilla.org/r/51169/#review48611

Comment 9

[:Margaret Leibovic]

Comment on attachment 8751015 [details]
MozReview Request: Bug 1267467 - Strip about:reader when storing URIs in history r?margaret

https://reviewboard.mozilla.org/r/51753/#review49111

I'm worried there may be some nuance I'm missing here, but this seems fine to me. I don't like that we're continuing to spread the pattern of special-casing these about:reader URLs, but I suppose that's just the nature of our implementation decision to make reader view into a special page.

Comment 10

[:Margaret Leibovic]

Gijs, this bug also applies to desktop Firefox. You may want to consider filing a desktop bug as well.

Comment 11

[:Gijs (he/him)]

(In reply to :Margaret Leibovic from comment #10)
> Gijs, this bug also applies to desktop Firefox. You may want to consider
> filing a desktop bug as well.

Thanks, filed bug 1272373.

Comment 12

[Andrzej Hunt :ahunt]

(In reply to :Margaret Leibovic from comment #9)
> I'm worried there may be some nuance I'm missing here, but this seems fine
> to me. I don't like that we're continuing to spread the pattern of
> special-casing these about:reader URLs, but I suppose that's just the nature
> of our implementation decision to make reader view into a special page.

I feel like filtering out about:reader at this stage is the least intrusive thing we can do: we currently assume all pages in bookmarks/other homepanels are the raw URL (without about:reader), and we add the offline icon for any bookmark/history-item/etc that is stored offline. If we store the about:reader URL in history, then we'd probably need to strip that when displaying the history item. There are probably a bunch of other special cases (e.g. sharing), so this is probably the least painful approach.

The advantage of this approach is that we show one page in history when you visit a page, and then enter reader mode (otherwise we'd have one normal version, one reader version). The disadvantage is that (unless the page is bookmarked from RV), the history item will open the non-readerview version of the page.

Comment 13

[Andrzej Hunt :ahunt]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=950df3a46ea1

Comment 14

[Andrzej Hunt :ahunt]

I'll probably wait a bit with landing this since there's some interesting discussion happening in Bug 1272373 (the equivalent desktop bug), and whatever the solution there is might help make the android side better too. (I don't really have much knowledge of the docshell / history / etc, so my solution is unlikely to be optimal.)

Comment 15

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/b1f926af38405c963cb7cc6db9e35ec099c7619a
Bug 1267467 - Don't filter out reader view pages when storing history r=jchen

https://hg.mozilla.org/integration/fx-team/rev/2dc0440c0d6a3741a9d7791f9c0a180f78a91747
Bug 1267467 - Strip about:reader when storing URIs in history r=margaret

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b1f926af3840
https://hg.mozilla.org/mozilla-central/rev/2dc0440c0d6a

Comment 17

[Teodora Vermesan (:TeoVermesan)]

After opening some websites and after that an item from reading list in a new tab, The reading list item is at the top of history.

Verified as fixed using:
Device: Galaxy note 5 (Android 5.1.1)
Build: Firefox for Android 49.0a1 (2016-05-26)