For it to be possible to add a page to the reading list without headless gecko, we need to be able to do so without fetching it. The simplest solution to this problem appears to be to support adding even pages that we cannot open in reader mode. When it is time to read such a page, we should respond by simply opening it in "normal" mode, not reader mode. Otherwise, implementation of "add to reading list" in overlays becomes impossible: we don't have time to fetch the page and figure out if we can even open it in reader mode. We don't even have the *capability* to do that without headless Gecko, which is a nightmare for another day.

I appear to have been slightly dense: simply inserting rows into the database is sufficient to add something to the reading list. The problem is when the page is incompatible with reader mode, we currently just die when we attempt to load it. Instead, what should happen is the page should load normally. This way, pages added to the reading list "blindly" (without loading the page and determining if it's susceptible to reader-mode-ification) can still be read (the share overlay does this).

Let's gently morph this.

I'd be interested in taking up this bug, could you point me in the right directions as to which classes I'll need to look at?

Tristan We have ways of putting a URL into the Reading List. Let's not focus too much on those ways for now, except to say that the Share Handler's "Add to Reading List" is one of them. Using that feature, then going to Fennec and looking at the Reading List will show the URL. Tapping the row will end up showing an error. I think that error is shown here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#547 My first "simple" idea is too just try to load the URL instead of showing the error. Maybe that means we can change: this._showError(gStrings.GetStringFromName("aboutReader.loadError")); to this._win.location.href = url; Thoughts from anyone?

That sounds like a good direction to me! Let's definitely try to do the simplest thing first. As a follow-up, maybe we could have another bug that uses a toast or something to let the user know we did try to open the page in reader mode but failed. Is there a bug filed on morphing our reader mode cache to let us add things to it when we add them to the reading list? That's definitely a much bigger project than this bug, but that would be a good long-term improvement.

That seems to do the trick, built and tested on Android 4.1.2. I agree that it would be good to inform the user that we tried to load the page in reader mode, but I'm not quite sure how to do that.

(In reply to Tristan Pollitt from comment #6) > That seems to do the trick, built and tested on Android 4.1.2. ... I spent a day trying to make this work... \o/

(In reply to Chris Kitching [:ckitching] from comment #7) > (In reply to Tristan Pollitt from comment #6) > > That seems to do the trick, built and tested on Android 4.1.2. > > ... I spent a day trying to make this work... > > \o/ What couldn't you get working? The way I tested this was to add a non reader-friendly URL (such as twitter.com) to the reading list via the share functionality, and then open it in the reading list. Before the patch it shows the error, after applying the patch it loads the page as normal.

(In reply to Tristan Pollitt from comment #8) > (In reply to Chris Kitching [:ckitching] from comment #7) > > (In reply to Tristan Pollitt from comment #6) > > > That seems to do the trick, built and tested on Android 4.1.2. > > > > ... I spent a day trying to make this work... > > > > \o/ > > What couldn't you get working? The way I tested this was to add a non > reader-friendly URL (such as twitter.com) to the reading list via the share > functionality, and then open it in the reading list. Before the patch it > shows the error, after applying the patch it loads the page as normal. My patch as-was when I gave up on it: https://www.dropbox.com/s/fuzzvrnuzygbbig/readingListFix.patch?dl=0 Whenever I tried to open a URL that wasn't reader-friendly the browser would exit. I never figured out what the error, if any, was. Nothing seemed to be output anywhere useful... *shrug* This is probably why I should never write Javascript. It's brittle as hell and likes silently failing. I'm extremely glad your patch works, since I've not been looking forward to getting back to fixing this :D Thanks!

(Note the large amount of crazy desperate debug code in there. The only really interesting change is on line 137 of the diff)

Comment on attachment 8485420 [details] [diff] [review] aboutReader.patch Review of attachment 8485420 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ps: In theory, we should only redirect when trying to access reading list items. But this would be a little complicated to do because the reading list state is fetched asynchronously when the reader UI starts -- and in parallel with the document parsing. Accessing an invalid reader page outside the reading list is an edge case though. This should be fine for now. ::: mobile/android/chrome/content/aboutReader.js @@ +544,5 @@ > gChromeWin.Reader.parseDocumentFromURL(url, function(article) { > if (article) > this._showContent(article); > else > + this._win.location.href = url; Looks good. ps: In theory, we should only redirect when trying to access reading list items. But this would be a little complicated to do because the reading list state is fetched asynchronously when the reader UI starts -- and in parallel with the document parsing. Accessing an invalid reader page outside the reading list is an edge case though. This should be fine for now.

Thanks for the review. Do we need to run this on the try server before integrating?

(In reply to Tristan Pollitt from comment #12) > Thanks for the review. Do we need to run this on the try server before > integrating? Yes, do you have commit access to push patches to Try? I can push it otherwise.

I assume Tristan does not pushed as https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e4c2a78551e3

(In reply to Kevin Brosnan [:kbrosnan] from comment #14) > I assume Tristan does not pushed as > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e4c2a78551e3 Looks green.

Tristan if you have not already consider applying for level 1 access https://www.mozilla.org/hacking/committer/

(In reply to Kevin Brosnan [:kbrosnan] from comment #17) > Tristan if you have not already consider applying for level 1 access > https://www.mozilla.org/hacking/committer/ Will do. Thanks for your help everyone :)

Verified as fixed in Build: Firefox for Android 35.0a1 (2014-09-21) Device: Asus Transformer Pad TF300T (Android 4.2.1) and Motorola Razr (Android 4.1.2)