Blimp Settings framework on the c++ side

blimp/client/core/settings/settings.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 "blimp/client/core/settings/settings.h"
+
+#include "base/command_line.h"
+#include "blimp/client/core/settings/settings_observer.h"
+#include "blimp/client/core/settings/settings_prefs.h"
+#include "blimp/client/core/switches/blimp_client_switches.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/pref_service.h"
+
+namespace blimp {
+namespace client {
+
+Settings::Settings(PrefService* local_state)
+    : local_state_(local_state),
+      blimp_enabled_(false),
+      record_whole_document_(false),
+      show_network_stats_(false) {
+  InitializeFromCommandLineAndPref();
+}
+
+Settings::~Settings() = default;
+
+void Settings::AddObserver(SettingsObserver* observer) {
+  observers_.AddObserver(observer);
+}
+
+void Settings::RemoveObserver(SettingsObserver* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+void Settings::SetShowNetworkStats(bool enable) {
+  if (show_network_stats_ == enable)
+    return;
+
+  show_network_stats_ = enable;
+  FOR_EACH_OBSERVER(SettingsObserver, observers_,
+                    OnShowNetworkStatsChanged(show_network_stats_));
+}
+
+void Settings::SetEnableBlimpMode(bool enable) {

[battre, 2016/10/10 09:28:30]

this is not a very idiomatic of the PrefService. I suggest to change it in a
couple of ways:

Settings::SetEnableBlimpMode and blimp_enabled would become as simple as

void Settings::SetEnableBlimpMode(bool enable) {
  local_state_->SetBoolean(prefs::kBlimpEnabled, enable);
}

bool blimp_enabled() {
  return local_state_->GetBoolean(prefs::kBlimpEnabled);
}

[Menglin, 2016/10/10 17:51:35]

Acknowledged.

+  if (blimp_enabled_ == enable)
+    return;
+
+  blimp_enabled_ = enable;
+  local_state_->SetBoolean(prefs::kBlimpEnabled, enable);
+  FOR_EACH_OBSERVER(SettingsObserver, observers_, OnBlimpModeEnabled(enable));

[battre, 2016/10/10 09:28:30]

If a SettingsObserver subscribed to prefs instead of being notified here, you'd
are also notified if somebody else changes the prefs directly (via extension,
policy, ...).

The SettingsObserver would need to implement the OnPreferenceChange interface
and dispatch to its On... functions.

This would become
void Settings::SetEnableBlimpMode(bool enable) {
  local_state_->SetBoolean(prefs::kBlimpEnabled, enable);
}

void SettingsObserver::OnPreferenceChanged(
    PrefService* service,
    const std::string& pref_name) {
  if (pref_name == prefs::kBlimpEnabled) {
    OnBlimpModeEnabled(service->GetBoolean(pref_name));
    OnRestartRequired();
  } else if (...) {
  }
}

[Bernhard Bauer, 2016/10/10 13:02:18]

Alternatively, keep the SettingsObserver interface, listen to pref changes in
this class, and then dispatch to observers. That would also fit in with
listening for changes to settings that aren't persisted.

[Menglin, 2016/10/10 17:51:35]

Thank you for your comments!
I think it's better to listen to pref changes in the Settings class (have
Settings implements PrefObserver), and in its implementation of
OnPreferenceChanged dispatches to its observers , because we do have
non-persisted settings. 

[David Trainor- moved to gerrit, 2016/10/10 21:18:28]

Ah yeah that's a really good point r.e. pushing prefs from external sources.  Is
there a general pattern for resolving settings overridden via the command line? 
Do we update the pref storage at that point or just enable the feature for the
current run of chrome (in a non-persistent way)?

+  FOR_EACH_OBSERVER(SettingsObserver, observers_, OnRestartRequired());
+}
+
+void Settings::SetRecordWholeDocument(bool enable) {
+  if (record_whole_document_ == enable)
+    return;
+
+  record_whole_document_ = enable;
+  local_state_->SetBoolean(prefs::kRecordWholeDocument, enable);
+  FOR_EACH_OBSERVER(SettingsObserver, observers_, OnRestartRequired());

[Menglin, 2016/10/24 22:44:58]

David, as we discussed, when this gets changed, no restart required, just need
to invoke OnRecordWholeDocumentChanged(bool) (implemented in SettingsFeature).

+}
+
+void Settings::InitializeFromCommandLineAndPref() {
+  const base::CommandLine* cmd_line = base::CommandLine::ForCurrentProcess();
+
+  // Get blimp_enabled_ from cmd line or persistent storage.
+  blimp_enabled_ = cmd_line->HasSwitch(switches::kEnableBlimp) ||
+      local_state_->GetBoolean(prefs::kBlimpEnabled);

[Menglin, 2016/10/10 23:49:01]

David, I also talked to khushal about this. looks like for blimp, it makes sense
to not save the command line to the prefs, meaning if the 1st time I start the
app using --enable-blimp via the command line; and the 2nd time, I start the app
without that command line, it should start the app without the mode enabled.

so that for blimp enabled, I can do
if (cmd_line->HasSwitch(switches::kEnableBlimp) ||
local_state_->GetBoolean(prefs::kBlimpEnabled))
  OnBlimpModeEnabled(true);

But for record_whole_document, khushal was asking why we need to restart the
app. I know we wanted to restart the app and then send off the message to the
engine in PushSettings(). but can we have something like OnRecordWholeDocument()
and send the message in that function, and this will call the SettingsFeature? 

Maybe I missed your reason on why we need a restart for record_whole_document
...

[Bernhard Bauer, 2016/10/11 09:17:04]

CommandLinePrefStore will do exactly that for you :)

+
+  // Get record_whole_document_ from cmd line or persistent storage.
+  record_whole_document_ =
+      cmd_line->HasSwitch(switches::kDownloadWholeDocument) ||
+      local_state_->GetBoolean(prefs::kRecordWholeDocument);

[battre, 2016/10/10 09:28:30]

Commandline parameters are typically handled via
https://cs.chromium.org/chromium/src/chrome/browser/prefs/command_line_pref_s...

Not sure whether this is feasible for blimp.

[Menglin, 2016/10/10 17:51:35]

blimp has been keeping its cmd line here.
https://cs.chromium.org/chromium/src/blimp/client/core/switches/blimp_client_...

I think the reason was in the beginning, blimp was a standalone apk and we
didn't want to integration especially into chrome/browser. but now blimp is a
mode in clank, I'm not sure we still want to keep our cmd line this way or using
the command_line_pref_store. dtrainor@, wdyt?

[David Trainor- moved to gerrit, 2016/10/10 21:18:29]

Ideally we wouldn't bleed all blimp switches into the chrome/ layer.  I like
that method, but is there a reason it's in chrome/?  Can we add a TODO/bug to
pull that class out to the proper component?

[Bernhard Bauer, 2016/10/11 09:17:04]

CommandLinePrefStore right now contains all the Chrome prefs that can be set
from the command line, so that would have to stay in Chrome. What we would
probably want to do is extract a base class that would live in the component
(and rename the class in chrome/ to ChromeCommandLinePrefStore).

[David Trainor- moved to gerrit, 2016/10/11 16:25:31]

Hmm I haven't looked into how to actually use this much (mlliu@ - you should
check this out), but would we have to augment PrefValueStore to allow people to
set more than one CommandLinePrefStore?  It seems like it can only be
set/overridden, which means either Blimp or Chrome will end up winning depending
on who sets the ValueMapPrefStore when.

[Bernhard Bauer, 2016/10/11 16:45:35]

Wait, now I'm slightly confused. Do you want to use the _same_ PrefService as
Chrome (i.e. the one backed by $USER_DATA_DIR/Local State)? If so, registering
one set of prefs from Chrome and one from Blimp would be somewhat involved. If
not, you can just create your own PrefService instance (backed by its own JSON
file) and set the CommandLinePrefStore to whatever you want there.

[Menglin, 2016/10/11 16:57:38]

we are using chrome's user profile prefs.
https://cs.chromium.org/chromium/src/chrome/browser/prefs/browser_prefs.cc?l=676

I didn't know about creating our own PrefService.

+}
+
+// static
+void Settings::RegisterPrefs(PrefRegistrySimple* registry) {
+  registry->RegisterBooleanPref(prefs::kBlimpEnabled, false);
+  registry->RegisterBooleanPref(prefs::kRecordWholeDocument, false);
+}
+
+}  // namespace client
+}  // namespace blimp