[translate] Add translate ranker model loader.

components/translate/core/browser/ranker_model_loader.h

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_TRANSLATE_CORE_BROWSER_RANKER_MODEL_LOADER_H_
+#define COMPONENTS_TRANSLATE_CORE_BROWSER_RANKER_MODEL_LOADER_H_
+
+#include <memory>
+#include <string>
+
+#include "base/files/file_path.h"
+#include "base/memory/ref_counted.h"
+#include "base/sequence_checker.h"
+#include "base/threading/thread.h"
+#include "url/gurl.h"
+
+namespace chrome_intelligence {
+class RankerModel;
+}  // namespace chrome_intelligence
+
+namespace translate {
+
+// Enumeration denoting the outcome of an attempt to download the model. This
+// must be kept in sync with the TranslateRankerModelStatus enum in
+// histograms.xml
+// TODO(rogerm): rename the enum in histograms.xml to be more generic

[groby-ooo-7-16, 2017/02/14 20:56:48]

Can you do that as part of this CL?

[Roger McFarlane (Chromium), 2017/02/21 22:16:51]

Done.

+enum class RankerModelStatus : int {
+  OK = 0,
+  DOWNLOAD_THROTTLED = 1,
+  DOWNLOAD_FAILED = 2,
+  PARSE_FAILED = 3,
+  VALIDATION_FAILED = 4,
+  INCOMPATIBLE = 5,
+
+  // Insert new values above this line.
+  MAX
+};
+
+// If enabled, downloads a translate ranker model and uses it to determine
+// whether the user should be given a translation prompt or not.
+class RankerModelLoader {
+ public:
+  // Callback to validate a ranker model on behalf of the model loader client.
+  // For example, the callback might validate that the model is compatible with
+  // the features generated when ranking translation offerings. This callback
+  // may be called on any sequence and must, therefore, be thread-safe.

[fdoray, 2017/02/15 20:07:40]

+ // This callback may be invoked after RankerModelLoader has been destroyed and
therefore shouldn't access memory that it doesn't own.

(Since the callback outlives RankerModelLoader, there is no way to know when it
is safe to release memory that is accessed but not owned by the callback.)

[Roger McFarlane (Chromium), 2017/02/21 22:16:50]

Done.

+  using ValidateCallback = base::Callback<RankerModelStatus(

[hamelphi, 2017/02/13 19:18:58]

s\ValidateCallback\ValidateModelCallback ?

I think it would make it more obvious what the purpose of this callback is.
I don't insist.

[Roger McFarlane (Chromium), 2017/02/21 22:16:50]

Done.

+      const chrome_intelligence::RankerModel& model)>;
+
+  // Called to transfer ownership of a loaded model back to the model loader
+  // client. This will be called on the sequence on which the model loader was

[groby-ooo-7-16, 2017/02/14 20:56:48]

Since we're having a set of callbacks here, does it make sense to use a Delegate
instead of several callbacks?

[fdoray, 2017/02/15 20:07:40]

Since ValidateCallback can be invoked after the RankerModelLoader has been
destroyed, the Delegate would have to be owned by the RankerModelLoader. This is
undesirable.

[Roger McFarlane (Chromium), 2017/02/21 22:16:50]

Acknowledged.

[Roger McFarlane (Chromium), 2017/02/21 22:16:51]

fdoray@ suggested to revert to callbacks (instead of an observer/delegate) to
simplify lifetime and dependency management.

+  // constructed.
+  using OnModelAvailableCallback = base::Callback<void(
+      std::unique_ptr<chrome_intelligence::RankerModel> model)>;
+
+  // |valildate_callback| may be called on any sequence; it must, therefore,

[groby-ooo-7-16, 2017/02/14 20:56:48]

s/valildate/validate

[Roger McFarlane (Chromium), 2017/02/21 22:16:50]

Done.

+  // be thread-safe.
+  //
+  // |on_model_available_callback| will be called on the sequence on which the
+  // ranker model loader is constructed.
+  //
+  // |model_path| denotes the file path

[groby-ooo-7-16, 2017/02/14 20:56:48]

nit: end with period.

More importantly: This probably deserves an explanation why there is both a URL
and a path.

[Roger McFarlane (Chromium), 2017/02/21 22:16:50]

Done.

+  //
+  // |uma_prefix| will be used to start all UMA metrics genereted by this

[hamelphi, 2017/02/13 19:18:58]

generated

[hamelphi, 2017/02/13 19:18:58]

Nit.
I was confused a bit by "used to start..."
Maybe "will be used as a prefix for..."

[Roger McFarlane (Chromium), 2017/02/21 22:16:50]

Done.

[Roger McFarlane (Chromium), 2017/02/21 22:16:51]

Done.

+  // loader.
+  RankerModelLoader(const ValidateCallback& validate_callback,
+                    const OnModelAvailableCallback& on_model_available_callback,
+                    const base::FilePath& model_path,
+                    const GURL& model_url,
+                    const std::string& uma_prefix);
+
+  ~RankerModelLoader();
+
+  // Asynchronously initiates loading the model from the cache file path and URL
+  // previously configured.

[groby-ooo-7-16, 2017/02/14 20:56:48]

Why not pass path and URL here, then?

[Roger McFarlane (Chromium), 2017/02/21 22:16:50]

The loader is stateful. Passing the path and url here would imply (to me at
least) that the caller should expect to be able to Start() multiple path/url
loads, which isn't supported in this iteration.

+  void Start();
+
+  // Call this method periodically to notify the model loader the network might
+  // be available. If a model download is pending, this will trigger (subject to

[groby-ooo-7-16, 2017/02/14 20:56:48]

That's probably not the right name if we call this periodically, then. I'd
expect this to only be called on network signal edges as is. 

[Roger McFarlane (Chromium), 2017/02/21 22:16:50]

Renamed to NotifyOfRankerActivity

+  // retry and frequency limits) a download attempt.
+  void NotifyOfNetworkAvailability();
+
+ private:
+  // The model loader backend to which the actual loading functionality is
+  // delegated.
+  class Backend;

[groby-ooo-7-16, 2017/02/14 20:56:48]

Why do we have a separate backend?

[fdoray, 2017/02/15 20:07:40]

The Backend performs asynchronous operations and can outlive the
RankerModelLoader. We don't want to block in ~RankerModelLoader if a Backend
operation is in progress.
Maybe:

// The backend that handles asynchronous loads.

[Roger McFarlane (Chromium), 2017/02/21 22:16:50]

Done.

[Roger McFarlane (Chromium), 2017/02/21 22:16:51]

The threading and task-scheduling folks recommended that I break up the classes
by the sequences on which they run... to not have multiple threads/sequences
running through the same class instance.

+
+  // The task runner on which backend tasks are performed.
+  const scoped_refptr<base::SequencedTaskRunner> backend_task_runner_;
+
+  // The model loader backend to which the actual loading functionality is
+  // delegated.

[fdoray, 2017/02/15 20:07:40]

+ // When the RankerModelLoader is destroyed, ownership of |backend_| is
transferred to a delete task posted to |backend_task_runner_|. That means that
Backend can outlive RankerModelLoader.

[Roger McFarlane (Chromium), 2017/02/21 22:16:50]

Done.

+  std::unique_ptr<Backend> backend_;
+
+  DISALLOW_COPY_AND_ASSIGN(RankerModelLoader);
+};
+
+}  // namespace translate
+
+#endif  // COMPONENTS_TRANSLATE_CORE_BROWSER_RANKER_MODEL_LOADER_H_