PreferencesManager

ash/common/shelf/shelf_controller.cc

Index: ash/common/shelf/shelf_controller.cc
diff --git a/ash/common/shelf/shelf_controller.cc b/ash/common/shelf/shelf_controller.cc
index 043fec9e7513655c295521b1b328c4b116ffe463..6faca21efd858706b3ce1663d0f24b85d0ca611e 100644
--- a/ash/common/shelf/shelf_controller.cc
+++ b/ash/common/shelf/shelf_controller.cc
@@ -7,11 +7,15 @@
 #include "ash/common/shelf/shelf_item_delegate.h"
 #include "ash/common/shelf/shelf_menu_model.h"
 #include "ash/common/shelf/wm_shelf.h"
+#include "ash/common/shell_delegate.h"
 #include "ash/common/wm_lookup.h"
 #include "ash/common/wm_root_window_controller.h"
 #include "ash/common/wm_shell.h"
 #include "ash/common/wm_window.h"
 #include "base/strings/utf_string_conversions.h"
+#include "services/preferences/public/cpp/pref_observer_store.h"
+#include "services/preferences/public/interfaces/preferences.mojom.h"
+#include "services/service_manager/public/cpp/connector.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
@@ -164,10 +168,29 @@ WmShelf* GetShelfForDisplay(int64_t display_id) {
 
 ShelfController::ShelfController() {}
 
-ShelfController::~ShelfController() {}
+ShelfController::~ShelfController() {
+  if (store_)
+    store_->RemoveObserver(this);
+}
 
 void ShelfController::BindRequest(mojom::ShelfControllerRequest request) {
   bindings_.AddBinding(this, std::move(request));
+
+  // Sample of connecting to the PreferencesManager

[msw, 2016/11/18 00:32:57]

aside: reducing some boilerplate might be nice.

[jonross, 2016/11/18 00:47:58]

Moving this construction to WmShell would centralize the mojom boilerplate.

Then various ash components could add themselves as observers of the single
PrefObserverStore

+  WmShell* shell = WmShell::Get();
+  service_manager::Connector* connector =
+      shell->delegate()->GetShellConnector();
+  if (!connector)
+    return;
+  prefs::mojom::PreferencesManagerPtr pref_manager_ptr;
+  connector->ConnectToInterface("service:content_browser", &pref_manager_ptr);
+  store_ = new PrefObserverStore(std::move(pref_manager_ptr));
+  std::set<std::string> keys;
+  // chrome::common::pref_names::kShelfAutoHideBehavior

[msw, 2016/11/18 00:32:57]

aside: I guess we'll move ash-relevant pref names to ash, right?
(we'll probably have to figure this out on a case-by-case basis)

[jonross, 2016/11/18 00:47:58]

An issue with this is that the consts need to be in both locations. Chrome has
strict restrictions on registering preferences at startup.

I'm not sure the best place to put them to have them shared between ash/chrome

[James Cook, 2016/11/18 00:50:34]

+1. Most components have a pref_names.cc file, we could do the same.

[msw, 2016/11/18 00:58:13]

Yeah, ash/public/cpp/pref_names.h could declare these for ash and chrome.

[jonross, 2016/11/18 01:01:09]

That SGTM

+  const std::string key("auto_hide_behavior");
+  keys.insert(key);
+  store_->AddObserver(this);
+  store_->Init(keys);
 }
 
 void ShelfController::NotifyShelfCreated(WmShelf* shelf) {
@@ -193,6 +216,23 @@ void ShelfController::NotifyShelfAutoHideBehaviorChanged(WmShelf* shelf) {
   observers_.ForAllPtrs([behavior, display_id](mojom::ShelfObserver* observer) {
     observer->OnAutoHideBehaviorChanged(behavior, display_id);
   });
+
+  // Using Preferences to set the value. the chrome ShelfObserver can seprately
+  // listen to preference changes to make any according UI state changes.
+  std::string behaviour_pref;
+  switch (behavior) {

[msw, 2016/11/18 00:32:57]

aside: would it make sense to store the behavior enum value?
(perhaps this is how the current pref works, I didn't check)

[jonross, 2016/11/18 00:47:58]

Yeah in most cases it would make sense to just have an enum define behaviour,
and send that across.

For this particular case the chrome-side is storing the behaviour as
string-prefs. Then converting to enums and sending them over mojo.

As we switch to using the prefs::mojom it wouls be a good idea to clean this up.

+    case SHELF_AUTO_HIDE_BEHAVIOR_ALWAYS:
+      behaviour_pref = "Always";
+      break;
+    case SHELF_AUTO_HIDE_BEHAVIOR_NEVER:
+      behaviour_pref = "Never";
+      break;
+    default:
+      behaviour_pref = "ooops";
+  }
+  std::unique_ptr<base::Value> value(new base::StringValue(behaviour_pref));
+  if (store_)
+    store_->SetValue("auto_hide_behavior", std::move(value), 0);
 }
 
 void ShelfController::AddObserver(
@@ -281,4 +321,32 @@ void ShelfController::SetItemImage(const std::string& app_id,
   model_.Set(index, item);
 }
 
+void ShelfController::OnPrefValueChanged(const std::string& key) {
+  const base::Value* value = nullptr;
+  store_->GetValue("auto_hide_behavior", &value);
+  if (!value)
+    return;
+  std::string actual_value;
+  value->GetAsString(&actual_value);
+  LOG(ERROR) << "JR OnPrefValueChanged " << key << " : " << actual_value
+             << "\n";
+
+  // Call a method that needs the pref here
+}
+
+void ShelfController::OnInitializationCompleted(bool succeeded) {

[msw, 2016/11/18 00:32:57]

aside: I wonder if it'd make sense to just call OnPrefValueChanged for each of
the initialized prefs. That would probably work in this case, but might not make
sense in other cases... luckily, it seems easy for this impl to do that itself
(manually call OnPrefValueChanged(key1); OnPrefValueChanged(key2);)

[jonross, 2016/11/18 00:47:58]

This init/changed paradigm is just brought over from PrefStore to match existing
behaviour. Was trying to reduce the api changes.  There's also a weird creation
order bug in ValueMapPrefStore which causes crashes when asking for prefs before
init is done.

However this is something that could be handled within the new PrefObserverStore
quite easily. Especially if it made it so that ash only needs to listen to one
of these.

[jonross, 2016/11/19 00:28:20]

I've updated this based on your suggestion. Further digging also showed that
most OnInitCompleted was just used to handle error states.

PrefObserverStore will not fire off the OnInitCompleted, then will fire
OnPrefValueChanged per key.

+  LOG(ERROR) << "JR OnInitializationCompleted\n";
+  if (!succeeded)
+    return;
+  const base::Value* value = nullptr;
+  store_->GetValue("auto_hide_behavior", &value);
+  if (!value)
+    return;
+  std::string actual_value;
+  value->GetAsString(&actual_value);
+  LOG(ERROR) << "\tJR auto_hide_behavior: " << actual_value << "\n";
+
+  // Call a method that needs the pref here
+}
+
 }  // namespace ash