Closed
Bug 1150695
Opened 8 years ago
Closed 8 years ago
Move isProbablyReaderable function to Readability.js
Categories
(Toolkit :: Reader Mode, defect)
Toolkit
Reader Mode
Tracking
()
RESOLVED
FIXED
mozilla40
People
(Reporter: Margaret, Assigned: Margaret)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
See discussion here: https://github.com/mozilla/readability/issues/85 I'll make a PR for this, then I'll update ReaderMode.jsm in this bug.
Comment 1•8 years ago
|
||
Looks like this got fixed a little while ago in https://github.com/mozilla/readability/pull/96
Assignee | ||
Comment 2•8 years ago
|
||
(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #1) > Looks like this got fixed a little while ago in > https://github.com/mozilla/readability/pull/96 Yes, I just merged that this morning and landed it on fx-team :) Today I'm going to work on actually using that in ReaderMode.jsm.
Comment 3•8 years ago
|
||
\o/
Assignee | ||
Updated•8 years ago
|
status-firefox38:
--- → affected
status-firefox39:
--- → affected
Assignee | ||
Comment 4•8 years ago
|
||
/r/6623 - Bug 1150695 - Use isProbablyReaderable function from Readability.js. r=Gijs Pull down this commit: hg pull -r eda318f683d424bf4c0c85a4475798926daa1a4f https://reviewboard-hg.mozilla.org/gecko/
Attachment #8588132 -
Flags: review?(gijskruitbosch+bugs)
Comment 5•8 years ago
|
||
Comment on attachment 8588132 [details] MozReview Request: bz://1150695/margaret https://reviewboard.mozilla.org/r/6621/#review5573 Apologies for the delay due to the public holidays here. ::: toolkit/components/reader/ReaderMode.jsm (Diff revision 1) > + let sandbox = {}; > + Services.scriptloader.loadSubScript("resource://gre/modules/reader/Readability.js", sandbox); > + return sandbox["Readability"]; Nit: Because this isn't an actual sandbox, just a scope, can we call it that instead?
Attachment #8588132 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #5) > Comment on attachment 8588132 [details] > MozReview Request: bz://1150695/margaret > > https://reviewboard.mozilla.org/r/6621/#review5573 > > Apologies for the delay due to the public holidays here. > > ::: toolkit/components/reader/ReaderMode.jsm > (Diff revision 1) > > + let sandbox = {}; > > + Services.scriptloader.loadSubScript("resource://gre/modules/reader/Readability.js", sandbox); > > + return sandbox["Readability"]; > > Nit: Because this isn't an actual sandbox, just a scope, can we call it that > instead? Yeah, sounds good. I copied this from what we do to load subscripts in mobile's browser.js.
Assignee | ||
Comment 7•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/89e45c21e631
Comment 8•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/89e45c21e631
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8588132 [details] MozReview Request: bz://1150695/margaret Approval Request Comment [Feature/regressing bug #]: reading list [User impact if declined]: it is harder to test/develop our logic to decide whether or not to show the reader view [Describe test coverage new/current, TreeHerder]: landed on m-c, where there's some basic test coverage for the reader view buton [Risks and why]: low-risk, not a logic change, but rather a refactoring moving some of the logic to the readability github repo [String/UUID change made/needed]: none Note: This depends on uplifting a recent verison of Readability.js. See bug 1152022.
Attachment #8588132 -
Flags: approval-mozilla-beta?
Attachment #8588132 -
Flags: approval-mozilla-aurora?
Comment 10•8 years ago
|
||
Margaret, as bug 1152022 didn't land, are you ok to wait until beta 4 (gtb: next monday) for this uplift?
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #10) > Margaret, as bug 1152022 didn't land, are you ok to wait until beta 4 (gtb: > next monday) for this uplift? Yes, that's fine. I just wanted to nom this to get it on the radar before I forgot about it :)
Flags: needinfo?(margaret.leibovic)
Comment 12•8 years ago
|
||
Comment on attachment 8588132 [details]
MozReview Request: bz://1150695/margaret
Should be in 38 beta 4.
Attachment #8588132 -
Flags: approval-mozilla-beta?
Attachment #8588132 -
Flags: approval-mozilla-beta+
Attachment #8588132 -
Flags: approval-mozilla-aurora?
Attachment #8588132 -
Flags: approval-mozilla-aurora+
Updated•8 years ago
|
Flags: qe-verify-
Assignee | ||
Comment 15•8 years ago
|
||
Attachment #8588132 -
Attachment is obsolete: true
Attachment #8619956 -
Flags: review+
Assignee | ||
Comment 16•8 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•