Last Comment Bug 1318605 - Do language detection for cases where we don't obtain a dir attribute
: Do language detection for cases where we don't obtain a dir attribute
Status: VERIFIED FIXED
:
Product: Toolkit
Classification: Components
Component: Reader Mode (show other bugs)
: unspecified
: Unspecified Unspecified
-- normal (vote)
: mozilla53
Assigned To: Evan Tseng [:evanxd]
:
: :Gijs
Mentors:
Depends on: 1173548
Blocks: 1265304
  Show dependency treegraph
 
Reported: 2016-11-18 00:12 PST by Evan Tseng [:evanxd]
Modified: 2017-02-20 08:10 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
WIP Patch (3.15 KB, patch)
2016-11-18 02:39 PST, Evan Tseng [:evanxd]
no flags Details | Diff | Splinter Review
Bug 1318605 - Do language detection to get text direction, (58 bytes, text/x-review-board-request)
2016-11-24 23:41 PST, Evan Tseng [:evanxd]
gijskruitbosch+bugs: review+
Details | Review

Description User image Evan Tseng [:evanxd] 2016-11-18 00:12:54 PST
> for cases where that does not obtain a dir attribute, in the reader page JS/HTML (not in the readability code itself, where we can do the other stuff), abstract the language detection that we're doing for narrate anyway out, and make it more accessible, then use that to determine a fallback rtl/ltr-ness as per comment #13 . [1]

Do language detection for cases don't obtain a dir attribute.

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1173548#c21
Comment 1 User image Evan Tseng [:evanxd] 2016-11-18 02:39:20 PST
Created attachment 8812126 [details] [diff] [review]
WIP Patch

It's the WIP patch. Next step, need to figure out how to determine text direction by language.
Comment 2 User image Evan Tseng [:evanxd] 2016-11-21 02:26:15 PST
> I mean that we should call LanguageDetector.jsm in reader mode code instead of in narrator code, and the result be saved somewhere where it is accessible to the narrator code - the narrator code can depend on the reader mode code but not vice versa.

Hi Gijs,

According to you said for cases where that does not obtain a dir attribute[1], I have another idea to instead of saving the language detector result somewhere, how about we just add new a param "language" for "NarrateControls.jsm" and "Narrator.jsm" objects? It will looks like,

1. Do language detection in "AboutReader.jsm" and assign the result into the "language" variable
2. new a "NarrateControls" and a "Narrator" objects with `new NarrateControls(mm, win, language)` and `new Narrator(win, language)`

I think the idea could be simple and clear. What do you think?

If we really want to do the saving result in somewhere idea, could you tell me more clues. Honestly, I don't know which place could be good enough one to save the result.

