Last Comment Bug 1265304 - Display estimated reading time in Reader Mode
: Display estimated reading time in Reader Mode
Status: RESOLVED FIXED
[reader-mode-firefox-integration]
:
Product: Toolkit
Classification: Components
Component: Reader Mode (show other bugs)
: unspecified
: All All
P5 enhancement with 1 vote (vote)
: mozilla53
Assigned To: fiveNinePlusR
: Oana Horvath
: :Gijs
Mentors:
Depends on: 1318605 1323415
Blocks:
  Show dependency treegraph
 
Reported: 2016-04-18 01:04 PDT by Federico 'klez' Culloca
Modified: 2017-02-13 08:03 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
53+


Attachments
bug1265304-reader_mode_time.diff (5.00 KB, patch)
2016-10-26 11:17 PDT, fiveNinePlusR
gijskruitbosch+bugs: feedback+
Details | Diff | Splinter Review
bug1265304-patch.diff (7.05 KB, patch)
2016-11-29 19:20 PST, fiveNinePlusR
gijskruitbosch+bugs: feedback+
Details | Diff | Splinter Review
reading time screen shot for ux review. (242.37 KB, image/png)
2016-11-30 11:02 PST, fiveNinePlusR
no flags Details
What the reader page looks like on FF50 for the same url. (258.03 KB, image/png)
2016-11-30 11:04 PST, fiveNinePlusR
no flags Details
Screen Shot 2016-11-30 at 11.40.12.png (454.86 KB, image/png)
2016-11-30 11:41 PST, emanuela [ux team]
no flags Details
bug1265304-1.diff (11.95 KB, patch)
2016-12-06 18:59 PST, fiveNinePlusR
gijskruitbosch+bugs: review+
Details | Diff | Splinter Review
Screenshot for UI Review (186.11 KB, image/png)
2016-12-06 19:00 PST, fiveNinePlusR
fiveNinePlusR: ui‑review+
Details
bug1265304-2.diff (11.96 KB, patch)
2016-12-07 15:18 PST, fiveNinePlusR
fiveNinePlusR: review+
Details | Diff | Splinter Review
bug1265304-3.diff (17.21 KB, patch)
2016-12-08 16:38 PST, fiveNinePlusR
gijskruitbosch+bugs: review+
Details | Diff | Splinter Review
Screenshot review for RTL content with LTR chrome locale (54.48 KB, image/png)
2016-12-08 17:57 PST, fiveNinePlusR
fiveNinePlusR: ui‑review+
Details
bug1265304-4.diff (17.04 KB, patch)
2016-12-10 15:11 PST, fiveNinePlusR
fiveNinePlusR: review+
Details | Diff | Splinter Review
bug1265304-5.diff (17.11 KB, patch)
2016-12-11 21:35 PST, fiveNinePlusR
gijskruitbosch+bugs: review+
Details | Diff | Splinter Review

Description User image Federico 'klez' Culloca 2016-04-18 01:04:24 PDT
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.
Comment 1 User image :Gijs 2016-07-14 07:47:16 PDT
I'd happily take a patch but I don't think we'll prioritize this for working on otherwise.
Comment 2 User image fiveNinePlusR 2016-10-24 16:54:16 PDT
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
Comment 3 User image :Gijs 2016-10-26 03:26:02 PDT
(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.
Comment 4 User image fiveNinePlusR 2016-10-26 11:17:12 PDT
Created attachment 8804835 [details] [diff] [review]
bug1265304-reader_mode_time.diff

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.
Comment 5 User image :Gijs 2016-10-28 02:37:38 PDT
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. :-)
Comment 6 User image :Gijs 2016-10-28 02:41:57 PDT
(Apologies for the delay, by the way - I was away yesterday. I generally try to get back to review requests within 1 working day.)
Comment 7 User image fiveNinePlusR 2016-11-29 19:20:06 PST
Created attachment 8815564 [details] [diff] [review]
bug1265304-patch.diff
Comment 8 User image fiveNinePlusR 2016-11-29 19:22:26 PST
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?
Comment 9 User image :Gijs 2016-11-30 02:09:44 PST
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.
Comment 10 User image :Gijs 2016-11-30 02:24:11 PST
Comment on attachment 8815564 [details] [diff] [review]
bug1265304-patch.diff

