Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2349073002:
Blimp Settings framework on the c++ side (Closed)
|
Created:
4 years, 3 months ago by Menglin Modified:
4 years, 1 month ago CC:
anandc+watch-blimp_chromium.org, chromium-reviews, dtrainor+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, perumaal+watch-blimp_chromium.org, scf+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBlimp Settings framework on the c++ side
This CL includes:
1. Settings class: where the settings are, through which the settings can
be set and retrieved. It is owned by BlimpClientContextImpl. Hook it up
with BlimpClientContext, so that a PrefService* is passed to store the
settings that needs to be saved persistently.
2. SettingsObserver interface, whose implementations will trigger
different actions on the change of the settings.
3. Added blimp_enabled_, record_whole_document_ and show_network_stats_ to Settings class.
4. SettingsFeature takes a Settings* in the constructor. Add a PushSettings() to the necessary settings to the engine.
BUG=647848
Committed: https://crrev.com/ee3a405c87a05fdf014fae275d5b25fb95e4cda1
Cr-Commit-Position: refs/heads/master@{#428524}
Patch Set 1 #
Total comments: 43
Patch Set 2 : Settings doesn't own its observer anymore. SettingsFeature subclass SettingsObserver #
Total comments: 25
Patch Set 3 : Remove the back pointer in SettingsObserver, sync to head #Patch Set 4 : Move the Android-only profile prefs to chrome/browser/android/preferences/browser_android_prefs.h #
Total comments: 3
Patch Set 5 : Merge branch 'refs/heads/master' into settings #
Total comments: 38
Patch Set 6 : Remove OnRecordWholeDocumentChanged from SettingsObserver and SettingsFeature #
Total comments: 8
Patch Set 7 : nits and sync to head #Patch Set 8 : add deps #Patch Set 9 : include //components/prefs:test_support for //blimp/client/core/context:unit_tests #
Total comments: 17
Patch Set 10 : use ChromeCommandLinePrefStore and sync to head #Patch Set 11 : fix trybot error #
Total comments: 16
Patch Set 12 : Merge branch 'refs/heads/master' into settings #
Total comments: 10
Patch Set 13 : move the PrefService logic from blimp_main.cc #
Total comments: 8
Patch Set 14 : blimp_main.cc, nits and sync to head #
Total comments: 2
Patch Set 15 : nits and sync to head #Messages
Total messages: 103 (49 generated)
mlliu@chromium.org changed reviewers: + dtrainor@chromium.org
On 2016/09/16 at 23:36:42, Menglin wrote: > mlliu@chromium.org changed reviewers: > + dtrainor@chromium.org Hi David, ptal. Since this CL is getting big, I'll save the jni/java part for next CL. Thanks!
https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... File blimp/client/core/settings/settings.cc (right): https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.cc:22: Settings::~Settings() {} = default https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.cc:30: for (const auto& observer : observers_) { Use the observer macro? https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.cc:37: local_state_->SetBoolean(prefs::kBlimpEnabled, enable); Notify the enable blimp mode state changed first? https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.cc:44: if (record_whole_document_ == enable) return; Put this on the others as well? https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.cc:50: } Also requires restart https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.cc:53: void Settings::InitializeFromCommandLineAndPref() { Let's be consistent with how we do this (call set or just set the variable). https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... File blimp/client/core/settings/settings.h (right): https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.h:28: // Initializes blimp_enabled_, and record_whole_document_ from the command Maybe don't include this. I expect it to get out of date really quickly. wdyt? https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.h:30: void InitializeFromCommandLineAndPref(); Can this just happen in the constructor? https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.h:32: void AddObserver(std::unique_ptr<SettingsObserver> observer); I think giving ownership of the observer is a bit odd here. It should probably be a raw pointer. https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.h:33: void RemoveObserver(std::unique_ptr<SettingsObserver> observer) {} Implement? https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.h:35: void SetEnableBlimpMode(bool enable); Comment on these? Maybe also include what command line would set them (if one exists) https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... File blimp/client/core/settings/settings_feature.h (right): https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings_feature.h:15: class SettingsFeature : public BlimpMessageProcessor { Should this class just implement a SettingsObserver and take the Settings object in the constructor? It probably also doesn't need to listen to any settings changes yet because it can just query the value from the Settings object. https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... File blimp/client/core/settings/settings_observer.h (right): https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings_observer.h:8: #include "base/observer_list.h" Remove this? https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings_observer.h:21: SettingsObserver() {} try = default on this too? https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... File blimp/client/core/settings/settings_unittest.cc (right): https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings_unittest.cc:26: MOCK_METHOD1(OnShowNetworkStatsChanged, void(bool)); What about enable? https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings_unittest.cc:42: ~SettingsTest() override {} = default? https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings_unittest.cc:44: MockSettingsObserver* observer_; make this the unique_ptr once you move AddObserver to take raw pointers? https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings_unittest.cc:53: EXPECT_CALL(*observer_, OnShowNetworkStatsChanged(false)). Times(1); If we don't expect calls on sets to the same value, validate that the initial value is as expected too and that calls to set the same value don't trigger an observer call? https://codereview.chromium.org/2349073002/diff/1/blimp/engine/browser_tests/... File blimp/engine/browser_tests/integration_test.cc (right): https://codereview.chromium.org/2349073002/diff/1/blimp/engine/browser_tests/... blimp/engine/browser_tests/integration_test.cc:86: TestingPrefServiceSimple prefs; Won't this die right after this call? Move this to the first class member?
Description was changed from ========== Blimp Settings framework on the c++ side This CL includes: 1. Settings class: where the settings are, through which the settings can be set and retrieved. It is owned by BlimpClientContextImpl. Hook it up with BlimpClientContext, so that a PrefService* is passed to store the settings that needs to be saved persistently. 2. SettingsObserver interface, whose implementations will trigger different actions on the change of the settings. 3. Added EnableBlimp and ShowNetworkStats to Settings class. EnableBlimp can be set/changed by cmd_line, pref storage, and UI. ShowNetworkStats is set/changed by UI. 4. Move RecordWholeDocument from SettingsFeature to Settings. It can be set/changed by cmd_line, pref storage, and UI. BUG=647848 ========== to ========== Blimp Settings framework on the c++ side This CL includes: 1. Settings class: where the settings are, through which the settings can be set and retrieved. It is owned by BlimpClientContextImpl. Hook it up with BlimpClientContext, so that a PrefService* is passed to store the settings that needs to be saved persistently. 2. SettingsObserver interface, whose implementations will trigger different actions on the change of the settings. 3. Added EnableBlimp and ShowNetworkStats to Settings class. EnableBlimp can be set/changed by cmd_line, pref storage, and UI. ShowNetworkStats is set/changed by UI. 4. SettingsFeature subclass SettingsObserver and implements OnRecordWholeDocumentChanged BUG=647848 ==========
mlliu@chromium.org changed reviewers: + thestig@chromium.org
Hi David, new patch uploaded, ptal. Thanks thestig@chromium.org: Please review changes in chrome/browser/ Menglin https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... File blimp/client/core/settings/settings.cc (right): https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.cc:22: Settings::~Settings() {} On 2016/09/27 04:10:06, David Trainor wrote: > = default Done. https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.cc:30: for (const auto& observer : observers_) { On 2016/09/27 04:10:06, David Trainor wrote: > Use the observer macro? Done. https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.cc:37: local_state_->SetBoolean(prefs::kBlimpEnabled, enable); On 2016/09/27 04:10:06, David Trainor wrote: > Notify the enable blimp mode state changed first? Done. https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.cc:44: if (record_whole_document_ == enable) return; On 2016/09/27 04:10:06, David Trainor wrote: > Put this on the others as well? Done. https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.cc:50: } On 2016/09/27 04:10:06, David Trainor wrote: > Also requires restart Done. https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.cc:53: void Settings::InitializeFromCommandLineAndPref() { On 2016/09/27 04:10:06, David Trainor wrote: > Let's be consistent with how we do this (call set or just set the variable). ok. After the change, InitializeFromCommandLineAndPref() only takes care of updating private members and saving to the persistent storage. sending the settings to the engine is moved to SettingsFeature::PushSettings() https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... File blimp/client/core/settings/settings.h (right): https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.h:28: // Initializes blimp_enabled_, and record_whole_document_ from the command On 2016/09/27 04:10:06, David Trainor wrote: > Maybe don't include this. I expect it to get out of date really quickly. wdyt? Done. https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.h:30: void InitializeFromCommandLineAndPref(); On 2016/09/27 04:10:06, David Trainor wrote: > Can this just happen in the constructor? Yes. Then I need to test the constructor because the logic in InitializeFromCommandLineAndPref() needs to be tested. I make a few adjustment in the settings_unittest.cc https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.h:32: void AddObserver(std::unique_ptr<SettingsObserver> observer); On 2016/09/27 04:10:06, David Trainor wrote: > I think giving ownership of the observer is a bit odd here. It should probably > be a raw pointer. Done. https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.h:33: void RemoveObserver(std::unique_ptr<SettingsObserver> observer) {} On 2016/09/27 04:10:06, David Trainor wrote: > Implement? Done. https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.h:35: void SetEnableBlimpMode(bool enable); On 2016/09/27 04:10:06, David Trainor wrote: > Comment on these? Maybe also include what command line would set them (if one > exists) Done. https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... File blimp/client/core/settings/settings_feature.h (right): https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings_feature.h:15: class SettingsFeature : public BlimpMessageProcessor { On 2016/09/27 04:10:06, David Trainor wrote: > Should this class just implement a SettingsObserver and take the Settings object > in the constructor? > > It probably also doesn't need to listen to any settings changes yet because it > can just query the value from the Settings object. That's a great suggestion. I think it's good to make SettingsFeature to subclass SettingsObserver now. And change SetRecordWholeDocument to OnRecordWholeDocumentChanged because it does the right thing (send this setting to the engine) when record_whole_document_ is changed. And then I don't need record_whole_document_observer.h. In the future, we just need another SettingsObserver to implement the rest of its virtual function. https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... File blimp/client/core/settings/settings_observer.h (right): https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings_observer.h:8: #include "base/observer_list.h" On 2016/09/27 04:10:06, David Trainor wrote: > Remove this? Done. https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings_observer.h:21: SettingsObserver() {} On 2016/09/27 04:10:06, David Trainor wrote: > try = default on this too? I am passing a Settings* to the constructor. please see the updated code https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... File blimp/client/core/settings/settings_unittest.cc (right): https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings_unittest.cc:26: MOCK_METHOD1(OnShowNetworkStatsChanged, void(bool)); On 2016/09/27 04:10:06, David Trainor wrote: > What about enable? Done. https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings_unittest.cc:42: ~SettingsTest() override {} On 2016/09/27 04:10:06, David Trainor wrote: > = default? Done. https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings_unittest.cc:44: MockSettingsObserver* observer_; On 2016/09/27 04:10:07, David Trainor wrote: > make this the unique_ptr once you move AddObserver to take raw pointers? Since I moved InitializeFromCommandLineAndPref to Settings' constructor, I need to test the constructor as well. So I'm moving settings_ and observer_ out of the SettingsTest to be able to test the constructor with command line configured. https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings_unittest.cc:53: EXPECT_CALL(*observer_, OnShowNetworkStatsChanged(false)). Times(1); On 2016/09/27 04:10:07, David Trainor wrote: > If we don't expect calls on sets to the same value, validate that the initial > value is as expected too and that calls to set the same value don't trigger an > observer call? Done. https://codereview.chromium.org/2349073002/diff/1/blimp/engine/browser_tests/... File blimp/engine/browser_tests/integration_test.cc (right): https://codereview.chromium.org/2349073002/diff/1/blimp/engine/browser_tests/... blimp/engine/browser_tests/integration_test.cc:86: TestingPrefServiceSimple prefs; On 2016/09/27 04:10:07, David Trainor wrote: > Won't this die right after this call? Move this to the first class member? Right. thank you https://codereview.chromium.org/2349073002/diff/1/chrome/browser/prefs/browse... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/2349073002/diff/1/chrome/browser/prefs/browse... chrome/browser/prefs/browser_prefs.cc:405: in fact i'm adding this to the wrong place. I should add it to RegisterProfilePrefs. The local prefs are saved in BrowserProcess::local_state()
https://codereview.chromium.org/2349073002/diff/20001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/2349073002/diff/20001/chrome/browser/DEPS#new... chrome/browser/DEPS:4: "+blimp/client/public", If the Android-only portion of chrome::RegisterLocalState() moved into chrome/browser/android, do you think this can be omitted, since chrome/browser/android/DEPS already has this rule? i.e. Do for foresee any other part of chrome/browser outside of chrome/browser/android that will need to access blimp/client/public?
On 2016/09/30 23:55:35, Lei Zhang wrote: > https://codereview.chromium.org/2349073002/diff/20001/chrome/browser/DEPS > File chrome/browser/DEPS (right): > > https://codereview.chromium.org/2349073002/diff/20001/chrome/browser/DEPS#new... > chrome/browser/DEPS:4: "+blimp/client/public", > If the Android-only portion of chrome::RegisterLocalState() moved into > chrome/browser/android, do you think this can be omitted, since > chrome/browser/android/DEPS already has this rule? > > i.e. Do for foresee any other part of chrome/browser outside of > chrome/browser/android that will need to access blimp/client/public? Meant to say RegisterProfilePrefs() instead of RegisterLocalState(), but essentially split the Android-only portion of chrome/browser/prefs/browser_prefs.cc out.
https://codereview.chromium.org/2349073002/diff/20001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/2349073002/diff/20001/chrome/browser/DEPS#new... chrome/browser/DEPS:4: "+blimp/client/public", On 2016/09/30 23:55:35, Lei Zhang wrote: > If the Android-only portion of chrome::RegisterLocalState() moved into > chrome/browser/android, do you think this can be omitted, since > chrome/browser/android/DEPS already has this rule? > > i.e. Do for foresee any other part of chrome/browser outside of > chrome/browser/android that will need to access blimp/client/public? I know you meant RegisterProfilePrefs() :) For now I can't think of any other part of chrome/browser needs access to blimp/client/public. David, wdyt?
https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... File blimp/client/core/settings/settings_unittest.cc (right): https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings_unittest.cc:44: MockSettingsObserver* observer_; On 2016/09/30 23:48:57, Menglin wrote: > On 2016/09/27 04:10:07, David Trainor wrote: > > make this the unique_ptr once you move AddObserver to take raw pointers? > > Since I moved InitializeFromCommandLineAndPref to Settings' constructor, I need > to test the constructor as well. So I'm moving settings_ and observer_ out of > the SettingsTest to be able to test the constructor with command line > configured. sg! https://codereview.chromium.org/2349073002/diff/1/chrome/browser/prefs/browse... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/2349073002/diff/1/chrome/browser/prefs/browse... chrome/browser/prefs/browser_prefs.cc:405: On 2016/09/30 23:48:57, Menglin wrote: > in fact i'm adding this to the wrong place. I should add it to > RegisterProfilePrefs. The local prefs are saved in BrowserProcess::local_state() What's the difference between the two? I'm not quite sure I know the scope of each type of pref storage. Profile prefs seems correct, especially since the object we're keying off of is a profile helper. But just check and make sure we understand the implications :). https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/blimp... File blimp/client/core/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.cc:97: settings_(new Settings(local_state)), base::MakeUnique https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.cc:105: settings_feature_(new SettingsFeature(settings_.get())), base::MakeUnique https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.cc:125: settings_feature_->PushSettings(); I wonder if this would make more sense being done after OnConnected? I assume this just queues up the message to be sent once we're connected now though. If that's the case I'm fine with it for now. https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... File blimp/client/core/settings/settings.cc (right): https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... blimp/client/core/settings/settings.cc:36: if (show_network_stats_ == enable) return; if (show_network_stats_ == enable) return; Same with other places https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... blimp/client/core/settings/settings.cc:69: // Get blimp_enabled_ from cmd line and persistent storage, and save the blimp_enabled_ -> record_whole_document_? https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... File blimp/client/core/settings/settings.h (right): https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... blimp/client/core/settings/settings.h:34: // Change show_network_stats_, and save it to the persistent storage. Flip this comment and the one below it? Also mention this can be set with --record-whole-document? https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... File blimp/client/core/settings/settings_feature.cc (right): https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... blimp/client/core/settings/settings_feature.cc:34: void SettingsFeature::SendUserAgentOSVersionInfo(const std::string& osVersion) { Oops we never call this! See crbug.com/652032 that I just filed to track it. https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... blimp/client/core/settings/settings_feature.cc:50: OnRecordWholeDocumentChanged(settings()->record_whole_document()); Can we just build one big EngineSettingsMessage and set all the values on the proto that we need? I think we can set more than one value on the EngineSettingsMessage at once. https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... File blimp/client/core/settings/settings_observer.h (right): https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... blimp/client/core/settings/settings_observer.h:19: virtual void OnBlimpModeEnabled(bool enable) {} Comment on these methods (well, mainly on OnRestartRequired). https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... blimp/client/core/settings/settings_observer.h:27: explicit SettingsObserver(Settings* settings); I'd get rid of the back pointer and make the owner of the observer call AddObserver and RemoveObserver directly. I don't think we need to have the same level of connectedness that we have for BlimpContents and BlimpContentsObserver. https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... blimp/client/core/settings/settings_observer.h:31: Settings* settings_; Follow up on above, remove the back pointer :) https://codereview.chromium.org/2349073002/diff/20001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/2349073002/diff/20001/chrome/browser/DEPS#new... chrome/browser/DEPS:4: "+blimp/client/public", On 2016/10/01 00:08:31, Menglin wrote: > On 2016/09/30 23:55:35, Lei Zhang wrote: > > If the Android-only portion of chrome::RegisterLocalState() moved into > > chrome/browser/android, do you think this can be omitted, since > > chrome/browser/android/DEPS already has this rule? > > > > i.e. Do for foresee any other part of chrome/browser outside of > > chrome/browser/android that will need to access blimp/client/public? > > I know you meant RegisterProfilePrefs() :) > > For now I can't think of any other part of chrome/browser needs access to > blimp/client/public. David, wdyt? I'm fine with relying on the DEPS from chrome/browser/android :). If we really need to access blimp/client/public in chrome/browser in the future we can add a DEPS then.
HI David, new patch upload. ptal thanks! Menglin https://codereview.chromium.org/2349073002/diff/1/chrome/browser/prefs/browse... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/2349073002/diff/1/chrome/browser/prefs/browse... chrome/browser/prefs/browser_prefs.cc:405: On 2016/10/01 03:26:34, David Trainor wrote: > On 2016/09/30 23:48:57, Menglin wrote: > > in fact i'm adding this to the wrong place. I should add it to > > RegisterProfilePrefs. The local prefs are saved in > BrowserProcess::local_state() > > What's the difference between the two? I'm not quite sure I know the scope of > each type of pref storage. Profile prefs seems correct, especially since the > object we're keying off of is a profile helper. But just check and make sure we > understand the implications :). I asked tommy. I think conceptually local prefs are shared among different profiles. And it makes sense to apply the blimp prefs to the current profile. https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/blimp... File blimp/client/core/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.cc:97: settings_(new Settings(local_state)), On 2016/10/01 03:26:34, David Trainor wrote: > base::MakeUnique Done. https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.cc:105: settings_feature_(new SettingsFeature(settings_.get())), On 2016/10/01 03:26:34, David Trainor wrote: > base::MakeUnique Done. https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.cc:125: settings_feature_->PushSettings(); On 2016/10/01 03:26:34, David Trainor wrote: > I wonder if this would make more sense being done after OnConnected? I assume > this just queues up the message to be sent once we're connected now though. If > that's the case I'm fine with it for now. Yeah this just just queues up the message until connected. https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... File blimp/client/core/settings/settings.cc (right): https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... blimp/client/core/settings/settings.cc:36: if (show_network_stats_ == enable) return; On 2016/10/01 03:26:34, David Trainor wrote: > if (show_network_stats_ == enable) > return; > > Same with other places Done. https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... blimp/client/core/settings/settings.cc:69: // Get blimp_enabled_ from cmd line and persistent storage, and save the On 2016/10/01 03:26:34, David Trainor wrote: > blimp_enabled_ -> record_whole_document_? Yeah. bad idea to copy around... thanks! https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... File blimp/client/core/settings/settings.h (right): https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... blimp/client/core/settings/settings.h:34: // Change show_network_stats_, and save it to the persistent storage. On 2016/10/01 03:26:34, David Trainor wrote: > Flip this comment and the one below it? Also mention this can be set with > --record-whole-document? Done. https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... File blimp/client/core/settings/settings_feature.cc (right): https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... blimp/client/core/settings/settings_feature.cc:50: OnRecordWholeDocumentChanged(settings()->record_whole_document()); On 2016/10/01 03:26:35, David Trainor wrote: > Can we just build one big EngineSettingsMessage and set all the values on the > proto that we need? I think we can set more than one value on the > EngineSettingsMessage at once. Done. https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... File blimp/client/core/settings/settings_observer.h (right): https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... blimp/client/core/settings/settings_observer.h:19: virtual void OnBlimpModeEnabled(bool enable) {} On 2016/10/01 03:26:35, David Trainor wrote: > Comment on these methods (well, mainly on OnRestartRequired). Done. https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... blimp/client/core/settings/settings_observer.h:27: explicit SettingsObserver(Settings* settings); On 2016/10/01 03:26:35, David Trainor wrote: > I'd get rid of the back pointer and make the owner of the observer call > AddObserver and RemoveObserver directly. I don't think we need to have the same > level of connectedness that we have for BlimpContents and BlimpContentsObserver. Done. https://codereview.chromium.org/2349073002/diff/20001/blimp/client/core/setti... blimp/client/core/settings/settings_observer.h:31: Settings* settings_; On 2016/10/01 03:26:35, David Trainor wrote: > Follow up on above, remove the back pointer :) Done.
https://codereview.chromium.org/2349073002/diff/20001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/2349073002/diff/20001/chrome/browser/DEPS#new... chrome/browser/DEPS:4: "+blimp/client/public", On 2016/10/01 03:26:35, David Trainor wrote: > I'm fine with relying on the DEPS from chrome/browser/android :). If we really > need to access blimp/client/public in chrome/browser in the future we can add a > DEPS then. So do we want to refactor the Android-only portion of RegisterLocalState() and RegisterProfilePrefs() into, say, chrome/browser/android/preferences/browser_prefs.cc?
On 2016/10/03 23:13:54, Lei Zhang wrote: > https://codereview.chromium.org/2349073002/diff/20001/chrome/browser/DEPS > File chrome/browser/DEPS (right): > > https://codereview.chromium.org/2349073002/diff/20001/chrome/browser/DEPS#new... > chrome/browser/DEPS:4: "+blimp/client/public", > On 2016/10/01 03:26:35, David Trainor wrote: > > I'm fine with relying on the DEPS from chrome/browser/android :). If we > really > > need to access blimp/client/public in chrome/browser in the future we can add > a > > DEPS then. > > So do we want to refactor the Android-only portion of RegisterLocalState() and > RegisterProfilePrefs() into, say, > chrome/browser/android/preferences/browser_prefs.cc? ... either as part of this CL, or as a separate CL for this CL to depend on.
On 2016/10/03 23:14:39, Lei Zhang wrote: > On 2016/10/03 23:13:54, Lei Zhang wrote: > > https://codereview.chromium.org/2349073002/diff/20001/chrome/browser/DEPS > > File chrome/browser/DEPS (right): > > > > > https://codereview.chromium.org/2349073002/diff/20001/chrome/browser/DEPS#new... > > chrome/browser/DEPS:4: "+blimp/client/public", > > On 2016/10/01 03:26:35, David Trainor wrote: > > > I'm fine with relying on the DEPS from chrome/browser/android :). If we > > really > > > need to access blimp/client/public in chrome/browser in the future we can > add > > a > > > DEPS then. > > > > So do we want to refactor the Android-only portion of RegisterLocalState() and > > RegisterProfilePrefs() into, say, > > chrome/browser/android/preferences/browser_prefs.cc? > > ... either as part of this CL, or as a separate CL for this CL to depend on. ok. i'll separate the Android prefs into the android subdirectory in this CL.
New patch uploaded. In patch 4, the Android-only profile prefs are moved to chrome/browser/android/preferences/browser_android_prefs.h Thanks! Menglin
https://codereview.chromium.org/2349073002/diff/60001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/2349073002/diff/60001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:678: ::android::RegisterUserProfilePrefs(registry); Shouldn't this be RegisterProfilePrefs() and shouldn't it be called around like 576?
https://codereview.chromium.org/2349073002/diff/60001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/2349073002/diff/60001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:678: ::android::RegisterUserProfilePrefs(registry); On 2016/10/04 01:19:10, Lei Zhang wrote: > Shouldn't this be RegisterProfilePrefs() and shouldn't it be called around like > 576? Oh thought doing something similar to the chromeos::PowerPrefs::RegisterUserProfilePrefs(registry); at line 674
lgtm if dtrainor approves as well. https://codereview.chromium.org/2349073002/diff/60001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/2349073002/diff/60001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:678: ::android::RegisterUserProfilePrefs(registry); On 2016/10/04 01:21:46, Menglin wrote: > On 2016/10/04 01:19:10, Lei Zhang wrote: > > Shouldn't this be RegisterProfilePrefs() and shouldn't it be called around > like > > 576? > > Oh thought doing something similar to the > chromeos::PowerPrefs::RegisterUserProfilePrefs(registry); at line 674 Ah, ok.
On 2016/10/04 01:19:10, Lei Zhang wrote: > https://codereview.chromium.org/2349073002/diff/60001/chrome/browser/prefs/br... > File chrome/browser/prefs/browser_prefs.cc (right): > > https://codereview.chromium.org/2349073002/diff/60001/chrome/browser/prefs/br... > chrome/browser/prefs/browser_prefs.cc:678: > ::android::RegisterUserProfilePrefs(registry); > Shouldn't this be RegisterProfilePrefs() and shouldn't it be called around like > 576? ok. I made it RegisterProfilePrefs(), please see in patch 5. thanks
I'm fine either way, though actually I liked patch set 4 better once you mentioned why.
https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... File blimp/client/core/settings/settings.cc (right): https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.cc:68: // Get blimp_enabled_ from cmd line and persistent storage. s/and/or https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.cc:72: // Get record_whole_document_ from cmd line and persistent storage, and save s/and/or https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.cc:77: local_state_->SetBoolean(prefs::kRecordWholeDocument, record_whole_document_); I'm not sure we want to do this right? We don't with blimp_enabled_ either. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... File blimp/client/core/settings/settings.h (right): https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.h:31: // --enable-blimp . after https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.h:32: void SetEnableBlimpMode(bool enable); 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. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.h:38: // Change show_network_stats_, and save it to the persistent storage. Does (Should?) this actually save to persistent storage? https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.h:52: void InitializeFromCommandLineAndPref(); Describe what this does/how it's used. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.h:54: // blimp_enabled_ is init from command line/PrefService. It can be changed s/init/initialized/ https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.h:55: // from UI. s/from UI/from the UI/ https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.h:58: // Used to avoid sending unnecessary messages to engine. SettingsFeature sends 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. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.h:62: // show_network_stats_ can only be set from UI, and the value is not stored 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. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... File blimp/client/core/settings/settings_feature.cc (right): https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings_feature.cc:18: if (settings_) { Remove {} if single line after if. Same below. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings_feature.cc:34: void SettingsFeature::OnRecordWholeDocumentChanged(bool enable) { We shouldn't need this. IIUC we only expect this to work after restarts. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings_feature.cc:42: void SettingsFeature::SendUserAgentOSVersionInfo(const std::string& osVersion) { Add a line saying remove this method and tie it to your TODO down below. We won't need this once we send it with the initial settings values. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... File blimp/client/core/settings/settings_feature.h (right): https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings_feature.h:22: explicit SettingsFeature(Settings* settings); Can you describe the expectation that Settings outlive this SettingsFeature? https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings_feature.h:33: void OnRecordWholeDocumentChanged(bool enable) override; Actually I'm not sure we need this yet. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... File blimp/client/core/settings/settings_observer.h (right): https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings_observer.h:28: // Invoked when Blimp mode or record_whole_document changed in the Settings. Make this more generic. Invoked when a settings changed that requires the application to restart before it can take effect. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/session/bl... File blimp/client/session/blimp_client_session.cc (right): https://codereview.chromium.org/2349073002/diff/80001/blimp/client/session/bl... blimp/client/session/blimp_client_session.cc:157: settings_feature_->OnRecordWholeDocumentChanged(true); Since we don't really need this method, I'm actually OK with deprecating the Record Whole Document command line in this session until we move the standalone app to 0.6. https://codereview.chromium.org/2349073002/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/browser_android_prefs.h (right): https://codereview.chromium.org/2349073002/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/browser_android_prefs.h:5: #ifndef CHROME_BROWSER_ANDROID_PREFERENCES_BROWSER_ANDROID_PREFS_H_ Should this file be browser_prefs_android instead of browser_android_prefs?
Hi David, new patch upload. PTAL. Thanks! Menglin https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... File blimp/client/core/settings/settings.cc (right): https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.cc:68: // Get blimp_enabled_ from cmd line and persistent storage. On 2016/10/05 23:46:20, David Trainor wrote: > s/and/or Done. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.cc:72: // Get record_whole_document_ from cmd line and persistent storage, and save On 2016/10/05 23:46:20, David Trainor wrote: > s/and/or Done. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.cc:77: local_state_->SetBoolean(prefs::kRecordWholeDocument, record_whole_document_); On 2016/10/05 23:46:20, David Trainor wrote: > I'm not sure we want to do this right? We don't with blimp_enabled_ either. Done. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... File blimp/client/core/settings/settings.h (right): https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.h:31: // --enable-blimp On 2016/10/05 23:46:20, David Trainor wrote: > . after Done. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.h:32: void SetEnableBlimpMode(bool enable); On 2016/10/05 23:46:20, David Trainor wrote: > 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. I added the comments at each private members. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.h:38: // Change show_network_stats_, and save it to the persistent storage. On 2016/10/05 23:46:21, David Trainor wrote: > Does (Should?) this actually save to persistent storage? No it shouldn't.. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.h:52: void InitializeFromCommandLineAndPref(); On 2016/10/05 23:46:21, David Trainor wrote: > Describe what this does/how it's used. Done. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.h:54: // blimp_enabled_ is init from command line/PrefService. It can be changed On 2016/10/05 23:46:20, David Trainor wrote: > s/init/initialized/ The comments are updated in the new format https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.h:55: // from UI. On 2016/10/05 23:46:21, David Trainor wrote: > s/from UI/from the UI/ Done. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.h:58: // Used to avoid sending unnecessary messages to engine. SettingsFeature sends On 2016/10/05 23:46:20, David Trainor wrote: > 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. Done. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings.h:62: // show_network_stats_ can only be set from UI, and the value is not stored On 2016/10/05 23:46:20, David Trainor wrote: > 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. Thanks, this is a much better way. Otherwise even I am already confused about which can be changed in what way. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... File blimp/client/core/settings/settings_feature.cc (right): https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings_feature.cc:18: if (settings_) { On 2016/10/05 23:46:21, David Trainor wrote: > Remove {} if single line after if. Same below. don't need the observer setup since SettingsFeature doesn't have to subclass SettingsObserver after remove OnRecordWholeDocumentChanged https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings_feature.cc:34: void SettingsFeature::OnRecordWholeDocumentChanged(bool enable) { On 2016/10/05 23:46:21, David Trainor wrote: > We shouldn't need this. IIUC we only expect this to work after restarts. OK. I'm removing this method, in the mean time, SettingsFeature doesn't have to subclass SettingsObserver. My understanding is when record_whole_document_ is changed, restart will be triggered, and the message will be sent in PushSettings after the restart, so we don't need this method. cmiiw https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings_feature.cc:42: void SettingsFeature::SendUserAgentOSVersionInfo(const std::string& osVersion) { On 2016/10/05 23:46:21, David Trainor wrote: > Add a line saying remove this method and tie it to your TODO down below. We > won't need this once we send it with the initial settings values. Done. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... File blimp/client/core/settings/settings_feature.h (right): https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings_feature.h:22: explicit SettingsFeature(Settings* settings); On 2016/10/05 23:46:21, David Trainor wrote: > Can you describe the expectation that Settings outlive this SettingsFeature? Done. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings_feature.h:33: void OnRecordWholeDocumentChanged(bool enable) override; On 2016/10/05 23:46:21, David Trainor wrote: > Actually I'm not sure we need this yet. Done. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... File blimp/client/core/settings/settings_observer.h (right): https://codereview.chromium.org/2349073002/diff/80001/blimp/client/core/setti... blimp/client/core/settings/settings_observer.h:28: // Invoked when Blimp mode or record_whole_document changed in the Settings. On 2016/10/05 23:46:21, David Trainor wrote: > Make this more generic. Invoked when a settings changed that requires the > application to restart before it can take effect. Done. https://codereview.chromium.org/2349073002/diff/80001/blimp/client/session/bl... File blimp/client/session/blimp_client_session.cc (right): https://codereview.chromium.org/2349073002/diff/80001/blimp/client/session/bl... blimp/client/session/blimp_client_session.cc:157: settings_feature_->OnRecordWholeDocumentChanged(true); On 2016/10/05 23:46:21, David Trainor wrote: > Since we don't really need this method, I'm actually OK with deprecating the > Record Whole Document command line in this session until we move the standalone > app to 0.6. Done. https://codereview.chromium.org/2349073002/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/browser_android_prefs.h (right): https://codereview.chromium.org/2349073002/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/browser_android_prefs.h:5: #ifndef CHROME_BROWSER_ANDROID_PREFERENCES_BROWSER_ANDROID_PREFS_H_ On 2016/10/05 23:46:21, David Trainor wrote: > Should this file be browser_prefs_android instead of browser_android_prefs? Done.
lgtm % nits. Thanks! https://codereview.chromium.org/2349073002/diff/100001/blimp/client/core/sett... File blimp/client/core/settings/settings.h (right): https://codereview.chromium.org/2349073002/diff/100001/blimp/client/core/sett... blimp/client/core/settings/settings.h:50: // It gets blimp_enabled_ and record_whole_document_ from command line or Maybe keep this generic. It'll get dated quickly. "Pulls initial settings values from command line or persistent storage." https://codereview.chromium.org/2349073002/diff/100001/blimp/client/core/sett... File blimp/client/core/settings/settings_feature.cc (right): https://codereview.chromium.org/2349073002/diff/100001/blimp/client/core/sett... blimp/client/core/settings/settings_feature.cc:19: SettingsFeature::~SettingsFeature() {} Does = default work? https://codereview.chromium.org/2349073002/diff/100001/blimp/client/core/sett... File blimp/client/core/settings/settings_observer.h (right): https://codereview.chromium.org/2349073002/diff/100001/blimp/client/core/sett... blimp/client/core/settings/settings_observer.h:13: class SettingsObserver { Leave OnRecordWholeDocumentChanged here IMO. https://codereview.chromium.org/2349073002/diff/100001/chrome/browser/android... File chrome/browser/android/preferences/browser_prefs_android.h (right): https://codereview.chromium.org/2349073002/diff/100001/chrome/browser/android... chrome/browser/android/preferences/browser_prefs_android.h:10: } // namespace user_prefs
Description was changed from ========== Blimp Settings framework on the c++ side This CL includes: 1. Settings class: where the settings are, through which the settings can be set and retrieved. It is owned by BlimpClientContextImpl. Hook it up with BlimpClientContext, so that a PrefService* is passed to store the settings that needs to be saved persistently. 2. SettingsObserver interface, whose implementations will trigger different actions on the change of the settings. 3. Added EnableBlimp and ShowNetworkStats to Settings class. EnableBlimp can be set/changed by cmd_line, pref storage, and UI. ShowNetworkStats is set/changed by UI. 4. SettingsFeature subclass SettingsObserver and implements OnRecordWholeDocumentChanged BUG=647848 ========== to ========== Blimp Settings framework on the c++ side This CL includes: 1. Settings class: where the settings are, through which the settings can be set and retrieved. It is owned by BlimpClientContextImpl. Hook it up with BlimpClientContext, so that a PrefService* is passed to store the settings that needs to be saved persistently. 2. SettingsObserver interface, whose implementations will trigger different actions on the change of the settings. 3. Added blimp_enabled_, record_whole_document_ and show_network_stats_ to Settings class. 4. SettingsFeature takes a Settings* in the constructor. Add a PushSettings() to the necessary settings to the engine. BUG=647848 ==========
https://codereview.chromium.org/2349073002/diff/100001/blimp/client/core/sett... File blimp/client/core/settings/settings.h (right): https://codereview.chromium.org/2349073002/diff/100001/blimp/client/core/sett... blimp/client/core/settings/settings.h:50: // It gets blimp_enabled_ and record_whole_document_ from command line or On 2016/10/07 07:04:59, David Trainor wrote: > Maybe keep this generic. It'll get dated quickly. "Pulls initial settings > values from command line or persistent storage." very well worded. thank you https://codereview.chromium.org/2349073002/diff/100001/blimp/client/core/sett... File blimp/client/core/settings/settings_feature.cc (right): https://codereview.chromium.org/2349073002/diff/100001/blimp/client/core/sett... blimp/client/core/settings/settings_feature.cc:19: SettingsFeature::~SettingsFeature() {} On 2016/10/07 07:04:59, David Trainor wrote: > Does = default work? yeah! Done. https://codereview.chromium.org/2349073002/diff/100001/blimp/client/core/sett... File blimp/client/core/settings/settings_observer.h (right): https://codereview.chromium.org/2349073002/diff/100001/blimp/client/core/sett... blimp/client/core/settings/settings_observer.h:13: class SettingsObserver { On 2016/10/07 07:04:59, David Trainor wrote: > Leave OnRecordWholeDocumentChanged here IMO. Done. https://codereview.chromium.org/2349073002/diff/100001/chrome/browser/android... File chrome/browser/android/preferences/browser_prefs_android.h (right): https://codereview.chromium.org/2349073002/diff/100001/chrome/browser/android... chrome/browser/android/preferences/browser_prefs_android.h:10: } On 2016/10/07 07:04:59, David Trainor wrote: > // namespace user_prefs Done.
The CQ bit was checked by mlliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2349073002/#ps120001 (title: "nits and sync to head")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
mlliu@chromium.org changed reviewers: + battre@chromium.org
Hi battre@, could you review this change? I'm not sure I got all owners' green but it's still saying missing owners.. Thanks! Menglin
The CQ bit was checked by mlliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mlliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/08 00:49:11, Menglin wrote: > Hi battre@, could you review this change? I'm not sure I got all owners' green > but it's still saying missing owners.. > > Thanks! > > Menglin ah cuz i changed blimp/client/core/DEPS. battre@ please review the change. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
battre@chromium.org changed reviewers: + bauerb@chromium.org
Hi. I am a bit concerned that this is not a very idiomatic of the PrefService that may break if in the future any of the following happens because the PrefService is not the source of truth anymore: - Somebody sets a preference directly via PrefService::Set...(); (this would be an intended use of the API - e.g. in a browser test) - An enterprise policy changes a preference. - An extension changes a preference. Typically in all of these cases, the PrefService takes care that proper notifications are sent. Your CL has the outlier that one pref is not persisted. I guess you could implement that via a PrefService::AddPrefInitObserver callback (I guess this would be a bit hacky) or keep the base::ObserverList<SettingsObserver> observers_;. +bauerb to see whether he has a nice idea for the latter issue. Bernhard, do you have strong feelings about keeping the PrefService the source of truth? https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... File blimp/client/core/settings/settings.cc (right): https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:44: void Settings::SetEnableBlimpMode(bool enable) { 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); } https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:50: FOR_EACH_OBSERVER(SettingsObserver, observers_, OnBlimpModeEnabled(enable)); 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 (...) { } } https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:73: local_state_->GetBoolean(prefs::kRecordWholeDocument); 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.
On 2016/10/10 09:28:30, battre wrote: > Hi. > > I am a bit concerned that this is not a very idiomatic of the PrefService that > may break if in the future any of the following happens because the PrefService > is not the source of truth anymore: > - Somebody sets a preference directly via PrefService::Set...(); (this would be > an intended use of the API - e.g. in a browser test) > - An enterprise policy changes a preference. > - An extension changes a preference. > > Typically in all of these cases, the PrefService takes care that proper > notifications are sent. > > Your CL has the outlier that one pref is not persisted. I guess you could > implement that via a PrefService::AddPrefInitObserver callback (I guess this > would be a bit hacky) or keep the base::ObserverList<SettingsObserver> > observers_;. > > +bauerb to see whether he has a nice idea for the latter issue. > > Bernhard, do you have strong feelings about keeping the PrefService the source > of truth? Yeah, I agree with Dominic's opinion that PrefService will already do a bunch of things for you here. Of course, you don't *have* to use them, in particular if you know for sure this code is going to be the only customer, but you never know whether some requirement like support for enterprise policies will come up later, and at that point being able to use the existing support in PrefService will be much nicer than reimplementing it. https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... File blimp/client/core/settings/settings.cc (right): https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:50: FOR_EACH_OBSERVER(SettingsObserver, observers_, OnBlimpModeEnabled(enable)); On 2016/10/10 09:28:30, battre wrote: > 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 (...) { > } > } 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.
https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... File blimp/client/core/settings/settings.cc (right): https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:44: void Settings::SetEnableBlimpMode(bool enable) { On 2016/10/10 09:28:30, battre wrote: > 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); > } Acknowledged. https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:50: FOR_EACH_OBSERVER(SettingsObserver, observers_, OnBlimpModeEnabled(enable)); On 2016/10/10 13:02:18, Bernhard Bauer wrote: > On 2016/10/10 09:28:30, battre wrote: > > 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 (...) { > > } > > } > > 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. 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. https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:73: local_state_->GetBoolean(prefs::kRecordWholeDocument); On 2016/10/10 09:28:30, battre wrote: > 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. 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?
https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... File blimp/client/core/settings/settings.cc (right): https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:50: FOR_EACH_OBSERVER(SettingsObserver, observers_, OnBlimpModeEnabled(enable)); On 2016/10/10 17:51:35, Menglin wrote: > On 2016/10/10 13:02:18, Bernhard Bauer wrote: > > On 2016/10/10 09:28:30, battre wrote: > > > 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 (...) { > > > } > > > } > > > > 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. > > 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. > 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)? https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:73: local_state_->GetBoolean(prefs::kRecordWholeDocument); On 2016/10/10 17:51:35, Menglin wrote: > On 2016/10/10 09:28:30, battre wrote: > > 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. > > 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? 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?
https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... File blimp/client/core/settings/settings.cc (right): https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:68: local_state_->GetBoolean(prefs::kBlimpEnabled); 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 ...
https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... File blimp/client/core/settings/settings.cc (right): https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:68: local_state_->GetBoolean(prefs::kBlimpEnabled); On 2016/10/10 23:49:01, Menglin wrote: > 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); CommandLinePrefStore will do exactly that for you :) > 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 > ... https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:73: local_state_->GetBoolean(prefs::kRecordWholeDocument); On 2016/10/10 21:18:29, David Trainor wrote: > On 2016/10/10 17:51:35, Menglin wrote: > > On 2016/10/10 09:28:30, battre wrote: > > > 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. > > > > 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? > > 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? > 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).
https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... File blimp/client/core/settings/settings.cc (right): https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:73: local_state_->GetBoolean(prefs::kRecordWholeDocument); On 2016/10/11 09:17:04, Bernhard Bauer wrote: > On 2016/10/10 21:18:29, David Trainor wrote: > > On 2016/10/10 17:51:35, Menglin wrote: > > > On 2016/10/10 09:28:30, battre wrote: > > > > 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. > > > > > > 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? > > > > 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? > > > > 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). 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.
https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... File blimp/client/core/settings/settings.cc (right): https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:73: local_state_->GetBoolean(prefs::kRecordWholeDocument); On 2016/10/11 16:25:31, David Trainor wrote: > On 2016/10/11 09:17:04, Bernhard Bauer wrote: > > On 2016/10/10 21:18:29, David Trainor wrote: > > > On 2016/10/10 17:51:35, Menglin wrote: > > > > On 2016/10/10 09:28:30, battre wrote: > > > > > 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. > > > > > > > > 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? > > > > > > 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? > > > > > > > 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). > > 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. 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.
https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... File blimp/client/core/settings/settings.cc (right): https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:73: local_state_->GetBoolean(prefs::kRecordWholeDocument); On 2016/10/11 16:45:35, Bernhard Bauer wrote: > On 2016/10/11 16:25:31, David Trainor wrote: > > On 2016/10/11 09:17:04, Bernhard Bauer wrote: > > > On 2016/10/10 21:18:29, David Trainor wrote: > > > > On 2016/10/10 17:51:35, Menglin wrote: > > > > > On 2016/10/10 09:28:30, battre wrote: > > > > > > 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. > > > > > > > > > > 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? > > > > > > > > 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? > > > > > > > > > > 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). > > > > 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. > > 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. 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.
On 2016/10/11 16:45:35, Bernhard Bauer wrote: > https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... > File blimp/client/core/settings/settings.cc (right): > > https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... > blimp/client/core/settings/settings.cc:73: > local_state_->GetBoolean(prefs::kRecordWholeDocument); > On 2016/10/11 16:25:31, David Trainor wrote: > > On 2016/10/11 09:17:04, Bernhard Bauer wrote: > > > On 2016/10/10 21:18:29, David Trainor wrote: > > > > On 2016/10/10 17:51:35, Menglin wrote: > > > > > On 2016/10/10 09:28:30, battre wrote: > > > > > > 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. > > > > > > > > > > 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? > > > > > > > > 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? > > > > > > > > > > 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). > > > > 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. > > 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. iiuc, if we create our own PrefService, that means in BlimpClientContext, instead of passing the PrefService* of chrome's profile prefs, we create our own PrefServiceFactory and set_command_line_prefs for it. since component doesn't have a command line store. We need to extract a base class to live in component. And to do that I basically need to separate out the simple switches, proxy mode, ssl switches ... which are in component/ in the base class, and the rest in ChromeCommandLinePrefStore? So then do i need a BlimpCommandLinePrefStore? i'm not sure where we'd plug in the blimp switches Do we have to use CommandLinePrefStore ? looks like separating a base class for CommandLinePrefStore in component/ requires some work. Also a question about CommandLinePrefStore, it will invoke the observers. https://cs.chromium.org/chromium/src/chrome/browser/prefs/command_line_pref_s... will the command line trying to save the value to the persisted storage? Thanks!
On 2016/10/11 23:24:48, Menglin wrote: > On 2016/10/11 16:45:35, Bernhard Bauer wrote: > > > https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... > > File blimp/client/core/settings/settings.cc (right): > > > > > https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... > > blimp/client/core/settings/settings.cc:73: > > local_state_->GetBoolean(prefs::kRecordWholeDocument); > > On 2016/10/11 16:25:31, David Trainor wrote: > > > On 2016/10/11 09:17:04, Bernhard Bauer wrote: > > > > On 2016/10/10 21:18:29, David Trainor wrote: > > > > > On 2016/10/10 17:51:35, Menglin wrote: > > > > > > On 2016/10/10 09:28:30, battre wrote: > > > > > > > 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. > > > > > > > > > > > > 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? > > > > > > > > > > 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? > > > > > > > > > > > > > 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). > > > > > > 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. > > > > 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. > > iiuc, if we create our own PrefService, that means in BlimpClientContext, > instead of passing the PrefService* of chrome's profile prefs, we create our own > PrefServiceFactory and set_command_line_prefs for it. since component doesn't > have a command line store. We need to extract a base class to live in component. > And to do that I basically need to separate out the simple switches, proxy mode, > ssl switches ... which are in component/ in the base class, and the rest in > ChromeCommandLinePrefStore? So then do i need a BlimpCommandLinePrefStore? i'm > not sure where we'd plug in the blimp switches > > Do we have to use CommandLinePrefStore ? looks like separating a base class for > CommandLinePrefStore in component/ requires some work. > > Also a question about CommandLinePrefStore, it will invoke the observers. > https://cs.chromium.org/chromium/src/chrome/browser/prefs/command_line_pref_s... > will the command line trying to save the value to the persisted storage? > > Thanks! Actually as I look at the code more carefully, maybe we can have our very simple BlimpCommandLinePrefStore (parrallel to CommandlinePrefStore) which subclass ValueMapPrefStore? In BlimpCommandLinePrefStore. it'll just have a small map for blimp switches->prefs, and have similar logic that CommandlinePrefStore has. And if we don't want to expose our prefs to chrome, we don't need to use the same PrefService as Chrome. I'll discuss with dtrainor@ for a decision
On 2016/10/12 00:04:18, Menglin wrote: > On 2016/10/11 23:24:48, Menglin wrote: > > On 2016/10/11 16:45:35, Bernhard Bauer wrote: > > > > > > https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... > > > File blimp/client/core/settings/settings.cc (right): > > > > > > > > > https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... > > > blimp/client/core/settings/settings.cc:73: > > > local_state_->GetBoolean(prefs::kRecordWholeDocument); > > > On 2016/10/11 16:25:31, David Trainor wrote: > > > > On 2016/10/11 09:17:04, Bernhard Bauer wrote: > > > > > On 2016/10/10 21:18:29, David Trainor wrote: > > > > > > On 2016/10/10 17:51:35, Menglin wrote: > > > > > > > On 2016/10/10 09:28:30, battre wrote: > > > > > > > > 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. > > > > > > > > > > > > > > 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? > > > > > > > > > > > > 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? > > > > > > > > > > > > > > > > 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). > > > > > > > > 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. > > > > > > 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. > > > > iiuc, if we create our own PrefService, that means in BlimpClientContext, > > instead of passing the PrefService* of chrome's profile prefs, we create our > own > > PrefServiceFactory and set_command_line_prefs for it. since component doesn't > > have a command line store. We need to extract a base class to live in > component. > > And to do that I basically need to separate out the simple switches, proxy > mode, > > ssl switches ... which are in component/ in the base class, and the rest in > > ChromeCommandLinePrefStore? So then do i need a BlimpCommandLinePrefStore? i'm > > not sure where we'd plug in the blimp switches > > > > Do we have to use CommandLinePrefStore ? looks like separating a base class > for > > CommandLinePrefStore in component/ requires some work. > > > > Also a question about CommandLinePrefStore, it will invoke the observers. > > > https://cs.chromium.org/chromium/src/chrome/browser/prefs/command_line_pref_s... > > will the command line trying to save the value to the persisted storage? > > > > Thanks! > > Actually as I look at the code more carefully, maybe we can have our very simple > BlimpCommandLinePrefStore (parrallel to CommandlinePrefStore) which subclass > ValueMapPrefStore? In BlimpCommandLinePrefStore. it'll just have a small map for > blimp switches->prefs, and have similar logic that CommandlinePrefStore has. > > And if we don't want to expose our prefs to chrome, we don't need to use the > same PrefService as Chrome. I'll discuss with dtrainor@ for a decision Hi Bernhard, I thought about how to share CommandLinePrefStore between blimp and chrome. We'd like to keep the blimp_switch_map within blimp. Say static BlimpClientContext::ApplySwicthes() hooks up the bilmp switches with the prefs. That means we need to call SetValue(const std::string& key, std::unique_ptr<base::Value> value,uint32_t flags) in BlimpClientContext::ApplySwicthes(), and that will requires passing a CommandLinePrefStore* to BlimpClientContext. However, from looking through the code, I don't think it's the intention of the design to expose a CommandLinePrefStore*?? Because after CommandLinePrefStore is created, it's immediately set to the factory. It doesn't have any public API except the constructor; neither its owner nor the class which holds its pointer doesn't expose that pointer. cmiiw. So looks like this approach doesn't work. Another approach that may work is to make our blimp switches and prefs accessible to CommandLinePrefStore (currently they're not accessible outside of blimp). So that in CommandLinePrefStore it can have something like: ommandLinePrefStore::CommandLinePrefStore(const base::CommandLine* command_line) : command_line_(command_line) { #if defined(OS_ANDROID) ApplyBlimpSwitch() #endif ApplySimpleSwitches(); ApplyProxyMode(); ... } When you mentioned about delegating handling Blimp command line switches to something in blimp/, were you thinking about doing that by passing the pointer? or am I missing anything? Thanks!
On 2016/10/13 03:56:59, Menglin wrote: > On 2016/10/12 00:04:18, Menglin wrote: > > On 2016/10/11 23:24:48, Menglin wrote: > > > On 2016/10/11 16:45:35, Bernhard Bauer wrote: > > > > > > > > > > https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... > > > > File blimp/client/core/settings/settings.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... > > > > blimp/client/core/settings/settings.cc:73: > > > > local_state_->GetBoolean(prefs::kRecordWholeDocument); > > > > On 2016/10/11 16:25:31, David Trainor wrote: > > > > > On 2016/10/11 09:17:04, Bernhard Bauer wrote: > > > > > > On 2016/10/10 21:18:29, David Trainor wrote: > > > > > > > On 2016/10/10 17:51:35, Menglin wrote: > > > > > > > > On 2016/10/10 09:28:30, battre wrote: > > > > > > > > > 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. > > > > > > > > > > > > > > > > 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? > > > > > > > > > > > > > > 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? > > > > > > > > > > > > > > > > > > > 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). > > > > > > > > > > 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. > > > > > > > > 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. > > > > > > iiuc, if we create our own PrefService, that means in BlimpClientContext, > > > instead of passing the PrefService* of chrome's profile prefs, we create our > > own > > > PrefServiceFactory and set_command_line_prefs for it. since component > doesn't > > > have a command line store. We need to extract a base class to live in > > component. > > > And to do that I basically need to separate out the simple switches, proxy > > mode, > > > ssl switches ... which are in component/ in the base class, and the rest in > > > ChromeCommandLinePrefStore? So then do i need a BlimpCommandLinePrefStore? > i'm > > > not sure where we'd plug in the blimp switches > > > > > > Do we have to use CommandLinePrefStore ? looks like separating a base class > > for > > > CommandLinePrefStore in component/ requires some work. > > > > > > Also a question about CommandLinePrefStore, it will invoke the observers. > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/prefs/command_line_pref_s... > > > will the command line trying to save the value to the persisted storage? > > > > > > Thanks! > > > > Actually as I look at the code more carefully, maybe we can have our very > simple > > BlimpCommandLinePrefStore (parrallel to CommandlinePrefStore) which subclass > > ValueMapPrefStore? In BlimpCommandLinePrefStore. it'll just have a small map > for > > blimp switches->prefs, and have similar logic that CommandlinePrefStore has. > > > > And if we don't want to expose our prefs to chrome, we don't need to use the > > same PrefService as Chrome. I'll discuss with dtrainor@ for a decision > > Hi Bernhard, I thought about how to share CommandLinePrefStore between blimp and > chrome. > > We'd like to keep the blimp_switch_map within blimp. Say static > BlimpClientContext::ApplySwicthes() hooks up the bilmp switches with the prefs. > That means we need to call SetValue(const std::string& key, > std::unique_ptr<base::Value> value,uint32_t flags) in > BlimpClientContext::ApplySwicthes(), and that will requires passing a > CommandLinePrefStore* to BlimpClientContext. > > However, from looking through the code, I don't think it's the intention of the > design to expose a CommandLinePrefStore*?? Because after CommandLinePrefStore is > created, it's immediately set to the factory. It doesn't have any public API > except the constructor; neither its owner nor the class which holds its pointer > doesn't expose that pointer. cmiiw. So looks like this approach doesn't work. > If we refactor CommandLinePrefStore and move it to the pref component, could we just expose a few methods like: ApplySwitches(Map...); ApplyBooleanSwitches(Map...); ... etc. Then Chrome could call those methods to add the Chrome switches and we could pass CommandLinePrefStore to Blimp and let Blimp call the methods it needs to add the Blimp switches as well. Since the call on BlimpClientContext will be static we could probably call it right as we build the CommandLinePrefStore as well (before we give it to the factory). The method would probably look like: static BlimpClientContext::RegisterCommandLinePrefOverrides(CommandLinePrefStore* store); wdyt? Does that make sense? > Another approach that may work is to make our blimp switches and prefs > accessible to CommandLinePrefStore (currently they're not accessible outside of > blimp). So that in CommandLinePrefStore it can have something like: > ommandLinePrefStore::CommandLinePrefStore(const base::CommandLine* command_line) > : command_line_(command_line) { > #if defined(OS_ANDROID) > ApplyBlimpSwitch() > #endif > > ApplySimpleSwitches(); > ApplyProxyMode(); > ... > } > This might have the issue of not working with other embedders. We have a Linux shell client (and potentially an Android shell client) that should still work with command line args. We'd need to build a separate CommandLinePrefStore for those anyway. > When you mentioned about delegating handling Blimp command line switches to > something in blimp/, were you thinking about doing that by passing the pointer? > or am I missing anything? If I'm reading this right, it sounds like what I'm also proposing above, so it makes sense to me! :) > > Thanks!
On 2016/10/13 05:12:26, David Trainor wrote: > On 2016/10/13 03:56:59, Menglin wrote: > > On 2016/10/12 00:04:18, Menglin wrote: > > > On 2016/10/11 23:24:48, Menglin wrote: > > > > On 2016/10/11 16:45:35, Bernhard Bauer wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... > > > > > File blimp/client/core/settings/settings.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... > > > > > blimp/client/core/settings/settings.cc:73: > > > > > local_state_->GetBoolean(prefs::kRecordWholeDocument); > > > > > On 2016/10/11 16:25:31, David Trainor wrote: > > > > > > On 2016/10/11 09:17:04, Bernhard Bauer wrote: > > > > > > > On 2016/10/10 21:18:29, David Trainor wrote: > > > > > > > > On 2016/10/10 17:51:35, Menglin wrote: > > > > > > > > > On 2016/10/10 09:28:30, battre wrote: > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > 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? > > > > > > > > > > > > > > > > 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? > > > > > > > > > > > > > > > > > > > > > > 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). > > > > > > > > > > > > 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. > > > > > > > > > > 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. > > > > > > > > iiuc, if we create our own PrefService, that means in BlimpClientContext, > > > > instead of passing the PrefService* of chrome's profile prefs, we create > our > > > own > > > > PrefServiceFactory and set_command_line_prefs for it. since component > > doesn't > > > > have a command line store. We need to extract a base class to live in > > > component. > > > > And to do that I basically need to separate out the simple switches, proxy > > > mode, > > > > ssl switches ... which are in component/ in the base class, and the rest > in > > > > ChromeCommandLinePrefStore? So then do i need a BlimpCommandLinePrefStore? > > i'm > > > > not sure where we'd plug in the blimp switches > > > > > > > > Do we have to use CommandLinePrefStore ? looks like separating a base > class > > > for > > > > CommandLinePrefStore in component/ requires some work. > > > > > > > > Also a question about CommandLinePrefStore, it will invoke the observers. > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/prefs/command_line_pref_s... > > > > will the command line trying to save the value to the persisted storage? > > > > > > > > Thanks! > > > > > > Actually as I look at the code more carefully, maybe we can have our very > > simple > > > BlimpCommandLinePrefStore (parrallel to CommandlinePrefStore) which subclass > > > ValueMapPrefStore? In BlimpCommandLinePrefStore. it'll just have a small map > > for > > > blimp switches->prefs, and have similar logic that CommandlinePrefStore has. > > > > > > And if we don't want to expose our prefs to chrome, we don't need to use the > > > same PrefService as Chrome. I'll discuss with dtrainor@ for a decision > > > > Hi Bernhard, I thought about how to share CommandLinePrefStore between blimp > and > > chrome. > > > > We'd like to keep the blimp_switch_map within blimp. Say static > > BlimpClientContext::ApplySwicthes() hooks up the bilmp switches with the > prefs. > > That means we need to call SetValue(const std::string& key, > > std::unique_ptr<base::Value> value,uint32_t flags) in > > BlimpClientContext::ApplySwicthes(), and that will requires passing a > > CommandLinePrefStore* to BlimpClientContext. > > > > However, from looking through the code, I don't think it's the intention of > the > > design to expose a CommandLinePrefStore*?? Because after CommandLinePrefStore > is > > created, it's immediately set to the factory. It doesn't have any public API > > except the constructor; neither its owner nor the class which holds its > pointer > > doesn't expose that pointer. cmiiw. So looks like this approach doesn't work. > > > > If we refactor CommandLinePrefStore and move it to the pref component, could we > just expose a few methods like: > > ApplySwitches(Map...); > ApplyBooleanSwitches(Map...); > ... etc. > > Then Chrome could call those methods to add the Chrome switches and we could > pass CommandLinePrefStore to Blimp and let Blimp call the methods it needs to > add the Blimp switches as well. Since the call on BlimpClientContext will be > static we could probably call it right as we build the CommandLinePrefStore as > well (before we give it to the factory). > > The method would probably look like: > static > BlimpClientContext::RegisterCommandLinePrefOverrides(CommandLinePrefStore* > store); > > wdyt? Does that make sense? > > > Another approach that may work is to make our blimp switches and prefs > > accessible to CommandLinePrefStore (currently they're not accessible outside > of > > blimp). So that in CommandLinePrefStore it can have something like: > > ommandLinePrefStore::CommandLinePrefStore(const base::CommandLine* > command_line) > > : command_line_(command_line) { > > #if defined(OS_ANDROID) > > ApplyBlimpSwitch() > > #endif > > > > ApplySimpleSwitches(); > > ApplyProxyMode(); > > ... > > } > > > > This might have the issue of not working with other embedders. We have a Linux > shell client (and potentially an Android shell client) that should still work > with command line args. We'd need to build a separate CommandLinePrefStore for > those anyway. > > > When you mentioned about delegating handling Blimp command line switches to > > something in blimp/, were you thinking about doing that by passing the > pointer? > > or am I missing anything? > > If I'm reading this right, it sounds like what I'm also proposing above, so it > makes sense to me! :) > > > > > Thanks! Yes, that looks right to me. Essentially, we would mirror what we already do for preference registration (chrome/ code calls a static method in blimp/, passing an object that can be used to register prefs), but with command line switches instead.
The CQ bit was checked by mlliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by mlliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi David and Bernhard, please review Patch 11. In this patch, 1. I fixed the trybots errors from patch 9 and 10 2. I added BlimpClientContext::ApplyBlimpSwitches to delegate handling Blimp command line switches 3. Settings subclass PrefObserver, and in its constructor, add OnPreferenceChanged as the invoked function when prefs::kBlimpEnabled or prefs::kRecordWholeDocument is changed. 4. SettingsFeature implements void OnRecordWholeDocumentChanged from SettingsObserver 5. We are no longer able to test that the observer are invoked only when the pref settings are changed. Because in the settings_unittest.cc, we use TestingPrefServiceSimple and hence TestingPrefStore. TestingPrefStore doesn't have the logic to check if the value is the same as the existing value as JsonPrefStore does (https://cs.chromium.org/chromium/src/components/prefs/json_pref_store.cc?l=232). But since in real case, we're using JsonPrefStore for android and SegregatedPrefStore, and both have the value-checking logic, it should automatically takes care of that for us. PTAL. Thanks! Menglin https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... File blimp/client/core/settings/settings.cc (right): https://codereview.chromium.org/2349073002/diff/1/blimp/client/core/settings/... blimp/client/core/settings/settings.cc:50: } On 2016/09/30 23:48:57, Menglin wrote: > On 2016/09/27 04:10:06, David Trainor wrote: > > Also requires restart > > Done. I'm removing OnRestartRequired() for this and using OnRecordWholeDocumentChanged https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... File blimp/client/core/settings/settings.cc (right): https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:60: FOR_EACH_OBSERVER(SettingsObserver, observers_, OnRestartRequired()); David, as we discussed, when this gets changed, no restart required, just need to invoke OnRecordWholeDocumentChanged(bool) (implemented in SettingsFeature). https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... File blimp/client/core/settings/settings.h (right): https://codereview.chromium.org/2349073002/diff/160001/blimp/client/core/sett... blimp/client/core/settings/settings.h:51: void InitializeFromCommandLineAndPref(); I dont think we need this anymore. because switches will be applied to the prefs when the PrefService is created
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Nice! On 2016/10/24 22:44:59, Menglin wrote: > 5. We are no longer able to test that the observer are invoked only when the > pref settings are changed. Because in the settings_unittest.cc, we use > TestingPrefServiceSimple and hence TestingPrefStore. TestingPrefStore doesn't > have the logic to check if the value is the same as the existing value as > JsonPrefStore does > (https://cs.chromium.org/chromium/src/components/prefs/json_pref_store.cc?l=232). Actually (sorry to bring this up again), is that really the case? https://cs.chromium.org/chromium/src/components/prefs/testing_pref_store.cc?r... does seem to have that check. https://codereview.chromium.org/2349073002/diff/200001/blimp/client/app/linux... File blimp/client/app/linux/blimp_main.cc (right): https://codereview.chromium.org/2349073002/diff/200001/blimp/client/app/linux... blimp/client/app/linux/blimp_main.cc:50: std::unique_ptr<blimp::client::BlimpClientContext> context = FWIW, I would consider using auto here, as you already spell out the type on the next line, and it might help reducing the Tower Of Indentation here. https://codereview.chromium.org/2349073002/diff/200001/blimp/client/app/linux... blimp/client/app/linux/blimp_main.cc:56: (context_delegate.get())->pref_service())); Is this indented correctly? https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... File blimp/client/core/settings/settings.cc (right): https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:53: observer.OnShowNetworkStatsChanged(show_network_stats_); Nit: For the if-statement above, you leave out braces for the body. I'm not sure if Blimp code has a particular preference one way or another, but staying consistent would be good. https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:71: for (auto& observer : observers_) { Is it intentional that this will iterate over all observers twice rather than doing it once and calling both methods? https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... File blimp/client/core/settings/settings.h (right): https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... blimp/client/core/settings/settings.h:51: bool blimp_enabled() { Nit: I think I would use CamelCase, ie. GetBlimpEnabled() or IsBlimpEnabled(), as this is not actually as simple as reading a member, and it would make it consistent with the setter. https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... blimp/client/core/settings/settings.h:71: Nit: superfluous empty line. https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... blimp/client/core/settings/settings.h:82: boolean_switch_map_[]; Does this need to be declared in the header? https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... File blimp/client/core/settings/settings_observer.h (right): https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... blimp/client/core/settings/settings_observer.h:28: // Invoked when a settings changed that requires the application to restart Nit: "setting"
On 2016/10/25 14:30:51, Bernhard Bauer wrote: > Nice! > > On 2016/10/24 22:44:59, Menglin wrote: > > 5. We are no longer able to test that the observer are invoked only when the > > pref settings are changed. Because in the settings_unittest.cc, we use > > TestingPrefServiceSimple and hence TestingPrefStore. TestingPrefStore doesn't > > have the logic to check if the value is the same as the existing value as > > JsonPrefStore does > > > (https://cs.chromium.org/chromium/src/components/prefs/json_pref_store.cc?l=232). > > Actually (sorry to bring this up again), is that really the case? > https://cs.chromium.org/chromium/src/components/prefs/testing_pref_store.cc?r... > does seem to have that check. Thanks for pointing it out. I checked again. What's happening is say in my test code, I have Settings settings(&prefs); // here blimp_enabled() is false MockSettingsObserver observer(&settings); settings.SetEnableBlimpMode(false); // here though blimp_enabled() is false, it will invoked the observer function. settings.SetEnableBlimpMode(false); // but when settings the value the second time, it doesn't invoked the observer function So looks like it invokes the observer the 1st time the value is set, no matter what the initial value is. > > https://codereview.chromium.org/2349073002/diff/200001/blimp/client/app/linux... > File blimp/client/app/linux/blimp_main.cc (right): > > https://codereview.chromium.org/2349073002/diff/200001/blimp/client/app/linux... > blimp/client/app/linux/blimp_main.cc:50: > std::unique_ptr<blimp::client::BlimpClientContext> context = > FWIW, I would consider using auto here, as you already spell out the type on the > next line, and it might help reducing the Tower Of Indentation here. > > https://codereview.chromium.org/2349073002/diff/200001/blimp/client/app/linux... > blimp/client/app/linux/blimp_main.cc:56: > (context_delegate.get())->pref_service())); > Is this indented correctly? > > https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... > File blimp/client/core/settings/settings.cc (right): > > https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... > blimp/client/core/settings/settings.cc:53: > observer.OnShowNetworkStatsChanged(show_network_stats_); > Nit: For the if-statement above, you leave out braces for the body. I'm not sure > if Blimp code has a particular preference one way or another, but staying > consistent would be good. > > https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... > blimp/client/core/settings/settings.cc:71: for (auto& observer : observers_) { > Is it intentional that this will iterate over all observers twice rather than > doing it once and calling both methods? > > https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... > File blimp/client/core/settings/settings.h (right): > > https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... > blimp/client/core/settings/settings.h:51: bool blimp_enabled() { > Nit: I think I would use CamelCase, ie. GetBlimpEnabled() or IsBlimpEnabled(), > as this is not actually as simple as reading a member, and it would make it > consistent with the setter. > > https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... > blimp/client/core/settings/settings.h:71: > Nit: superfluous empty line. > > https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... > blimp/client/core/settings/settings.h:82: boolean_switch_map_[]; > Does this need to be declared in the header? > > https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... > File blimp/client/core/settings/settings_observer.h (right): > > https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... > blimp/client/core/settings/settings_observer.h:28: // Invoked when a settings > changed that requires the application to restart > Nit: "setting"
new patch uploaded. ptal regarding to the notification of TestingPrefStore, I talked to Bernhard offline. The 1st time SetValue gets called, an internal map in the PrefValueMap is empty, so when i call SetValue the 1st time, no matter what value (true/false) i want to set, it'll invoke the observer function any way https://cs.chromium.org/chromium/src/components/prefs/pref_value_map.cc?sq=pa... And that's intended because the source of the pref value changes: initially it's the default value, but once it's set it's coming from the user pref store prefservice distinguishes sources, because that's what allows e.g. to show when a preference is managed by policy. My test code used to check if the observer is invoked when SetValue is called with the default value (default is false, and I SetValue(false)), and that's why i thought TestingPrefStore doesn't have the filter logic In the new patch, I changed the test code a little bit. Thanks! https://codereview.chromium.org/2349073002/diff/200001/blimp/client/app/linux... File blimp/client/app/linux/blimp_main.cc (right): https://codereview.chromium.org/2349073002/diff/200001/blimp/client/app/linux... blimp/client/app/linux/blimp_main.cc:50: std::unique_ptr<blimp::client::BlimpClientContext> context = On 2016/10/25 14:30:50, Bernhard Bauer wrote: > FWIW, I would consider using auto here, as you already spell out the type on the > next line, and it might help reducing the Tower Of Indentation here. Done. https://codereview.chromium.org/2349073002/diff/200001/blimp/client/app/linux... blimp/client/app/linux/blimp_main.cc:56: (context_delegate.get())->pref_service())); On 2016/10/25 14:30:50, Bernhard Bauer wrote: > Is this indented correctly? Done. https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... File blimp/client/core/settings/settings.cc (right): https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:53: observer.OnShowNetworkStatsChanged(show_network_stats_); On 2016/10/25 14:30:50, Bernhard Bauer wrote: > Nit: For the if-statement above, you leave out braces for the body. I'm not sure > if Blimp code has a particular preference one way or another, but staying > consistent would be good. Done. https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:71: for (auto& observer : observers_) { On 2016/10/25 14:30:50, Bernhard Bauer wrote: > Is it intentional that this will iterate over all observers twice rather than > doing it once and calling both methods? should do it once and call both methods. I was just mechanically switching from FOR_EACH_OBSERVER to the for (auto& observer : observers_) format :( https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... File blimp/client/core/settings/settings.h (right): https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... blimp/client/core/settings/settings.h:51: bool blimp_enabled() { On 2016/10/25 14:30:50, Bernhard Bauer wrote: > Nit: I think I would use CamelCase, ie. GetBlimpEnabled() or IsBlimpEnabled(), > as this is not actually as simple as reading a member, and it would make it > consistent with the setter. Done. https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... blimp/client/core/settings/settings.h:71: On 2016/10/25 14:30:50, Bernhard Bauer wrote: > Nit: superfluous empty line. Done. https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... blimp/client/core/settings/settings.h:82: boolean_switch_map_[]; On 2016/10/25 14:30:50, Bernhard Bauer wrote: > Does this need to be declared in the header? no it doesn't have to. Done. https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... File blimp/client/core/settings/settings_observer.h (right): https://codereview.chromium.org/2349073002/diff/200001/blimp/client/core/sett... blimp/client/core/settings/settings_observer.h:28: // Invoked when a settings changed that requires the application to restart On 2016/10/25 14:30:50, Bernhard Bauer wrote: > Nit: "setting" Done.
The CQ bit was checked by mlliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2349073002/diff/220001/blimp/client/app/linux... File blimp/client/app/linux/blimp_client_context_delegate_linux.cc (right): https://codereview.chromium.org/2349073002/diff/220001/blimp/client/app/linux... blimp/client/app/linux/blimp_client_context_delegate_linux.cc:20: pref_registry_->RegisterBooleanPref(prefs::kBlimpEnabled, false); Follow up on the comment in the header. Yeah just move this to main() and set it all up before it's passed into BlimpClientContext::Create https://codereview.chromium.org/2349073002/diff/220001/blimp/client/app/linux... File blimp/client/app/linux/blimp_client_context_delegate_linux.h (right): https://codereview.chromium.org/2349073002/diff/220001/blimp/client/app/linux... blimp/client/app/linux/blimp_client_context_delegate_linux.h:13: } // namespace user_prefs https://codereview.chromium.org/2349073002/diff/220001/blimp/client/app/linux... blimp/client/app/linux/blimp_client_context_delegate_linux.h:14: class PrefService; new line before this https://codereview.chromium.org/2349073002/diff/220001/blimp/client/app/linux... blimp/client/app/linux/blimp_client_context_delegate_linux.h:40: std::unique_ptr<PrefService> pref_service_; Can these just be members in the linux blimp_main main() method? https://codereview.chromium.org/2349073002/diff/220001/blimp/client/app/linux... File blimp/client/app/linux/blimp_main.cc (right): https://codereview.chromium.org/2349073002/diff/220001/blimp/client/app/linux... blimp/client/app/linux/blimp_main.cc:27: int main(int argc, const char**argv) { Should we call RegisterPrefs and the AppendCommandSwitches here?
new patch uploaded. ptal. thanks! https://codereview.chromium.org/2349073002/diff/220001/blimp/client/app/linux... File blimp/client/app/linux/blimp_client_context_delegate_linux.cc (right): https://codereview.chromium.org/2349073002/diff/220001/blimp/client/app/linux... blimp/client/app/linux/blimp_client_context_delegate_linux.cc:20: pref_registry_->RegisterBooleanPref(prefs::kBlimpEnabled, false); On 2016/10/26 01:14:54, David Trainor wrote: > Follow up on the comment in the header. Yeah just move this to main() and set > it all up before it's passed into BlimpClientContext::Create Done. https://codereview.chromium.org/2349073002/diff/220001/blimp/client/app/linux... File blimp/client/app/linux/blimp_client_context_delegate_linux.h (right): https://codereview.chromium.org/2349073002/diff/220001/blimp/client/app/linux... blimp/client/app/linux/blimp_client_context_delegate_linux.h:13: } On 2016/10/26 01:14:54, David Trainor wrote: > // namespace user_prefs Done. https://codereview.chromium.org/2349073002/diff/220001/blimp/client/app/linux... blimp/client/app/linux/blimp_client_context_delegate_linux.h:14: class PrefService; On 2016/10/26 01:14:54, David Trainor wrote: > new line before this Done. https://codereview.chromium.org/2349073002/diff/220001/blimp/client/app/linux... blimp/client/app/linux/blimp_client_context_delegate_linux.h:40: std::unique_ptr<PrefService> pref_service_; On 2016/10/26 01:14:55, David Trainor wrote: > Can these just be members in the linux blimp_main main() method? Done. https://codereview.chromium.org/2349073002/diff/220001/blimp/client/app/linux... File blimp/client/app/linux/blimp_main.cc (right): https://codereview.chromium.org/2349073002/diff/220001/blimp/client/app/linux... blimp/client/app/linux/blimp_main.cc:27: int main(int argc, const char**argv) { On 2016/10/26 01:14:55, David Trainor wrote: > Should we call RegisterPrefs and the AppendCommandSwitches here? Done.
The CQ bit was checked by mlliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2349073002/diff/240001/chrome/browser/prefs/c... File chrome/browser/prefs/chrome_command_line_pref_store.cc (right): https://codereview.chromium.org/2349073002/diff/240001/chrome/browser/prefs/c... chrome/browser/prefs/chrome_command_line_pref_store.cc:21: #include "chrome/browser/android/preferences/command_line_pref_store_android.h" Put this behind #if defined(OS_ANDROID) down around like 34.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2349073002/diff/240001/blimp/client/app/linux... File blimp/client/app/linux/blimp_main.cc (right): https://codereview.chromium.org/2349073002/diff/240001/blimp/client/app/linux... blimp/client/app/linux/blimp_main.cc:55: scoped_refptr<user_prefs::PrefRegistrySyncable> pref_registry = Shouldn't we do the following? blimp::client::RegisterPrefs(...); blimp::client::ApplyBlimpSwitches(...)?
LGTM with nits: https://codereview.chromium.org/2349073002/diff/240001/blimp/client/core/sett... File blimp/client/core/settings/settings.cc (right): https://codereview.chromium.org/2349073002/diff/240001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:72: for (auto& observer : observers_) { No braces here either. https://codereview.chromium.org/2349073002/diff/240001/blimp/client/core/sett... File blimp/client/core/settings/settings.h (right): https://codereview.chromium.org/2349073002/diff/240001/blimp/client/core/sett... blimp/client/core/settings/settings.h:52: return local_state_->GetBoolean(prefs::kBlimpEnabled); And now move the implementation out of line :)
The CQ bit was checked by mlliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
new patch uploaded. David, ptal. Thanks! https://codereview.chromium.org/2349073002/diff/240001/blimp/client/app/linux... File blimp/client/app/linux/blimp_main.cc (right): https://codereview.chromium.org/2349073002/diff/240001/blimp/client/app/linux... blimp/client/app/linux/blimp_main.cc:55: scoped_refptr<user_prefs::PrefRegistrySyncable> pref_registry = On 2016/10/26 07:00:25, David Trainor (OOO to oct 31) wrote: > Shouldn't we do the following? > > blimp::client::RegisterPrefs(...); > blimp::client::ApplyBlimpSwitches(...)? to do ApplyBlimpSwitches, i create a subclass of CommandLinePrefStore here (say BlimpShellCommandLinePrefStore), because we can't use ChromeCommandLinePrefStore here and we can't directly create a CommandLinePrefStore (the constructor is protected). BlimpShellCommandLinePrefStore will have a public constructor and just call blimp::client::BlimpClientContext::ApplyBlimpSwitches(this) in its constructor also, the pref_service_factory needs to set its user_prefs, otherwise it'll crash. I set a InMemoryPrefStore because we just need something for the blimp shell. (see the new patch) https://codereview.chromium.org/2349073002/diff/240001/blimp/client/core/sett... File blimp/client/core/settings/settings.cc (right): https://codereview.chromium.org/2349073002/diff/240001/blimp/client/core/sett... blimp/client/core/settings/settings.cc:72: for (auto& observer : observers_) { On 2016/10/26 09:47:23, Bernhard Bauer wrote: > No braces here either. Done. https://codereview.chromium.org/2349073002/diff/240001/blimp/client/core/sett... File blimp/client/core/settings/settings.h (right): https://codereview.chromium.org/2349073002/diff/240001/blimp/client/core/sett... blimp/client/core/settings/settings.h:52: return local_state_->GetBoolean(prefs::kBlimpEnabled); On 2016/10/26 09:47:23, Bernhard Bauer wrote: > And now move the implementation out of line :) Done. https://codereview.chromium.org/2349073002/diff/240001/chrome/browser/prefs/c... File chrome/browser/prefs/chrome_command_line_pref_store.cc (right): https://codereview.chromium.org/2349073002/diff/240001/chrome/browser/prefs/c... chrome/browser/prefs/chrome_command_line_pref_store.cc:21: #include "chrome/browser/android/preferences/command_line_pref_store_android.h" On 2016/10/26 01:52:05, Lei Zhang wrote: > Put this behind #if defined(OS_ANDROID) down around like 34. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mlliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm https://codereview.chromium.org/2349073002/diff/260001/blimp/client/app/linux... File blimp/client/app/linux/blimp_main.cc (right): https://codereview.chromium.org/2349073002/diff/260001/blimp/client/app/linux... blimp/client/app/linux/blimp_main.cc:40: ~BlimpShellCommandLinePrefStore() override {} = default?
The CQ bit was checked by mlliu@chromium.org to run a CQ dry run
https://codereview.chromium.org/2349073002/diff/260001/blimp/client/app/linux... File blimp/client/app/linux/blimp_main.cc (right): https://codereview.chromium.org/2349073002/diff/260001/blimp/client/app/linux... blimp/client/app/linux/blimp_main.cc:40: ~BlimpShellCommandLinePrefStore() override {} On 2016/10/28 04:25:44, David Trainor (OOO to oct 31) wrote: > = default? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mlliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, bauerb@chromium.org, dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2349073002/#ps280001 (title: "nits and sync to head")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Blimp Settings framework on the c++ side This CL includes: 1. Settings class: where the settings are, through which the settings can be set and retrieved. It is owned by BlimpClientContextImpl. Hook it up with BlimpClientContext, so that a PrefService* is passed to store the settings that needs to be saved persistently. 2. SettingsObserver interface, whose implementations will trigger different actions on the change of the settings. 3. Added blimp_enabled_, record_whole_document_ and show_network_stats_ to Settings class. 4. SettingsFeature takes a Settings* in the constructor. Add a PushSettings() to the necessary settings to the engine. BUG=647848 ========== to ========== Blimp Settings framework on the c++ side This CL includes: 1. Settings class: where the settings are, through which the settings can be set and retrieved. It is owned by BlimpClientContextImpl. Hook it up with BlimpClientContext, so that a PrefService* is passed to store the settings that needs to be saved persistently. 2. SettingsObserver interface, whose implementations will trigger different actions on the change of the settings. 3. Added blimp_enabled_, record_whole_document_ and show_network_stats_ to Settings class. 4. SettingsFeature takes a Settings* in the constructor. Add a PushSettings() to the necessary settings to the engine. BUG=647848 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Blimp Settings framework on the c++ side This CL includes: 1. Settings class: where the settings are, through which the settings can be set and retrieved. It is owned by BlimpClientContextImpl. Hook it up with BlimpClientContext, so that a PrefService* is passed to store the settings that needs to be saved persistently. 2. SettingsObserver interface, whose implementations will trigger different actions on the change of the settings. 3. Added blimp_enabled_, record_whole_document_ and show_network_stats_ to Settings class. 4. SettingsFeature takes a Settings* in the constructor. Add a PushSettings() to the necessary settings to the engine. BUG=647848 ========== to ========== Blimp Settings framework on the c++ side This CL includes: 1. Settings class: where the settings are, through which the settings can be set and retrieved. It is owned by BlimpClientContextImpl. Hook it up with BlimpClientContext, so that a PrefService* is passed to store the settings that needs to be saved persistently. 2. SettingsObserver interface, whose implementations will trigger different actions on the change of the settings. 3. Added blimp_enabled_, record_whole_document_ and show_network_stats_ to Settings class. 4. SettingsFeature takes a Settings* in the constructor. Add a PushSettings() to the necessary settings to the engine. BUG=647848 Committed: https://crrev.com/ee3a405c87a05fdf014fae275d5b25fb95e4cda1 Cr-Commit-Position: refs/heads/master@{#428524} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/ee3a405c87a05fdf014fae275d5b25fb95e4cda1 Cr-Commit-Position: refs/heads/master@{#428524} |
