Closed
Bug 1096612
Opened 10 years ago
Closed 10 years ago
Regression: stop offering reader-mode on video
Categories
(Firefox for Android Graveyard :: Reader View, defect)
Tracking
(firefox36 affected, fennec36+)
RESOLVED
FIXED
Firefox 36
People
(Reporter: aaronmt, Assigned: Margaret)
References
Details
(Keywords: regression, reproducible)
Attachments
(1 file, 2 obsolete files)
8.15 KB,
patch
|
bnicholson
:
review+
|
Details | Diff | Splinter Review |
Visit http://people.mozilla.org/~atrain/mobile/tests/wat.mp4, see reader mode icon in the URL bar. Tapping it does nothing.
Reporter | ||
Updated•10 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•10 years ago
|
Keywords: regressionwindow-wanted,
reproducible
Reporter | ||
Updated•10 years ago
|
Flags: in-testsuite?
Reporter | ||
Comment 1•10 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•10 years ago
|
||
Re-setting flags on correction
Reporter | ||
Comment 3•10 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 5•10 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•10 years ago
|
||
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 7•10 years ago
|
||
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 8•10 years ago
|
||
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•10 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•10 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•10 years ago
|
||
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 12•10 years ago
|
||
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•10 years ago
|
||
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 14•10 years ago
|
||
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•10 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•10 years ago
|
||
Comment 17•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Updated•10 years ago
|
tracking-fennec: ? → 36+
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•