Quirks Client for downloading and providing display profiles

components/quirks_client/quirks_client_manager.cc

Index: components/quirks_client/quirks_client_manager.cc
diff --git a/components/quirks_client/quirks_client_manager.cc b/components/quirks_client/quirks_client_manager.cc
new file mode 100644
index 0000000000000000000000000000000000000000..4c03861a67def9c7c7a4b89b1be2017117d6dfd7
--- /dev/null
+++ b/components/quirks_client/quirks_client_manager.cc
@@ -0,0 +1,187 @@
+// 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.
+
+#include "components/quirks_client/quirks_client_manager.h"
+
+#include "base/rand_util.h"
+#include "base/thread_task_runner_handle.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/scoped_user_pref_update.h"
+#include "components/quirks_client/pref_names.h"
+#include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "url/gurl.h"
+
+namespace quirks_client {
+
+namespace {
+
+QUIRKS_CLIENT_EXPORT QuirksClientManager* g_manager = nullptr;

[Bernhard Bauer, 2016/02/12 14:51:21]

Wait, do you need to export a symbol from an anonymous namespace?

[Greg Levin, 2016/02/15 21:53:52]

I did until recently.  Without it, calls into Quirks code from ui and file
threads would find different instances of this global if I didn't use the
EXPORT.  But it seems that the recent switch from target type = 'static_library'
to '<(component)' has solved that.  So EXPORT removed.  Thanks!

+
+// How often we query Quirks Server.
+const int kDaysBetweenServerChecks = 30;
+
+}  // namespace
+
+////////////////////////////////////////////////////////////////////////////////
+// QuirksClientManager
+
+QuirksClientManager::QuirksClientManager(
+    Delegate* delegate,
+    base::SequencedWorkerPool* blocking_pool,
+    PrefService* local_state,
+    net::URLRequestContextGetter* url_context_getter)
+    : delegate_(delegate),
+      blocking_pool_(blocking_pool),
+      local_state_(local_state),
+      url_context_getter_(url_context_getter),
+      task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      is_new_device_(false),
+      weak_ptr_factory_(this) {}
+
+QuirksClientManager::~QuirksClientManager() {
+  clients_.clear();
+  g_manager = nullptr;
+  delete delegate_;

[Bernhard Bauer, 2016/02/12 14:51:21]

Make |delegate_| a scoped_ptr?

[Greg Levin, 2016/02/15 21:53:53]

Done.

+}
+
+// static
+void QuirksClientManager::Initialize(
+    scoped_ptr<Delegate> delegate,
+    base::SequencedWorkerPool* blocking_pool,
+    PrefService* local_state,
+    net::URLRequestContextGetter* url_context_getter) {
+  if (QuirksClient::IsEnabled())
+    g_manager = new QuirksClientManager(delegate.release(), blocking_pool,
+                                        local_state, url_context_getter);
+}
+
+// static
+void QuirksClientManager::Shutdown() {
+  delete g_manager;
+}
+
+// static
+QuirksClientManager* QuirksClientManager::Get() {
+  DCHECK(g_manager);
+  return g_manager;
+}
+
+// static
+void QuirksClientManager::RegisterPrefs(PrefRegistrySimple* registry) {
+  registry->RegisterDictionaryPref(prefs::kQuirksClientLastServerCheck);
+}
+
+void QuirksClientManager::RunClient(
+    int64_t product_id,
+    const DownloadFinishedCallback& on_download_finished) {
+  DCHECK(blocking_pool()->RunsTasksOnCurrentThread());
+
+  // Record this on file thread for future use on ui thread.
+  is_new_device_ = (delegate_->GetDaysSinceOobe() <= kDaysBetweenServerChecks);
+
+  task_runner_->PostTask(FROM_HERE,
+                         base::Bind(&QuirksClientManager::RunClientOnUIThread,
+                                    weak_ptr_factory_.GetWeakPtr(), product_id,

[Bernhard Bauer, 2016/02/12 14:51:21]

This is extremely subtle. Assuming that accessing |this| is safe here on a
background thread (see my other comment above), you can get a weak pointer from
the factory, but you can't do _anything_ with it apart from passing it to the
thread where it will be dereferenced (that includes checking it for validity!).
So, if you keeps this, I think it would at least warrant a comment, to make sure
that this is not going to be just blindly copied into a situation where it is
actually incorrect.

[Greg Levin, 2016/02/16 03:14:46]

See comment on related comment, but I'll keep working on this.  Any additional
advice would be appreciated.

+                                    on_download_finished));
+}
+
+void QuirksClientManager::DeleteClient(QuirksClient* client) {
+  std::map<QuirksClient*, scoped_ptr<QuirksClient>>::iterator it =
+      clients_.find(client);
+  if (it != clients_.end())
+    clients_.erase(it);
+}
+
+bool QuirksClientManager::NeedToCheckServer(int64_t product_id) {
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());
+  double last_check = 0.0;
+  const base::DictionaryValue* dict =
+      local_state_->GetDictionary(prefs::kQuirksClientLastServerCheck);
+  if (dict)

[Bernhard Bauer, 2016/02/12 14:51:21]

This isn't necessary -- GetDictionary() is guaranteed to return a non-null
pointer.

[Greg Levin, 2016/02/15 21:53:52]

Done.  Thanks for the clarifying link below.  I saw a lot of examples of
checking GetDictionary() != NULL, so followed that pattern out of caution.

+    dict->GetDouble(IdToHexString(product_id), &last_check);
+
+  if (last_check != 0.0) {
+    const base::TimeDelta time_since =
+        base::Time::Now() - base::Time::FromDoubleT(last_check);
+
+    // Don't need server check if we've checked within last 30 days.
+    if (time_since < base::TimeDelta::FromDays(kDaysBetweenServerChecks)) {
+      VLOG(2) << time_since.InDays()
+              << " days since last Quirks Server check for display "
+              << IdToHexString(product_id);
+      return false;
+    }
+
+    return true;
+  }
+
+  // On new devices, we want to check server immediately.
+  if (is_new_device_)
+    return true;
+
+  // Otherwise, for the first check on an older device, we want to stagger
+  // it over 30 days, so artificially set last check accordingly.
+  const int rand_days = base::RandInt(0, kDaysBetweenServerChecks);
+  const base::Time fake_last_check =
+      base::Time::Now() - base::TimeDelta::FromDays(rand_days);
+  SetLastServerCheck(product_id, fake_last_check);
+  VLOG(2) << "Delaying first Quirks Server check by "
+          << kDaysBetweenServerChecks - rand_days << " days.";
+  return false;
+}
+
+void QuirksClientManager::SetLastServerCheck(int64_t product_id,
+                                             const base::Time& last_check) {
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());

