Regression: stop offering reader-mode on video

RESOLVED FIXED in Firefox 36

Status

()

Firefox for Android
Reader View
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: aaronmt, Assigned: Margaret)

Tracking

({regression, reproducible})

36 Branch
Firefox 36
regression, reproducible
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox36 affected, fennec36+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Updated

3 years ago
tracking-fennec: --- → ?
status-firefox33: --- → unaffected
status-firefox34: --- → ?
status-firefox35: --- → ?
status-firefox36: --- → affected
Keywords: regression
Summary: Stop offering reader-mode on video → Regression: stop offering reader-mode on video
(Reporter)

Updated

3 years ago
Keywords: regressionwindow-wanted, reproducible
(Reporter)

Updated

3 years ago
Flags: in-testsuite?
(Reporter)

Comment 1

3 years ago
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
(Reporter)

Comment 2

3 years ago
Re-setting flags on correction
status-firefox33: unaffected → ---
status-firefox34: ? → ---
status-firefox35: ? → ---
(Reporter)

Comment 3

3 years ago
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
(Assignee)

Comment 4

3 years ago
I can investigate.
Assignee: nobody → margaret.leibovic
(Assignee)

Comment 5

3 years ago
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.
Blocks: 1087722
Keywords: regressionwindow-wanted
(Assignee)

Comment 6

3 years ago
Created attachment 8520897 [details] [diff] [review]
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.
Attachment #8520897 - Flags: review?(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.
Attachment #8520897 - Flags: review?(bnicholson) → review+
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.
Attachment #8520897 - Flags: review+ → review-
(Assignee)

Comment 9

3 years ago
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.
(Assignee)

Comment 10

3 years ago
(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.
(Assignee)

Comment 11

3 years ago
Created attachment 8520931 [details] [diff] [review]
Never reject promise returned by parseDocumentFromTab

I think this is a clearer way of doing this, and makes the public API more explicit.
Attachment #8520897 - Attachment is obsolete: true
Attachment #8520931 - Flags: review?(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
(Assignee)

Comment 13

3 years ago
Created attachment 8521028 [details] [diff] [review]
Properly handle exceptions thrown while parsing article

I believe this matches what we talked about on IRC.
Attachment #8520931 - Attachment is obsolete: true
Attachment #8520931 - Flags: review?(bnicholson)
Attachment #8521028 - Flags: review?(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.
Attachment #8521028 - Flags: review?(bnicholson) → review+
(Assignee)

Comment 15

3 years ago
(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.
(Assignee)

Comment 16

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/2216c4695b89
https://hg.mozilla.org/mozilla-central/rev/2216c4695b89
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
tracking-fennec: ? → 36+
You need to log in before you can comment on or make changes to this bug.