|
|
Created:
3 years, 10 months ago by Ken Rockot(use gerrit already) Modified:
3 years, 10 months ago CC:
chromium-reviews, davemoore+watch_chromium.org, oshima+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRe-enable preferences service access for browser+ash
Fixes the preferences manifest to provide the correct interface.
Adds a browser test to ensure that the browser can connect
to the preferences service and change / observer the default
profile PrefStore.
Also fixes a bug where PrefStoreClient was using path expansion
when setting dictionary keys, even though it's backed by a flat
key-value store.
BUG=None
TEST=browser_tests/PreferencesServiceBrowserTest.Basic
Review-Url: https://codereview.chromium.org/2698913005
Cr-Commit-Position: refs/heads/master@{#451452}
Committed: https://chromium.googlesource.com/chromium/src/+/905e389f8130659ead4b44dad07f976605622025
Patch Set 1 #Patch Set 2 : . #
Total comments: 9
Patch Set 3 : . #
Messages
Total messages: 46 (27 generated)
The CQ bit was checked by rockot@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...
Description was changed from ========== Re-enable preferences service access for browser+ash Fixes the preferences manifest to provide the correct interface. Adds a browser test to ensure that both Chrome and ash can connect to the preferences service and change / observer the default profile PrefStore. BUG=None TEST=browser_tests/PreferencesServiceBrowserTest.Basic ========== to ========== Re-enable preferences service access for browser+ash Fixes the preferences manifest to provide the correct interface. Adds a browser test to ensure that both Chrome and ash can connect to the preferences service and change / observer the default profile PrefStore. Also fixes a bug where PrefStoreClient was using path expansion when setting dictionary keys, even though it's backed by a flat key-value store. BUG=None TEST=browser_tests/PreferencesServiceBrowserTest.Basic ==========
rockot@chromium.org changed reviewers: + jonross@chromium.org
Jon PTAL overall, thanks!
Description was changed from ========== Re-enable preferences service access for browser+ash Fixes the preferences manifest to provide the correct interface. Adds a browser test to ensure that both Chrome and ash can connect to the preferences service and change / observer the default profile PrefStore. Also fixes a bug where PrefStoreClient was using path expansion when setting dictionary keys, even though it's backed by a flat key-value store. BUG=None TEST=browser_tests/PreferencesServiceBrowserTest.Basic ========== to ========== Re-enable preferences service access for browser+ash Fixes the preferences manifest to provide the correct interface. Adds a browser test to ensure that the browser can connect to the preferences service and change / observer the default profile PrefStore. Also fixes a bug where PrefStoreClient was using path expansion when setting dictionary keys, even though it's backed by a flat key-value store. BUG=None TEST=browser_tests/PreferencesServiceBrowserTest.Basic ==========
The CQ bit was checked by rockot@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...
Looks mostly great, thanks for spotting this! A few testing related questions. https://codereview.chromium.org/2698913005/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/preferences_chromeos_browsertest.cc (right): https://codereview.chromium.org/2698913005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/preferences_chromeos_browsertest.cc:194: if (key == pref_name_) Have this be an EXPECT_EQ? That way we fail explicitly vs never exiting the run loop https://codereview.chromium.org/2698913005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/preferences_chromeos_browsertest.cc:297: PrefService* pref_service = browser()->profile()->GetPrefs(); This exposes the test to other preferences being updated by the browser. Would it make sense to use a TestingProfileManager here to control a TestingProfile directly? see: preferences_service_unittest.cc https://codereview.chromium.org/2698913005/diff/20001/chrome/browser/prefs/pr... File chrome/browser/prefs/preferences_manifest.json (right): https://codereview.chromium.org/2698913005/diff/20001/chrome/browser/prefs/pr... chrome/browser/prefs/preferences_manifest.json:7: "preferences_manager": [ "prefs::mojom::PreferencesServiceFactory" ] Thanks for this!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2698913005/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/preferences_chromeos_browsertest.cc (right): https://codereview.chromium.org/2698913005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/preferences_chromeos_browsertest.cc:194: if (key == pref_name_) On 2017/02/16 at 21:53:35, jonross wrote: > Have this be an EXPECT_EQ? That way we fail explicitly vs never exiting the run loop I think the test should tolerate other prefs being set. See other inline comments. https://codereview.chromium.org/2698913005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/preferences_chromeos_browsertest.cc:297: PrefService* pref_service = browser()->profile()->GetPrefs(); On 2017/02/16 at 21:53:35, jonross wrote: > This exposes the test to other preferences being updated by the browser. Would it make sense to use a TestingProfileManager here to control a TestingProfile directly? > > see: preferences_service_unittest.cc I started going down this route just to see what it would look like. My conclusion is that I don't think it makes sense to use a TestingProfileManager in a browser test (in fact there is virtually zero precedent for this). The purpose of this test is to act as a browser integration test, so the more mocking that we do, the less useful the test is. I guess it would still test that PreferencesConnectionManager is reachable via a service connector, but it would have to create a whole lot of scaffolding just to prove that point and it would provide strictly less meaningful test coverage. Generally those sorts of tests seem to be fragile and expensive to maintain. At least, whenever I find myself having to maintain such mock-heavy tests, it makes me very sad. I think the only real risk here is that someone introduces a change which causes the standard Chrome OS browser test environment to modify the kMouseSensitivity preference arbitrarily while this test is running. That seems rather unlikely. We *could* introduce some new test-only prefs to be registed in normal Profiles within the test environment if we want to be extra safe, but I stopped shy of that here to keep this change as small as possible.
LGTM https://codereview.chromium.org/2698913005/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/preferences_chromeos_browsertest.cc (right): https://codereview.chromium.org/2698913005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/preferences_chromeos_browsertest.cc:194: if (key == pref_name_) On 2017/02/17 00:00:31, Ken Rockot wrote: > On 2017/02/16 at 21:53:35, jonross wrote: > > Have this be an EXPECT_EQ? That way we fail explicitly vs never exiting the > run loop > > I think the test should tolerate other prefs being set. See other inline > comments. SGTM https://codereview.chromium.org/2698913005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/preferences_chromeos_browsertest.cc:297: PrefService* pref_service = browser()->profile()->GetPrefs(); On 2017/02/17 00:00:31, Ken Rockot wrote: > On 2017/02/16 at 21:53:35, jonross wrote: > > This exposes the test to other preferences being updated by the browser. Would > it make sense to use a TestingProfileManager here to control a TestingProfile > directly? > > > > see: preferences_service_unittest.cc > > I started going down this route just to see what it would look like. > > My conclusion is that I don't think it makes sense to use a > TestingProfileManager in a browser test (in fact there is virtually zero > precedent for this). > > The purpose of this test is to act as a browser integration test, so the more > mocking that we do, the less useful the test is. I guess it would still test > that PreferencesConnectionManager is reachable via a service connector, but it > would have to create a whole lot of scaffolding just to prove that point and it > would provide strictly less meaningful test coverage. > > Generally those sorts of tests seem to be fragile and expensive to maintain. At > least, whenever I find myself having to maintain such mock-heavy tests, it makes > me very sad. > > I think the only real risk here is that someone introduces a change which causes > the standard Chrome OS browser test environment to modify the kMouseSensitivity > preference arbitrarily while this test is running. That seems rather unlikely. > > We *could* introduce some new test-only prefs to be registed in normal Profiles > within the test environment if we want to be extra safe, but I stopped shy of > that here to keep this change as small as possible. Thanks for the clarification. I'm fine with treating this as a full integration test. If something does introduce changes to the chosen pref for the test, that will be caught at submission time. So pretty early on, and we can adjust as necessary. This SGTM.
Thanks! On Feb 16, 2017 6:15 PM, <jonross@chromium.org> wrote: > LGTM > > > https://codereview.chromium.org/2698913005/diff/20001/ > chrome/browser/chromeos/preferences_chromeos_browsertest.cc > File chrome/browser/chromeos/preferences_chromeos_browsertest.cc > (right): > > https://codereview.chromium.org/2698913005/diff/20001/ > chrome/browser/chromeos/preferences_chromeos_browsertest.cc#newcode194 > chrome/browser/chromeos/preferences_chromeos_browsertest.cc:194: if (key > == pref_name_) > On 2017/02/17 00:00:31, Ken Rockot wrote: > > On 2017/02/16 at 21:53:35, jonross wrote: > > > Have this be an EXPECT_EQ? That way we fail explicitly vs never > exiting the > > run loop > > > > I think the test should tolerate other prefs being set. See other > inline > > comments. > > SGTM > > https://codereview.chromium.org/2698913005/diff/20001/ > chrome/browser/chromeos/preferences_chromeos_browsertest.cc#newcode297 > chrome/browser/chromeos/preferences_chromeos_browsertest.cc:297: > PrefService* pref_service = browser()->profile()->GetPrefs(); > On 2017/02/17 00:00:31, Ken Rockot wrote: > > On 2017/02/16 at 21:53:35, jonross wrote: > > > This exposes the test to other preferences being updated by the > browser. Would > > it make sense to use a TestingProfileManager here to control a > TestingProfile > > directly? > > > > > > see: preferences_service_unittest.cc > > > > I started going down this route just to see what it would look like. > > > > My conclusion is that I don't think it makes sense to use a > > TestingProfileManager in a browser test (in fact there is virtually > zero > > precedent for this). > > > > The purpose of this test is to act as a browser integration test, so > the more > > mocking that we do, the less useful the test is. I guess it would > still test > > that PreferencesConnectionManager is reachable via a service > connector, but it > > would have to create a whole lot of scaffolding just to prove that > point and it > > would provide strictly less meaningful test coverage. > > > > Generally those sorts of tests seem to be fragile and expensive to > maintain. At > > least, whenever I find myself having to maintain such mock-heavy > tests, it makes > > me very sad. > > > > I think the only real risk here is that someone introduces a change > which causes > > the standard Chrome OS browser test environment to modify the > kMouseSensitivity > > preference arbitrarily while this test is running. That seems rather > unlikely. > > > > We *could* introduce some new test-only prefs to be registed in normal > Profiles > > within the test environment if we want to be extra safe, but I stopped > shy of > > that here to keep this change as small as possible. > > Thanks for the clarification. I'm fine with treating this as a full > integration test. > > If something does introduce changes to the chosen pref for the test, > that will be caught at submission time. So pretty early on, and we can > adjust as necessary. > > This SGTM. > > https://codereview.chromium.org/2698913005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by rockot@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2695403002 Patch 80001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by rockot@chromium.org
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 rockot@chromium.org
rockot@chromium.org changed reviewers: + sky@chromium.org
One more for you sky@ :) Please take a look as toplevel owner
https://codereview.chromium.org/2698913005/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/preferences_chromeos_browsertest.cc (right): https://codereview.chromium.org/2698913005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/preferences_chromeos_browsertest.cc:178: EXPECT_TRUE(store->GetValue(key, &value)); If this returns false, won't the next line crash? Maybe make this function return a bool and take an int* so that callers can do: ASSERT(GetIntegerPrefValue(store, key, &value)); ASSERT_EQ(..., value) ?
https://codereview.chromium.org/2698913005/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/preferences_chromeos_browsertest.cc (right): https://codereview.chromium.org/2698913005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/preferences_chromeos_browsertest.cc:178: EXPECT_TRUE(store->GetValue(key, &value)); On 2017/02/17 at 17:44:22, sky wrote: > If this returns false, won't the next line crash? Maybe make this function return a bool and take an int* so that callers can do: > > ASSERT(GetIntegerPrefValue(store, key, &value)); > ASSERT_EQ(..., value) > ? Done
The CQ bit was checked by rockot@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...
LGTM
The CQ bit was unchecked by rockot@chromium.org
The CQ bit was checked by rockot@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jonross@chromium.org Link to the patchset: https://codereview.chromium.org/2698913005/#ps40001 (title: ".")
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: linux_chromium_chromeos_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 rockot@chromium.org
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by rockot@chromium.org
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by rockot@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487411988963610, "parent_rev": "91d8afa35df85de452b08ca5c7032fb1456a541a", "commit_rev": "905e389f8130659ead4b44dad07f976605622025"}
Message was sent while issue was closed.
Description was changed from ========== Re-enable preferences service access for browser+ash Fixes the preferences manifest to provide the correct interface. Adds a browser test to ensure that the browser can connect to the preferences service and change / observer the default profile PrefStore. Also fixes a bug where PrefStoreClient was using path expansion when setting dictionary keys, even though it's backed by a flat key-value store. BUG=None TEST=browser_tests/PreferencesServiceBrowserTest.Basic ========== to ========== Re-enable preferences service access for browser+ash Fixes the preferences manifest to provide the correct interface. Adds a browser test to ensure that the browser can connect to the preferences service and change / observer the default profile PrefStore. Also fixes a bug where PrefStoreClient was using path expansion when setting dictionary keys, even though it's backed by a flat key-value store. BUG=None TEST=browser_tests/PreferencesServiceBrowserTest.Basic Review-Url: https://codereview.chromium.org/2698913005 Cr-Commit-Position: refs/heads/master@{#451452} Committed: https://chromium.googlesource.com/chromium/src/+/905e389f8130659ead4b44dad07f... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/905e389f8130659ead4b44dad07f... |