Blimp Settings framework on the c++ side

blimp/client/core/settings/settings.h

+// 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.
+
+#ifndef BLIMP_CLIENT_CORE_SETTINGS_SETTINGS_H_
+#define BLIMP_CLIENT_CORE_SETTINGS_SETTINGS_H_
+
+#include "base/macros.h"
+#include "base/observer_list.h"
+
+class PrefRegistrySimple;
+class PrefService;
+
+namespace blimp {
+namespace client {
+
+class SettingsObserver;
+
+class Settings {
+ public:
+  explicit Settings(PrefService* local_state);
+  virtual ~Settings();
+
+  static void RegisterPrefs(PrefRegistrySimple* registry);
+
+  void AddObserver(SettingsObserver* observer);
+  void RemoveObserver(SettingsObserver* observer);
+
+  // Change blimp_enabled_, and save it to the persistent storage.
+  // blimp_enabled_ can also be enabled by adding command line flag
+  // --enable-blimp

[David Trainor- moved to gerrit, 2016/10/05 23:46:20]

. after

[Menglin, 2016/10/06 22:36:38]

Done.

+  void SetEnableBlimpMode(bool enable);

[David Trainor- moved to gerrit, 2016/10/05 23:46:20]

Add some comments about what each of these settings are.  Not all of them are
tracked in the prefs header so they won't all have comments there.

[Menglin, 2016/10/06 22:36:38]

I added the comments at each private members.

+
+  // Change record_whole_document_. record_whole_document_ can also be enabled
+  // by adding command line flag --record-whole-document.
+  void SetRecordWholeDocument(bool enable);
+
+  // Change show_network_stats_, and save it to the persistent storage.

[David Trainor- moved to gerrit, 2016/10/05 23:46:21]

Does (Should?) this actually save to persistent storage?

[Menglin, 2016/10/06 22:36:38]

No it shouldn't.. 

+  void SetShowNetworkStats(bool enable);
+
+  bool blimp_enabled() { return blimp_enabled_; }
+  bool record_whole_document() { return record_whole_document_; }
+  bool show_network_stats() { return show_network_stats_; }
+
+ private:
+  // The pref service used to store persist the settings.
+  PrefService* local_state_;
+
+  // A list of all the observers of the Blimp Settings.
+  base::ObserverList<SettingsObserver> observers_;
+
+  void InitializeFromCommandLineAndPref();

[David Trainor- moved to gerrit, 2016/10/05 23:46:21]

Describe what this does/how it's used.

[Menglin, 2016/10/06 22:36:38]

Done.

+
+  // blimp_enabled_ is init from command line/PrefService. It can be changed

[David Trainor- moved to gerrit, 2016/10/05 23:46:20]

s/init/initialized/

[Menglin, 2016/10/06 22:36:38]

The comments are updated in the new format

+  // from UI.

[David Trainor- moved to gerrit, 2016/10/05 23:46:21]

s/from UI/from the UI/

[Menglin, 2016/10/06 22:36:38]

Done.

+  bool blimp_enabled_;
+
+  // Used to avoid sending unnecessary messages to engine. SettingsFeature sends

[David Trainor- moved to gerrit, 2016/10/05 23:46:20]

This determines whether or not the engine sends the whole webpage at once or
pieces of it based on the current viewport.

I wouldn't mention SettingsFeature here.

[Menglin, 2016/10/06 22:36:38]

Done.

+  // this to the engine.
+  bool record_whole_document_;
+
+  // show_network_stats_ can only be set from UI, and the value is not stored

[David Trainor- moved to gerrit, 2016/10/05 23:46:20]

Maybe for all of these (or the function calls to set/get them), have some
consistent comment format?  Maybe something like:

// What this does blah blah blah.
// Can be changed via: command line, settings dialog.
// Persisted across restarts: Yes/No.

[Menglin, 2016/10/06 22:36:38]

Thanks, this is a much better way. Otherwise even I am already confused about
which can be changed in what way.

+  // persistently.
+  bool show_network_stats_;
+
+  DISALLOW_COPY_AND_ASSIGN(Settings);
+};
+
+}  // namespace client
+}  // namespace blimp
+
+#endif  // BLIMP_CLIENT_CORE_SETTINGS_SETTINGS_H_