For articles in the reading list.

Er this is not the same.

Not the same issue. Re-opening.

Just a quick note: if we want to sync the scroll position across devices, we'll have to store it in our content provider DB. rnewman, I wonder if sync already has a data schema for "annotating" bookmarks with properties?

It doesn't at the moment, unless you count folders and tags and such! This data doesn't sound like it really belongs closely tied to a bookmark; it's more like the Kindle position tracker. You could conceivably track positions on history pages, too. In that regard this is really part of session state -- the same thing that Firefox itself uses to restore your page position after a restart. IMO we ought to aim for parity with Kindle: that is, support enough data to say "Your current furthest read position is 76%, on Richard's Galaxy, from Friday. Advance to that position?". (If only the world hadn't already used the word bookmark to point to a webpage, rather than to a section of text, as it does for a book! Maybe the noun phrase here is "page marker"?) So I'd suggest a different table -- "page_markers" -- for this, with a schema something like: • GUID -- so we can 'own' a record somewhere. If you don't care about current Sync support, this can be omitted, assuming that future technologies provide salted hash IDs. Just hash client ID + item identifier. • GUID of reading list item, or URI if it's stable -- just needs to be a stable identifier shared between devices. • Client ID (null for us) -- for joining to the client table. • Position (in whatever format works across devices) • Date last updated This will support incremental updates (for future PICLing), supports positions across multiple devices (optionally not advancing), supports finding the maximum (for prompting), and allows for cleanup, all without hampering devices that don't understand position tracking. And we can keep in our heads the idea that this is a subset of session state, which will avoid us getting bitten in the future.

