Allow TranslateRanker to override decisions taken by heuristics.

Allow TranslateRanker to override decisions taken by heuristics.

BUG=646711
Review-Url: https://codereview.chromium.org/2697703004
Cr-Commit-Position: refs/heads/master@{#466143}
Committed: https://chromium.googlesource.com/chromium/src/+/f662e8cd2f8358dff843dd6e803f5d9f68d55774

[hamelphi, 2017-02-15 00:31:10]

hamelphi@chromium.org changed reviewers:
+ rogerm@chromium.org

[hamelphi, 2017-02-23 16:20:15]

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

[groby-ooo-7-16, 2017-02-23 19:08:06]

May I have some test, please? :)

https://codereview.chromium.org/2697703004/diff/20001/chrome/browser/translat...
File chrome/browser/translate/chrome_translate_client.cc (right):

https://codereview.chromium.org/2697703004/diff/20001/chrome/browser/translat...
chrome/browser/translate/chrome_translate_client.cc:241: if
(translate::TranslateRanker::IsDecisionOverrideEnabled()) {
You could filter out the common code into a helper, which would result in both
instances becoming something like

if (DecisionOverride(MATCHES_PREVIOUS_LANGUAGE))
  return;

making things somewhat more readable.

[hamelphi, 2017-03-01 00:40:03]

Added several unittests and merged with changes from Roger's translate_ranker
refactor CL.

https://codereview.chromium.org/2697703004/diff/20001/chrome/browser/translat...
File chrome/browser/translate/chrome_translate_client.cc (right):

https://codereview.chromium.org/2697703004/diff/20001/chrome/browser/translat...
chrome/browser/translate/chrome_translate_client.cc:241: if
(translate::TranslateRanker::IsDecisionOverrideEnabled()) {
On 2017/02/23 19:08:06, groby wrote:
> You could filter out the common code into a helper, which would result in both
> instances becoming something like
> 
> if (DecisionOverride(MATCHES_PREVIOUS_LANGUAGE))
>   return;
> 
> making things somewhat more readable.

Done. I moved the logging logic to TranslateManager.

[hamelphi, 2017-03-01 00:40:41]

hamelphi@chromium.org changed reviewers:
- rogerm@chromium.org

[hamelphi, 2017-03-01 22:59:20]

Description was changed from

==========
Allow TranslateRanker to override decisions taken by heuristics.

BUG=646711
==========

to

==========
Allow TranslateRanker to override decisions taken by heuristics.

BUG=646711
==========

[hamelphi, 2017-03-01 22:59:20]

hamelphi@chromium.org changed reviewers:
+ asvitkine@chromium.org

[hamelphi, 2017-03-01 23:00:11]

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

[commit-bot: I haz the power, 2017-03-01 23:00:58]

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-02 00:05:36]

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

[commit-bot: I haz the power, 2017-03-02 00:05:37]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-03-06 22:08:20]

lgtm

https://codereview.chromium.org/2697703004/diff/60001/components/translate/co...
File components/translate/core/browser/translate_ranker_impl.cc (right):

https://codereview.chromium.org/2697703004/diff/60001/components/translate/co...
components/translate/core/browser/translate_ranker_impl.cc:194:
translate::kTranslateRankerDecisionOverride);
Nit: Remove translate:: since it's already in this namespace.

[groby-ooo-7-16, 2017-03-08 00:45:33]

lgtm

[hamelphi, 2017-04-07 20:54:37]

groby@ PTAL

I merged with the latest changes, and had to mock TranslateAcceptLanguages for
the unittest to run.

[groby-ooo-7-16, 2017-04-10 20:52:47]

I'd _prefer_ if you'd unify the testing code first, unless this is "MUST LAND
FOR 59".

https://codereview.chromium.org/2697703004/diff/140001/chrome/browser/transla...
File chrome/browser/translate/chrome_translate_client.cc (right):