Blake, can you ui-review the design / look-and-feel of this addition?
Comment 11 User image Blake Winton (:bwinton) (:☕️) 2016-11-30 09:09:16 PST
(I'm going to redirect this to someone else, but we're still deciding who.  ;)
Comment 12 User image Madhava Enros [:madhava] 2016-11-30 10:38:07 PST
No matter whom, a screenshot would speed things up!
Comment 13 User image fiveNinePlusR 2016-11-30 11:02:50 PST
Created attachment 8815826 [details]
reading time screen shot for ux review.

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.
Comment 14 User image fiveNinePlusR 2016-11-30 11:04:43 PST
Created attachment 8815829 [details]
What the reader page looks like on FF50 for the same url.
Comment 15 User image emanuela [ux team] 2016-11-30 11:41:42 PST
Created attachment 8815841 [details]
Screen Shot 2016-11-30 at 11.40.12.png

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.
Comment 16 User image Philip [:pwalm] 2016-11-30 11:45:09 PST
We could even throw a little clock icon in there!
Comment 17 User image Blake Winton (:bwinton) (:☕️) 2016-11-30 11:50:13 PST
Comment on attachment 8815564 [details] [diff] [review]
bug1265304-patch.diff

Redirecting to Emanuela…  :)
Comment 18 User image Blake Winton (:bwinton) (:☕️) 2016-11-30 11:50:33 PST
Comment on attachment 8815826 [details]
reading time screen shot for ux review.

Redirecting to Emanuela…  :)
Comment 19 User image fiveNinePlusR 2016-12-02 16:56:42 PST
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?
Comment 20 User image :Gijs 2016-12-05 09:17:22 PST
(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.
Comment 21 User image fiveNinePlusR 2016-12-05 15:26:18 PST
> 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.
Comment 22 User image :Gijs 2016-12-06 10:33:09 PST
(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!
Comment 23 User image fiveNinePlusR 2016-12-06 18:59:09 PST
Created attachment 8817096 [details] [diff] [review]
bug1265304-1.diff
Comment 24 User image fiveNinePlusR 2016-12-06 19:00:36 PST
Created attachment 8817098 [details]
Screenshot for UI Review
Comment 25 User image :Gijs 2016-12-07 12:15:31 PST
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
Comment 26 User image fiveNinePlusR 2016-12-07 15:18:32 PST
Created attachment 8817273 [details] [diff] [review]
bug1265304-2.diff
Comment 27 User image emanuela [ux team] 2016-12-08 11:07:15 PST
Comment on attachment 8817098 [details]
Screenshot for UI Review

It looks good for me!
Comment 28 User image fiveNinePlusR 2016-12-08 16:38:32 PST
Created attachment 8817520 [details] [diff] [review]
bug1265304-3.diff

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
Comment 29 User image fiveNinePlusR 2016-12-08 17:57:36 PST
Created attachment 8817533 [details]
Screenshot review for RTL content with LTR chrome locale

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.
Comment 30 User image :Gijs 2016-12-09 20:10:35 PST
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.
Comment 31 User image fiveNinePlusR 2016-12-10 15:11:16 PST
Created attachment 8817832 [details] [diff] [review]
bug1265304-4.diff
Comment 32 User image Pulsebot 2016-12-11 07:52:02 PST
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
Comment 33 User image Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2016-12-11 09:07:10 PST
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.
Comment 34 User image fiveNinePlusR 2016-12-11 21:35:24 PST
Created attachment 8817937 [details] [diff] [review]
bug1265304-5.diff

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.
Comment 35 User image :Gijs 2016-12-13 02:27:08 PST
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).
Comment 36 User image Pulsebot 2016-12-13 02:30:08 PST
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
Comment 37 User image Carsten Book [:Tomcat] 2016-12-13 07:46:39 PST
https://hg.mozilla.org/mozilla-central/rev/81a0bc9f0fcd
Comment 38 User image fiveNinePlusR 2016-12-13 13:49:12 PST
> 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!
Comment 39 User image Francesco Lodolo [:flod] 2016-12-13 22:42:15 PST
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.
Comment 40 User image Jared Wein [:jaws] (please needinfo? me) 2016-12-13 22:58:10 PST
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?
Comment 41 User image Francesco Lodolo [:flod] 2016-12-13 23:26:50 PST
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.
Comment 42 User image :Gijs 2016-12-14 03:23:44 PST
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!
Comment 43 User image fiveNinePlusR 2016-12-14 10:49:31 PST
Gijs, The bug for comment 38 is: bug 1323331

Jared, I'll take care of the localization issues in bug 1323415
Comment 44 User image Pascal Chevrel:pascalc 2017-01-20 03:07:37 PST
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)]:
Comment 45 User image Kaartic Sivaraam 2017-01-27 06:52:46 PST
This seems to be a good feature but could cause confusion to others. Please see bug 1334483
Comment 46 User image Ritu Kothari (:ritu) 2017-01-28 15:26:20 PST
Added to Fx53 Aurora release notes.
Comment 47 User image Oana Horvath 2017-02-13 08:03:30 PST
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

Note You need to log in before you can comment on or make changes to this bug.