Parse JSON and compute off the UI thread for policy printers.

Change ExternalPrinters to perform slow operations off the UI thread
to avoid introducing jank.  Also add infrastructure to observe
preferences and manage instances by user.

Bug: 758680
Change-Id: I56f4dae9f0c2062cfe177fcdac88785978874217
Reviewed-on: https://chromium-review.googlesource.com/826705
Commit-Queue: Sean Kau <skau@chromium.org>
Reviewed-by: Xiaoqian Dai OOO until Jan 2nd <xdai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526087}

chrome/browser/chromeos/BUILD.gn

@@ -1410,6 +1410,10 @@ source_set("chromeos") {
     "printing/cups_printers_manager.h",
     "printing/external_printers.cc",
     "printing/external_printers.h",
+    "printing/external_printers_factory.cc",
+    "printing/external_printers_factory.h",
+    "printing/external_printers_pref_bridge.cc",
+    "printing/external_printers_pref_bridge.h",
     "printing/ppd_provider_factory.cc",
     "printing/ppd_provider_factory.h",
     "printing/printer_configurer.cc",

chrome/browser/chromeos/printing/external_printers.h

@@ -5,30 +5,30 @@
 #ifndef CHROME_BROWSER_CHROMEOS_PRINTING_EXTERNAL_PRINTERS_H_
 #define CHROME_BROWSER_CHROMEOS_PRINTING_EXTERNAL_PRINTERS_H_
 
+#include <map>
 #include <memory>
 #include <string>
 #include <utility>
 #include <vector>
 
-#include "base/macros.h"
-#include "chrome/browser/chromeos/policy/cloud_external_data_policy_observer.h"
+#include "chromeos/chromeos_export.h"
 #include "chromeos/printing/printer_configuration.h"
 
 namespace chromeos {
 
 // Manages download and parsing of the external policy printer configuration and
 // enforces restrictions.
-class ExternalPrinters {
+class CHROMEOS_EXPORT ExternalPrinters {
  public:
   // Choose the policy for printer access.
   enum AccessMode {
-    UNSET,
+    UNSET = -1,
     // Printers in the blacklist are disallowed.  Others are allowed.
-    BLACKLIST_ONLY,
+    BLACKLIST_ONLY = 0,
     // Only printers in the whitelist are allowed.
-    WHITELIST_ONLY,
+    WHITELIST_ONLY = 1,
     // All printers in the policy are allowed.
-    ALL_ACCESS
+    ALL_ACCESS = 2
   };
 
   // Observer is notified when the computed set of printers change.  It is
@@ -36,8 +36,12 @@ class ExternalPrinters {
   // observing.
   class Observer {
    public:
-    // Called when the computed set of printers changes.
-    virtual void OnPrintersChanged() = 0;
+    // Called when the printers have changed and should be queried.  |valid| is
+    // true if |printers| is based on a valid policy.  |printers| are the
+    // printers that should be available to the user.
+    virtual void OnPrintersChanged(
+        bool valid,
+        const std::map<const std::string, const Printer>& printers) = 0;
   };
 
   // Creates a handler for the external printer policies.
@@ -45,24 +49,35 @@ class ExternalPrinters {
 
   virtual ~ExternalPrinters() = default;
 
+  virtual void AddObserver(Observer* observer) = 0;
+  virtual void RemoveObserver(Observer* observer) = 0;
+
   // Parses |data| which is the contents of the bulk printes file and extracts
   // printer information.  The file format is assumed to be JSON.
   virtual void SetData(std::unique_ptr<std::string> data) = 0;
   // Removes all printer data and invalidates the configuration.
   virtual void ClearData() = 0;
 
-  virtual void AddObserver(Observer* observer) = 0;
-  virtual void RemoveObserver(Observer* observer) = 0;
-
+  // Set the access mode which chooses the type of filtering that is performed.
   virtual void SetAccessMode(AccessMode mode) = 0;
+  // Set the |blacklist| which excludes printers with the given id if access
+  // mode is BLACKLIST_ONLY.
   virtual void SetBlacklist(const std::vector<std::string>& blacklist) = 0;
+  // Set the |whitelist| which is the complete list of printers that are show to
+  // the user.  This is in effect if access mode is WHITELIST_ONLY.
   virtual void SetWhitelist(const std::vector<std::string>& whitelist) = 0;
 
-  // Returns true if the printer configuration has been downloaded and parsed.
+  // Returns true if the printer map has been computed from a valid policy.
+  // Returns false otherwise.  This is computed asynchronously.  Observe
+  // OnPrintersChanged() to be notified when it is updated.  This may never
+  // become true if a user does not have the appropriate printer policies.
   virtual bool IsPolicySet() const = 0;
 
-  // Returns all the printers available from the policy.
-  virtual const std::vector<Printer>& GetPrinters() const = 0;
+  // Returns a refernce to a map of the computed set of printers.  The map is
+  // empty if either the policy was empty or we are yet to receive the full
+  // policy. Use IsPolicySet() to differentiate betweeen the two.
+  virtual const std::map<const std::string, const Printer>& GetPrinters()
+      const = 0;
 };
 
 }  // namespace chromeos

chrome/browser/chromeos/printing/external_printers_factory.cc

+// Copyright 2017 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 "chrome/browser/chromeos/printing/external_printers_factory.h"
+
+#include <memory>
+
+#include "base/lazy_instance.h"
+#include "chrome/browser/chromeos/printing/external_printers.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/profiles/profile.h"
+#include "components/user_manager/user.h"
+
+namespace chromeos {
+
+namespace {
+
+base::LazyInstance<ExternalPrintersFactory>::DestructorAtExit
+    g_printers_factory = LAZY_INSTANCE_INITIALIZER;
+
+}  // namespace
+
+// static
+ExternalPrintersFactory* ExternalPrintersFactory::Get() {
+  return g_printers_factory.Pointer();
+}
+
+ExternalPrinters* ExternalPrintersFactory::GetForAccountId(
+    const AccountId& account_id) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  auto found = printers_by_user_.find(account_id);
+  if (found != printers_by_user_.end()) {
+    return found->second.get();
+  }
+
+  printers_by_user_[account_id] = ExternalPrinters::Create();
+  return printers_by_user_[account_id].get();
+}
+
+ExternalPrinters* ExternalPrintersFactory::GetForProfile(Profile* profile) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  const user_manager::User* user =
+      ProfileHelper::Get()->GetUserByProfile(profile);
+  if (!user)
+    return nullptr;
+
+  return GetForAccountId(user->GetAccountId());
+}
+
+void ExternalPrintersFactory::RemoveForUserId(const AccountId& account_id) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  printers_by_user_.erase(account_id);
+}
+
+void ExternalPrintersFactory::Shutdown() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  printers_by_user_.clear();
+}
+
+ExternalPrintersFactory::ExternalPrintersFactory() = default;
+ExternalPrintersFactory::~ExternalPrintersFactory() = default;
+
+}  // namespace chromeos

