Adds ChineseScriptClassifier to predict zh-Hant or zh-Hans for input detected as zh.

Adds ChineseScriptClassifier to predict zh-Hant or zh-Hans for input detected as zh.

BUG=684609
Review-Url: https://codereview.chromium.org/2732023003
Cr-Commit-Position: refs/heads/master@{#455383}
Committed: https://chromium.googlesource.com/chromium/src/+/87c6f931ecc4e25ce694d0c4039d4acdae572b60

[riesa, 2017-03-06 23:14:12]

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

[commit-bot: I haz the power, 2017-03-06 23:14:53]

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

[riesa, 2017-03-06 23:16:21]

Description was changed from

==========
Adds ChineseScriptClassifier to predict zh-Hant or zh-Hans for input detected as
zh.

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

to

==========
Adds ChineseScriptClassifier to predict zh-Hant or zh-Hans for input detected as
zh.

BUG=684609
==========

[commit-bot: I haz the power, 2017-03-06 23:42:15]

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

[commit-bot: I haz the power, 2017-03-06 23:42:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[riesa, 2017-03-07 00:07:51]

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

[riesa, 2017-03-07 00:08:34]

riesa@chromium.org changed reviewers:
+ groby@chromium.org

[commit-bot: I haz the power, 2017-03-07 00:08:45]

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

[riesa, 2017-03-07 00:10:13]

This change adds the component that makes a deterministic classification of any
Chinese-detected input. This fixes crbug.com/684609; we should merge into M57.

[commit-bot: I haz the power, 2017-03-07 00:42:19]

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

[commit-bot: I haz the power, 2017-03-07 00:42:20]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[riesa, 2017-03-07 01:01:15]

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

[commit-bot: I haz the power, 2017-03-07 01:02:43]

Dry run: 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, 2017-03-07 01:35:07]

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

[commit-bot: I haz the power, 2017-03-07 01:35:08]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[riesa, 2017-03-07 02:39:35]

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

[commit-bot: I haz the power, 2017-03-07 02:40:45]

Dry run: 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, 2017-03-07 03:11:17]

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

[commit-bot: I haz the power, 2017-03-07 03:11:18]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[riesa, 2017-03-07 04:02:03]

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

[commit-bot: I haz the power, 2017-03-07 04:03:28]

Dry run: 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, 2017-03-07 04:34:26]

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

[commit-bot: I haz the power, 2017-03-07 04:34:27]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[riesa, 2017-03-07 05:14:07]

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

[commit-bot: I haz the power, 2017-03-07 05:15:04]

Dry run: 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, 2017-03-07 05:45:59]

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

[commit-bot: I haz the power, 2017-03-07 05:46:00]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[riesa, 2017-03-07 05:55:51]

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

[commit-bot: I haz the power, 2017-03-07 05:56:28]

Dry run: 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, 2017-03-07 06:28:17]

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

[commit-bot: I haz the power, 2017-03-07 06:28:18]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[riesa, 2017-03-07 19:09:36]

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

[commit-bot: I haz the power, 2017-03-07 19:11:15]

Dry run: 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, 2017-03-07 21:51:47]

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

[commit-bot: I haz the power, 2017-03-07 21:51:48]

Dry run: This issue passed the CQ dry run.

[groby-ooo-7-16, 2017-03-08 00:41:55]