[Bernhard Bauer, 2016/02/12 14:51:21]

Do you want to check that you are on your origin thread here? If so, use
task_runner_->RunsTasksOnCurrentThread(). This just checks that there is _a_
thread task runner for the current thread (which in practice means there is a
MessageLoop), but doesn't say anything about which thread it is.

(It kind of works for checking you're _not_ on a worker pool thread, because
those don't have a message loop or a ThreadTaskRunnerHandle, but that's just an
artifact of how SequencedWorkerPool is implemented.)

[Greg Levin, 2016/02/15 21:53:52]

It doesn't need to be the same thread (as far as I know).  I just have that here
because the Get/SetPref code calls CalledOnValidThread().

+  DictionaryPrefUpdate dict(local_state_, prefs::kQuirksClientLastServerCheck);
+  dict->SetDouble(IdToHexString(product_id), last_check.ToDoubleT());
+}
+
+// TODO(glevin): Add code to record UMA stats here.  Also need to set
+// request_reason_ in QuirksClient.
+void QuirksClientManager::RecordReasonUmaStat(
+    QuirksClient::RequestReason reason) {}
+
+void QuirksClientManager::RecordFileFoundUmaStat(bool success) {}
+
+scoped_ptr<net::URLFetcher> QuirksClientManager::CreateURLFetcher(
+    const std::string& url,
+    net::URLFetcherDelegate* delegate) {
+  scoped_ptr<net::URLFetcher> url_fetcher =
+      fake_quirks_fetcher_creator_.is_null()

[Bernhard Bauer, 2016/02/12 14:51:21]

Nit: I think I would split this up into two return statements:

  if (!fake_quirks_fetcher_creator_.is_null())
    return fake_quirks_fetcher_creator_.Run();

  return net::URLFetcher::Create(...);

[Greg Levin, 2016/02/15 21:53:52]

Done.

+          ? net::URLFetcher::Create(GURL(url), net::URLFetcher::GET, delegate)
+          : fake_quirks_fetcher_creator_.Run(GURL(url), delegate);
+  return url_fetcher;
+}
+
+// Initializes QuirksClient object on the ui thread, where most of its work
+// needs to be done.
+void QuirksClientManager::RunClientOnUIThread(
+    int64_t product_id,
+    const DownloadFinishedCallback& on_download_finished) {
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());
+  if (!ValidateOnUiThread()) {
+    VLOG(1) << "Quirks Client Manager not correctly initialized.";
+    return;
+  }
+
+  if (!NeedToCheckServer(product_id))
+    return;
+
+  QuirksClient* client =
+      new QuirksClient(product_id, on_download_finished, this);
+  clients_.insert(std::pair<QuirksClient*, scoped_ptr<QuirksClient>>(
+      client, make_scoped_ptr(client)));
+  client->StartDownload();
+}
+
+bool QuirksClientManager::ValidateOnUiThread() {
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());
+  return local_state_->CalledOnValidThread() &&
+         base::ThreadTaskRunnerHandle::IsSet() && task_runner_ &&

[Bernhard Bauer, 2016/02/12 14:51:21]

It's a bit strange that you have ThreadTaskRunnerHandle::IsSet() as a
precondition, but then check it here in code. If it has to be true (i.e. it
being false means a bug), you shouldn't have to check it again. See also
https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC....

[Greg Levin, 2016/02/15 21:53:52]

Done.  I've been going back and forth on whether to just remove this function. 
I added it for debugging, but with the current code, I don't know of any
circumstance under which it could fail.

+         task_runner_->RunsTasksOnCurrentThread();
+}
+
+}  // namespace quirks_client