https://codereview.chromium.org/2697703004/diff/140001/chrome/browser/transla...
chrome/browser/translate/chrome_translate_client.cc:228: if
(!base::FeatureList::IsEnabled(translate::kTranslateUI2016Q2) &&
I _really_ would prefer if this logic could be moved out of the UI code. This
section is growing instead of shrinking.

https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
File components/translate/core/browser/translate_accept_languages.h (right):

https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
components/translate/core/browser/translate_accept_languages.h:39:
TranslateAcceptLanguages();
I don't think that's necessary - see e.g MockTranslateClient

Or just set the acceptlanguage prefs?

https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
File components/translate/core/browser/translate_manager.cc (right):

https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
components/translate/core/browser/translate_manager.cc:234: if
(translate_ranker_->IsQueryEnabled() ||
This is more and more ranker-related logic that moves into the manager. Can we
instead move all of this over to the ranker?

I.e. this spot here would be just.

    bool ranker_should_offer_translation =
translate_ranker_->ShouldOfferTranslation(translate_event_);

The same goes for other spots.

https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
File components/translate/core/browser/translate_manager_unittest.cc (right):

https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
components/translate/core/browser/translate_manager_unittest.cc:102: :
mock_translate_accept_languages_(), driver_(driver), prefs_(prefs) {
Likely the TODO() here would be a good step to take.

[hamelphi, 2017-04-11 22:46:07]

https://codereview.chromium.org/2697703004/diff/140001/chrome/browser/transla...
File chrome/browser/translate/chrome_translate_client.cc (right):

https://codereview.chromium.org/2697703004/diff/140001/chrome/browser/transla...
chrome/browser/translate/chrome_translate_client.cc:228: if
(!base::FeatureList::IsEnabled(translate::kTranslateUI2016Q2) &&
On 2017/04/10 20:52:47, groby wrote:
> I _really_ would prefer if this logic could be moved out of the UI code. This
> section is growing instead of shrinking.

I moved the logic to TranslateManager. Let me know what you think. I have not
written the unittest yet, but I am working on it.

https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
File components/translate/core/browser/translate_accept_languages.h (right):

https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
components/translate/core/browser/translate_accept_languages.h:39:
TranslateAcceptLanguages();
On 2017/04/10 20:52:47, groby wrote:
> I don't think that's necessary - see e.g MockTranslateClient
> 
> Or just set the acceptlanguage prefs?
> 


Since TranslateAcceptLanguages has a non-default constructor, it needs to be
called by GMock, as far as I understand.
I tried to call the true constructor with the PrefService, but this crashes with
a segfault. 
#3 0x7f1281e7855d PrefService::GetString()
#4 0x0000044d741f translate::TranslateAcceptLanguages::InitAcceptLanguages()

I tried a bunch of ways, but could not figure out a way to correctly initialize
the TranslateAcceptLangues object. I am quite confused about what is what here.
For TranslateManager unittests, we should not require the logic from
TranslateAcceptLanguages anyway. I think that logic should be tested elsewhere.
Having this protected default constructor allows us to bypass the problematic
initialization.

https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
File components/translate/core/browser/translate_manager.cc (right):

https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
components/translate/core/browser/translate_manager.cc:234: if
(translate_ranker_->IsQueryEnabled() ||
On 2017/04/10 20:52:47, groby wrote:
> This is more and more ranker-related logic that moves into the manager. Can we
> instead move all of this over to the ranker?
> 
> I.e. this spot here would be just.
> 
>     bool ranker_should_offer_translation =
> translate_ranker_->ShouldOfferTranslation(translate_event_);
> 
> The same goes for other spots. 

Done.

https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
File components/translate/core/browser/translate_manager_unittest.cc (right):

https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
components/translate/core/browser/translate_manager_unittest.cc:102: :
mock_translate_accept_languages_(), driver_(driver), prefs_(prefs) {
On 2017/04/10 20:52:47, groby wrote:
> Likely the TODO() here would be a good step to take. 

I agree that test utils would be great. Maybe a bit out of scope for this CL?

[hamelphi, 2017-04-11 22:46:18]

On 2017/04/11 22:46:07, hamelphi wrote:
>
https://codereview.chromium.org/2697703004/diff/140001/chrome/browser/transla...
> File chrome/browser/translate/chrome_translate_client.cc (right):
> 
>
https://codereview.chromium.org/2697703004/diff/140001/chrome/browser/transla...
> chrome/browser/translate/chrome_translate_client.cc:228: if
> (!base::FeatureList::IsEnabled(translate::kTranslateUI2016Q2) &&
> On 2017/04/10 20:52:47, groby wrote:
> > I _really_ would prefer if this logic could be moved out of the UI code.
This
> > section is growing instead of shrinking.
> 
> I moved the logic to TranslateManager. Let me know what you think. I have not
> written the unittest yet, but I am working on it.
> 
>
https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
> File components/translate/core/browser/translate_accept_languages.h (right):
> 
>
https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
> components/translate/core/browser/translate_accept_languages.h:39:
> TranslateAcceptLanguages();
> On 2017/04/10 20:52:47, groby wrote:
> > I don't think that's necessary - see e.g MockTranslateClient
> > 
> > Or just set the acceptlanguage prefs?
> > 
> 
> 
> Since TranslateAcceptLanguages has a non-default constructor, it needs to be
> called by GMock, as far as I understand.
> I tried to call the true constructor with the PrefService, but this crashes
with
> a segfault. 
> #3 0x7f1281e7855d PrefService::GetString()
> #4 0x0000044d741f translate::TranslateAcceptLanguages::InitAcceptLanguages()
> 
> I tried a bunch of ways, but could not figure out a way to correctly
initialize
> the TranslateAcceptLangues object. I am quite confused about what is what
here.
> For TranslateManager unittests, we should not require the logic from
> TranslateAcceptLanguages anyway. I think that logic should be tested
elsewhere.
> Having this protected default constructor allows us to bypass the problematic
> initialization.
> 
>
https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
> File components/translate/core/browser/translate_manager.cc (right):
> 
>
https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
> components/translate/core/browser/translate_manager.cc:234: if
> (translate_ranker_->IsQueryEnabled() ||
> On 2017/04/10 20:52:47, groby wrote:
> > This is more and more ranker-related logic that moves into the manager. Can
we
> > instead move all of this over to the ranker?
> > 
> > I.e. this spot here would be just.
> > 
> >     bool ranker_should_offer_translation =
> > translate_ranker_->ShouldOfferTranslation(translate_event_);
> > 
> > The same goes for other spots. 
> 
> Done.
> 
>
https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
> File components/translate/core/browser/translate_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
> components/translate/core/browser/translate_manager_unittest.cc:102: :
> mock_translate_accept_languages_(), driver_(driver), prefs_(prefs) {
> On 2017/04/10 20:52:47, groby wrote:
> > Likely the TODO() here would be a good step to take. 
> 
> I agree that test utils would be great. Maybe a bit out of scope for this CL?


TODO: ShouldSuppressBubbleUI unittest

I would really like if we could land this for 59. This CL is the first step
towards understanding how ranker can help to increase recall. It has already
been pushed back for weeks because of the issues with the model loader. We have
a few teams that are awaiting eagerly for results, and I have pressure from
above to get this experiment going. So, I would argue that it is more critical
to land this for 59 than unifying the test code.

[hamelphi, 2017-04-12 21:10:53]

I moved more logic to Ranker.

TODO: unittest for TranslateRanker::RecordTranslateEvent and
TranslateRanker::ShouldOverrideDecision.

https://codereview.chromium.org/2697703004/diff/140001/chrome/browser/transla...
File chrome/browser/translate/chrome_translate_client.cc (right):

https://codereview.chromium.org/2697703004/diff/140001/chrome/browser/transla...
chrome/browser/translate/chrome_translate_client.cc:228: if
(!base::FeatureList::IsEnabled(translate::kTranslateUI2016Q2) &&
On 2017/04/11 22:46:07, hamelphi wrote:
> On 2017/04/10 20:52:47, groby wrote:
> > I _really_ would prefer if this logic could be moved out of the UI code.
This
> > section is growing instead of shrinking.
> 
> I moved the logic to TranslateManager. Let me know what you think. I have not
> written the unittest yet, but I am working on it.

Unittests done.

https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
File components/translate/core/browser/translate_accept_languages.h (right):

https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
components/translate/core/browser/translate_accept_languages.h:39:
TranslateAcceptLanguages();
On 2017/04/11 22:46:07, hamelphi wrote:
> On 2017/04/10 20:52:47, groby wrote:
> > I don't think that's necessary - see e.g MockTranslateClient
> > 
> > Or just set the acceptlanguage prefs?
> > 
> 
> 
> Since TranslateAcceptLanguages has a non-default constructor, it needs to be
> called by GMock, as far as I understand.
> I tried to call the true constructor with the PrefService, but this crashes
with
> a segfault. 
> #3 0x7f1281e7855d PrefService::GetString()
> #4 0x0000044d741f translate::TranslateAcceptLanguages::InitAcceptLanguages()
> 
> I tried a bunch of ways, but could not figure out a way to correctly
initialize
> the TranslateAcceptLangues object. I am quite confused about what is what
here.
> For TranslateManager unittests, we should not require the logic from
> TranslateAcceptLanguages anyway. I think that logic should be tested
elsewhere.
> Having this protected default constructor allows us to bypass the problematic
> initialization.

This is not necessary anymore, since I do not call InitiateTranslation in my
unitttests.

[hamelphi, 2017-04-12 22:28:55]

hamelphi@chromium.org changed reviewers:
+ rogerm@chromium.org

[hamelphi, 2017-04-12 22:28:56]

Uploaded the last unittests.

Ready for review. groby@, PTAL

Adding rogerm@ as reviewer since I refactored TranslateRanker quite a bit.

[hamelphi, 2017-04-12 22:29:22]

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

[commit-bot: I haz the power, 2017-04-12 22:30:52]

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-04-12 22:43:04]

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

[commit-bot: I haz the power, 2017-04-12 22:43:05]

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

[hamelphi, 2017-04-12 22:53:15]

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

[commit-bot: I haz the power, 2017-04-12 22:53:48]

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-04-12 23:05:07]

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

[commit-bot: I haz the power, 2017-04-12 23:05:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hamelphi, 2017-04-13 15:02:30]

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

[commit-bot: I haz the power, 2017-04-13 15:03:11]

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-04-13 16:08:21]

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

[commit-bot: I haz the power, 2017-04-13 16:08:22]

Dry run: This issue passed the CQ dry run.

[Roger McFarlane (Chromium), 2017-04-19 16:16:16]

lgtm

Nice!

https://codereview.chromium.org/2697703004/diff/310001/components/translate/c...
File components/translate/core/browser/translate_manager.cc (right):

https://codereview.chromium.org/2697703004/diff/310001/components/translate/c...
components/translate/core/browser/translate_manager.cc:610:
LANGUAGE_DISABLED_BY_AUTO_BLACKLIST)));
nit: I'd rather see this as seperate clauses

 // documentation...
 if (...)
   return true;

 // documentation...
 if (...)
   return true;

 // No reason found to suppress.
 return false;

https://codereview.chromium.org/2697703004/diff/310001/components/translate/c...
File components/translate/core/browser/translate_manager.h (right):

https://codereview.chromium.org/2697703004/diff/310001/components/translate/c...
components/translate/core/browser/translate_manager.h:134: const std::string&
source_language);
why is this bubble ui specific?

[hamelphi, 2017-04-19 18:19:48]

Thanks Roger.

groby@ PTAL.

https://codereview.chromium.org/2697703004/diff/310001/components/translate/c...
File components/translate/core/browser/translate_manager.cc (right):

https://codereview.chromium.org/2697703004/diff/310001/components/translate/c...
components/translate/core/browser/translate_manager.cc:610:
LANGUAGE_DISABLED_BY_AUTO_BLACKLIST)));
On 2017/04/19 16:16:16, Roger McFarlane (Chromium) wrote:
> nit: I'd rather see this as seperate clauses
> 
>  // documentation...
>  if (...)
>    return true;
> 
>  // documentation...
>  if (...)
>    return true;
> 
>  // No reason found to suppress.
>  return false;