chrome/browser/chromeos/printing/external_printers_factory.h

+// Copyright 2017 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 CHROME_BROWSER_CHROMEOS_PRINTING_EXTERNAL_PRINTERS_FACTORY_H_
+#define CHROME_BROWSER_CHROMEOS_PRINTING_EXTERNAL_PRINTERS_FACTORY_H_
+
+#include <map>
+#include <memory>
+
+#include "base/lazy_instance.h"
+#include "base/macros.h"
+#include "base/sequence_checker.h"
+#include "chrome/browser/chromeos/printing/external_printers.h"
+#include "components/signin/core/account_id/account_id.h"
+
+class Profile;
+
+namespace chromeos {
+
+// Dispenses ExternalPrinters objects based on account id.  Access to this
+// object should be sequenced.
+class ExternalPrintersFactory {
+ public:
+  static ExternalPrintersFactory* Get();
+
+  // Returns a pointer to the ExternalPrinters registered for |account_id|. If
+  // an ExternalPrinters does not exist, one will be created for |account_id|.
+  // The returned object remains valid until RemoveForUserId or Shutdown is
+  // called.
+  ExternalPrinters* GetForAccountId(const AccountId& account_id);
+
+  // Returns a pointer to the ExternalPrinters registered for |profile| which
+  // could be null if |profile| does not map to a valid AccountId. The returned
+  // object remains valid until RemoveForUserId or Shutdown is called.
+  ExternalPrinters* GetForProfile(Profile* profile);
+
+  // Deletes the ExternalPrinters registered for |account_id|.
+  void RemoveForUserId(const AccountId& account_id);
+
+  // Tear down all ExternalPrinters.
+  void Shutdown();
+
+ private:
+  friend struct base::LazyInstanceTraitsBase<ExternalPrintersFactory>;
+
+  ExternalPrintersFactory();
+  ~ExternalPrintersFactory();
+
+  std::map<AccountId, std::unique_ptr<ExternalPrinters>> printers_by_user_;
+  SEQUENCE_CHECKER(sequence_checker_);
+
+  DISALLOW_COPY_AND_ASSIGN(ExternalPrintersFactory);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_PRINTING_EXTERNAL_PRINTERS_FACTORY_H_

chrome/browser/chromeos/printing/external_printers_pref_bridge.cc

+// 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 "chrome/browser/chromeos/printing/external_printers_pref_bridge.h"
+
+#include <string>
+#include <vector>
+
+#include "base/values.h"
+#include "chrome/browser/chromeos/printing/external_printers.h"
+#include "chrome/browser/chromeos/printing/external_printers_factory.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/pref_names.h"
+#include "components/policy/policy_constants.h"
+#include "components/pref_registry/pref_registry_syncable.h"
+#include "components/prefs/pref_service.h"
+
+namespace chromeos {
+
+namespace {
+
+// Extracts the list of strings named |policy_name| from |prefs| and returns it.
+std::vector<std::string> FromPrefs(const PrefService* prefs,
+                                   const std::string& policy_name) {
+  std::vector<std::string> string_list;
+  const base::ListValue* list = prefs->GetList(policy_name);
+  for (const base::Value& value : *list) {
+    if (value.is_string()) {
+      string_list.push_back(value.GetString());
+    }
+  }
+
+  return string_list;
+}
+
+}  // namespace
+
+// static
+void ExternalPrintersPrefBridge::RegisterProfilePrefs(
+    user_prefs::PrefRegistrySyncable* registry,
+    const ExternalPrinterPolicies& policies) {
+  // Default value for access mode is blacklist.
+  registry->RegisterIntegerPref(policies.access_mode,
+                                ExternalPrinters::BLACKLIST_ONLY);
+  registry->RegisterListPref(policies.blacklist);
+  registry->RegisterListPref(policies.whitelist);
+}
+
+ExternalPrintersPrefBridge::ExternalPrintersPrefBridge(
+    const ExternalPrinterPolicies& policies,
+    Profile* profile)
+    : profile_(profile), policies_(policies) {
+  pref_change_registrar_.Init(profile_->GetPrefs());
+
+  pref_change_registrar_.Add(
+      policies_.access_mode,
+      base::BindRepeating(&ExternalPrintersPrefBridge::AccessModeUpdated,
+                          base::Unretained(this)));
+  pref_change_registrar_.Add(
+      policies_.blacklist,
+      base::BindRepeating(&ExternalPrintersPrefBridge::BlacklistUpdated,
+                          base::Unretained(this)));
+  pref_change_registrar_.Add(
+      policies_.whitelist,
+      base::BindRepeating(&ExternalPrintersPrefBridge::WhitelistUpdated,
+                          base::Unretained(this)));
+  Initialize();
+}
+
+void ExternalPrintersPrefBridge::Initialize() {
+  BlacklistUpdated();
+  WhitelistUpdated();
+  AccessModeUpdated();
+}
+
+void ExternalPrintersPrefBridge::AccessModeUpdated() {
+  const PrefService* prefs = profile_->GetPrefs();
+  ExternalPrinters::AccessMode mode = ExternalPrinters::UNSET;
+  int mode_val = prefs->GetInteger(policies_.access_mode);
+  if (mode_val >= ExternalPrinters::BLACKLIST_ONLY &&
+      mode_val <= ExternalPrinters::ALL_ACCESS) {
+    mode = static_cast<ExternalPrinters::AccessMode>(mode_val);
+  } else {
+    LOG(ERROR) << "Unrecognized access mode";
+    return;
+  }
+
+  auto* printers = ExternalPrintersFactory::Get()->GetForProfile(profile_);
+  if (printers)
+    printers->SetAccessMode(mode);
+}
+
+void ExternalPrintersPrefBridge::BlacklistUpdated() {
+  auto* printers = ExternalPrintersFactory::Get()->GetForProfile(profile_);
+  if (printers)
+    printers->SetBlacklist(
+        FromPrefs(profile_->GetPrefs(), policies_.blacklist));
+}
+
+void ExternalPrintersPrefBridge::WhitelistUpdated() {
+  auto* printers = ExternalPrintersFactory::Get()->GetForProfile(profile_);
+  if (printers)
+    printers->SetWhitelist(
+        FromPrefs(profile_->GetPrefs(), policies_.whitelist));
+}
+
+}  // namespace chromeos

chrome/browser/chromeos/printing/external_printers_pref_bridge.h

+// Copyright 2017 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 CHROME_BROWSER_CHROMEOS_PRINTING_EXTERNAL_PRINTERS_PREF_BRIDGE_H_
+#define CHROME_BROWSER_CHROMEOS_PRINTING_EXTERNAL_PRINTERS_PREF_BRIDGE_H_
+
+#include <string>
+
+#include "base/macros.h"
+#include "components/prefs/pref_change_registrar.h"
+
+class Profile;
+
+namespace user_prefs {
+class PrefRegistrySyncable;
+}
+
+namespace chromeos {
+
+class ExternalPrinters;
+
+// A collection of preference names representing the external printer fields.
+struct ExternalPrinterPolicies {
+  std::string access_mode;
+  std::string blacklist;
+  std::string whitelist;
+};
+
+// Observe preference changes and propogate changes to ExternalPrinters.
+class ExternalPrintersPrefBridge {
+ public:
+  static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry,
+                                   const ExternalPrinterPolicies& policies);
+
+  ExternalPrintersPrefBridge(const ExternalPrinterPolicies& policies,
+                             Profile* profile);
+
+ private:
+  // Retrieve initial values for preferences.
+  void Initialize();
+
+  // Handle update for the access mode policy.
+  void AccessModeUpdated();
+
+  // Handle updates for the blacklist policy.
+  void BlacklistUpdated();
+
+  // Handle updates for the whitelist policy.
+  void WhitelistUpdated();
+
+  Profile* profile_;
+  const ExternalPrinterPolicies policies_;
+  PrefChangeRegistrar pref_change_registrar_;
+
+  DISALLOW_COPY_AND_ASSIGN(ExternalPrintersPrefBridge);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_PRINTING_EXTERNAL_PRINTERS_PREF_BRIDGE_H_