Modifying language detection code

Modifying language detection code

If the language detection model CLD3 predicts a transliterated language that is
currently not supported, then the prediction is ignored.

For more information, please see the bug entry below.

BUG=649002
Committed: https://crrev.com/52247dcede791eae0c75996335a9b2bb0d9df236
Cr-Commit-Position: refs/heads/master@{#421451}

[Anton Bakalov, 2016-09-21 16:13:49]

The CQ bit was checked by abakalov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 16:14:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Anton Bakalov, 2016-09-21 16:20:29]

abakalov@chromium.org changed reviewers:
+ andrewhayden@chromium.org

[Andrew Hayden (chromium.org), 2016-09-21 16:43:08]

Please add a BUG=, and describe the main points of the e-mail thread where this
was agreed upon.

[Anton Bakalov, 2016-09-21 16:43:46]

Description was changed from

==========
Modifying language_detection_util.cc, so that if CLD3 predicts a
transliterated language that is currently not supported, then the
prediction is ignored.

BUG=
==========

to

==========
Modifying language_detection_util.cc, so that if CLD3 predicts a
transliterated language that is currently not supported, then the
prediction is ignored.

BUG=649002
==========

[Anton Bakalov, 2016-09-21 16:50:44]

Description was changed from

==========
Modifying language_detection_util.cc, so that if CLD3 predicts a
transliterated language that is currently not supported, then the
prediction is ignored.

BUG=649002
==========

to

==========
Modifying language_detection_util.cc, so that if CLD3 predicts a
transliterated language that is currently not supported, then the
prediction is ignored.

For more information, please see the bug entry below.

BUG=649002
==========

[commit-bot: I haz the power, 2016-09-21 17:06:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 17:06:31]

Dry run: This issue passed the CQ dry run.

[Andrew Hayden (chromium.org), 2016-09-22 12:11:32]

LGTM

[Anton Bakalov, 2016-09-27 17:24:29]

The CQ bit was checked by abakalov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 17:24:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Anton Bakalov, 2016-09-27 17:31:45]

Description was changed from

==========
Modifying language_detection_util.cc, so that if CLD3 predicts a
transliterated language that is currently not supported, then the
prediction is ignored.

For more information, please see the bug entry below.

BUG=649002
==========

to

==========
Modifying code related to language detection, so that if CLD3 predicts a
transliterated language that is currently not supported, then the
prediction is ignored.

For more information, please see the bug entry below.

BUG=649002
==========

[Anton Bakalov, 2016-09-27 17:57:44]

abakalov@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2016-09-27 18:13:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 18:13:50]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-09-27 18:34:43]

https://codereview.chromium.org/2359793003/diff/20001/extensions/renderer/i18...
File extensions/renderer/i18n_custom_bindings.cc (right):

https://codereview.chromium.org/2359793003/diff/20001/extensions/renderer/i18...
extensions/renderer/i18n_custom_bindings.cc:138: detected_languages->clear();
Isn't this always the case?  Could we DCHECK(detected_languages->empty())
instead?

https://codereview.chromium.org/2359793003/diff/20001/extensions/renderer/i18...
extensions/renderer/i18n_custom_bindings.cc:154: // Ignore languages that are
currently not supported.
Can we expand this comment to include why these aren't supported (and under
which circumstances they would become supported) and where this list came from?

[Anton Bakalov, 2016-09-27 19:57:03]

The CQ bit was checked by abakalov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 19:57:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Anton Bakalov, 2016-09-27 20:00:48]

Thanks for the good suggestions, Devlin!

https://codereview.chromium.org/2359793003/diff/20001/extensions/renderer/i18...
File extensions/renderer/i18n_custom_bindings.cc (right):

https://codereview.chromium.org/2359793003/diff/20001/extensions/renderer/i18...
extensions/renderer/i18n_custom_bindings.cc:138: detected_languages->clear();
On 2016/09/27 18:34:42, Devlin (catching up) wrote:
> Isn't this always the case?  Could we DCHECK(detected_languages->empty())
> instead?

Done.

https://codereview.chromium.org/2359793003/diff/20001/extensions/renderer/i18...
extensions/renderer/i18n_custom_bindings.cc:154: // Ignore languages that are
currently not supported.
On 2016/09/27 18:34:42, Devlin (catching up) wrote:
> Can we expand this comment to include why these aren't supported (and under
> which circumstances they would become supported) and where this list came
from?