Done.

https://codereview.chromium.org/2697703004/diff/310001/components/translate/c...
File components/translate/core/browser/translate_manager.h (right):

https://codereview.chromium.org/2697703004/diff/310001/components/translate/c...
components/translate/core/browser/translate_manager.h:134: const std::string&
source_language);
On 2017/04/19 16:16:16, Roger McFarlane (Chromium) wrote:
> why is this bubble ui specific?

I don't know the reasons why this logic is only applied to BubbleUI. Groby@
might have more insights on this?

[groby-ooo-7-16, 2017-04-20 19:45:57]

lgtm

https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
File components/translate/core/browser/translate_accept_languages.h (right):

https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
components/translate/core/browser/translate_accept_languages.h:39:
TranslateAcceptLanguages();
On 2017/04/12 21:10:53, hamelphi wrote:
> On 2017/04/11 22:46:07, hamelphi wrote:
> > On 2017/04/10 20:52:47, groby wrote:
> > > I don't think that's necessary - see e.g MockTranslateClient
> > > 
> > > Or just set the acceptlanguage prefs?
> > > 
> > 
> > 
> > Since TranslateAcceptLanguages has a non-default constructor, it needs to be
> > called by GMock, as far as I understand.
> > I tried to call the true constructor with the PrefService, but this crashes
> with
> > a segfault. 
> > #3 0x7f1281e7855d PrefService::GetString()
> > #4 0x0000044d741f translate::TranslateAcceptLanguages::InitAcceptLanguages()
> > 
> > I tried a bunch of ways, but could not figure out a way to correctly
> initialize
> > the TranslateAcceptLangues object. I am quite confused about what is what
> here.
> > For TranslateManager unittests, we should not require the logic from
> > TranslateAcceptLanguages anyway. I think that logic should be tested
> elsewhere.
> > Having this protected default constructor allows us to bypass the
problematic
> > initialization.
> 
> This is not necessary anymore, since I do not call InitiateTranslation in my
> unitttests.

