Closed Bug 1265304 Opened 8 years ago Closed 7 years ago

Display estimated reading time in Reader Mode

Categories

(Toolkit :: Reader Mode, enhancement, P5)

enhancement

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
relnote-firefox --- 53+
firefox53 --- verified

People

(Reporter: djthrottle, Assigned: fiveNinePlusR)

References

Details

(Whiteboard: [reader-mode-firefox-integration])

Attachments

(3 files, 9 obsolete files)

It would be cool to have an estimated reading time field on top of an article when read in Reader Mode.

For example, articles on Medium (e.g. https://medium.com/the-story/read-time-and-you-bc2048ab620c#.o6uf2ydhh) have an estimated reading time on top. Incidentally, the linked article also explains how they estimate it.
Component: General → Reader Mode
Product: Firefox → Toolkit
I'd happily take a patch but I don't think we'll prioritize this for working on otherwise.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P5
Whiteboard: [reader-mode-firefox-integration]
Assignee: nobody → fiveNinePlusR
How sophisticated should we be with the timing metric? Seems like there is some variability in speed based on languages:

http://iovs.arvojournals.org/article.aspx?articleid=2166061
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to fiveNinePlusR from comment #2)
> How sophisticated should we be with the timing metric? Seems like there is
> some variability in speed based on languages:
> 
> http://iovs.arvojournals.org/article.aspx?articleid=2166061

We do do some language detection already. It seems like detection based on number of characters could then use the speeds from table 2 in that article for the languages where we have that information, and fall back to English where we do not. I wouldn't trust our ability to detect words in different languages, and I also don't think the value of this feature would be worth the code complexity of writing specific word detection for all languages.
Flags: needinfo?(gijskruitbosch+bugs)
Attached patch bug1265304-reader_mode_time.diff (obsolete) — Splinter Review
First pass with just English support... just curious if those areas are where it should be implemented in your opinion. I will see about using that table data for other languages.
Attachment #8804835 - Flags: feedback?(gijskruitbosch+bugs)
Comment on attachment 8804835 [details] [diff] [review]
bug1265304-reader_mode_time.diff

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

::: toolkit/components/reader/AboutReader.jsm
@@ +742,5 @@
>    },
>  
> +  _formatReadTime: function(lowEstimate, highEstimate) {
> +    if (lowEstimate == highEstimate) {
> +      return highEstimate + " " + gStrings.GetStringFromName("aboutReader.readEstimateMinute");

You should use something like:

gStrings.formatStringFromName("aboutReader.readEstimateMinute", [highEstimate], 1);

(the array is an array of replacement values to go into the string, the last value is the length of that array because the API is old and dumb and C++ based)

Note that for this to work you'll need to update the string to include a "%S" for where you want the value to go.

@@ +745,5 @@
> +    if (lowEstimate == highEstimate) {
> +      return highEstimate + " " + gStrings.GetStringFromName("aboutReader.readEstimateMinute");
> +    }
> +
> +    return `${lowEstimate}-${highEstimate} ` + gStrings.GetStringFromName("aboutReader.readEstimateMinute");

You should then probably use a different string here, with an array of 2 parameters and two %S's in the string, because other languages might not use the - to delimit the range, or they might put the low estimate 'after' the high one (e.g. right-to-left languages).

::: toolkit/components/reader/ReaderMode.jsm
@@ +447,4 @@
>      article.title = Cc["@mozilla.org/parserutils;1"].getService(Ci.nsIParserUtils)
>                                                      .convertToPlainText(article.title, flags, 0);
>  
> +    [article.readTimeHigh, article.readTimeLow] = this._getReadTime(article.content);

The destructuring here is quite clever... but I wonder if it would make more sense to rename the _getReadTime helper to _assignReadTime, and to pass it the |article| object, and have the helper assign directly to the article.

@@ +504,5 @@
> +   *
> +   * @param length The length in words of the content.
> +   * @return Array
> +   */
> +  _getReadTime: function (article) {

I believe article.content is HTML - you probably need to use the parserutils, like in the line above the callsite, to convert to plaintext before determining the words-based reading time.

Nit: we can use ES6 for methods as well, ie:

_getReadTime(article) {

@@ +507,5 @@
> +   */
> +  _getReadTime: function (article) {
> +    const wordsPerMinuteLow = 228 - 30;
> +    const wordsPerMinuteHigh = 228 + 30;
> +    const length = article.split(" ").length;

I don't think this is an appropriate way of determining the number of words, and it also feels fairly performance-intensive (.split on a long string is not fun) for something so ephemeral.

Can we just use the character reading speed instead, and use article.length?

@@ +509,5 @@
> +    const wordsPerMinuteLow = 228 - 30;
> +    const wordsPerMinuteHigh = 228 + 30;
> +    const length = article.split(" ").length;
> +    const readingTimeSlow = (length / wordsPerMinuteLow).toFixed();
> +    const readingTimeFast = (length / wordsPerMinuteHigh).toFixed();

I would prefer using Math.ceil() instead of .toFixed(), so that the result is still a number, and so that we don't end up saying, for very short articles, that this is a "0" minute read. :-)

::: toolkit/components/reader/content/aboutReader.html
@@ +19,4 @@
>        <a id="reader-domain" class="domain"></a>
>        <div class="domain-border"></div>
>        <h1 id="reader-title"></h1>
> +      <h2 id="reader-estimated-time"></h2>

I think this should go underneath the byline, and it should probably be a <span> or <div> instead of an <h2>, as it isn't semantically a header (nor should it be really big)? We should just get a UI/UX review on the next iteration of your patch. :-)
Attachment #8804835 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
(Apologies for the delay, by the way - I was away yesterday. I generally try to get back to review requests within 1 working day.)
Attached patch bug1265304-patch.diff (obsolete) — Splinter Review
Attachment #8804835 - Attachment is obsolete: true
Attachment #8815564 - Flags: ui-review?
Attachment #8815564 - Flags: feedback?(gijskruitbosch+bugs)
This is a patch that does the language detection in a blocking fashion. Not sure the best approach to do it asynchronously though without a lot of work and extra complexity. Perhaps you have some insight Gijs?
Depends on: 1318605
Comment on attachment 8815564 [details] [diff] [review]
bug1265304-patch.diff

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

This looks pretty good. Bug 1318605 will add a language detection promise already, so we can piggyback on the code it adds for the next version. I expect it'll land again either today or tomorrow. I'll do a trypush in a second for the ui review and ping someone specific. The next patch (based on bug 1318605) can ask for review? instead of feedback. :-)

::: toolkit/components/reader/ReaderMode.jsm
@@ +532,5 @@
> +      const charactersPerMinuteHigh = readingSpeed.cpm + readingSpeed.variance;
> +      const length = article.length;
> +
> +      article.readingTimeSlow = Math.ceil(length / charactersPerMinuteLow);
> +      article.readingTimeFast  = Math.ceil(length / charactersPerMinuteHigh);

Nit: these variable names should probably include the unit. Maybe "readingTimeMinsSlow" and "readingTimeMinsFast" or something.

@@ +540,5 @@
> +  /**
> +   * Returns the reading speed of a selection of languages with likely variance.
> +   *
> +   * @return object with characters per minute and variance. Defaults to English
> +   *         if no suitable language is found in the collection.

The comment should probably explain where the data is sourced from.

@@ +560,5 @@
> +      [ "ru", {cpm: 986,  variance: 175 } ],
> +      [ "sk", {cpm: 885,  variance: 145 } ],
> +      [ "es", {cpm: 1025, variance: 127 } ],
> +      [ "sv", {cpm: 917,  variance: 156 } ],
> +      [ "tr", {cpm: 1054, variance: 156 } ]

Nit: please sort the list alphabetically, maybe with english specialcased at the top (or maybe not - I'm not 100% sure).

Nit: please add a trailing comma at the end.
Attachment #8815564 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
Comment on attachment 8815564 [details] [diff] [review]
bug1265304-patch.diff

Blake, can you ui-review the design / look-and-feel of this addition?
Attachment #8815564 - Flags: ui-review? → ui-review?(bwinton)
(I'm going to redirect this to someone else, but we're still deciding who.  ;)
No matter whom, a screenshot would speed things up!
Attached image reading time screen shot for ux review. (obsolete) —
I just noticed that there seems to be a regression in readability.js for the attribution byline. But this screenshot is for the estimated time UI approval and my feature didn't cause that.
Attachment #8815826 - Flags: ui-review?
Attached image Screen Shot 2016-11-30 at 11.40.12.png (obsolete) —
Thank you for the screenshot.

I have the feeling that we are missing a bit of hierarchy here.

My suggestion here is to create a meta-tags area where the date and the reading time can live happily ever after, with no competition with the author name.



What I did:

- reduce the margin-bottom on .credits (suggestion: 10px);

- create a new div for meta-data with a smaller font-size (suggestion: 0.65em)  and a 15px margin-bottom;

- add a <hr> element for a stronger separation with the actual content.
We could even throw a little clock icon in there!
Comment on attachment 8815564 [details] [diff] [review]
bug1265304-patch.diff

Redirecting to Emanuela…  :)
Attachment #8815564 - Flags: ui-review?(bwinton) → ui-review?(emanuela)
Comment on attachment 8815826 [details]
reading time screen shot for ux review.

Redirecting to Emanuela…  :)
Attachment #8815826 - Flags: ui-review? → ui-review?(emanuela)
Looking at the patch for bug 1318605 it brings the language detection outside of the ReaderMode.jsm and into the display controller AboutReader.jsm. Would it be acceptable to put the language detection, text direction, and reading time calculation into the _readerParse area of ReaderMode.jsm? I think there might be a race condition using promises inside the display controller(AboutReader) and I think it makes sense to do the article processing in the ReaderMode controller because that is where the raw HTML gets transformed into a readability article with all the associated attributes. 

If loading time is a concern we could send events to the display controller when the language is detected but I imagine that a synchronous solution where we do not return the article object until the reading direction and estimated time to read it (similar approach to the above patch) are attached to the article object would be sufficiently fast. Is telemetry data collected for this feature to confirm this?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to fiveNinePlusR from comment #19)
> Looking at the patch for bug 1318605 it brings the language detection
> outside of the ReaderMode.jsm and into the display controller
> AboutReader.jsm. Would it be acceptable to put the language detection, text
> direction, and reading time calculation into the _readerParse area of
> ReaderMode.jsm?

We could potentially do that, though we'd need to keep the checks to avoid doing this on Android.

> I think there might be a race condition using promises
> inside the display controller(AboutReader)

Can you clarify what kind of race conditions you're worried about?

> and I think it makes sense to do
> the article processing in the ReaderMode controller because that is where
> the raw HTML gets transformed into a readability article with all the
> associated attributes. 

This seems like a fair point, yes.

> If loading time is a concern we could send events to the display controller
> when the language is detected but I imagine that a synchronous solution
> where we do not return the article object until the reading direction and
> estimated time to read it (similar approach to the above patch) are attached
> to the article object would be sufficiently fast.

That's certainly possible. Have you checked the speed of the language detector? I don't really know how fast/slow it is...

> Is telemetry data
> collected for this feature to confirm this?

There used to be, but not anymore. All telemetry probes expire after a while to avoid us collecting loads of data unnecessarily. That's what happened to that probe, and then we removed the code for it as well...

TBH, I would be OK with some limited local testing if the additional checks don't take significantly more time. On Android, where CPU resources are more limited, we won't be doing this anyway.
Flags: needinfo?(gijskruitbosch+bugs)
> That's certainly possible. Have you checked the speed of the language detector? I don't really know how fast/slow it is...

I ran a few tests on my local machine (early 2011 MBP, 2.3GHz i7, 16GB RAM) and it seemed to consistently perform at the 20-150ms for language detection on the largest pages I could find. 

205,177 characters @ 113ms 2:25:00 read time
10k @ 51ms 0:10:00 read time
55k @ 158ms ~0:45:00 read time
19k @ 19ms, 
18k @ 22ms, 
127k @ 115ms load time 1:25:00 read time

Language detection speed seem to be only mildly related to page size in characters.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to fiveNinePlusR from comment #21)
> > That's certainly possible. Have you checked the speed of the language detector? I don't really know how fast/slow it is...
> 
> I ran a few tests on my local machine (early 2011 MBP, 2.3GHz i7, 16GB RAM)
> and it seemed to consistently perform at the 20-150ms for language detection
> on the largest pages I could find. 
> 
> 205,177 characters @ 113ms 2:25:00 read time
> 10k @ 51ms 0:10:00 read time
> 55k @ 158ms ~0:45:00 read time
> 19k @ 19ms, 
> 18k @ 22ms, 
> 127k @ 115ms load time 1:25:00 read time
> 
> Language detection speed seem to be only mildly related to page size in
> characters.

OK, this seems like we should be OK then. Thanks for checking!
Flags: needinfo?(gijskruitbosch+bugs)
Attached patch bug1265304-1.diff (obsolete) — Splinter Review
Attachment #8815564 - Attachment is obsolete: true
Attachment #8815826 - Attachment is obsolete: true
Attachment #8815829 - Attachment is obsolete: true
Attachment #8815841 - Attachment is obsolete: true
Attachment #8815564 - Flags: ui-review?(emanuela)
Attachment #8815826 - Flags: ui-review?(emanuela)
Attachment #8817096 - Flags: ui-review?(emanuela)
Attachment #8817096 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8817096 [details] [diff] [review]
bug1265304-1.diff

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

Code-wise this looks pretty good to me - thanks!

::: toolkit/components/reader/ReaderMode.jsm
@@ +534,5 @@
> +  },
> +
> +  _maybeAssignTextDirection(article) {
> +    // TODO: Remove the hardcoded language codes below once bug 1320265 is resolved.
> +    if (["ar", "fa", "he", "ug", "ur"].includes(this._foundLanguage)) {

This should check we don't already have an article.dir set.

@@ +587,5 @@
> +    ]);
> +
> +    return readingSpeed.get(lang) || readingSpeed.get("en");
> +  },
> +

Nit: no trailing blank line
Attachment #8817096 - Flags: review?(gijskruitbosch+bugs) → review+
Attached patch bug1265304-2.diff (obsolete) — Splinter Review
Attachment #8817096 - Attachment is obsolete: true
Attachment #8817096 - Flags: ui-review?(emanuela)
Attachment #8817273 - Flags: review+
Attachment #8817098 - Flags: ui-review?(emanuela)
Comment on attachment 8817098 [details]
Screenshot for UI Review

It looks good for me!
Attached patch bug1265304-3.diff (obsolete) — Splinter Review
Adds a few tests, respects the browser locale for displaying the reading time estimate, and fixes a few things from the last review.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=150452010e0fb48bfb878523792fb33ad63a8b97
Attachment #8817273 - Attachment is obsolete: true
Attachment #8817520 - Flags: review?(gijskruitbosch+bugs)
The language that the browser is in could be in a LTR language which is where the string translation would be pulled from even though the content of the page is RTL with a different language. We want to align the text with the content but keep the estimated time correctly oriented for the display language direction.
Attachment #8817533 - Flags: ui-review?(emanuela)
Comment on attachment 8817520 [details] [diff] [review]
bug1265304-3.diff

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

This looks good! Minor nits below, then this can land.

::: toolkit/components/reader/AboutReader.jsm
@@ +21,5 @@
>  var gStrings = Services.strings.createBundle("chrome://global/locale/aboutReader.properties");
>  
>  const gIsFirefoxDesktop = Services.appinfo.ID == "{ec8030f7-c20a-464f-9b0e-13a3a9e97384}";
> +var chromeReg = Components.classes["@mozilla.org/chrome/chrome-registry;1"].
> +    getService(Components.interfaces.nsIXULChromeRegistry);

You can use:

XPCOMUtils.defineLazyServiceGetter(this, "gChromeRegistry",
    "@mozilla.org/chrome/chrome-registry;1", Ci.nsIXULChromeRegistry);

::: toolkit/components/reader/ReaderMode.jsm
@@ +567,5 @@
> +   */
> +  _getReadingSpeedForLanguage(lang) {
> +    const readingSpeed = new Map([
> +      [ "en", {cpm: 987,  variance: 118 } ],
> +      [ "ar",	{cpm: 612,  variance: 88 } ],

Nit: odd spacing here.

::: toolkit/components/reader/test/browser_readerMode_readingTime.js
@@ +18,5 @@
> +    yield pageShownPromise;
> +    yield ContentTask.spawn(browser, null, function* () {
> +      // make sure there is a reading time on the page and that it displays the correct information
> +      let readingTimeElement = content.document.getElementById("reader-estimated-time");
> +      ok(readingTimeElement !== null, "Reading time element should be in document");

Nit: ok(readingTimeElement, "Reading time element should be in document");

@@ +19,5 @@
> +    yield ContentTask.spawn(browser, null, function* () {
> +      // make sure there is a reading time on the page and that it displays the correct information
> +      let readingTimeElement = content.document.getElementById("reader-estimated-time");
> +      ok(readingTimeElement !== null, "Reading time element should be in document");
> +      ok(readingTimeElement.innerText === "9-12 min", "Reading time should be '9-12 min'");

is(readingTimeElement.textContent, "9-12 min", ...);

@@ +38,5 @@
> +    yield ContentTask.spawn(browser, null, function* () {
> +      // make sure there is a reading time on the page and that it displays the correct information
> +      let readingTimeElement = content.document.getElementById("reader-estimated-time");
> +      ok(readingTimeElement !== null, "Reading time element should be in document");
> +      ok(readingTimeElement.innerText === "1 min", "Reading time should be '1 min'");

Same nits as above.
Attachment #8817520 - Flags: review?(gijskruitbosch+bugs) → review+
Attached patch bug1265304-4.diff (obsolete) — Splinter Review
Attachment #8817520 - Attachment is obsolete: true
Attachment #8817832 - Flags: review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a03728016ff
Adds an estimated time range to reader mode, r=gijs,ui-r=emanuela
Sorry, had to back this out for failing browser_narrate.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e7b1039e20c9dbf75028d57156c646597c9293f

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9a03728016ff7b76c4850375303e84cce00e4ac4
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=40468030&repo=mozilla-inbound

[task 2016-12-11T16:48:08.859988Z] 16:48:08     INFO - Entering test bound testNarrate
[task 2016-12-11T16:48:08.861880Z] 16:48:08     INFO - Buffered messages logged at 16:48:01
[task 2016-12-11T16:48:08.864576Z] 16:48:08     INFO - Console message: [JavaScript Error: "Error requesting favicon URL for about:reader content: favicon not found for uri" {file: "resource://app/modules/ReaderParent.jsm" line: 66}]
[task 2016-12-11T16:48:08.866170Z] 16:48:08     INFO - onRejection@resource://app/modules/ReaderParent.jsm:66:13
[task 2016-12-11T16:48:08.868504Z] 16:48:08     INFO - process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:935:21
[task 2016-12-11T16:48:08.871425Z] 16:48:08     INFO - walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
[task 2016-12-11T16:48:08.873145Z] 16:48:08     INFO - Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:11
[task 2016-12-11T16:48:08.874972Z] 16:48:08     INFO - schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7
[task 2016-12-11T16:48:08.880913Z] 16:48:08     INFO - completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:711:7
[task 2016-12-11T16:48:08.883431Z] 16:48:08     INFO - promiseFaviconLinkUrl/<@resource://gre/modules/PlacesUtils.jsm:1678:9
[task 2016-12-11T16:48:08.884711Z] 16:48:08     INFO - 
[task 2016-12-11T16:48:08.888024Z] 16:48:08     INFO - Buffered messages finished
[task 2016-12-11T16:48:08.889866Z] 16:48:08     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/narrate/test/browser_narrate.js | Uncaught exception -  - timed out after 50 tries.

Please fix the issue and submit an updated patch. Thank you.
Flags: needinfo?(fiveNinePlusR)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=95972e98861004a8e82aa630b82f5dca161cccb1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb422086d820273ecc93b632fa0a726672ca12bc

Not sure those treeherder jobs do enough tests but it should pass now as the tests pass locally after these changes.
Attachment #8817832 - Attachment is obsolete: true
Flags: needinfo?(fiveNinePlusR)
Attachment #8817937 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8817937 [details] [diff] [review]
bug1265304-5.diff

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

Makes sense!

I'll land this again with the nit below addressed...

Note that if this sticks this time, we should file a followup bug to get rid of the promise-based (this._foundLanguage()) infrastructure in AboutReader.jsm for the article language entirely, as the information is now guaranteed to be present already...

::: toolkit/components/reader/test/browser_readerMode_readingTime.js
@@ +19,5 @@
> +    yield ContentTask.spawn(browser, null, function* () {
> +      // make sure there is a reading time on the page and that it displays the correct information
> +      let readingTimeElement = content.document.getElementById("reader-estimated-time");
> +      ok(readingTimeElement, "Reading time element should be in document");
> +      ok(readingTimeElement.textContent === "9-12 min", "Reading time should be '9-12 min'");

You missed the same nit as for the last patch (which I fixed on checkin at the time), ie to use is() to compare these things - so rather than ok(foo == bar), use is(foo, bar), because that will ensure the test framework prints the expected and actual value (gives more information about what's going wrong if the test fails).
Attachment #8817937 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/81a0bc9f0fcd
Add an estimated time range to reader mode, r=gijs,ui-r=emanuela
https://hg.mozilla.org/mozilla-central/rev/81a0bc9f0fcd
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
> Note that if this sticks this time, we should file a followup bug to get rid of the promise-based (this._foundLanguage()) infrastructure in AboutReader.jsm for the article language entirely, as the information is now guaranteed to be present already...

I can take care of that. 

> You missed the same nit as for the last patch

Not sure why I didn't notice that change. Thanks for taking care of that!
As a general reference for the future, it would be nice to add a localization note when there are parameters involved (in particular aboutReader.estimatedReadTimeRange). It's also potentially confusing (min -> minimum, minutes?)

According to CLDR, the approach without plural form should be safe, but en-US should use 'min.'
http://www.unicode.org/cldr/charts/30/by_type/date_&_time.fields.html#16faa1db9d3012d

Maybe a native speaker could confirm that.
The abbreviation here doesn't seem necessary and we have the space to use the full "minutes" so we should use "minutes" instead. It will also remove the ambiguity.

fiveNinePlusR, can you file a follow-up to use "minutes" and add a localization note?
Flags: needinfo?(fiveNinePlusR)
If we use minutes we'll need to use a proper plural form for "%S minutes", while I'm not completely sure what we can do with "%S-%S minutes". Personally I would stick with an abbreviation.
Depends on: 1323415
This was UI-reviewed, so I'm not sure we really need to change much about the contents of the strings in use... but we should probably add localization comments. I filed bug 1323415 to deal with that and any potential l10n changes.

fiveNinePlusR, can you file the followup for comment #38 about this._foundLanguage? Thanks!
Gijs, The bug for comment 38 is: bug 1323331

Jared, I'll take care of the localization issues in bug 1323415
Flags: needinfo?(fiveNinePlusR)
Attachment #8817098 - Flags: ui-review?(emanuela) → ui-review+
Attachment #8817533 - Flags: ui-review?(emanuela) → ui-review+
Release Note Request (optional, but appreciated)
[Why is this notable]: Visible improvement to the Reader mode
[Affects Firefox for Android]: yes
[Suggested wording]: The Reader Mode now displays the estimated reading time for the page
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
This seems to be a good feature but could cause confusion to others. Please see bug 1334483
QA Contact: oana.horvath
I will take this feature as QA and I've created a test plan which you can find here: https://wiki.mozilla.org/QA/Fennec/Estimated_Reading_Time
This feature was verified on Fennec Aurora 53, based on the test plan located here: https://wiki.mozilla.org/QA/Fennec/Estimated_Reading_Time.
All tests have passed, on the following devices: 
Phones:
-Samsung Galaxy Note 4 (Android 5.0.1)
-LG G4 (Android 6.0)
-Lenovo A536 (Android 4.4.2)
Tablets:
-Asus Transformers Pad TF300T (Android 4.2.1)
-Huawei MediaPad M2 (Android 5.1.1)
-Asus ZenPad 8.0 Z380KL (Android 6.0.1)
-HTC Nexus 9 (Android 7.1.1)
Status: RESOLVED → VERIFIED
This feature was verified on 53.0a2 and 53.0b7 across the following platforms: Win 8.1 x64, Ubuntu 14.04 x64, Mac OS X 10.11. 
More details on testplan:
https://wiki.mozilla.org/QA/Estimated_Reading_Time
Extremely useful feature. Great work!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: