1107097 - Don't try to parse error pages

Status: RESOLVED INCOMPLETE

(Toolkit :: Reader Mode, defect, P5)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

We shouldn't attempt to reader-ize Error Pages.

Comment 1

[:Margaret Leibovic]

Did you file this because you saw this happen, or because you think it might happen?

Looking at our _shouldCheckUri logic, it seems like we don't parse "about:" pages, and about:neterror would be one of those (I must have missed this before when I thought we were parsing about:reader pages):
http://mxr.mozilla.org/mozilla-central/soure/mobile/android/chrome/content/Reader.js#257

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

I think snorp saw it. I did not.

looking at the code:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/Reader.js#189

Does browser.currentURI return the intended URI or the about:neterror URL? I don't remember offhand. I thought browser.contentDocument.documentURI got the "about:neterror" URL.

Comment 3

[:Margaret Leibovic]

Removing the dependency on bug 1102450, since this isn't about the Readability library itself.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

See also bug 1134810 - may be able to fix both bugs with the same change.

Comment 5

[Justin Dolske [:Dolske]]

At the moment I'm not getting the reader button offered on any feeds I've tried, but that might just be luck. Seems like it would still be fair to explicitly suppress it on things like this where we really to know that reader view isn't the right thing to offer.

But also bumping to P5, because a RSS-only bug is rather minor.

Comment 6

[:Gijs (he/him)]

I don't think we currently would propose to readerize any error pages because of document.documentURI indeed being about:neterror?whatever.

As for feeds (which aren't quite the same...), I haven't been able to find any that we give a reader mode option for - neither on main news sites nor on blogs. In this case document.documentURI isn't about:feeds (though that is indeed the document that is being loaded, sort of). I think it's unlikely that's possible, though, because AFAICT we don't let HTML markup be rendered, and we just generate some <div>s instead, which won't trigger the isProbablyReaderable code (it only checks <p> and <pre> nodes). So going to resolve as incomplete, and if we end up with a testcase (perhaps after we change behaviour for isProbablyReaderable or whatever) where this happens, we can create a new bug with a testcase and/or concrete STR.