Splitting this off from bug 1147337. This can also handle moving the _getOriginalURL logic into a shared place (and creating a unit test for it).

Asking mcomella to take a look at the mobile changes here. This should address the issues we talked about in bug 1147337.

Comment on attachment 8589384 [details] [diff] [review] Factor out logic to get original URL from reader URL into shared place, and handle malformed URI excpetions Review of attachment 8589384 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, r=me with nits possibly addressed. ::: browser/modules/ReaderParent.jsm @@ +188,1 @@ > }, Nit: If parseReaderUrl is now a pure proxy to ReaderMode.getOriginalUrl, can't we just call the latter directly? ::: mobile/android/chrome/content/browser.js @@ +4564,5 @@ > + if (originalUrl) { > + return originalUrl; > + } > + > + return url; Nit: `return originalUrl || url;` should works here, or the more verbose return originalUrl ? originalUrl : url; ::: toolkit/components/reader/AboutReader.jsm @@ +790,2 @@ > } > + return url; Nit: Same as above.

(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #2) > Comment on attachment 8589384 [details] [diff] [review] > Factor out logic to get original URL from reader URL into shared place, and > handle malformed URI excpetions > > Review of attachment 8589384 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, r=me with nits possibly addressed. > > ::: browser/modules/ReaderParent.jsm > @@ +188,1 @@ > > }, > > Nit: If parseReaderUrl is now a pure proxy to ReaderMode.getOriginalUrl, > can't we just call the latter directly? I considered that, but ReaderParent.parseReaderUrl is called elsewhere in the tree. But I could look into updating those call sites as well. Just a matter of deciding how invasive to make this patch :)

On further consideration, I think it makes sense to just get rid of this `parseReaderUrl` method. This touches more desktop front-end stuff, so I'm asking jaws to review that part.

Sounds like just a refactoring, so P5?

(In reply to Justin Dolske [:Dolske] from comment #5) > Sounds like just a refactoring, so P5? Not just a refactoring... decodeURIComponent is throwing exceptions which break reader view for some poorly formed URLs. We run into this with this test case from bug 1147337: http://moztw.org/space/?;$%^^

Comment on attachment 8589794 [details] [diff] [review] (v2) Factor out logic to get original URL from reader URL into shared place, and handle malformed URI excpetions Review of attachment 8589794 [details] [diff] [review]: ----------------------------------------------------------------- Nice. :D ::: mobile/android/chrome/content/Reader.js @@ -4,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > "use strict"; > > -XPCOMUtils.defineLazyModuleGetter(this, "ReaderMode", "resource://gre/modules/ReaderMode.jsm"); ReaderMode still seems to be used here - why are you removing it? Is it becauses the imports from browser.js are shared here?

(In reply to Michael Comella (:mcomella) from comment #7) > ::: mobile/android/chrome/content/Reader.js > @@ -4,5 @@ > > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > > > "use strict"; > > > > -XPCOMUtils.defineLazyModuleGetter(this, "ReaderMode", "resource://gre/modules/ReaderMode.jsm"); > > ReaderMode still seems to be used here - why are you removing it? Is it > becauses the imports from browser.js are shared here? Correct. Reader.js is loaded as a subscript in browser.js, so any imports here would be redundant.

Comment on attachment 8589794 [details] [diff] [review] (v2) Factor out logic to get original URL from reader URL into shared place, and handle malformed URI excpetions Review of attachment 8589794 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-readinglist.js @@ +251,5 @@ > let uri; > if (this.enabled && state == "valid") { > uri = gBrowser.currentURI; > if (uri.schemeIs("about")) > + uri = ReaderMode.getOriginalUrl(uri.spec); I vaguely wonder whether it's worth making getOriginalUrl deal with being passed an nsIURI, but I guess maybe not? ::: browser/base/content/test/general/browser_readerMode.js @@ +100,5 @@ > + is(ReaderMode.getOriginalUrl(url), null, "Did not find original URL from non-reader URL"); > + > + let badUrl = "http://foo.com/?;$%^^"; > + is(ReaderMode.getOriginalUrl("about:reader?url=" + encodeURIComponent(badUrl)), badUrl, "Found original URL from encoded malformed URL"); > + is(ReaderMode.getOriginalUrl("about:reader?url=" + badUrl), badUrl, "Found original URL from non-encoded malformed URL"); Please also add assertions here for the case that throws an exception.

(In reply to :Gijs Kruitbosch from comment #9) > ::: browser/base/content/test/general/browser_readerMode.js > @@ +100,5 @@ > > + is(ReaderMode.getOriginalUrl(url), null, "Did not find original URL from non-reader URL"); > > + > > + let badUrl = "http://foo.com/?;$%^^"; > > + is(ReaderMode.getOriginalUrl("about:reader?url=" + encodeURIComponent(badUrl)), badUrl, "Found original URL from encoded malformed URL"); > > + is(ReaderMode.getOriginalUrl("about:reader?url=" + badUrl), badUrl, "Found original URL from non-encoded malformed URL"); > > Please also add assertions here for the case that throws an exception. I'm a bit confused by this comment. This function doesn't throw anymore, and these checks are the cases that used to throw (but now go through the catch statement in getOriginalUrl.

(In reply to :Margaret Leibovic from comment #10) > (In reply to :Gijs Kruitbosch from comment #9) > > > ::: browser/base/content/test/general/browser_readerMode.js > > @@ +100,5 @@ > > > + is(ReaderMode.getOriginalUrl(url), null, "Did not find original URL from non-reader URL"); > > > + > > > + let badUrl = "http://foo.com/?;$%^^"; > > > + is(ReaderMode.getOriginalUrl("about:reader?url=" + encodeURIComponent(badUrl)), badUrl, "Found original URL from encoded malformed URL"); > > > + is(ReaderMode.getOriginalUrl("about:reader?url=" + badUrl), badUrl, "Found original URL from non-encoded malformed URL"); > > > > Please also add assertions here for the case that throws an exception. > > I'm a bit confused by this comment. This function doesn't throw anymore, and > these checks are the cases that used to throw (but now go through the catch > statement in getOriginalUrl. Ah, then I must have misunderstood. Carry on!

Comment on attachment 8589794 [details] [diff] [review] (v2) Factor out logic to get original URL from reader URL into shared place, and handle malformed URI excpetions Approval Request Comment [Feature/regressing bug #]: this breaks reader mode for weird escape sequences, e.g. bug 1147337 comment 27 [User impact if declined]: breaking reader mode [Describe test coverage new/current, TreeHerder]: has tests! [Risks and why]: pretty low, making us more fault-tolerant [String/UUID change made/needed]: nope

Comment on attachment 8589794 [details] [diff] [review] (v2) Factor out logic to get original URL from reader URL into shared place, and handle malformed URI excpetions Should be in 38 beta 5. We will still have RL enabled in this beta.