(In reply to Lucas Rocha (:lucasr) from comment #4) > Just a quick note: if we want to sync the scroll position across devices, > we'll have to store it in our content provider DB. > > rnewman, I wonder if sync already has a data schema for "annotating" > bookmarks with properties? We don't sync articles in the Reading List yet. They are stored in an IndexedDB, so the position should be saved with it.

(In reply to Richard Newman [:rnewman] from comment #5) > It doesn't at the moment, unless you count folders and tags and such! > > This data doesn't sound like it really belongs closely tied to a bookmark; > it's more like the Kindle position tracker. You could conceivably track > positions on history pages, too. In that regard this is really part of > session state -- the same thing that Firefox itself uses to restore your > page position after a restart. Not a goal > IMO we ought to aim for parity with Kindle: that is, support enough data to > say "Your current furthest read position is 76%, on Richard's Galaxy, from > Friday. Advance to that position?". This sounds like a nice goal when we start Syncing the Reading List.

(In reply to Mark Finkle (:mfinkle) from comment #6) > We don't sync articles in the Reading List yet. They are stored in an > IndexedDB, so the position should be saved with it. Really? I thought we had a readinglist root in browser.db?

(In reply to Richard Newman [:rnewman] from comment #8) > (In reply to Mark Finkle (:mfinkle) from comment #6) > > > We don't sync articles in the Reading List yet. They are stored in an > > IndexedDB, so the position should be saved with it. > > Really? I thought we had a readinglist root in browser.db? This is true, but for any offline viewing, we store the content in an IndexedDB. The Reader object does the work: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6588 The actual "reading list" is maintained as bookmarks in browser.db, but I question storing any state with the bookmark - or even keeping the list in bookmarks. Currently, the "reader" is an HTML page/app, which limits some of it's ability to put it's data in a ContentProvider accessible location and explains why we're using HTML storage, like IndexedDB. Will we keep the reader in HTML? Will we convert to a native Android UI? I don't know the answers to these questions yet, but the outcome will affect the storage systems and how we attempt to tie into any Sync system.

(In reply to Mark Finkle (:mfinkle) from comment #9) > This is true, but for any offline viewing, we store the content in an > IndexedDB. The Reader object does the work: > http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/ > browser.js#6588 Interesting! > The actual "reading list" is maintained as bookmarks in browser.db, but I > question storing any state with the bookmark - or even keeping the list in > bookmarks. Concur. I don't think we get much value out of it, at least until there's a plan to sync it (which I think is something on which Karen should opine.) Perhaps we should be thinking of Reading List as a web app? > Will we keep the reader in HTML? Will we convert to a native Android UI? I > don't know the answers to these questions yet, but the outcome will affect > the storage systems and how we attempt to tie into any Sync system. Glad we're on the same page :D It might be wise to figure out where reading list will be in 18 months, before we decide how to approach what I might term [pocket-parity]…

Comment on attachment 8391514 [details] [diff] [review] bug-857990.patch Review of attachment 8391514 [details] [diff] [review]: ----------------------------------------------------------------- I wonder if we should have a separate store just for the scroll position to avoid re-saving to whole article content just to change the scroll position. I'm not sure IndexedDB is smart about only saving what has changed in the record. Maybe have a look at the performance numbers around these options? ::: mobile/android/chrome/content/Readability.js @@ +1437,5 @@ > return { title: articleTitle, > byline: this._articleByline, > dir: this._articleDir, > + content: articleContent.innerHTML, > + scrollPosition: 0 }; Readability.js shouldn't really have to mess with scroll position. This should be initialized somewhere in browser.js (Reader) or aboutReader.js. ::: mobile/android/chrome/content/aboutReader.js @@ +291,5 @@ > Services.obs.removeObserver(this, "Reader:Remove"); > Services.obs.removeObserver(this, "Reader:ListCountReturn"); > Services.obs.removeObserver(this, "Reader:ListCountUpdated"); > Services.obs.removeObserver(this, "Reader:ListStatusReturn"); > + this._storeScrollPosition(); I do wonder if this is a reliable spot to trigger this. Please test going back and forth in the session history, in different scenarios, to make sure this works fine. @@ +685,5 @@ > + // visible since it changes the height of the page. There is no callback > + // for that, wait ~10ms. > + this._win.setTimeout(function() { > + this._win.scrollTo(0, this._doc.body.scrollHeight * scrollPosition); > + }.bind(this), 10); If you just want to wait for the layout round to finish before running this, I think using a 0 is fine here. @@ +876,5 @@ > + > + _storeScrollPosition : function Reader_storeScrollPosition() { > + if (!this._article || this._isReadingListItem != 1) { > + return; > + } nit: add empty line here.

(In reply to Lucas Rocha (:lucasr) from comment #12) > I wonder if we should have a separate store just for the scroll position to > avoid re-saving to whole article content just to change the scroll position. > I'm not sure IndexedDB is smart about only saving what has changed in the > record. Maybe have a look at the performance numbers around these options? > yeah i tried both. monitoring the time it takes to store the scroll position. didn't see a delay. > ::: mobile/android/chrome/content/aboutReader.js > @@ +291,5 @@ > > Services.obs.removeObserver(this, "Reader:Remove"); > > Services.obs.removeObserver(this, "Reader:ListCountReturn"); > > Services.obs.removeObserver(this, "Reader:ListCountUpdated"); > > Services.obs.removeObserver(this, "Reader:ListStatusReturn"); > > + this._storeScrollPosition(); > > I do wonder if this is a reliable spot to trigger this. Please test going > back and forth in the session history, in different scenarios, to make sure > this works fine. > tested both. it works fine.

> yeah i tried both. monitoring the time it takes to store the scroll > position. didn't see a delay. Seagull observation: this depends on flash block size versus article content size, and on write throughput. Try testing it on a device with known deficiencies there (Galaxy S), and a huuuge article, e.g., something off PG: http://www.gutenberg.org/files/45157/45157-h/45157-h.htm

(In reply to Richard Newman [:rnewman] from comment #15) > Seagull observation: this depends on flash block size versus article content > size, and on write throughput. > > Try testing it on a device with known deficiencies there (Galaxy S), and a > huuuge article, e.g., something off PG: > > http://www.gutenberg.org/files/45157/45157-h/45157-h.htm Tested with a large article, saw no difference in time to update article. I'll try and find a bad phone. article: http://mag.newsweek.com/2014/03/14/bitcoin-satoshi-nakamoto.html

(In reply to Sola Ogunsakin [:sola] from comment #16) > Tested with a large article, saw no difference in time to update article. > I'll try and find a bad phone. > > article: http://mag.newsweek.com/2014/03/14/bitcoin-satoshi-nakamoto.html That's a small article :) Try with that 200KB ebook.

(In reply to Richard Newman [:rnewman] from comment #17) > That's a small article :) > > Try with that 200KB ebook. Readability couldn't parse it. Also I don't think that's a very realistic use case :)

(In reply to Sola Ogunsakin [:sola] from comment #18) > > Try with that 200KB ebook. > > Readability couldn't parse it. Also I don't think that's a very realistic > use case :) It might not be something we intend(ed) users to use Reading List for, but think: * Our icon is a book. * We call it "Reading List". * It saves stuff for you to read on planes/trains/etc. When you search for "free books" on Google, the first hit is Project Gutenberg. Abandoning for a moment your programmer understanding of how we store this stuff, what about our presentation suggests that you can only use it for relatively short content? I mean, I read thousand-page books on Kindle, which from an affordance perspective is really no different. I'd be quite surprised if, when Reading List/reader mode has strong adoption, users *don't* try putting long-form stuff -- not just ebooks, but also LRB/LARB-length articles, diet guides, novellas, short stories, Anarchist's Cookbook-style text files, huge comment threads/discussion archives, etc. -- into RL. That is to say: on the contrary, I don't think it's "realistic" to expect users to guess how big a page can get before they can no longer switch it into reader mode! Truly long-form text is a use case we should consider, if only to make the failure mode less painful.

Comment on attachment 8392465 [details] [diff] [review] bug-857990.patch Review of attachment 8392465 [details] [diff] [review]: ----------------------------------------------------------------- This is looking pretty good. But I'd like to see a more detailed investigation around the performance of the scroll position update here (especially on Gingerbread phones). Just to see what the numbers look like.

Comment on attachment 8404143 [details] [diff] [review] bug-909618.patch Review of attachment 8404143 [details] [diff] [review]: ----------------------------------------------------------------- Wrong bug? :-)

(In reply to Lucas Rocha (:lucasr) from comment #22) > Wrong bug? :-) oops

Revisiting this bug, I'm wondering if we should continue this approach of storing the scroll position in a client-only cache, or if we should try storing it in the content provider so that it will be easier to sync across devices. Since we're not actually syncing anything yet, I feel like we should just continue with this client cache approach, but I wanted to bring this point up for reconsideration. We could try to revive the patch in here and see how it performs, since it looks like the question of performance is the only thing that prevented it from landing previously.

(In reply to :Margaret Leibovic from comment #24) > Since we're not actually syncing anything yet, I feel like we should just > continue with this client cache approach, but I wanted to bring this point > up for reconsideration. I would anticipate non-trivial changes to the storage layer to support a backend service -- remote insertion of readerified content, cache management, versioning, etc. etc. -- so I think it's fine to proceed in the short term. We'll fix things up later.

Margaret, care to mentor this bug?

Sure, rnewman and I can co-mentor it.

It seems to me that saving scroll position as a measured unit will break when screen sizes change (mobile phones: portrait vs. landscape, desktop: different window sizes). Could we instead store a selector to an element in the viewport? Something like `p:nth-child(5)` and then use scrollIntoView to put that element in to the viewport? This would work across devices as long as the markup is stable. DevTools already has a way to retrieve a unique selector for a DOM node.

We're not going to continue to invest in the reading list, so we shouldn't store more information with reading list items. We should try to restore scroll position when restoring a reader view tab, but that should be covered by bug 810981.

I would like to kindly suggest reopening this bug. On mobile, Firefox reloads the page all the time: switching tabs, sometimes even just when switching apps. Reader more is the only fix one has to make Firefox usable in a mobile context where connectivity is random. So fixing bug 810981 does not address the issue because it's not just about maintaining scroll position on session restore, but on reload. I don't care about syncing. I mean it's kind of nice but really I just want Reader mode to simply work on my mobile without scrolling back to the top all the time.