PreferencesManager

ash/common/shelf/shelf_controller.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 "ash/common/shelf/shelf_controller.h"
 
 #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"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/resources/grit/ui_resources.h"
 
 namespace ash {
 
 namespace {
 
@@ skipping 132 matching lines @@
   // The controller may be null for invalid ids or for displays being removed.
   WmRootWindowController* root_window_controller =
       WmLookup::Get()->GetRootWindowControllerWithDisplayId(display_id);
   return root_window_controller ? root_window_controller->GetShelf() : nullptr;
 }
 
 }  // namespace
 
 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) {
   // Notify observers, Chrome will set alignment and auto-hide from prefs.
   int64_t display_id = shelf->GetWindow()->GetDisplayNearestWindow().id();
   observers_.ForAllPtrs([display_id](mojom::ShelfObserver* observer) {
     observer->OnShelfCreated(display_id);
   });
 }
 
 void ShelfController::NotifyShelfAlignmentChanged(WmShelf* shelf) {
   ShelfAlignment alignment = shelf->alignment();
   int64_t display_id = shelf->GetWindow()->GetDisplayNearestWindow().id();
   observers_.ForAllPtrs(
       [alignment, display_id](mojom::ShelfObserver* observer) {
         observer->OnAlignmentChanged(alignment, display_id);
       });
 }
 
 void ShelfController::NotifyShelfAutoHideBehaviorChanged(WmShelf* shelf) {
   ShelfAutoHideBehavior behavior = shelf->auto_hide_behavior();
   int64_t display_id = shelf->GetWindow()->GetDisplayNearestWindow().id();
   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(
     mojom::ShelfObserverAssociatedPtrInfo observer) {
   mojom::ShelfObserverAssociatedPtr observer_ptr;
   observer_ptr.Bind(std::move(observer));
   observers_.AddPtr(std::move(observer_ptr));
 }
 
 void ShelfController::SetAlignment(ShelfAlignment alignment,
@@ skipping 68 matching lines @@
   if (!app_id_to_shelf_id_.count(app_id))
     return;
   ShelfID shelf_id = app_id_to_shelf_id_[app_id];
   int index = model_.ItemIndexByID(shelf_id);
   DCHECK_GE(index, 0);
   ShelfItem item = *model_.ItemByID(shelf_id);
   item.image = GetShelfIconFromBitmap(image);
   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