Just FYI: It crashes on GetString because the property is not registered, I
think. Any test using TranslatePrefs must register a few properties (see
existing tests)

https://codereview.chromium.org/2697703004/diff/330001/chrome/browser/transla...
File chrome/browser/translate/chrome_translate_client.cc (right):

https://codereview.chromium.org/2697703004/diff/330001/chrome/browser/transla...
chrome/browser/translate/chrome_translate_client.cc:227: }
Woohoo! Thank you! (Long term, I wonder if this needs to be unified with the
Infobar code, but that's def. out of scope)

[hamelphi, 2017-04-20 19:57:37]

The CQ bit was checked by hamelphi@chromium.org

[hamelphi, 2017-04-20 19:57:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, rogerm@chromium.org
Link to the patchset: https://codereview.chromium.org/2697703004/#ps330001
(title: "Make ShouldSuppressBubbleUI easier to read.")

[commit-bot: I haz the power, 2017-04-20 19:58:21]

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

[hamelphi, 2017-04-20 20:46:53]

On 2017/04/20 19:45:57, groby wrote:
> lgtm
> 
>
https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
> File components/translate/core/browser/translate_accept_languages.h (right):
> 
>
https://codereview.chromium.org/2697703004/diff/140001/components/translate/c...
> components/translate/core/browser/translate_accept_languages.h:39:
> TranslateAcceptLanguages();
> On 2017/04/12 21:10:53, hamelphi wrote:
> > On 2017/04/11 22:46:07, hamelphi wrote:
> > > On 2017/04/10 20:52:47, groby wrote:
> > > > I don't think that's necessary - see e.g MockTranslateClient
> > > > 
> > > > Or just set the acceptlanguage prefs?
> > > > 
> > > 
> > > 
> > > Since TranslateAcceptLanguages has a non-default constructor, it needs to
be
> > > called by GMock, as far as I understand.
> > > I tried to call the true constructor with the PrefService, but this
crashes
> > with
> > > a segfault. 
> > > #3 0x7f1281e7855d PrefService::GetString()
> > > #4 0x0000044d741f
translate::TranslateAcceptLanguages::InitAcceptLanguages()
> > > 
> > > I tried a bunch of ways, but could not figure out a way to correctly
> > initialize
> > > the TranslateAcceptLangues object. I am quite confused about what is what
> > here.
> > > For TranslateManager unittests, we should not require the logic from
> > > TranslateAcceptLanguages anyway. I think that logic should be tested
> > elsewhere.
> > > Having this protected default constructor allows us to bypass the
> problematic
> > > initialization.
> > 
> > This is not necessary anymore, since I do not call InitiateTranslation in my
> > unitttests.
> 
> Just FYI: It crashes on GetString because the property is not registered, I
> think. Any test using TranslatePrefs must register a few properties (see
> existing tests)
> 
>
https://codereview.chromium.org/2697703004/diff/330001/chrome/browser/transla...
> File chrome/browser/translate/chrome_translate_client.cc (right):
> 
>
https://codereview.chromium.org/2697703004/diff/330001/chrome/browser/transla...
> chrome/browser/translate/chrome_translate_client.cc:227: }
> Woohoo! Thank you! (Long term, I wonder if this needs to be unified with the
> Infobar code, but that's def. out of scope)

Thanks for the review!

[commit-bot: I haz the power, 2017-04-20 21:29:51]

CQ is committing da patch.

Bot data: {"patchset_id": 330001, "attempt_start_ts": 1492718257555390,
"parent_rev": "c836f4a7f26caed6ecbbddf3f281388ec3aa8078", "commit_rev":
"f662e8cd2f8358dff843dd6e803f5d9f68d55774"}

[commit-bot: I haz the power, 2017-04-20 21:30:01]

Description was changed from

==========
Allow TranslateRanker to override decisions taken by heuristics.

BUG=646711
==========

to

==========
Allow TranslateRanker to override decisions taken by heuristics.

BUG=646711

Review-Url: https://codereview.chromium.org/2697703004
Cr-Commit-Position: refs/heads/master@{#466143}
Committed:
https://chromium.googlesource.com/chromium/src/+/f662e8cd2f8358dff843dd6e803f...
==========

[commit-bot: I haz the power, 2017-04-20 21:30:03]

Committed patchset #18 (id:330001) as
https://chromium.googlesource.com/chromium/src/+/f662e8cd2f8358dff843dd6e803f...