Mostly reviewed for technical functionality - I wouldn't know how to distinguish
hant/hans properly :)

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
File components/translate/core/language_detection/chinese_script_classifier.cc
(right):

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
components/translate/core/language_detection/chinese_script_classifier.cc:78:
VLOG(1) << "Original input:\n" << input_subset;
Are you planning to keep the VLOG forever, or is there an expiration date?

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
components/translate/core/language_detection/chinese_script_classifier.cc:88:
index < original_input.length() && index < hans_input.length() &&
nit: please do compute min outside loop

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
components/translate/core/language_detection/chinese_script_classifier.cc:92:
const auto hans_char = hans_input.charAt(index);
Bit concerned by the fact that charAt needs to repeatedly traverse the entire
string up to index. Do we have UTF8 iterators?

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
components/translate/core/language_detection/chinese_script_classifier.cc:94: if
(hans_char == hant_char) {
Possible simplification (no braces, less branches, no continue)

if (original_char == hans_char)
  ++hans_count;
if (original_char == hant_char)
  ++hant_count;

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
File components/translate/core/language_detection/chinese_script_classifier.h
(right):

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
components/translate/core/language_detection/chinese_script_classifier.h:31:
bool IsInitialized() const;
Maybe use a factory mode instead? (It's a personal peeve - so if you feel
strongly about ctor/IsInitialized instead, I won't hold up the CL for it)

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
components/translate/core/language_detection/chinese_script_classifier.h:35:
static const char kChineseSimplifiedCode[];
Why keep those as class members, as opposed to anon namespace?

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
File components/translate/core/language_detection/language_detection_util.cc
(right):

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
components/translate/core/language_detection/language_detection_util.cc:226:
language = translate::kUnknownLanguageCode;
Isn't this technically simplified Chinese? Which is zh-CN?

[riesa, 2017-03-08 01:47:42]

Thanks for reviewing!

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
File components/translate/core/language_detection/chinese_script_classifier.cc
(right):

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
components/translate/core/language_detection/chinese_script_classifier.cc:78:
VLOG(1) << "Original input:\n" << input_subset;
On 2017/03/08 00:41:54, groby wrote:
> Are you planning to keep the VLOG forever, or is there an expiration date?

Yes, I was planning to keep it in case of bug reports. But it sounds like
keeping these are discouraged in Chromium?

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
components/translate/core/language_detection/chinese_script_classifier.cc:88:
index < original_input.length() && index < hans_input.length() &&
On 2017/03/08 00:41:54, groby wrote:
> nit: please do compute min outside loop

Done.

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
components/translate/core/language_detection/chinese_script_classifier.cc:92:
const auto hans_char = hans_input.charAt(index);
On 2017/03/08 00:41:54, groby wrote:
> Bit concerned by the fact that charAt needs to repeatedly traverse the entire
> string up to index. Do we have UTF8 iterators?

Hm, from what I can tell charAt is just indexing right into a UChar (a 16-bit
wide char) array.
https://cs.chromium.org/chromium/src/third_party/icu/source/common/unicode/un...

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
components/translate/core/language_detection/chinese_script_classifier.cc:94: if
(hans_char == hant_char) {
On 2017/03/08 00:41:54, groby wrote:
> Possible simplification (no braces, less branches, no continue)
> 
> if (original_char == hans_char)
>   ++hans_count;
> if (original_char == hant_char)
>   ++hant_count;

I originally had it this way in an earlier patch, and I had been going back and
forth on whether to go back to it.

The current if-structure makes debugging easier when we know exactly how many
and which script-unique chars to look for; these are what determine the
classification. If we have made it past the (hans_char == hant_char) check, then
every time I increment I know I found something unique. In the simpler case
which is functionally and productionally equivalent, I don't know this
information.

Since this code will be run many more times in production than I will run it to
debug something, and since you had a similar thought, then I will change it
back.

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
File components/translate/core/language_detection/chinese_script_classifier.h
(right):

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
components/translate/core/language_detection/chinese_script_classifier.h:31:
bool IsInitialized() const;
On 2017/03/08 00:41:55, groby wrote:
> Maybe use a factory mode instead? (It's a personal peeve - so if you feel
> strongly about ctor/IsInitialized instead, I won't hold up the CL for it)

My understanding is that the factory model is not ideal for very frequently
allocated classes since it is allocated on the heap which adds a (slight) time
and memory overhead. Due to the structure of language_detection_utils I think
that is our case right now.

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
components/translate/core/language_detection/chinese_script_classifier.h:35:
static const char kChineseSimplifiedCode[];
On 2017/03/08 00:41:55, groby wrote:
> Why keep those as class members, as opposed to anon namespace?

Just a style/organization preference. But in a massive codebase like Chromium
maybe I guess there can be some functional benefits to moving these into an
anonymous namespace in the .cc file. Done.

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
File components/translate/core/language_detection/language_detection_util.cc
(right):

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
components/translate/core/language_detection/language_detection_util.cc:226:
language = translate::kUnknownLanguageCode;
On 2017/03/08 00:41:55, groby wrote:
> Isn't this technically simplified Chinese? Which is zh-CN?

This would only execute if the classifier failed to initialize properly, and
Classify() returned the empty string. I chose to return Unk here because it:
(1) makes it easier to debug in translate-internals, and
(2) wouldn't cause triggering issues by returning zh-CN if the page was actually
zh-TW or vice-versa.

[riesa, 2017-03-08 01:48:00]

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

[commit-bot: I haz the power, 2017-03-08 01:49:11]

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

[groby1, 2017-03-08 02:00:56]

groby@google.com changed reviewers:
+ groby@google.com

[groby1, 2017-03-08 02:00:57]

LGTM

Definitely OK with VLOG for this release, we should just think about
DVLOG/removal in the longer term.

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
File components/translate/core/language_detection/chinese_script_classifier.cc
(right):

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
components/translate/core/language_detection/chinese_script_classifier.cc:78:
VLOG(1) << "Original input:\n" << input_subset;
On 2017/03/08 01:47:42, riesa wrote:
> On 2017/03/08 00:41:54, groby wrote:
> > Are you planning to keep the VLOG forever, or is there an expiration date?
> 
> Yes, I was planning to keep it in case of bug reports. But it sounds like
> keeping these are discouraged in Chromium?

I'm torn. Each VLOG increases binary size, which affects how easy it is for
people to download Chrome. (Ping me if you'd like details)

But I do see that they'd be helpful for debugging, although it requires
modifying the cmdline to even see output. Would DVLOG be OK?

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
components/translate/core/language_detection/chinese_script_classifier.cc:92:
const auto hans_char = hans_input.charAt(index);
On 2017/03/08 01:47:42, riesa wrote:
> On 2017/03/08 00:41:54, groby wrote:
> > Bit concerned by the fact that charAt needs to repeatedly traverse the
entire
> > string up to index. Do we have UTF8 iterators?
> 
> Hm, from what I can tell charAt is just indexing right into a UChar (a 16-bit
> wide char) array.
>
https://cs.chromium.org/chromium/src/third_party/icu/source/common/unicode/un...

Gah. I misread the types to be still UTF8. Never mind then :) (Modern CJK chars
all live in the BMP, correct?)

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
components/translate/core/language_detection/chinese_script_classifier.cc:94: if
(hans_char == hant_char) {
On 2017/03/08 01:47:42, riesa wrote:
> On 2017/03/08 00:41:54, groby wrote:
> > Possible simplification (no braces, less branches, no continue)
> > 
> > if (original_char == hans_char)
> >   ++hans_count;
> > if (original_char == hant_char)
> >   ++hant_count;
> 
> I originally had it this way in an earlier patch, and I had been going back
and
> forth on whether to go back to it.
> 
> The current if-structure makes debugging easier when we know exactly how many
> and which script-unique chars to look for; these are what determine the
> classification. If we have made it past the (hans_char == hant_char) check,
then
> every time I increment I know I found something unique. In the simpler case
> which is functionally and productionally equivalent, I don't know this
> information.
> 
> Since this code will be run many more times in production than I will run it
to
> debug something, and since you had a similar thought, then I will change it
> back.

I don't think there's a performance penalty worth considering. If this is
cleaner to you and helps debugging, keep it. I just have a bias towards shorter
code :)

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
File components/translate/core/language_detection/language_detection_util.cc
(right):

https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
components/translate/core/language_detection/language_detection_util.cc:226:
language = translate::kUnknownLanguageCode;
On 2017/03/08 01:47:42, riesa wrote:
> On 2017/03/08 00:41:55, groby wrote:
> > Isn't this technically simplified Chinese? Which is zh-CN?
> 
> This would only execute if the classifier failed to initialize properly, and
> Classify() returned the empty string. I chose to return Unk here because it:
> (1) makes it easier to debug in translate-internals, and
> (2) wouldn't cause triggering issues by returning zh-CN if the page was
actually
> zh-TW or vice-versa.

Acknowledged.

[riesa, 2017-03-08 02:17:20]

On 2017/03/08 02:00:57, groby1 wrote:
> LGTM
> 
> Definitely OK with VLOG for this release, we should just think about
> DVLOG/removal in the longer term.
> 
>
https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
> File components/translate/core/language_detection/chinese_script_classifier.cc
> (right):
> 
>
https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
> components/translate/core/language_detection/chinese_script_classifier.cc:78:
> VLOG(1) << "Original input:\n" << input_subset;
> On 2017/03/08 01:47:42, riesa wrote:
> > On 2017/03/08 00:41:54, groby wrote:
> > > Are you planning to keep the VLOG forever, or is there an expiration date?
> > 
> > Yes, I was planning to keep it in case of bug reports. But it sounds like
> > keeping these are discouraged in Chromium?
> 
> I'm torn. Each VLOG increases binary size, which affects how easy it is for
> people to download Chrome. (Ping me if you'd like details)
> 
> But I do see that they'd be helpful for debugging, although it requires
> modifying the cmdline to even see output. Would DVLOG be OK?

DVLOG is totally fine. Changed.

> 
>
https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
> components/translate/core/language_detection/chinese_script_classifier.cc:92:
> const auto hans_char = hans_input.charAt(index);
> On 2017/03/08 01:47:42, riesa wrote:
> > On 2017/03/08 00:41:54, groby wrote:
> > > Bit concerned by the fact that charAt needs to repeatedly traverse the
> entire
> > > string up to index. Do we have UTF8 iterators?
> > 
> > Hm, from what I can tell charAt is just indexing right into a UChar (a
16-bit
> > wide char) array.
> >
>
https://cs.chromium.org/chromium/src/third_party/icu/source/common/unicode/un...
> 
> Gah. I misread the types to be still UTF8. Never mind then :) (Modern CJK
chars
> all live in the BMP, correct?)

Yup, and CJK takes up most of it! There might be some very rare or archaic
codepoints in other planes.

> 
>
https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
> components/translate/core/language_detection/chinese_script_classifier.cc:94:
if
> (hans_char == hant_char) {
> On 2017/03/08 01:47:42, riesa wrote:
> > On 2017/03/08 00:41:54, groby wrote:
> > > Possible simplification (no braces, less branches, no continue)
> > > 
> > > if (original_char == hans_char)
> > >   ++hans_count;
> > > if (original_char == hant_char)
> > >   ++hant_count;
> > 
> > I originally had it this way in an earlier patch, and I had been going back
> and
> > forth on whether to go back to it.
> > 
> > The current if-structure makes debugging easier when we know exactly how
many
> > and which script-unique chars to look for; these are what determine the
> > classification. If we have made it past the (hans_char == hant_char) check,
> then
> > every time I increment I know I found something unique. In the simpler case
> > which is functionally and productionally equivalent, I don't know this
> > information.
> > 
> > Since this code will be run many more times in production than I will run it
> to
> > debug something, and since you had a similar thought, then I will change it
> > back.
> 
> I don't think there's a performance penalty worth considering. If this is
> cleaner to you and helps debugging, keep it. I just have a bias towards
shorter
> code :)
> 
>
https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
> File components/translate/core/language_detection/language_detection_util.cc
> (right):
> 
>
https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
> components/translate/core/language_detection/language_detection_util.cc:226:
> language = translate::kUnknownLanguageCode;
> On 2017/03/08 01:47:42, riesa wrote:
> > On 2017/03/08 00:41:55, groby wrote:
> > > Isn't this technically simplified Chinese? Which is zh-CN?
> > 
> > This would only execute if the classifier failed to initialize properly, and
> > Classify() returned the empty string. I chose to return Unk here because it:
> > (1) makes it easier to debug in translate-internals, and
> > (2) wouldn't cause triggering issues by returning zh-CN if the page was
> actually
> > zh-TW or vice-versa.
> 
> Acknowledged.

[riesa, 2017-03-08 02:19:39]

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

[commit-bot: I haz the power, 2017-03-08 02:20:47]

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

[groby-ooo-7-16, 2017-03-08 02:21:30]

On 2017/03/08 02:17:20, riesa wrote:
> On 2017/03/08 02:00:57, groby1 wrote:
> > LGTM
> > 
> > Definitely OK with VLOG for this release, we should just think about
> > DVLOG/removal in the longer term.
> > 
> >
>
https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
> > File
components/translate/core/language_detection/chinese_script_classifier.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
> >
components/translate/core/language_detection/chinese_script_classifier.cc:78:
> > VLOG(1) << "Original input:\n" << input_subset;
> > On 2017/03/08 01:47:42, riesa wrote:
> > > On 2017/03/08 00:41:54, groby wrote:
> > > > Are you planning to keep the VLOG forever, or is there an expiration
date?
> > > 
> > > Yes, I was planning to keep it in case of bug reports. But it sounds like
> > > keeping these are discouraged in Chromium?
> > 
> > I'm torn. Each VLOG increases binary size, which affects how easy it is for
> > people to download Chrome. (Ping me if you'd like details)
> > 
> > But I do see that they'd be helpful for debugging, although it requires
> > modifying the cmdline to even see output. Would DVLOG be OK?
> 
> DVLOG is totally fine. Changed.
> 
> > 
> >
>
https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
> >
components/translate/core/language_detection/chinese_script_classifier.cc:92:
> > const auto hans_char = hans_input.charAt(index);
> > On 2017/03/08 01:47:42, riesa wrote:
> > > On 2017/03/08 00:41:54, groby wrote:
> > > > Bit concerned by the fact that charAt needs to repeatedly traverse the
> > entire
> > > > string up to index. Do we have UTF8 iterators?
> > > 
> > > Hm, from what I can tell charAt is just indexing right into a UChar (a
> 16-bit
> > > wide char) array.
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/icu/source/common/unicode/un...
> > 
> > Gah. I misread the types to be still UTF8. Never mind then :) (Modern CJK
> chars
> > all live in the BMP, correct?)
> 
> Yup, and CJK takes up most of it! There might be some very rare or archaic
> codepoints in other planes.
> 
> > 
> >
>
https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
> >
components/translate/core/language_detection/chinese_script_classifier.cc:94:
> if
> > (hans_char == hant_char) {
> > On 2017/03/08 01:47:42, riesa wrote:
> > > On 2017/03/08 00:41:54, groby wrote:
> > > > Possible simplification (no braces, less branches, no continue)
> > > > 
> > > > if (original_char == hans_char)
> > > >   ++hans_count;
> > > > if (original_char == hant_char)
> > > >   ++hant_count;
> > > 
> > > I originally had it this way in an earlier patch, and I had been going
back
> > and
> > > forth on whether to go back to it.
> > > 
> > > The current if-structure makes debugging easier when we know exactly how
> many
> > > and which script-unique chars to look for; these are what determine the
> > > classification. If we have made it past the (hans_char == hant_char)
check,
> > then
> > > every time I increment I know I found something unique. In the simpler
case
> > > which is functionally and productionally equivalent, I don't know this
> > > information.
> > > 
> > > Since this code will be run many more times in production than I will run
it
> > to
> > > debug something, and since you had a similar thought, then I will change
it
> > > back.
> > 
> > I don't think there's a performance penalty worth considering. If this is
> > cleaner to you and helps debugging, keep it. I just have a bias towards
> shorter
> > code :)
> > 
> >
>
https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
> > File components/translate/core/language_detection/language_detection_util.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2732023003/diff/130001/components/translate/c...
> > components/translate/core/language_detection/language_detection_util.cc:226:
> > language = translate::kUnknownLanguageCode;
> > On 2017/03/08 01:47:42, riesa wrote:
> > > On 2017/03/08 00:41:55, groby wrote:
> > > > Isn't this technically simplified Chinese? Which is zh-CN?
> > > 
> > > This would only execute if the classifier failed to initialize properly,
and
> > > Classify() returned the empty string. I chose to return Unk here because
it:
> > > (1) makes it easier to debug in translate-internals, and
> > > (2) wouldn't cause triggering issues by returning zh-CN if the page was
> > actually
> > > zh-TW or vice-versa.
> > 
> > Acknowledged.

Still LGTM, this time even from the right account :)

[commit-bot: I haz the power, 2017-03-08 03:43:01]

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

[commit-bot: I haz the power, 2017-03-08 03:43:02]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[riesa, 2017-03-08 03:46:32]

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

[commit-bot: I haz the power, 2017-03-08 03:47:03]

Dry run: 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, 2017-03-08 04:46:11]

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

[commit-bot: I haz the power, 2017-03-08 04:46:11]

Dry run: This issue passed the CQ dry run.

[riesa, 2017-03-08 04:58:49]

The CQ bit was checked by riesa@chromium.org

[riesa, 2017-03-08 04:58:50]

The patchset sent to the CQ was uploaded after l-g-t-m from groby@google.com
Link to the patchset: https://codereview.chromium.org/2732023003/#ps170001
(title: "VLOG to DVLOG")

[commit-bot: I haz the power, 2017-03-08 04:59:00]

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, 2017-03-08 05:04:18]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1488949129966780,
"parent_rev": "1fb60dd61b52c8d31f072843ae8d81de0811dd80", "commit_rev":
"87c6f931ecc4e25ce694d0c4039d4acdae572b60"}

[commit-bot: I haz the power, 2017-03-08 05:06:06]

Description was changed from

==========
Adds ChineseScriptClassifier to predict zh-Hant or zh-Hans for input detected as
zh.

BUG=684609
==========

to

==========
Adds ChineseScriptClassifier to predict zh-Hant or zh-Hans for input detected as
zh.

BUG=684609

Review-Url: https://codereview.chromium.org/2732023003
Cr-Commit-Position: refs/heads/master@{#455383}
Committed:
https://chromium.googlesource.com/chromium/src/+/87c6f931ecc4e25ce694d0c4039d...
==========

[commit-bot: I haz the power, 2017-03-08 05:06:08]

Committed patchset #10 (id:170001) as
https://chromium.googlesource.com/chromium/src/+/87c6f931ecc4e25ce694d0c4039d...