Integrate RankerModelLoader with TranslateRanker.

Integrate RankerModelLoader with TranslateRanker.

This CL replaces the model loading code in the TranslateRanker with
the use of the RankerModelLoader.

* the TranslateRanker transitions to a KeyedService
* the TranslateRanker is split into interface and implementation,
  facilitationt mocking the TranslateRanker in tests.
* Browser-level TransalteRankerFactories are added in order to
  pass the appropriate model cache path and model url to the ranker
  and, in turn, the model loader.
* Metrics providers have been promoted to browser level to allow for
  accessing it as a keyed servie across all loaded profiles.

BUG=646711, 697665, 698057
Review-Url: https://codereview.chromium.org/2785493004
Cr-Commit-Position: refs/heads/master@{#462038}
Committed: https://chromium.googlesource.com/chromium/src/+/95c5541219a088802bf259cc36ab7fd4a7ae98e7

[Roger McFarlane (Chromium), 2017-03-29 05:40:32]

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

[commit-bot: I haz the power, 2017-03-29 05:41:09]

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

[Roger McFarlane (Chromium), 2017-03-29 05:47:51]

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

[Roger McFarlane (Chromium), 2017-03-29 05:47:51]

PTAL?

The diffs between this CL and the last attempt to land this CL are enclose
below.

Sumary:
- iterate over loaded profiles instead of last used
  profiles, to avoid cdbug/698057
- merges in the change to enable logging by default,
  adding a unittest along the way
- don't copy TEPs to the logging cache if logging
  isn't enabled.

diff --git a/chrome/browser/translate/translate_ranker_metrics_provider.cc
b/chrome/browser/translate/translate_ranker_metrics_provider.cc
index 7790108b1d05..2feb883eae05 100644
--- a/chrome/browser/translate/translate_ranker_metrics_provider.cc
+++ b/chrome/browser/translate/translate_ranker_metrics_provider.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/translate/translate_ranker_metrics_provider.h"
 
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/translate/translate_ranker_factory.h"
 #include "components/metrics/proto/chrome_user_metrics_extension.pb.h"
@@ -14,7 +15,9 @@ namespace translate {
 
 void TranslateRankerMetricsProvider::ProvideGeneralMetrics(
     metrics::ChromeUserMetricsExtension* uma_proto) {
-  for (auto* profile : ProfileManager::GetLastOpenedProfiles()) {
+  std::vector<Profile*> loaded_profiles =
+      g_browser_process->profile_manager()->GetLoadedProfiles();
+  for (Profile* profile : loaded_profiles) {
     TranslateRanker* ranker =
         TranslateRankerFactory::GetInstance()->GetForBrowserContext(profile);
     if (!ranker)
diff --git a/components/translate/core/browser/translate_manager.cc
b/components/translate/core/browser/translate_manager.cc
index 60ae0bc5910c..65beb4edaf45 100644
--- a/components/translate/core/browser/translate_manager.cc
+++ b/components/translate/core/browser/translate_manager.cc
@@ -231,7 +231,8 @@ void TranslateManager::InitiateTranslation(const
std::string& page_lang) {
   }
 
   bool ranker_should_offer_translation = true;
-  if (translate_ranker_->IsQueryEnabled()) {
+  if (translate_ranker_->IsQueryEnabled() ||
+      translate_ranker_->IsEnforcementEnabled()) {
     ranker_should_offer_translation =
translate_ranker_->ShouldOfferTranslation(
         *translate_prefs, language_code, target_lang);
     translate_event_->set_ranker_request_timestamp_sec(
@@ -605,7 +606,8 @@ void TranslateManager::RecordTranslateEvent(int event_type)
{
       static_cast<metrics::TranslateEventProto::EventType>(event_type));
   translate_event_->set_event_timestamp_sec(
       (base::TimeTicks::Now() - base::TimeTicks()).InSeconds());
-  translate_ranker_->AddTranslateEvent(*translate_event_);
+  if (translate_ranker_->IsLoggingEnabled())
+    translate_ranker_->AddTranslateEvent(*translate_event_);
 }
 
 }  // namespace translate
diff --git a/components/translate/core/browser/translate_ranker_impl.cc
b/components/translate/core/browser/translate_ranker_impl.cc
index 5155092f26b0..18641f7969e0 100644
--- a/components/translate/core/browser/translate_ranker_impl.cc
+++ b/components/translate/core/browser/translate_ranker_impl.cc
@@ -80,7 +80,7 @@ const base::Feature kTranslateRankerEnforcement{
     "TranslateRankerEnforcement", base::FEATURE_DISABLED_BY_DEFAULT};
 
 const base::Feature kTranslateRankerLogging{"TranslateRankerLogging",
-                                            base::FEATURE_DISABLED_BY_DEFAULT};
+                                            base::FEATURE_ENABLED_BY_DEFAULT};
 
 TranslateRankerFeatures::TranslateRankerFeatures() {}
 
diff --git a/components/translate/core/browser/translate_ranker_impl_unittest.cc
b/components/translate/core/browser/translate_ranker_impl_unittest.cc
index 7a665be6627c..4a190a5b0468 100644
--- a/components/translate/core/browser/translate_ranker_impl_unittest.cc
+++ b/components/translate/core/browser/translate_ranker_impl_unittest.cc
@@ -199,7 +199,7 @@ TEST_F(TranslateRankerImplTest, DisabledByDefault) {
   auto ranker = GetRankerForTest(0.5);
   EXPECT_FALSE(ranker->IsQueryEnabled());
   EXPECT_FALSE(ranker->IsEnforcementEnabled());
-  EXPECT_FALSE(ranker->IsLoggingEnabled());
+  EXPECT_TRUE(ranker->IsLoggingEnabled());
 }
 
 TEST_F(TranslateRankerImplTest, ExplicitlyDisabled) {
@@ -216,7 +216,7 @@ TEST_F(TranslateRankerImplTest, EnableQuery) {
   auto ranker = GetRankerForTest(0.5);
   EXPECT_TRUE(ranker->IsQueryEnabled());
   EXPECT_FALSE(ranker->IsEnforcementEnabled());
-  EXPECT_FALSE(ranker->IsLoggingEnabled());
+  EXPECT_TRUE(ranker->IsLoggingEnabled());
 }
 
 TEST_F(TranslateRankerImplTest, EnableEnforcement) {
@@ -224,7 +224,7 @@ TEST_F(TranslateRankerImplTest, EnableEnforcement) {
   auto ranker = GetRankerForTest(0.5);
   EXPECT_TRUE(ranker->IsQueryEnabled());
   EXPECT_TRUE(ranker->IsEnforcementEnabled());
-  EXPECT_FALSE(ranker->IsLoggingEnabled());
+  EXPECT_TRUE(ranker->IsLoggingEnabled());
 }
 
 TEST_F(TranslateRankerImplTest, EnableQueryAndEnforcement) {
@@ -232,7 +232,7 @@ TEST_F(TranslateRankerImplTest, EnableQueryAndEnforcement) {
   auto ranker = GetRankerForTest(0.5);
   EXPECT_TRUE(ranker->IsQueryEnabled());
   EXPECT_TRUE(ranker->IsEnforcementEnabled());
-  EXPECT_FALSE(ranker->IsLoggingEnabled());
+  EXPECT_TRUE(ranker->IsLoggingEnabled());
 }
 
 TEST_F(TranslateRankerImplTest, EnableLogging) {
@@ -243,6 +243,14 @@ TEST_F(TranslateRankerImplTest, EnableLogging) {
   EXPECT_TRUE(ranker->IsLoggingEnabled());
 }
 
+TEST_F(TranslateRankerImplTest, DisableLogging) {
+  InitFeatures({}, {kTranslateRankerLogging});
+  auto ranker = GetRankerForTest(0.5);
+  EXPECT_FALSE(ranker->IsQueryEnabled());
+  EXPECT_FALSE(ranker->IsEnforcementEnabled());
+  EXPECT_FALSE(ranker->IsLoggingEnabled());
+}
+
 TEST_F(TranslateRankerImplTest, EnableAll) {
   InitFeatures({kTranslateRankerQuery, kTranslateRankerEnforcement,
                 kTranslateRankerLogging},

[Roger McFarlane (Chromium), 2017-03-29 06:04:40]

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

[commit-bot: I haz the power, 2017-03-29 06:04:46]

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-29 06:51:16]

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

[commit-bot: I haz the power, 2017-03-29 06:51:17]

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

[Roger McFarlane (Chromium), 2017-03-29 15:34:09]

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

[commit-bot: I haz the power, 2017-03-29 15:34:47]

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-29 19:12:38]

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

[commit-bot: I haz the power, 2017-03-29 19:12:40]

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...)

[hamelphi, 2017-03-29 19:23:56]

lgtm

https://codereview.chromium.org/2785493004/diff/20001/components/translate/co...
File components/translate/core/browser/translate_manager.cc (right):

https://codereview.chromium.org/2785493004/diff/20001/components/translate/co...
components/translate/core/browser/translate_manager.cc:609: if
(translate_ranker_->IsLoggingEnabled())
We already gate on this in the AddTranslateEvent function. I don't think this is
necessary.

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

https://codereview.chromium.org/2785493004/diff/20001/components/translate/co...
components/translate/core/browser/translate_ranker_impl.cc:268: DVLOG(3) <<
"Adding translate ranker event.";
Move this logging in the if statement.

[Roger McFarlane (Chromium), 2017-03-30 15:32:58]

Thanks.

https://codereview.chromium.org/2785493004/diff/20001/components/translate/co...
File components/translate/core/browser/translate_manager.cc (right):

https://codereview.chromium.org/2785493004/diff/20001/components/translate/co...
components/translate/core/browser/translate_manager.cc:609: if
(translate_ranker_->IsLoggingEnabled())
On 2017/03/29 19:23:56, hamelphi wrote:
> We already gate on this in the AddTranslateEvent function. I don't think this
is
> necessary.

Done.

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

https://codereview.chromium.org/2785493004/diff/20001/components/translate/co...
components/translate/core/browser/translate_ranker_impl.cc:268: DVLOG(3) <<
"Adding translate ranker event.";
On 2017/03/29 19:23:56, hamelphi wrote:
> Move this logging in the if statement.

Done.

[Roger McFarlane (Chromium), 2017-03-30 18:09:07]

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

[commit-bot: I haz the power, 2017-03-30 18:09:43]

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

[Roger McFarlane (Chromium), 2017-03-31 16:03:43]

rogerm@chromium.org changed reviewers:
+ droger@chromium.org, rkaplow@chromium.org, sdefresne@chromium.org

[Roger McFarlane (Chromium), 2017-03-31 16:03:44]

PTAL?

This is an attempt to re-land the translate ranker model download and caching
code.

See previous: https://codereview.chromium.org/2726203002/

I'll attach a diff between this CL and the previous attempt.

droger@ is nominated as a stand-in for groby@ who reviewed the previous CL, but
is OOO.

[Roger McFarlane (Chromium), 2017-03-31 16:07:02]

Partial diff from https://codereview.chromium.org/2726203002/ follows

**********************************************************

diff --git a/chrome/browser/translate/translate_ranker_metrics_provider.cc
b/chrome/browser/translate/translate_ranker_metrics_provider.cc
index 7790108b1d05..2feb883eae05 100644
--- a/chrome/browser/translate/translate_ranker_metrics_provider.cc
+++ b/chrome/browser/translate/translate_ranker_metrics_provider.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/translate/translate_ranker_metrics_provider.h"
 
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/translate/translate_ranker_factory.h"
 #include "components/metrics/proto/chrome_user_metrics_extension.pb.h"
@@ -14,7 +15,9 @@ namespace translate {
 
 void TranslateRankerMetricsProvider::ProvideGeneralMetrics(
     metrics::ChromeUserMetricsExtension* uma_proto) {
-  for (auto* profile : ProfileManager::GetLastOpenedProfiles()) {
+  std::vector<Profile*> loaded_profiles =
+      g_browser_process->profile_manager()->GetLoadedProfiles();
+  for (Profile* profile : loaded_profiles) {
     TranslateRanker* ranker =
         TranslateRankerFactory::GetInstance()->GetForBrowserContext(profile);
     if (!ranker)
diff --git a/components/translate/core/browser/ranker_model_loader.cc
b/components/translate/core/browser/ranker_model_loader.cc
index 85ad5f95d2c7..4a8074467518 100644
--- a/components/translate/core/browser/ranker_model_loader.cc
+++ b/components/translate/core/browser/ranker_model_loader.cc
@@ -33,7 +33,7 @@ constexpr int kUrlFetcherId = 2;
 
 // The maximum number of model download attempts to make. Download may fail
 // due to server error or network availability issues.
-constexpr int kMaxRetryOn5xx = 8;
+constexpr int kMaxDownloadAttempts = 8;
 
 // The minimum duration, in minutes, between download attempts.
 constexpr int kMinRetryDelayMins = 3;
@@ -148,6 +148,9 @@ class RankerModelLoader::Backend {
   // TODO(rogerm): Use net::URLFetcher directly?
   std::unique_ptr<TranslateURLFetcher> url_fetcher_;
 
+  // The number of times the backend has attempted to download the model.
+  int download_attempts_;
+
   // The next time before which no new attempts to download the model should be
   // attempted.
   base::TimeTicks next_earliest_download_time_;
@@ -170,7 +173,9 @@ RankerModelLoader::Backend::Backend(
       internal_on_model_available_cb_(internal_on_model_available_cb),
       model_path_(model_path),
       model_url_(model_url),
-      uma_prefix_(uma_prefix) {
+      uma_prefix_(uma_prefix),
+      url_fetcher_(base::MakeUnique<TranslateURLFetcher>(kUrlFetcherId)),
+      download_attempts_(0) {
   sequence_checker_.DetachFromSequence();
 }
 
@@ -258,8 +263,11 @@ void RankerModelLoader::Backend::LoadFromFile() {
 void RankerModelLoader::Backend::AsyncLoadFromURL() {
   DCHECK(sequence_checker_.CalledOnValidSequence());
 
-  if (!model_url_.is_valid())
+  if (!model_url_.is_valid()) {
+    // Notify the loader that model loading has been abandoned.
+    TransferModelToClient(nullptr, true);
     return;
+  }
 
   // Do nothing if the download attempts should be throttled.
   if (base::TimeTicks::Now() < next_earliest_download_time_) {
@@ -267,41 +275,31 @@ void RankerModelLoader::Backend::AsyncLoadFromURL() {
     return;
   }
 
-  // Otherwise, initialize the model fetcher to be non-null and trigger an
-  // initial download attempt.
-  if (!url_fetcher_) {
-    url_fetcher_ = base::MakeUnique<TranslateURLFetcher>(kUrlFetcherId);
-    url_fetcher_->set_max_retry_on_5xx(kMaxRetryOn5xx);
-  }
-
   // If a request is already in flight, do not issue a new one.
   if (url_fetcher_->state() == TranslateURLFetcher::REQUESTING) {
     DVLOG(2) << "Download is in progress.";
     return;
   }
 
-  DVLOG(2) << "Downloading model from: " << model_url_;
+  // If all allowed attempts have been exhausted, notify the loader that the
+  // backend has abandoned the download.
+  if (++download_attempts_ > kMaxDownloadAttempts) {
+    DVLOG(2) << "Model download abandoned.";
+    ReportModelStatus(RankerModelStatus::DOWNLOAD_FAILED);
+    TransferModelToClient(nullptr, true);
+    return;
+  }
 
-  // Reset the time of the next earliest allowable download attempt.
-  next_earliest_download_time_ =
-      base::TimeTicks::Now() +
base::TimeDelta::FromMinutes(kMinRetryDelayMins);
+  DVLOG(2) << "Downloading model from: " << model_url_;
 
-  // Kick off the next download attempt.
+  // Kick off the next download attempt and reset the time of the next earliest
+  // allowable download attempt.
   download_start_time_ = base::TimeTicks::Now();
-  bool result = url_fetcher_->Request(
+  next_earliest_download_time_ =
+      download_start_time_ + base::TimeDelta::FromMinutes(kMinRetryDelayMins);
+  url_fetcher_->Request(
       model_url_, base::Bind(&RankerModelLoader::Backend::OnDownloadComplete,
                              base::Unretained(this)));
-
-  // The maximum number of download attempts has been surpassed. Don't make
-  // any further attempts.
-  if (!result) {
-    DVLOG(2) << "Model download abandoned.";
-    ReportModelStatus(RankerModelStatus::DOWNLOAD_FAILED);
-    url_fetcher_.reset();
-
-    // Notify the loader that model loading has been abandoned.
-    TransferModelToClient(nullptr, true);
-  }
 }
 
 void RankerModelLoader::Backend::OnDownloadComplete(int /* id */,
diff --git a/components/translate/core/browser/translate_manager.cc
b/components/translate/core/browser/translate_manager.cc
index 60ae0bc5910c..8491a2adb164 100644
--- a/components/translate/core/browser/translate_manager.cc
+++ b/components/translate/core/browser/translate_manager.cc
@@ -231,7 +231,8 @@ void TranslateManager::InitiateTranslation(const
std::string& page_lang) {
   }
 
   bool ranker_should_offer_translation = true;
-  if (translate_ranker_->IsQueryEnabled()) {
+  if (translate_ranker_->IsQueryEnabled() ||
+      translate_ranker_->IsEnforcementEnabled()) {
     ranker_should_offer_translation =
translate_ranker_->ShouldOfferTranslation(
         *translate_prefs, language_code, target_lang);
     translate_event_->set_ranker_request_timestamp_sec(
diff --git a/components/translate/core/browser/translate_ranker_impl.cc
b/components/translate/core/browser/translate_ranker_impl.cc
index 5155092f26b0..80f60a411c08 100644
--- a/components/translate/core/browser/translate_ranker_impl.cc
+++ b/components/translate/core/browser/translate_ranker_impl.cc
@@ -80,7 +80,7 @@ const base::Feature kTranslateRankerEnforcement{
     "TranslateRankerEnforcement", base::FEATURE_DISABLED_BY_DEFAULT};
 
 const base::Feature kTranslateRankerLogging{"TranslateRankerLogging",
-                                            base::FEATURE_DISABLED_BY_DEFAULT};
+                                            base::FEATURE_ENABLED_BY_DEFAULT};
 
 TranslateRankerFeatures::TranslateRankerFeatures() {}
 
@@ -134,7 +134,7 @@ void TranslateRankerFeatures::WriteTo(std::ostream& stream)
const {
 TranslateRankerImpl::TranslateRankerImpl(const base::FilePath& model_path,
                                          const GURL& model_url)
     : weak_ptr_factory_(this) {
-  if (IsQueryEnabled()) {
+  if (IsQueryEnabled() || IsEnforcementEnabled()) {
     model_loader_ = base::MakeUnique<RankerModelLoader>(
         base::Bind(&ValidateModel),
         base::Bind(&TranslateRankerImpl::OnModelAvailable,
@@ -180,8 +180,7 @@ bool TranslateRankerImpl::IsLoggingEnabled() {
 }
 
 bool TranslateRankerImpl::IsQueryEnabled() {
-  return base::FeatureList::IsEnabled(kTranslateRankerQuery) ||
-         IsEnforcementEnabled();
+  return base::FeatureList::IsEnabled(kTranslateRankerQuery);
 }
 
 bool TranslateRankerImpl::IsEnforcementEnabled() {
@@ -204,6 +203,9 @@ bool TranslateRankerImpl::ShouldOfferTranslation(
   // (or become False).
   const bool kDefaultResponse = true;
 
+  if (model_loader_)
+    model_loader_->NotifyOfRankerActivity();
+
   // If we don't have a model, request one and return the default.
   if (model_ == nullptr) {
     return kDefaultResponse;
@@ -266,9 +268,10 @@ void TranslateRankerImpl::FlushTranslateEvents(
 void TranslateRankerImpl::AddTranslateEvent(
     const metrics::TranslateEventProto& event) {
   DCHECK(sequence_checker_.CalledOnValidSequence());
-  DVLOG(3) << "Adding translate ranker event.";
-  if (IsLoggingEnabled())
+  if (IsLoggingEnabled()) {
+    DVLOG(3) << "Adding translate ranker event.";
     event_cache_.push_back(event);
+  }
 }
 
 void TranslateRankerImpl::OnModelAvailable(std::unique_ptr<RankerModel> model)
{

[Roger McFarlane (Chromium), 2017-03-31 16:07:54]

Remaining diff, follows...

**********************************************************

diff --git a/components/translate/core/browser/translate_ranker_impl_unittest.cc
b/components/translate/core/browser/translate_ranker_impl_unittest.cc
index 7a665be6627c..ae7c64c65308 100644
--- a/components/translate/core/browser/translate_ranker_impl_unittest.cc
+++ b/components/translate/core/browser/translate_ranker_impl_unittest.cc
@@ -199,7 +199,7 @@ TEST_F(TranslateRankerImplTest, DisabledByDefault) {
   auto ranker = GetRankerForTest(0.5);
   EXPECT_FALSE(ranker->IsQueryEnabled());
   EXPECT_FALSE(ranker->IsEnforcementEnabled());
-  EXPECT_FALSE(ranker->IsLoggingEnabled());
+  EXPECT_TRUE(ranker->IsLoggingEnabled());
 }
 
 TEST_F(TranslateRankerImplTest, ExplicitlyDisabled) {
@@ -216,15 +216,15 @@ TEST_F(TranslateRankerImplTest, EnableQuery) {
   auto ranker = GetRankerForTest(0.5);
   EXPECT_TRUE(ranker->IsQueryEnabled());
   EXPECT_FALSE(ranker->IsEnforcementEnabled());
-  EXPECT_FALSE(ranker->IsLoggingEnabled());
+  EXPECT_TRUE(ranker->IsLoggingEnabled());
 }
 
 TEST_F(TranslateRankerImplTest, EnableEnforcement) {
   InitFeatures({kTranslateRankerEnforcement}, {kTranslateRankerQuery});
   auto ranker = GetRankerForTest(0.5);
-  EXPECT_TRUE(ranker->IsQueryEnabled());
+  EXPECT_FALSE(ranker->IsQueryEnabled());
   EXPECT_TRUE(ranker->IsEnforcementEnabled());
-  EXPECT_FALSE(ranker->IsLoggingEnabled());
+  EXPECT_TRUE(ranker->IsLoggingEnabled());
 }
 
 TEST_F(TranslateRankerImplTest, EnableQueryAndEnforcement) {
@@ -232,7 +232,7 @@ TEST_F(TranslateRankerImplTest, EnableQueryAndEnforcement) {
   auto ranker = GetRankerForTest(0.5);
   EXPECT_TRUE(ranker->IsQueryEnabled());
   EXPECT_TRUE(ranker->IsEnforcementEnabled());
-  EXPECT_FALSE(ranker->IsLoggingEnabled());
+  EXPECT_TRUE(ranker->IsLoggingEnabled());
 }
 
 TEST_F(TranslateRankerImplTest, EnableLogging) {
@@ -243,6 +243,14 @@ TEST_F(TranslateRankerImplTest, EnableLogging) {
   EXPECT_TRUE(ranker->IsLoggingEnabled());
 }
 
+TEST_F(TranslateRankerImplTest, DisableLogging) {
+  InitFeatures({}, {kTranslateRankerLogging});
+  auto ranker = GetRankerForTest(0.5);
+  EXPECT_FALSE(ranker->IsQueryEnabled());
+  EXPECT_FALSE(ranker->IsEnforcementEnabled());
+  EXPECT_FALSE(ranker->IsLoggingEnabled());
+}
+
 TEST_F(TranslateRankerImplTest, EnableAll) {
   InitFeatures({kTranslateRankerQuery, kTranslateRankerEnforcement,
                 kTranslateRankerLogging},
diff --git a/testing/variations/fieldtrial_testing_config.json
b/testing/variations/fieldtrial_testing_config.json
index 0aca586efc47..c3c42ed3456c 100644
--- a/testing/variations/fieldtrial_testing_config.json
+++ b/testing/variations/fieldtrial_testing_config.json
@@ -2693,6 +2693,29 @@
             ]
         }
     ],
+    "TranslateRanker": [
+        {
+            "platforms": [
+                "android",
+                "chromeos",
+                "ios",
+                "linux",
+                "mac",
+                "win"
+            ],
+            "experiments": [
+                {
+                    "name": "EnforcementEnabled",
+                    "params": {
+                        "translate-ranker-model-url":
"https://www.gstatic.com/chrome/intelligence/assist/ranker/models/translate/2017/02/translate_ranker_model_20170217.pb.bin"
+                    },
+                    "enable_features": [
+                        "TranslateRankerEnforcement"
+                    ]
+                }
+            ]
+        }
+    ],
     "TranslateRankerLogging": [
         {
             "platforms": [

[Roger McFarlane (Chromium), 2017-03-31 16:08:44]

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

[commit-bot: I haz the power, 2017-03-31 16:09:26]

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-31 18:12:01]

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

[commit-bot: I haz the power, 2017-03-31 18:12:03]

Dry run: This issue passed the CQ dry run.

[rkaplow, 2017-04-03 03:14:25]

https://codereview.chromium.org/2785493004/diff/160001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2785493004/diff/160001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:113366: -  <int value="4"
label="Validation Failed"/>
you can actually leave the original enum. You're allowed to append enum entries.

[droger, 2017-04-03 09:18:08]

Only did a quick pass since this was already mostly reviewed.

LGTM with comments.

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

https://codereview.chromium.org/2785493004/diff/160001/chrome/browser/transla...
chrome/browser/translate/chrome_translate_client.cc:77:
translate::TranslateRankerFactory::GetInstance()
You can use TranslateRankerFactory::GetForBrowserContext directly.
No need for GetInstance().

https://codereview.chromium.org/2785493004/diff/160001/chrome/browser/transla...
File chrome/browser/translate/translate_ranker_factory.cc (right):

https://codereview.chromium.org/2785493004/diff/160001/chrome/browser/transla...
chrome/browser/translate/translate_ranker_factory.cc:11: #include
"components/translate/core/common/translate_switches.h"
Remove this include?

https://codereview.chromium.org/2785493004/diff/160001/chrome/browser/transla...
chrome/browser/translate/translate_ranker_factory.cc:12: #include
"components/variations/variations_associated_data.h"
Is this needed?

https://codereview.chromium.org/2785493004/diff/160001/chrome/browser/transla...
File chrome/browser/translate/translate_ranker_factory.h (right):

https://codereview.chromium.org/2785493004/diff/160001/chrome/browser/transla...
chrome/browser/translate/translate_ranker_factory.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2785493004/diff/160001/chrome/browser/transla...
chrome/browser/translate/translate_ranker_factory.h:19: content::BrowserContext*
browser_context);
Nit: I would forward-declare the BrowserContext class.

https://codereview.chromium.org/2785493004/diff/160001/chrome/browser/transla...
File chrome/browser/translate/translate_ranker_metrics_provider.cc (right):

https://codereview.chromium.org/2785493004/diff/160001/chrome/browser/transla...
chrome/browser/translate/translate_ranker_metrics_provider.cc:1: // Copyright
2016 The Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2785493004/diff/160001/chrome/browser/transla...
chrome/browser/translate/translate_ranker_metrics_provider.cc:22:
TranslateRankerFactory::GetInstance()->GetForBrowserContext(profile);
Remove GetInstance()

https://codereview.chromium.org/2785493004/diff/160001/components/translate/c...
File components/translate/core/browser/mock_translate_ranker.cc (right):

https://codereview.chromium.org/2785493004/diff/160001/components/translate/c...
components/translate/core/browser/mock_translate_ranker.cc:1: // Copyright 2016
The Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2785493004/diff/160001/components/translate/c...
File components/translate/core/browser/mock_translate_ranker.h (right):

https://codereview.chromium.org/2785493004/diff/160001/components/translate/c...
components/translate/core/browser/mock_translate_ranker.h:1: // Copyright 2016
The Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2785493004/diff/160001/components/translate/c...
components/translate/core/browser/mock_translate_ranker.h:37: //
TranslateRanker...
// TranslateRanker implementation:

https://codereview.chromium.org/2785493004/diff/160001/components/translate/c...
components/translate/core/browser/mock_translate_ranker.h:62: }  // namespace
testing
blank line between the two braces.

https://codereview.chromium.org/2785493004/diff/160001/ios/chrome/browser/tra...
File ios/chrome/browser/translate/chrome_ios_translate_client.mm (right):

https://codereview.chromium.org/2785493004/diff/160001/ios/chrome/browser/tra...
ios/chrome/browser/translate/chrome_ios_translate_client.mm:43:
translate::TranslateRankerFactory::GetInstance()->GetForBrowserState(
remove GetInstance()

[Roger McFarlane (Chromium), 2017-04-03 17:40:08]

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

[commit-bot: I haz the power, 2017-04-03 17:40:38]

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

[Roger McFarlane (Chromium), 2017-04-03 17:44:55]

Thanks.

rkaplow@... another look?

sdefresne@... ping?

rkaplow@ and sdefresne@... should I have a metrics provider for ios/web_view?
If, not, it looks like to should disable ranker logging  for ios/web_view?
(otherwise it will accumulate event records)

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

https://codereview.chromium.org/2785493004/diff/160001/chrome/browser/transla...
chrome/browser/translate/chrome_translate_client.cc:77:
translate::TranslateRankerFactory::GetInstance()
On 2017/04/03 09:18:08, droger wrote:
> You can use TranslateRankerFactory::GetForBrowserContext directly.
> No need for GetInstance().

Done.

https://codereview.chromium.org/2785493004/diff/160001/chrome/browser/transla...
File chrome/browser/translate/translate_ranker_factory.cc (right):

https://codereview.chromium.org/2785493004/diff/160001/chrome/browser/transla...
chrome/browser/translate/translate_ranker_factory.cc:11: #include
"components/translate/core/common/translate_switches.h"
On 2017/04/03 09:18:08, droger wrote:
> Remove this include?

Done.

https://codereview.chromium.org/2785493004/diff/160001/chrome/browser/transla...
chrome/browser/translate/translate_ranker_factory.cc:12: #include
"components/variations/variations_associated_data.h"
On 2017/04/03 09:18:08, droger wrote:
> Is this needed?

Nopes

https://codereview.chromium.org/2785493004/diff/160001/chrome/browser/transla...
File chrome/browser/translate/translate_ranker_factory.h (right):

https://codereview.chromium.org/2785493004/diff/160001/chrome/browser/transla...
chrome/browser/translate/translate_ranker_factory.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2017/04/03 09:18:08, droger wrote:
> 2017

Done.

https://codereview.chromium.org/2785493004/diff/160001/chrome/browser/transla...
chrome/browser/translate/translate_ranker_factory.h:19: content::BrowserContext*
browser_context);
On 2017/04/03 09:18:08, droger wrote:
> Nit: I would forward-declare the BrowserContext class.

Done.

https://codereview.chromium.org/2785493004/diff/160001/chrome/browser/transla...
File chrome/browser/translate/translate_ranker_metrics_provider.cc (right):

https://codereview.chromium.org/2785493004/diff/160001/chrome/browser/transla...
chrome/browser/translate/translate_ranker_metrics_provider.cc:1: // Copyright
2016 The Chromium Authors. All rights reserved.
On 2017/04/03 09:18:08, droger wrote:
> 2017

Done.

https://codereview.chromium.org/2785493004/diff/160001/chrome/browser/transla...
chrome/browser/translate/translate_ranker_metrics_provider.cc:22:
TranslateRankerFactory::GetInstance()->GetForBrowserContext(profile);
On 2017/04/03 09:18:08, droger wrote:
> Remove GetInstance()

Done.

https://codereview.chromium.org/2785493004/diff/160001/components/translate/c...
File components/translate/core/browser/mock_translate_ranker.cc (right):

https://codereview.chromium.org/2785493004/diff/160001/components/translate/c...
components/translate/core/browser/mock_translate_ranker.cc:1: // Copyright 2016
The Chromium Authors. All rights reserved.
On 2017/04/03 09:18:08, droger wrote:
> 2017

Done.

https://codereview.chromium.org/2785493004/diff/160001/components/translate/c...
File components/translate/core/browser/mock_translate_ranker.h (right):

https://codereview.chromium.org/2785493004/diff/160001/components/translate/c...
components/translate/core/browser/mock_translate_ranker.h:1: // Copyright 2016
The Chromium Authors. All rights reserved.
On 2017/04/03 09:18:08, droger wrote:
> 2017

Done.

https://codereview.chromium.org/2785493004/diff/160001/components/translate/c...
components/translate/core/browser/mock_translate_ranker.h:37: //
TranslateRanker...
On 2017/04/03 09:18:08, droger wrote:
> // TranslateRanker implementation:

Done.

https://codereview.chromium.org/2785493004/diff/160001/components/translate/c...
components/translate/core/browser/mock_translate_ranker.h:62: }  // namespace
testing
On 2017/04/03 09:18:08, droger wrote:
> blank line between the two braces.

Done.

https://codereview.chromium.org/2785493004/diff/160001/ios/chrome/browser/tra...
File ios/chrome/browser/translate/chrome_ios_translate_client.mm (right):

https://codereview.chromium.org/2785493004/diff/160001/ios/chrome/browser/tra...
ios/chrome/browser/translate/chrome_ios_translate_client.mm:43:
translate::TranslateRankerFactory::GetInstance()->GetForBrowserState(
On 2017/04/03 09:18:08, droger wrote:
> remove GetInstance()

Done.

https://codereview.chromium.org/2785493004/diff/160001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2785493004/diff/160001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:113366: -  <int value="4"
label="Validation Failed"/>
On 2017/04/03 03:14:25, rkaplow (slow) wrote:
> you can actually leave the original enum. You're allowed to append enum
entries.

 This change should've landed with https://codereview.chromium.org/2761363002/

The loader is more general than just translate, so the Translate prefix was
dropped form the name.

[Roger McFarlane (Chromium), 2017-04-03 17:47:16]

Description was changed from

==========
Integrate RankerModelLoader with TranslateRanker.

This CL replaces the model loading code in the TranslateRanker with
the use of the RankerModelLoader.

* the TranslateRanker transitions to a KeyedService
* the TranslateRanker is split into interface and implementation,
  facilitationt mocking the TranslateRanker in tests.
* Browser-level TransalteRankerFactories are added in order to
  pass the appropriate model cache path and model url to the ranker
  and, in turn, the model loader.
* Metrics providers have been promoted to browser level to allow for
  accessing it as a keyed servie across all loaded profiles.

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

to

==========
Integrate RankerModelLoader with TranslateRanker.

This CL replaces the model loading code in the TranslateRanker with
the use of the RankerModelLoader.

* the TranslateRanker transitions to a KeyedService
* the TranslateRanker is split into interface and implementation,
  facilitationt mocking the TranslateRanker in tests.
* Browser-level TransalteRankerFactories are added in order to
  pass the appropriate model cache path and model url to the ranker
  and, in turn, the model loader.
* Metrics providers have been promoted to browser level to allow for
  accessing it as a keyed servie across all loaded profiles.

BUG=646711, 697665, 698057
==========

[Roger McFarlane (Chromium), 2017-04-03 17:50:28]

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

[Roger McFarlane (Chromium), 2017-04-03 17:50:28]

+emaxx@... re: crbug/698057 - switched to using GetLoadedProfiles() instead of
GetLastOpenedProfiles() to avoid CHECK on CrOS.

[Roger McFarlane (Chromium), 2017-04-03 17:50:33]

+emaxx@... re: crbug/698057 - switched to using GetLoadedProfiles() instead of
GetLastOpenedProfiles() to avoid CHECK on CrOS.

[Roger McFarlane (Chromium), 2017-04-03 17:59:22]

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

[commit-bot: I haz the power, 2017-04-03 17:59:49]

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-03 19:12:53]

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

[commit-bot: I haz the power, 2017-04-03 19:12:54]

Dry run: This issue passed the CQ dry run.

[rkaplow, 2017-04-03 19:22:52]

lgtm

oh ok, missed that the intention was for the rename for the enum.

For ios webview, it doesn't look to me like we have any metrics collection
there. So you're right any logging should be disabled

[Roger McFarlane (Chromium), 2017-04-03 20:16:59]

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

[Roger McFarlane (Chromium), 2017-04-03 20:17:00]

+eugenebut for ios/web_view specific question...

eugenebut/sdefresne:

I was looking through the Chrome Translate Ranker Event logging in the above CL
and noting that there were metric providers for the other platforms but not for
IOS Web View.

This would suggest to me that (with translate event logging turned on by
default) we could end up accumulating event metrics that are never consumed (and
subsequently freed) by the metrics subsystem.

I've amended the CL to disable the logging if CWV_IMPLEMENTATION is defined.

Is this the right thing to do here?

[Eugene But (OOO till 7-30), 2017-04-03 20:59:23]

On 2017/04/03 20:17:00, Roger McFarlane (Chromium) wrote:
> +eugenebut for ios/web_view specific question...
> 
> eugenebut/sdefresne:
> 
> I was looking through the Chrome Translate Ranker Event logging in the above
CL
> and noting that there were metric providers for the other platforms but not
for
> IOS Web View.
> 
> This would suggest to me that (with translate event logging turned on by
> default) we could end up accumulating event metrics that are never consumed
(and
> subsequently freed) by the metrics subsystem.
> 
> I've amended the CL to disable the logging if CWV_IMPLEMENTATION is defined.
> 
> Is this the right thing to do here?
I think CWV_IMPLEMENTATION is a temporary hack to deal with header includes and
we should not use this macro in Chromium code. For ios/web_view we should not
report any metrics, but I'm not familiar with metrics code and don't really know
what is the best way to disable them.

[droger, 2017-04-04 08:57:16]

On 2017/04/03 20:17:00, Roger McFarlane (Chromium) wrote:
> +eugenebut for ios/web_view specific question...
> 
> eugenebut/sdefresne:
> 
> I was looking through the Chrome Translate Ranker Event logging in the above
CL
> and noting that there were metric providers for the other platforms but not
for
> IOS Web View.
> 
> This would suggest to me that (with translate event logging turned on by
> default) we could end up accumulating event metrics that are never consumed
(and
> subsequently freed) by the metrics subsystem.
> 
> I've amended the CL to disable the logging if CWV_IMPLEMENTATION is defined.
> 
> Is this the right thing to do here?

OS_IOS would be preferred.

Can you give more details though? What exactly is missing on iOS? Looking at the
code, it seems that the log are collected by the MetricsService, which does
exist on iOS. I'm not familiar with this though, so I may be missing something.

[sdefresne, 2017-04-04 09:40:23]

https://codereview.chromium.org/2785493004/diff/220001/components/translate/c...
File components/translate/core/browser/translate_ranker_impl.cc (right):

https://codereview.chromium.org/2785493004/diff/220001/components/translate/c...
components/translate/core/browser/translate_ranker_impl.cc:82: #if
defined(CWV_IMPLEMENTATION)
This macro will not be defined when building the file as part of building
ChromeWebView on iOS (the macro is only defined when building the files in
src/ios/web_view and is an implementation detail). Thus this test will always be
false, and the second branch will be taken on iOS, even when building
ChromeWebView on iOS.

All files in components will be built the same way when building both Chrome and
ChromeWebView (i.e. the same build flags, same macro defined, ...). You can
check that by adding #error in that branch of the #if and you'll see that the
build succeed on all iOS bots. The CWV_IMPLEMENTATION is only used to set the
symbol visibility while building the library (like BASE_EXPORT).

If you want this code to behave differently when used from Chrome or
ChromeWebView, then the configuration need to be done at runtime instead of
compile time.

[emaxx, 2017-04-04 13:22:38]

https://codereview.chromium.org/2785493004/diff/220001/chrome/browser/transla...
File chrome/browser/translate/translate_ranker_metrics_provider.cc (right):

https://codereview.chromium.org/2785493004/diff/220001/chrome/browser/transla...
chrome/browser/translate/translate_ranker_metrics_provider.cc:19:
g_browser_process->profile_manager()->GetLoadedProfiles();
Note that on Chrome OS there is a special sign-in profile which doesn't
correspond to any user session - it's used for hosting the login screen
controls. (Technically speaking, there are actually two profiles: one is the
Default profile and another is the incognito profile derived from it. Different
parts of Chrome choose themselves which one they want to use.)

So is this the expected behavior to track these UMA metrics for this profile?

In case it's not, then it should be probably checked here - e.g. using the
chromeos::ProfileHelper::IsSigninProfile method. Also the similar guard may need
to be added into TranslateRankerFactory (but, again, I don't know what is really
required for your feature).

[Roger McFarlane (Chromium), 2017-04-04 17:03:06]

droger@:

The question isn't about Chrome iOS, it's with the iOS Web View, which seems to
be a sibling target, that does not appear to have full metrics integration.

https://codereview.chromium.org/2785493004/diff/220001/chrome/browser/transla...
File chrome/browser/translate/translate_ranker_metrics_provider.cc (right):

https://codereview.chromium.org/2785493004/diff/220001/chrome/browser/transla...
chrome/browser/translate/translate_ranker_metrics_provider.cc:19:
g_browser_process->profile_manager()->GetLoadedProfiles();
On 2017/04/04 13:22:38, emaxx wrote:
> Note that on Chrome OS there is a special sign-in profile which doesn't
> correspond to any user session - it's used for hosting the login screen
> controls. (Technically speaking, there are actually two profiles: one is the
> Default profile and another is the incognito profile derived from it.
Different
> parts of Chrome choose themselves which one they want to use.)
> 
> So is this the expected behavior to track these UMA metrics for this profile?
> 
> In case it's not, then it should be probably checked here - e.g. using the
> chromeos::ProfileHelper::IsSigninProfile method. Also the similar guard may
need
> to be added into TranslateRankerFactory (but, again, I don't know what is
really
> required for your feature).

As long as I get back a valid set of profile pointers without triggering a
profile load (while missing the credentials to open the profile) I think I'm ok.
The events will only be populated for profiles the user is actively using to
browse; it should be fine if I grab the "special" profiles... they'll just have
an empty event cache.

https://codereview.chromium.org/2785493004/diff/220001/components/translate/c...
File components/translate/core/browser/translate_ranker_impl.cc (right):

https://codereview.chromium.org/2785493004/diff/220001/components/translate/c...
components/translate/core/browser/translate_ranker_impl.cc:82: #if
defined(CWV_IMPLEMENTATION)
On 2017/04/04 09:40:23, sdefresne wrote:
> This macro will not be defined when building the file as part of building
> ChromeWebView on iOS (the macro is only defined when building the files in
> src/ios/web_view and is an implementation detail). Thus this test will always
be
> false, and the second branch will be taken on iOS, even when building
> ChromeWebView on iOS.
> 
> All files in components will be built the same way when building both Chrome
and
> ChromeWebView (i.e. the same build flags, same macro defined, ...). You can
> check that by adding #error in that branch of the #if and you'll see that the
> build succeed on all iOS bots. The CWV_IMPLEMENTATION is only used to set the
> symbol visibility while building the library (like BASE_EXPORT).
> 
> If you want this code to behave differently when used from Chrome or
> ChromeWebView, then the configuration need to be done at runtime instead of
> compile time.

Thanks for clarifying that!

Are you aware of any way to check at run-time whether this is Chrome or
web-view?

[emaxx, 2017-04-04 17:12:38]

LGTM regarding the use of GetLoadedProfiles for avoiding misbehavior on Chrome
OS

https://codereview.chromium.org/2785493004/diff/220001/chrome/browser/transla...
File chrome/browser/translate/translate_ranker_metrics_provider.cc (right):

https://codereview.chromium.org/2785493004/diff/220001/chrome/browser/transla...
chrome/browser/translate/translate_ranker_metrics_provider.cc:19:
g_browser_process->profile_manager()->GetLoadedProfiles();
On 2017/04/04 17:03:06, Roger McFarlane (Chromium) wrote:
> As long as I get back a valid set of profile pointers without triggering a
> profile load (while missing the credentials to open the profile) I think I'm
ok.
> The events will only be populated for profiles the user is actively using to
> browse; it should be fine if I grab the "special" profiles... they'll just
have
> an empty event cache.

Thanks for the clarification.

[Eugene But (OOO till 7-30), 2017-04-04 18:19:00]

https://codereview.chromium.org/2785493004/diff/220001/components/translate/c...
File components/translate/core/browser/translate_ranker_impl.cc (right):

https://codereview.chromium.org/2785493004/diff/220001/components/translate/c...
components/translate/core/browser/translate_ranker_impl.cc:82: #if
defined(CWV_IMPLEMENTATION)
On 2017/04/04 17:03:06, Roger McFarlane (Chromium) wrote:
> On 2017/04/04 09:40:23, sdefresne wrote:
> > This macro will not be defined when building the file as part of building
> > ChromeWebView on iOS (the macro is only defined when building the files in
> > src/ios/web_view and is an implementation detail). Thus this test will
always
> be
> > false, and the second branch will be taken on iOS, even when building
> > ChromeWebView on iOS.
> > 
> > All files in components will be built the same way when building both Chrome
> and
> > ChromeWebView (i.e. the same build flags, same macro defined, ...). You can
> > check that by adding #error in that branch of the #if and you'll see that
the
> > build succeed on all iOS bots. The CWV_IMPLEMENTATION is only used to set
the
> > symbol visibility while building the library (like BASE_EXPORT).
> > 
> > If you want this code to behave differently when used from Chrome or
> > ChromeWebView, then the configuration need to be done at runtime instead of
> > compile time.
> 
> Thanks for clarifying that!
> 
> Are you aware of any way to check at run-time whether this is Chrome or
> web-view?
> 
> 
There is no way to check that. //component layer does not and should not know
about chrome and ios_web_view layers.

[Eugene But (OOO till 7-30), 2017-04-04 18:19:00]

https://codereview.chromium.org/2785493004/diff/220001/components/translate/c...
File components/translate/core/browser/translate_ranker_impl.cc (right):

https://codereview.chromium.org/2785493004/diff/220001/components/translate/c...
components/translate/core/browser/translate_ranker_impl.cc:82: #if
defined(CWV_IMPLEMENTATION)
On 2017/04/04 17:03:06, Roger McFarlane (Chromium) wrote:
> On 2017/04/04 09:40:23, sdefresne wrote:
> > This macro will not be defined when building the file as part of building
> > ChromeWebView on iOS (the macro is only defined when building the files in
> > src/ios/web_view and is an implementation detail). Thus this test will
always
> be
> > false, and the second branch will be taken on iOS, even when building
> > ChromeWebView on iOS.
> > 
> > All files in components will be built the same way when building both Chrome
> and
> > ChromeWebView (i.e. the same build flags, same macro defined, ...). You can
> > check that by adding #error in that branch of the #if and you'll see that
the
> > build succeed on all iOS bots. The CWV_IMPLEMENTATION is only used to set
the
> > symbol visibility while building the library (like BASE_EXPORT).
> > 
> > If you want this code to behave differently when used from Chrome or
> > ChromeWebView, then the configuration need to be done at runtime instead of
> > compile time.
> 
> Thanks for clarifying that!
> 
> Are you aware of any way to check at run-time whether this is Chrome or
> web-view?
> 
> 
There is no way to check that. //component layer does not and should not know
about chrome and ios_web_view layers.

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

Thanks.

eugenebut@ and/or sdefresne@: Another look?

I explicitly disable translate ranker event logging from the Chrome IOS Web
View's translate ranker factory.

https://codereview.chromium.org/2785493004/diff/220001/components/translate/c...
File components/translate/core/browser/translate_ranker_impl.cc (right):

https://codereview.chromium.org/2785493004/diff/220001/components/translate/c...
components/translate/core/browser/translate_ranker_impl.cc:82: #if
defined(CWV_IMPLEMENTATION)
On 2017/04/04 18:19:00, Eugene But wrote:
> On 2017/04/04 17:03:06, Roger McFarlane (Chromium) wrote:
> > On 2017/04/04 09:40:23, sdefresne wrote:
> > > This macro will not be defined when building the file as part of building
> > > ChromeWebView on iOS (the macro is only defined when building the files in
> > > src/ios/web_view and is an implementation detail). Thus this test will
> always
> > be
> > > false, and the second branch will be taken on iOS, even when building
> > > ChromeWebView on iOS.
> > > 
> > > All files in components will be built the same way when building both
Chrome
> > and
> > > ChromeWebView (i.e. the same build flags, same macro defined, ...). You
can
> > > check that by adding #error in that branch of the #if and you'll see that
> the
> > > build succeed on all iOS bots. The CWV_IMPLEMENTATION is only used to set
> the
> > > symbol visibility while building the library (like BASE_EXPORT).
> > > 
> > > If you want this code to behave differently when used from Chrome or
> > > ChromeWebView, then the configuration need to be done at runtime instead
of
> > > compile time.
> > 
> > Thanks for clarifying that!
> > 
> > Are you aware of any way to check at run-time whether this is Chrome or
> > web-view?
> > 
> > 
> There is no way to check that. //component layer does not and should not know
> about chrome and ios_web_view layers.

Fair enough.

Added a member to track whether or not logging is enabled. This is set shortly
after construction by the ios web view translate ranker factory.

[Eugene But (OOO till 7-30), 2017-04-04 19:51:57]

ios lgtm

[Roger McFarlane (Chromium), 2017-04-04 20:01:56]

Patchset #14 (id:260001) has been deleted

[Roger McFarlane (Chromium), 2017-04-04 20:05:26]

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

[commit-bot: I haz the power, 2017-04-04 20:06:33]

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-04 20:26:22]

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

[commit-bot: I haz the power, 2017-04-04 20:26:24]

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-...)

[Roger McFarlane (Chromium), 2017-04-04 20:40:36]

Patchset #15 (id:300001) has been deleted

[Roger McFarlane (Chromium), 2017-04-04 22:00:38]

Patchset #15 (id:320001) has been deleted

[Roger McFarlane (Chromium), 2017-04-04 22:00:50]

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

[commit-bot: I haz the power, 2017-04-04 22:01: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-04-05 01:36:15]

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

[commit-bot: I haz the power, 2017-04-05 01:36:16]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sdefresne, 2017-04-05 09:33:43]

lgtm

https://codereview.chromium.org/2785493004/diff/340001/ios/web_view/internal/...
File ios/web_view/internal/translate/web_view_translate_ranker_factory.cc
(right):

https://codereview.chromium.org/2785493004/diff/340001/ios/web_view/internal/...
ios/web_view/internal/translate/web_view_translate_ranker_factory.cc:50: return
std::move(ranker);  // Fails xcode-clang compile without the move.
Please change to the following so we can clean this once the xcode-clang issue
is fixed:

  // TODO(crbug.com/703565): remove std::move() once Xcode 9.0+ is required.
  return std::move(ranker);

[Roger McFarlane (Chromium), 2017-04-05 12:05:28]

The CQ bit was checked by rogerm@chromium.org

[Roger McFarlane (Chromium), 2017-04-05 12:05:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
hamelphi@chromium.org, droger@chromium.org, rkaplow@chromium.org,
emaxx@chromium.org, eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2785493004/#ps340001
(title: "fix bogus compile error")

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

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-05 12:11:57]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1491393928576820,
"parent_rev": "27711f411908663e20ac16bc6c3826e09e8b6d0b", "commit_rev":
"95c5541219a088802bf259cc36ab7fd4a7ae98e7"}

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

Description was changed from

==========
Integrate RankerModelLoader with TranslateRanker.

This CL replaces the model loading code in the TranslateRanker with
the use of the RankerModelLoader.

* the TranslateRanker transitions to a KeyedService
* the TranslateRanker is split into interface and implementation,
  facilitationt mocking the TranslateRanker in tests.
* Browser-level TransalteRankerFactories are added in order to
  pass the appropriate model cache path and model url to the ranker
  and, in turn, the model loader.
* Metrics providers have been promoted to browser level to allow for
  accessing it as a keyed servie across all loaded profiles.

BUG=646711, 697665, 698057
==========

to

==========
Integrate RankerModelLoader with TranslateRanker.

This CL replaces the model loading code in the TranslateRanker with
the use of the RankerModelLoader.

* the TranslateRanker transitions to a KeyedService
* the TranslateRanker is split into interface and implementation,
  facilitationt mocking the TranslateRanker in tests.
* Browser-level TransalteRankerFactories are added in order to
  pass the appropriate model cache path and model url to the ranker
  and, in turn, the model loader.
* Metrics providers have been promoted to browser level to allow for
  accessing it as a keyed servie across all loaded profiles.

BUG=646711, 697665, 698057

Review-Url: https://codereview.chromium.org/2785493004
Cr-Commit-Position: refs/heads/master@{#462038}
Committed:
https://chromium.googlesource.com/chromium/src/+/95c5541219a088802bf259cc36ab...
==========

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

Committed patchset #15 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/95c5541219a088802bf259cc36ab...

[Roger McFarlane (Chromium), 2017-04-07 02:35:07]

https://codereview.chromium.org/2785493004/diff/340001/ios/web_view/internal/...
File ios/web_view/internal/translate/web_view_translate_ranker_factory.cc
(right):

https://codereview.chromium.org/2785493004/diff/340001/ios/web_view/internal/...
ios/web_view/internal/translate/web_view_translate_ranker_factory.cc:50: return
std::move(ranker);  // Fails xcode-clang compile without the move.
On 2017/04/05 09:33:43, sdefresne wrote:
> Please change to the following so we can clean this once the xcode-clang issue
> is fixed:
> 
>   // TODO(crbug.com/703565): remove std::move() once Xcode 9.0+ is required.
>   return std::move(ranker);

Oops! Missed this.

Created and landed https://codereview.chromium.org/2800833002