And one more question, how to get text direction of an article by knowing the language detection result? Do we need to do something like
```
LanguageDetector.detectLanguage(text).then(result => {
  let language = result.confident ? result.language : null;
  let dir = null;
  switch (language) {
    case "ar":
    case "he":
    case "fa":
    case "ug":
    case "ur":
    case "syr":
      dir = "rtl";
  }
});

```[2]

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1173548#c24
[2]: http://searchfox.org/mozilla-central/source/browser/base/content/utilityOverlay.js#637-646
Comment 3 User image :Gijs 2016-11-21 03:59:31 PST
(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #2)
> > I mean that we should call LanguageDetector.jsm in reader mode code instead of in narrator code, and the result be saved somewhere where it is accessible to the narrator code - the narrator code can depend on the reader mode code but not vice versa.
> 
> Hi Gijs,
> 
> According to you said for cases where that does not obtain a dir
> attribute[1], I have another idea to instead of saving the language detector
> result somewhere, how about we just add new a param "language" for
> "NarrateControls.jsm" and "Narrator.jsm" objects? It will looks like,
> 
> 1. Do language detection in "AboutReader.jsm" and assign the result into the
> "language" variable
> 2. new a "NarrateControls" and a "Narrator" objects with `new
> NarrateControls(mm, win, language)` and `new Narrator(win, language)`
> 
> I think the idea could be simple and clear. What do you think?

There are different ways we could expose the language to the narrate code, and this is one of them. But the language detection code returns a promise, so I don't know if it makes sense to wait for the promise to resolve before calling the narrate code, or if we should pass through the promise (or have the narrate code fetch the promise off a page global or whatever). Do what looks best to you.

> And one more question, how to get text direction of an article by knowing
> the language detection result? Do we need to do something like
> ```
> LanguageDetector.detectLanguage(text).then(result => {
>   let language = result.confident ? result.language : null;
>   let dir = null;
>   switch (language) {
>     case "ar":
>     case "he":
>     case "fa":
>     case "ug":
>     case "ur":
>     case "syr":
>       dir = "rtl";
>   }
> });
> 
> ```[2]
> 
> [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1173548#c24
> [2]:
> http://searchfox.org/mozilla-central/source/browser/base/content/
> utilityOverlay.js#637-646

I don't know. Flod?
Comment 4 User image Francesco Lodolo [:flod] 2016-11-21 04:15:34 PST
It doesn't look like a great idea to have hardcoded lists around, even if that piece of code definitely covers the RTL locales we have (ar, fa, he, ur).

Trying to switch this question to Pike, in case he's aware of a way to do get direction from locale better than
http://searchfox.org/mozilla-central/source/browser/base/content/utilityOverlay.js#637-646
Comment 5 User image Axel Hecht [:Pike] 2016-11-21 04:44:20 PST
You can check intl.uidirection.* prefs, which is what the chrome registry does.

Note, there are languages that exist in both LTR and RTL scripts (Kurdish, for one), so the mapping from language to direction isn't trivial.

It'd be nice if cld2 actually exposed which scripts it found, 'cause that's what it uses to hint at languages, but I don't see that ad-hoc.

You could probably mock that and just pick a few code points.

If this wasn't reader mode (and thus android, too), we could also think about using ICU, I guess. uscript_isRightToLeft exists as an API, at least.
Comment 6 User image Jonathan Kew (:jfkthame) 2016-11-21 05:04:44 PST
(In reply to Axel Hecht [:Pike] from comment #5)
> You can check intl.uidirection.* prefs, which is what the chrome registry
> does.

Not once bug 1312049 (re-)lands; those prefs are going away.
Comment 7 User image Evan Tseng [:evanxd] 2016-11-24 01:58:16 PST
According to Comment 5 and Comment 6, we have two options here.

1. We need to export the `uloc_isRightToLeft`[1](it is used in the patch of Bug 1312049) method for checking RTL.
2. We use the hardcode solution at this stage, kind of like we did in [2].

Since we might not know when the exporting solution could be done and resolved, I would say we could implement the hardcode solution first.

At the same time, we should file a new bug for exporting `isRightToLeft` method or other more suitable method. Once the bug is fixed, then we remove the hardcode solution.

What do you think?

[1]: http://searchfox.org/mozilla-central/source/intl/icu/source/common/unicode/uloc.h#897-912
[2]: http://searchfox.org/mozilla-central/source/browser/base/content/utilityOverlay.js#637-646
Comment 8 User image :Gijs 2016-11-24 02:14:14 PST
(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #7)
> According to Comment 5 and Comment 6, we have two options here.
> 
> 1. We need to export the `uloc_isRightToLeft`[1](it is used in the patch of
> Bug 1312049) method for checking RTL.
> 2. We use the hardcode solution at this stage, kind of like we did in [2].
> 
> Since we might not know when the exporting solution could be done and
> resolved, I would say we could implement the hardcode solution first.
> 
> At the same time, we should file a new bug for exporting `isRightToLeft`
> method or other more suitable method. Once the bug is fixed, then we remove
> the hardcode solution.
> 
> What do you think?
> 
> [1]:
> http://searchfox.org/mozilla-central/source/intl/icu/source/common/unicode/
> uloc.h#897-912
> [2]:
> http://searchfox.org/mozilla-central/source/browser/base/content/
> utilityOverlay.js#637-646

This plan sounds sensible to me.
Comment 9 User image Evan Tseng [:evanxd] 2016-11-24 23:41:40 PST Comment hidden (mozreview-request)
Comment 10 User image Evan Tseng [:evanxd] 2016-11-24 23:46:08 PST Comment hidden (mozreview-request)
Comment 11 User image Evan Tseng [:evanxd] 2016-11-24 23:50:27 PST
Hi Gijs,

I wrote a patch for the idea(Comment 3). Could you help review it? Thanks.
Comment 12 User image Evan Tseng [:evanxd] 2016-11-25 00:08:15 PST
I've filed the exporting bug, Bug 1320265.
Comment 13 User image Evan Tseng [:evanxd] 2016-11-25 00:45:59 PST Comment hidden (mozreview-request)
Comment 14 User image Evan Tseng [:evanxd] 2016-11-25 00:48:02 PST Comment hidden (mozreview-request)
Comment 15 User image :Gijs 2016-11-25 02:23:10 PST
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review95676

::: toolkit/components/reader/AboutReader.jsm:128
(Diff revision 4)
> +    if (win.document.body.classList.contains("loaded")) {
> +      detect();
> +    } else {
> +      win.document.addEventListener("AboutReaderContentReady", detect);
> +    }

This doesn't make sense. We know we're always going to run before `_loadArticle`, so the body's classList will never contain "loaded". It also doesn't really make sense that we're listening for our own event here.

It looks like because we're constructing the narrate controls here, we need the promise to be ready already. But what I think would be clearer is to do something like this:

```js
this._languagePromise = new Promise(resolve => {
  this._foundLanguage = resolve;
});
```

here, and then in `_showContent`, call a new method called something like `_findLanguage`, that runs the language detection code after we've appended the main article text, calls `_foundLanguage()` with the language or null as you're doing here, and maybe saves it somewhere convenient for the directionality code to use.
Comment 16 User image Evan Tseng [:evanxd] 2016-11-25 03:18:10 PST
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review95676

> This doesn't make sense. We know we're always going to run before `_loadArticle`, so the body's classList will never contain "loaded". It also doesn't really make sense that we're listening for our own event here.
> 
> It looks like because we're constructing the narrate controls here, we need the promise to be ready already. But what I think would be clearer is to do something like this:
> 
> ```js
> this._languagePromise = new Promise(resolve => {
>   this._foundLanguage = resolve;
> });
> ```
> 
> here, and then in `_showContent`, call a new method called something like `_findLanguage`, that runs the language detection code after we've appended the main article text, calls `_foundLanguage()` with the language or null as you're doing here, and maybe saves it somewhere convenient for the directionality code to use.

There are two things here.

1. You're right. It(`win.document.body.classList`) will never cintain "loaded". So let's remove it.
2. In the patch, I just follow current code base logic to do language detection at same timing(before `this._loadArticle()`) as current code base (do language detection in `Narrator`[1]). So my question is why cannot we just follow same logic like we do now in code base and listen `AboutReaderContentReady` there? If we can follow it, I would say the solution in the patch is quite simple and OK. Does it make sense?

[1]: http://searchfox.org/mozilla-central/source/toolkit/components/narrate/Narrator.jsm#34-49
Comment 17 User image :Gijs 2016-11-25 03:24:03 PST
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review95676

> There are two things here.
> 
> 1. You're right. It(`win.document.body.classList`) will never cintain "loaded". So let's remove it.
> 2. In the patch, I just follow current code base logic to do language detection at same timing(before `this._loadArticle()`) as current code base (do language detection in `Narrator`[1]). So my question is why cannot we just follow same logic like we do now in code base and listen `AboutReaderContentReady` there? If we can follow it, I would say the solution in the patch is quite simple and OK. Does it make sense?
> 
> [1]: http://searchfox.org/mozilla-central/source/toolkit/components/narrate/Narrator.jsm#34-49

Because we don't actually do the detection here, we listen for an event here, and when the event fires, we do the language detection. This made sense when the code lived in the narrate module, because the narrate module had no other way of knowing when reader mode inserted the article content.

However, now we are moving the language detection code into reader mode code directly. That code is itself responsible for inserting the HTML that we then do language detection on. It would be needlessly complex for the code here to rely on an event that it fires itself, when it could just do the right thing immediately after having inserted the article text. The patch would be smaller and simpler if we could determine the language immediately - also because it avoids having to use the promise when determining the article direction (which is async, and might lead to a flash of wrongly-directed content).
Comment 18 User image Evan Tseng [:evanxd] 2016-11-27 23:16:13 PST Comment hidden (mozreview-request)
Comment 19 User image Evan Tseng [:evanxd] 2016-11-27 23:35:06 PST Comment hidden (mozreview-request)
Comment 20 User image Evan Tseng [:evanxd] 2016-11-27 23:47:50 PST
Updated patch for review comments.
Comment 21 User image Evan Tseng [:evanxd] 2016-11-27 23:58:18 PST
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review95676

> Because we don't actually do the detection here, we listen for an event here, and when the event fires, we do the language detection. This made sense when the code lived in the narrate module, because the narrate module had no other way of knowing when reader mode inserted the article content.
> 
> However, now we are moving the language detection code into reader mode code directly. That code is itself responsible for inserting the HTML that we then do language detection on. It would be needlessly complex for the code here to rely on an event that it fires itself, when it could just do the right thing immediately after having inserted the article text. The patch would be smaller and simpler if we could determine the language immediately - also because it avoids having to use the promise when determining the article direction (which is async, and might lead to a flash of wrongly-directed content).

Got it. Make sense, let's not depend on `AboutReaderContentReady` event. But we seems cannot determin the language without using a `promise`. The `LanguageDetector.detectLanguage` method just returns a `promise`.

How about we add a `callback` param into `_loadArticle` method, then we could do something like below to initialize the `NarrateControls` object?
```
this._loadArticle((article, languagePromise) => {
  if (win.speechSynthesis && Services.prefs.getBoolPref("narrate.enabled")) {
    new NarrateControls(mm, win, languagePromise);
  }
});
```

I've updated patch for the idea. Could you help review it? Thanks.
Comment 22 User image :Gijs 2016-11-28 02:41:02 PST
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review95916

::: toolkit/components/reader/AboutReader.jsm:642
(Diff revision 6)
>        this._showError();
>        return;
>      }
>  
> -    this._showContent(article);
> +    var languagePromise = new Promise(resolve => {
> +      LanguageDetector.detectLanguage(article.content).then(result => {

article.content is the un-security-checked HTML version of the content. We should be passing the language promise the text content, not the HTML.

I suggested in comment 15 how to deal with the promise. Create a promise, and store the `resolve` method on `this`, then call it after determining the language. If the language determination is async itself, you can simply add another `.then` handler to its promise that resolves the promise we passed to the narrate code. This avoids having to pass along a callback through several function signatures.

::: toolkit/components/reader/AboutReader.jsm:732
(Diff revision 6)
> +        switch (language) {
> +          case "ar":
> +          case "fa":
> +          case "he":
> +          case "ug":
> +          case "ur":
> +            this._contentElement.setAttribute("dir", "rtl");
> +            this._headerElement.setAttribute("dir", "rtl");

```js
if (["ar", "fa", "he", "ug", "ur"].includes(language)) {
  ...
}
```

Have you checked whether the language detection code returns only a language code for these languages and not things like `ar-IR` or whatever? We might need to post-process for this matching.
Comment 23 User image Evan Tseng [:evanxd] 2016-11-28 23:10:46 PST Comment hidden (mozreview-request)
Comment 24 User image Evan Tseng [:evanxd] 2016-11-28 23:22:20 PST
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review95916

> article.content is the un-security-checked HTML version of the content. We should be passing the language promise the text content, not the HTML.
> 
> I suggested in comment 15 how to deal with the promise. Create a promise, and store the `resolve` method on `this`, then call it after determining the language. If the language determination is async itself, you can simply add another `.then` handler to its promise that resolves the promise we passed to the narrate code. This avoids having to pass along a callback through several function signatures.

Sure, got the idea. Misunderstood it previously. Already updated the patch for it.

> ```js
> if (["ar", "fa", "he", "ug", "ur"].includes(language)) {
>   ...
> }
> ```
> 
> Have you checked whether the language detection code returns only a language code for these languages and not things like `ar-IR` or whatever? We might need to post-process for this matching.

We imported Compact Language Library to do language detection on Bug 971047. I checked the source code we imported at that time, `enerated_language.cc`[1] lists all language codes it can detect. It doesn't have something like `ar-IR`, it contains `ar`, `fa`, `he`, `ug`, and `ur`. So I think the code is good here.

[1]: http://searchfox.org/mozilla-central/source/browser/components/translation/cld2/internal/generated_language.cc#648-1261
Comment 25 User image Evan Tseng [:evanxd] 2016-11-28 23:22:45 PST
Updated patch for review comments.
Comment 26 User image Evan Tseng [:evanxd] 2016-11-28 23:27:26 PST Comment hidden (mozreview-request)
Comment 27 User image :Gijs 2016-11-29 02:12:58 PST
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review96434

::: toolkit/components/reader/AboutReader.jsm:726
(Diff revision 8)
> -    // Set "dir" attribute on content
> +      // Set "dir" attribute on content
> -    this._contentElement.setAttribute("dir", article.dir);
> +      this._contentElement.setAttribute("dir", article.dir);
> -    this._headerElement.setAttribute("dir", article.dir);
> +      this._headerElement.setAttribute("dir", article.dir);
> +    } else {
> +      this._languagePromise.then(language => {
> +        // TODO: Remove the below hard-code language codes once Bug 1320265 is resolved.

Nit: this comment would be slightly more idiomatic English if written like this:

// TODO: Remove the hardcoded language codes below once bug 1320265 is resolved.
Comment 28 User image Evan Tseng [:evanxd] 2016-11-29 02:21:06 PST Comment hidden (mozreview-request)
Comment 29 User image Evan Tseng [:evanxd] 2016-11-29 02:22:30 PST
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review96434

> Nit: this comment would be slightly more idiomatic English if written like this:
> 
> // TODO: Remove the hardcoded language codes below once bug 1320265 is resolved.

Sure, updated patch for it. Thanks.
Comment 30 User image Evan Tseng [:evanxd] 2016-11-29 02:24:26 PST
I've fixed the nit and pushed a new try for it. If everything is good, please help land the patch. Thanks.
Comment 31 User image Pulsebot 2016-11-29 07:47:27 PST
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cbde1c9e78d5
Do language detection to get text direction, r=Gijs
Comment 32 User image Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2016-11-29 09:28:41 PST
Backed out for failing test_reader_view.html and test_session_scroll_position.html on Android:

https://hg.mozilla.org/integration/autoland/rev/52a5ff34b3b36e789ed21c2665889b8833ec61b0

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=cbde1c9e78d55907934892db41c6d2d5ac32a5e5
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=7416448&repo=autoland

quote Gijs "we need to not use LanguageDetector on Android (it's desktop-only)"
Comment 33 User image Evan Tseng [:evanxd] 2016-11-29 19:51:47 PST
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #32)
> Backed out for failing test_reader_view.html and
> test_session_scroll_position.html on Android:
> 
> https://hg.mozilla.org/integration/autoland/rev/
> 52a5ff34b3b36e789ed21c2665889b8833ec61b0
> 
> Push with failures:
> https://treeherder.mozilla.org/#/
> jobs?repo=autoland&revision=cbde1c9e78d55907934892db41c6d2d5ac32a5e5
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=7416448&repo=autoland
> 
> quote Gijs "we need to not use LanguageDetector on Android (it's
> desktop-only)"

Hi Gijs,

Do we want to make `LanguageDetector` run on Android here?(I even don't know why LanguageDetector cannot run on Android?)

If so, let's file a new bug to do it. Or we just need to figure out a new way to get language code of an article?
Comment 34 User image :Gijs 2016-11-30 02:00:25 PST
(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #33)
> (In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on
> intermittent or backout) from comment #32)
> > Backed out for failing test_reader_view.html and
> > test_session_scroll_position.html on Android:
> > 
> > https://hg.mozilla.org/integration/autoland/rev/
> > 52a5ff34b3b36e789ed21c2665889b8833ec61b0
> > 
> > Push with failures:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=autoland&revision=cbde1c9e78d55907934892db41c6d2d5ac32a5e5
> > Failure log:
> > https://treeherder.mozilla.org/logviewer.html#?job_id=7416448&repo=autoland
> > 
> > quote Gijs "we need to not use LanguageDetector on Android (it's
> > desktop-only)"
> 
> Hi Gijs,
> 
> Do we want to make `LanguageDetector` run on Android here?(I even don't know
> why LanguageDetector cannot run on Android?)

I don't know. It'll impact apk size. It's a decision for the android team, I guess. We can file a separate bug. For now, we should make sure that the code here doesn't use LanguageDetector if on android.
Comment 35 User image Evan Tseng [:evanxd] 2016-11-30 19:50:32 PST Comment hidden (mozreview-request)
Comment 36 User image Evan Tseng [:evanxd] 2016-11-30 22:23:10 PST
Got it. I've updated patch to do platform detection to ensure we only do language detection on Firefox desktop.

Please help review the code. If everything is OK, please help land it. Thanks.
Comment 37 User image Evan Tseng [:evanxd] 2016-11-30 22:33:26 PST
File a new bug(Bug 1321477) to determine language code of a webpage on Fennec.
Comment 38 User image :Gijs 2016-12-01 02:12:14 PST
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review97074

::: toolkit/components/reader/AboutReader.jsm:20
(Diff revision 10)
>  XPCOMUtils.defineLazyModuleGetter(this, "AsyncPrefs", "resource://gre/modules/AsyncPrefs.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "NarrateControls", "resource://gre/modules/narrate/NarrateControls.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "Rect", "resource://gre/modules/Geometry.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "Task", "resource://gre/modules/Task.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "UITelemetry", "resource://gre/modules/UITelemetry.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "XulApp", "resource://gre/modules/commonjs/sdk/system/xul-app.jsm");

Where did this code come from? We shouldn't be using add-on SDK modules for this in Firefox/Toolkit code.

Instead, reuse `gIsFirefoxDesktop`, defined a few lines below.

::: toolkit/components/reader/AboutReader.jsm:821
(Diff revision 10)
>      this._doc.dispatchEvent(
>        new this._win.CustomEvent("AboutReaderContentReady", { bubbles: true, cancelable: false }));
>    },
>  
> +  _findLanguage: function(textContent) {
> +    XulApp.is("Firefox") && LanguageDetector.detectLanguage(textContent).then(result => {

Nit:

```js
if (gIsFirefoxDesktop) {
  LanguageDetector...
}
Comment 39 User image :Gijs 2016-12-01 02:12:56 PST
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review97074

> Where did this code come from? We shouldn't be using add-on SDK modules for this in Firefox/Toolkit code.
> 
> Instead, reuse `gIsFirefoxDesktop`, defined a few lines below.

To be clear, you'll need to pull that constant out into global scope.
Comment 40 User image Evan Tseng [:evanxd] 2016-12-01 06:04:07 PST Comment hidden (mozreview-request)
Comment 41 User image Evan Tseng [:evanxd] 2016-12-01 06:07:04 PST
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review97074

> To be clear, you'll need to pull that constant out into global scope.

Sure, I've updated for that.

> Nit:
> 
> ```js
> if (gIsFirefoxDesktop) {
>   LanguageDetector...
> }

Sure.
Comment 42 User image Evan Tseng [:evanxd] 2016-12-01 06:08:26 PST
I've updated patch for review comments. Please help review it again. Thanks.
Comment 43 User image :Gijs 2016-12-01 07:21:42 PST
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review97144

Nice, thanks!
Comment 44 User image Pulsebot 2016-12-01 07:22:42 PST
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/64cbe9e0ac0a
Do language detection to get text direction, r=Gijs
Comment 45 User image Wes Kocher (:KWierso) 2016-12-01 15:08:56 PST
https://hg.mozilla.org/mozilla-central/rev/64cbe9e0ac0a
Comment 46 User image Oana Horvath 2017-02-20 08:10:36 PST
Verified on Aurora 53.0a2, with several devices, the estimated reading time follows the text direction, based on text language.

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