1096612 - Regression: stop offering reader-mode on video

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Aaron Train [:aaronmt]]

Visit http://people.mozilla.org/~atrain/mobile/tests/wat.mp4, see reader mode icon in the URL bar. Tapping it does nothing.

Comment 1

[Aaron Train [:aaronmt]]

This happens when navigating to video from a directory listing oddly.

  Visit http://people.mozilla.org/~atrain/mobile/tests/
  Scroll down to any video in my listing, tap the video

Comment 2

[Aaron Train [:aaronmt]]

Re-setting flags on correction

Comment 3

[Aaron Train [:aaronmt]]

FTP's still hammered today, so far

Last good revision: 2114ef80f6ae (2014-11-06)
First bad revision: b62ccf3228ba (2014-11-07)

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2114ef80f6ae&tochange=b62ccf3228ba

Related to reader, I see 

7364b11dd6a9	Margaret Leibovic — Bug 1087722 - Convert reader mode API to use promises. r=rnewman,bnicholson

Comment 4

[:Margaret Leibovic]

I can investigate.

Comment 5

[:Margaret Leibovic]

It looks like this happens because parseDocumentFromTab throws an exception, rather than calling the callback with a null article like it used to, so we're not setting the reader active/enabled flags properly.

Comment 6

[:Margaret Leibovic]

Attachment: Return null articles in _readerParse instead of rejecting promise

This patch reverts _readerParse to its old behavior of just calling a callback with null, instead of throwing an error.

Alternately, we could update the parseDocumentFromTab call in browser.js to handle the exception (rather than just logging it), but I feel like it's simpler to keep it the way it is.

Comment 7

[Brian Nicholson (:bnicholson)]

Comment on attachment 8520897 [details] [diff] [review]
Return null articles in _readerParse instead of rejecting promise

Review of attachment 8520897 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, I think this actually makes more sense anyway since it's normal for pages to fail the reader parse and probably shouldn't be considered an exceptional situation.

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 8520897 [details] [diff] [review]
Return null articles in _readerParse instead of rejecting promise

Review of attachment 8520897 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I didn't look deep enough on the first go. I think we should continue to reject in a couple of cases (commented below).

::: mobile/android/chrome/content/Reader.js
@@ +294,5 @@
>        };
>  
> +      worker.onerror = evt => {
> +        this.log("Error in worker: " + evt.message);
> +        resolve(null);

This should actually throw since it's a real error. If this could cause the flags to break and you're resolving to null simply to avoid handling it up the chain, I'm not sure that's a good idea since any other exceptions thrown along the way will also reject and cause things to break. I think it's better to be safe and handle these.

@@ +310,5 @@
>            doc: new XMLSerializer().serializeToString(doc)
>          });
>        } catch (e) {
> +        this.log("Reader: could not build Readability arguments: " + e);
> +        resolve(null);

Same here.

Comment 9

[:Margaret Leibovic]

Okay, but in that case we'll still want to handle this rejection upstream somewhere to update the UI properly. Maybe eating it in _readerParse isn't right, but we could check the result in parseDocumentFromTab, and then never reject the promise returned by parseDocumentFromTab, so that consumers can use it without worrying about handling errors themselves.

Comment 10

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #9)
> Okay, but in that case we'll still want to handle this rejection upstream
> somewhere to update the UI properly. Maybe eating it in _readerParse isn't
> right, but we could check the result in parseDocumentFromTab, and then never
> reject the promise returned by parseDocumentFromTab, so that consumers can
> use it without worrying about handling errors themselves.

Actually, that's already what we do for getArticle. I must have just not thought through the parseDocumentFromTab case as well.

Comment 11

[:Margaret Leibovic]

Attachment: Never reject promise returned by parseDocumentFromTab

I think this is a clearer way of doing this, and makes the public API more explicit.

Comment 12

[Brian Nicholson (:bnicholson)]

Comment on attachment 8520931 [details] [diff] [review]
Never reject promise returned by parseDocumentFromTab

Review of attachment 8520931 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to :Margaret Leibovic from comment #10)
> Actually, that's already what we do for getArticle. I must have just not
> thought through the parseDocumentFromTab case as well.

Hm, I didn't notice that -- I think it would actually be better to just reject there, too. The problem with trying to muffle rejections everywhere is that it's hard to guarantee that we'll catch them all. Regular JS exceptions will also cause rejections, even when we don't explicitly call reject().

Any reason we don't want to just handle this at the top level [1]? I think the line there should actually be changed to .catch() in case anything inside the above block fails.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#4254

Comment 13

[:Margaret Leibovic]

Attachment: Properly handle exceptions thrown while parsing article

I believe this matches what we talked about on IRC.

Comment 14

[Brian Nicholson (:bnicholson)]

Comment on attachment 8521028 [details] [diff] [review]
Properly handle exceptions thrown while parsing article

Review of attachment 8521028 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

::: mobile/android/chrome/content/Reader.js
@@ +101,5 @@
>  
>      let uri = tab.browser.currentURI;
>      let urlWithoutRef = uri.specIgnoringRef;
>  
> +    let article = yield this.getArticle(urlWithoutRef, tabID).catch(e => {

We could probably just leave this line alone since we already throw a few lines above.

Comment 15

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #14)

> ::: mobile/android/chrome/content/Reader.js
> @@ +101,5 @@
> >  
> >      let uri = tab.browser.currentURI;
> >      let urlWithoutRef = uri.specIgnoringRef;
> >  
> > +    let article = yield this.getArticle(urlWithoutRef, tabID).catch(e => {
> 
> We could probably just leave this line alone since we already throw a few
> lines above.

I think we should keep this. We can't add an item to your reading list from a tab if we don't have the tab (why we throw), but we can still add the URL if parsing the article fails, so I think it would be good to continue with the rest of the function.

Comment 16

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/2216c4695b89

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2216c4695b89