Done.

[commit-bot: I haz the power, 2016-09-27 20:40:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 20:40:10]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-09-27 22:34:28]

lgtm % comment nit

https://codereview.chromium.org/2359793003/diff/40001/extensions/renderer/i18...
File extensions/renderer/i18n_custom_bindings.cc (right):

https://codereview.chromium.org/2359793003/diff/40001/extensions/renderer/i18...
extensions/renderer/i18n_custom_bindings.cc:158: // To maintain backwards
compatibility, xx-Latn predictions are ignored
We should still provide more detail here.  Why does a -Latn suffix break
existing functionality?  The goal here is to have a reader understand (at least
mostly) the motivation and what would change in the future.

If you prefer, you can also file a bug at crbug.com and link to it here.

[Devlin, 2016-09-27 22:37:12]

Also, please update the CL description.

Descriptions in Rietveld:
[First Line - becomes the title]

[CL Body]

BUG=[Bug number]



So as it stands now, your commit title would be
"Modifying code related to language detection, so that if CLD3 predicts a"

Each line should also be wrapped at 72 char for git friendliness.

[Anton Bakalov, 2016-09-28 03:22:46]

Description was changed from

==========
Modifying code related to language detection, so that if CLD3 predicts a
transliterated language that is currently not supported, then the
prediction is ignored.

For more information, please see the bug entry below.

BUG=649002
==========

to

==========
Modifying language detection code

If the language detection model CLD3 predicts a transliterated language that is 
currently not supported, then the prediction is ignored.

For more information, please see the bug entry below.

BUG=649002
==========

[Anton Bakalov, 2016-09-28 04:21:57]

Thanks, Devlin!

[Anton Bakalov, 2016-09-28 04:23:27]

https://codereview.chromium.org/2359793003/diff/40001/extensions/renderer/i18...
File extensions/renderer/i18n_custom_bindings.cc (right):

https://codereview.chromium.org/2359793003/diff/40001/extensions/renderer/i18...
extensions/renderer/i18n_custom_bindings.cc:158: // To maintain backwards
compatibility, xx-Latn predictions are ignored
On 2016/09/27 22:34:28, Devlin (OOO until Thursday) wrote:
> We should still provide more detail here.  Why does a -Latn suffix break
> existing functionality?  The goal here is to have a reader understand (at
least
> mostly) the motivation and what would change in the future.
> 
> If you prefer, you can also file a bug at http://crbug.com and link to it
here.

Done.

[Anton Bakalov, 2016-09-28 04:25:16]

The CQ bit was checked by abakalov@chromium.org

[Anton Bakalov, 2016-09-28 04:25:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
andrewhayden@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2359793003/#ps60001
(title: "Extending the comment about xx-Latn languages")

[commit-bot: I haz the power, 2016-09-28 04:25:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 05:07:36]

Description was changed from

==========
Modifying language detection code

If the language detection model CLD3 predicts a transliterated language that is 
currently not supported, then the prediction is ignored.

For more information, please see the bug entry below.

BUG=649002
==========

to

==========
Modifying language detection code

If the language detection model CLD3 predicts a transliterated language that is 
currently not supported, then the prediction is ignored.

For more information, please see the bug entry below.

BUG=649002
==========

[commit-bot: I haz the power, 2016-09-28 05:07:37]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-28 05:09:08]

Description was changed from

==========
Modifying language detection code

If the language detection model CLD3 predicts a transliterated language that is 
currently not supported, then the prediction is ignored.

For more information, please see the bug entry below.

BUG=649002
==========

to

==========
Modifying language detection code

If the language detection model CLD3 predicts a transliterated language that is
currently not supported, then the prediction is ignored.

For more information, please see the bug entry below.

BUG=649002

Committed: https://crrev.com/52247dcede791eae0c75996335a9b2bb0d9df236
Cr-Commit-Position: refs/heads/master@{#421451}
==========

[commit-bot: I haz the power, 2016-09-28 05:09:09]

Patchset 4 (id:??) landed as
https://crrev.com/52247dcede791eae0c75996335a9b2bb0d9df236
Cr-Commit-Position: refs/heads/master@{#421451}