Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14268009:
Support VariationsRestrictParameter in VariationsService for Chrome OS (Closed)
|
Created:
7 years, 8 months ago by Mathieu Modified:
7 years, 8 months ago CC:
chromium-reviews, MAD, Ilya Sherman, jar (doing other things) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionWhen determining the Variations server URL, get VariationsRestrictParameter from Chrome OS Settings in the case of Chrome OS.
BUG=232881
TEST=VariationsServiceTest unit test, VariationsServiceDevicePolicyTest browser test
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195533
Patch Set 1 #
Total comments: 13
Patch Set 2 : Browsertest + comments #
Total comments: 13
Patch Set 3 : Comments #
Total comments: 8
Patch Set 4 : Browser tests refactored #
Total comments: 6
Patch Set 5 : More loosely coupled #Patch Set 6 : Missing files #Patch Set 7 : Added TODO about MockImageBurnerClient #
Total comments: 24
Patch Set 8 : Nits #Patch Set 9 : fixed a test #
Messages
Total messages: 24 (0 generated)
Hi all, This is the feature code for the Chrome OS setting I've added in https://chromiumcodereview.appspot.com/14329002/. While I would like Alexei to review, I have some questions for Mattias. Mattias: I've noticed that DeviceSettingsProvider retrieves cached data on construction. Does that mean my code doesn't need to do specific actions to benefit from the cache? CrosSettings::GetString should be enough? Also, see my test. This is the way I've found to set a value in settings on the fly. I've taken it from another test. Is there a better way?
https://codereview.chromium.org/14268009/diff/1/chrome/browser/metrics/variat... File chrome/browser/metrics/variations/variations_service.cc (left): https://codereview.chromium.org/14268009/diff/1/chrome/browser/metrics/variat... chrome/browser/metrics/variations/variations_service.cc:214: if (local_state) { I think you previously had this if statement because of tests under which local_state is NULL. Is this no longer an issue? https://codereview.chromium.org/14268009/diff/1/chrome/browser/metrics/variat... File chrome/browser/metrics/variations/variations_service_unittest.cc (right): https://codereview.chromium.org/14268009/diff/1/chrome/browser/metrics/variat... chrome/browser/metrics/variations/variations_service_unittest.cc:387: chromeos::StubCrosSettingsProvider stub_settings_provider; I think it would be cleaner to do this in a separate test case. You can make a custom testing::Test subclass for that test that handles the lifetime of the StubCrosSettingsProvider. https://codereview.chromium.org/14268009/diff/1/chrome/browser/metrics/variat... chrome/browser/metrics/variations/variations_service_unittest.cc:392: cros_settings->GetProvider(chromeos::kReportDeviceVersionInfo); Indent 2 more. https://codereview.chromium.org/14268009/diff/1/chrome/browser/metrics/variat... chrome/browser/metrics/variations/variations_service_unittest.cc:409: cros_settings->RemoveSettingsProvider(&stub_settings_provider)); Indent 2 more.
LGTM with nits https://codereview.chromium.org/14268009/diff/1/chrome/browser/metrics/variat... File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/14268009/diff/1/chrome/browser/metrics/variat... chrome/browser/metrics/variations/variations_service.cc:125: #if defined(OS_CHROMEOS) You might want to add a comment here saying that you're reading the cached (i.e. untrusted value) because this starts early, and you can't reconfigure your service to use a different parameter at run time (which also justifies that you don't have an observer for pref changes). https://codereview.chromium.org/14268009/diff/1/chrome/browser/metrics/variat... chrome/browser/metrics/variations/variations_service.cc:232: if (!restrict_param.empty()) nit: multi-line statements should use braces https://codereview.chromium.org/14268009/diff/1/chrome/browser/metrics/variat... File chrome/browser/metrics/variations/variations_service_unittest.cc (right): https://codereview.chromium.org/14268009/diff/1/chrome/browser/metrics/variat... chrome/browser/metrics/variations/variations_service_unittest.cc:388: chromeos::CrosSettings* cros_settings = chromeos::CrosSettings::Get(); Sigh, CrosSettings mocking still sucks. Sorry about that.
On 2013/04/17 20:49:07, Mathieu Perreault wrote: > Hi all, > > This is the feature code for the Chrome OS setting I've added in > https://chromiumcodereview.appspot.com/14329002/. > > While I would like Alexei to review, I have some questions for Mattias. > > Mattias: > I've noticed that DeviceSettingsProvider retrieves cached data on construction. > Does that mean my code doesn't need to do specific actions to benefit from the > cache? CrosSettings::GetString should be enough? Correct. > > Also, see my test. This is the way I've found to set a value in settings on the > fly. I've taken it from another test. Is there a better way? Unfortunately, not at this time. We're aware this is rather awkward and will fix it eventually.
Thanks! mnissler: Please have a look at the new browsertest :) https://codereview.chromium.org/14268009/diff/1/chrome/browser/metrics/variat... File chrome/browser/metrics/variations/variations_service.cc (left): https://codereview.chromium.org/14268009/diff/1/chrome/browser/metrics/variat... chrome/browser/metrics/variations/variations_service.cc:214: if (local_state) { On 2013/04/17 21:24:36, Alexei Svitkine wrote: > I think you previously had this if statement because of tests under which > local_state is NULL. Is this no longer an issue? Still an issue, see GetVariationsServerURL(NULL) above. https://codereview.chromium.org/14268009/diff/1/chrome/browser/metrics/variat... File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/14268009/diff/1/chrome/browser/metrics/variat... chrome/browser/metrics/variations/variations_service.cc:232: if (!restrict_param.empty()) On 2013/04/18 10:39:39, Mattias Nissler wrote: > nit: multi-line statements should use braces Done. https://codereview.chromium.org/14268009/diff/1/chrome/browser/metrics/variat... File chrome/browser/metrics/variations/variations_service_unittest.cc (right): https://codereview.chromium.org/14268009/diff/1/chrome/browser/metrics/variat... chrome/browser/metrics/variations/variations_service_unittest.cc:387: chromeos::StubCrosSettingsProvider stub_settings_provider; On 2013/04/17 21:24:36, Alexei Svitkine wrote: > I think it would be cleaner to do this in a separate test case. > > You can make a custom testing::Test subclass for that test that handles the > lifetime of the StubCrosSettingsProvider. Done. https://codereview.chromium.org/14268009/diff/1/chrome/browser/metrics/variat... chrome/browser/metrics/variations/variations_service_unittest.cc:388: chromeos::CrosSettings* cros_settings = chromeos::CrosSettings::Get(); On 2013/04/18 10:39:39, Mattias Nissler wrote: > Sigh, CrosSettings mocking still sucks. Sorry about that. Hey at least it works ;) https://codereview.chromium.org/14268009/diff/1/chrome/browser/metrics/variat... chrome/browser/metrics/variations/variations_service_unittest.cc:392: cros_settings->GetProvider(chromeos::kReportDeviceVersionInfo); On 2013/04/17 21:24:36, Alexei Svitkine wrote: > Indent 2 more. Done. https://codereview.chromium.org/14268009/diff/1/chrome/browser/metrics/variat... chrome/browser/metrics/variations/variations_service_unittest.cc:409: cros_settings->RemoveSettingsProvider(&stub_settings_provider)); On 2013/04/17 21:24:36, Alexei Svitkine wrote: > Indent 2 more. Done.
https://codereview.chromium.org/14268009/diff/7001/chrome/browser/chromeos/po... File chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc (right): https://codereview.chromium.org/14268009/diff/7001/chrome/browser/chromeos/po... chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc:29: namespace em = enterprise_management; You're only using em:: once in the file, this doesn't warrant this line. https://codereview.chromium.org/14268009/diff/7001/chrome/browser/chromeos/po... chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc:77: } This seems like a lot of boilerplate for a pretty small test case. I'm assuming you copy-pasted most of this out of an existing test file somewhere? Is it possible to make a common test super-class file where this boiletplate will live, thus allowing each test to simply inherit from it without duplicating the code? https://codereview.chromium.org/14268009/diff/7001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service_unittest.cc (right): https://codereview.chromium.org/14268009/diff/7001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:112: // Not used. Initializes CrosSettings for testing. Nit: "Not used" -> "Not used directly" https://codereview.chromium.org/14268009/diff/7001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:429: chromeos::CrosSettings* cros_settings_; This should be private. https://codereview.chromium.org/14268009/diff/7001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:433: chromeos::ScopedTestCrosSettings scoped_test_cros_settings; You can remove this if you inherit from VariationsServiceTest.
Thanks! https://codereview.chromium.org/14268009/diff/7001/chrome/browser/chromeos/po... File chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc (right): https://codereview.chromium.org/14268009/diff/7001/chrome/browser/chromeos/po... chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc:29: namespace em = enterprise_management; On 2013/04/18 19:27:53, Alexei Svitkine wrote: > You're only using em:: once in the file, this doesn't warrant this line. Done. https://codereview.chromium.org/14268009/diff/7001/chrome/browser/chromeos/po... chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc:77: } On 2013/04/18 19:27:53, Alexei Svitkine wrote: > This seems like a lot of boilerplate for a pretty small test case. > > I'm assuming you copy-pasted most of this out of an existing test file > somewhere? > > Is it possible to make a common test super-class file where this boiletplate > will live, thus allowing each test to simply inherit from it without duplicating > the code? It comes from: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... In particular ExistingUserControllerPublicSessionTest, which subclasses a bigger class in the same file. The SetUpInProcessBrowserTestFixture methods ended up having a different number of expectations and this file mocks way fewer interfaces. Also, the two tests are in different namespaces (chromeos and policy). I agree it's some amount of boilerplate, perhaps mnissler@ can suggest a better way? https://codereview.chromium.org/14268009/diff/7001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service_unittest.cc (right): https://codereview.chromium.org/14268009/diff/7001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:112: // Not used. Initializes CrosSettings for testing. On 2013/04/18 19:27:53, Alexei Svitkine wrote: > Nit: "Not used" -> "Not used directly" Done. https://codereview.chromium.org/14268009/diff/7001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:112: // Not used. Initializes CrosSettings for testing. On 2013/04/18 19:27:53, Alexei Svitkine wrote: > Nit: "Not used" -> "Not used directly" Done. https://codereview.chromium.org/14268009/diff/7001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:429: chromeos::CrosSettings* cros_settings_; On 2013/04/18 19:27:53, Alexei Svitkine wrote: > This should be private. Compiler complains when it's used in the test if private. https://codereview.chromium.org/14268009/diff/7001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:433: chromeos::ScopedTestCrosSettings scoped_test_cros_settings; On 2013/04/18 19:27:53, Alexei Svitkine wrote: > You can remove this if you inherit from VariationsServiceTest. Done.
https://codereview.chromium.org/14268009/diff/7001/chrome/browser/chromeos/po... File chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc (right): https://codereview.chromium.org/14268009/diff/7001/chrome/browser/chromeos/po... chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc:77: } On 2013/04/18 19:51:14, Mathieu Perreault wrote: > On 2013/04/18 19:27:53, Alexei Svitkine wrote: > > This seems like a lot of boilerplate for a pretty small test case. > > > > I'm assuming you copy-pasted most of this out of an existing test file > > somewhere? > > > > Is it possible to make a common test super-class file where this boiletplate > > will live, thus allowing each test to simply inherit from it without > duplicating > > the code? > > It comes from: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > > In particular ExistingUserControllerPublicSessionTest, which subclasses a bigger > class in the same file. > > The SetUpInProcessBrowserTestFixture methods ended up having a different number > of expectations and this file mocks way fewer interfaces. > > Also, the two tests are in different namespaces (chromeos and policy). > > I agree it's some amount of boilerplate, perhaps mnissler@ can suggest a better > way? I suggest making a common superclass, derived from CrosInProcessBrowserTest, that will have utility methods for the parts that are common to both tests, for example RefreshDevicePolicy() and InstallOwnerKey() and changing this class and ExistingUserControllerTest to derive from it and call those methods. https://codereview.chromium.org/14268009/diff/7001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service_unittest.cc (right): https://codereview.chromium.org/14268009/diff/7001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:429: chromeos::CrosSettings* cros_settings_; On 2013/04/18 19:51:14, Mathieu Perreault wrote: > On 2013/04/18 19:27:53, Alexei Svitkine wrote: > > This should be private. > > Compiler complains when it's used in the test if private. Ah, I didn't see that you were using in that test. I suggest adding a method to this class to set the parameter and have the test call that. Then this can be private.
I agree with Alexei that it makes sense to create a base class that provides helpers for device policy setup. It should probably live in chrome/browser/chromeos/policy. Once we have it, we definitely want to refactor the existing device policy tests to use it. That's just FYI, I won't ask you to do all this for for us :) Feel free to decide how much effort you are willing to put in and we'll do the rest. https://codereview.chromium.org/14268009/diff/13001/chrome/browser/chromeos/p... File chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc (right): https://codereview.chromium.org/14268009/diff/13001/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc:45: .Times(AnyNumber()); I think these are just for silencing gmock? I don't think we should have to put these into each and every test fixture using MockDBusThreadManager. You might want to check with satorux@ on how this is supposed to be handled. https://codereview.chromium.org/14268009/diff/13001/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc:85: // cloud policy for device owner settings. Not sure who wrote that comment, but it's a bit unfortunate. The explanation should rather be: The local instance of the private half of the owner key must be dropped as otherwise the NSS library will tell Chrome that the key is available - which is incorrect and leads to Chrome behaving as if a local owner were logged in.
https://codereview.chromium.org/14268009/diff/13001/chrome/browser/chromeos/p... File chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc (right): https://codereview.chromium.org/14268009/diff/13001/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc:45: .Times(AnyNumber()); On 2013/04/19 10:37:00, Mattias Nissler wrote: > I think these are just for silencing gmock? I don't think we should have to put > these into each and every test fixture using MockDBusThreadManager. You might > want to check with satorux@ on how this is supposed to be handled. We are in the process of removing gmock from chromeos/dbus: crbug.com/223061 A recent patch from keybuk@ https://codereview.chromium.org/13927010/ implemented fakes instead of mocks
https://codereview.chromium.org/14268009/diff/13001/chrome/browser/chromeos/p... File chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc (right): https://codereview.chromium.org/14268009/diff/13001/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc:45: .Times(AnyNumber()); On 2013/04/19 11:46:20, satorux1 wrote: > On 2013/04/19 10:37:00, Mattias Nissler wrote: > > I think these are just for silencing gmock? I don't think we should have to > put > > these into each and every test fixture using MockDBusThreadManager. You might > > want to check with satorux@ on how this is supposed to be handled. > > > We are in the process of removing gmock from chromeos/dbus: crbug.com/223061 > > A recent patch from keybuk@ https://codereview.chromium.org/13927010/ > implemented fakes instead of mocks Sorry. my previous comment didn't answer your questions. I'm curious why image burner is involved here. Looks completely unrelated. Maybe we should just remove the mock image burner class if it's not used meaningfully
Created a common class for the browsertest. Please have a look. https://codereview.chromium.org/14268009/diff/13001/chrome/browser/chromeos/p... File chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc (right): https://codereview.chromium.org/14268009/diff/13001/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc:45: .Times(AnyNumber()); On 2013/04/19 11:46:20, satorux1 wrote: > On 2013/04/19 10:37:00, Mattias Nissler wrote: > > I think these are just for silencing gmock? I don't think we should have to > put > > these into each and every test fixture using MockDBusThreadManager. You might > > want to check with satorux@ on how this is supposed to be handled. > > > We are in the process of removing gmock from chromeos/dbus: crbug.com/223061 > > A recent patch from keybuk@ https://codereview.chromium.org/13927010/ > implemented fakes instead of mocks Fakes won't work for this browsertest. Seems like it's meant for unit tests. https://codereview.chromium.org/14268009/diff/13001/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc:45: .Times(AnyNumber()); On 2013/04/19 12:04:13, satorux1 wrote: > On 2013/04/19 11:46:20, satorux1 wrote: > > On 2013/04/19 10:37:00, Mattias Nissler wrote: > > > I think these are just for silencing gmock? I don't think we should have to > > put > > > these into each and every test fixture using MockDBusThreadManager. You > might > > > want to check with satorux@ on how this is supposed to be handled. > > > > > > We are in the process of removing gmock from chromeos/dbus: crbug.com/223061 > > > > A recent patch from keybuk@ https://codereview.chromium.org/13927010/ > > implemented fakes instead of mocks > > Sorry. my previous comment didn't answer your questions. I'm curious why image > burner is involved here. Looks completely unrelated. Maybe we should just remove > the mock image burner class if it's not used meaningfully It doesn't seem used, let me know if you want me to remove it from the codebase. For now I will leave these expectations. https://codereview.chromium.org/14268009/diff/13001/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc:85: // cloud policy for device owner settings. On 2013/04/19 10:37:00, Mattias Nissler wrote: > Not sure who wrote that comment, but it's a bit unfortunate. The explanation > should rather be: > > The local instance of the private half of the owner key must be dropped as > otherwise the NSS library will tell Chrome that the key is available - which is > incorrect and leads to Chrome behaving as if a local owner were logged in. Done.
Thanks for the refactoring! In addition to what we chatted about offline, here's a few other comments. https://codereview.chromium.org/14268009/diff/24001/chrome/browser/chromeos/p... File chrome/browser/chromeos/policy/device_policy_cros_browsertest.h (right): https://codereview.chromium.org/14268009/diff/24001/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browsertest.h:5: #ifndef CHROME_BROWSER_CHROMEOS_POLICY_DEVICE_POLICY_CROS_BROWSERTEST_H_ Rename this to device_policy_cros_browser_test.h/cc, similar to cros_in_process_browser_test.h. (underscore between 'browser' and 'test') (Since this isn't actually a browsertest test file, but a base class for other tests.) https://codereview.chromium.org/14268009/diff/24001/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browsertest.h:26: virtual ~DevicePolicyCrosBrowserTest() {} Move the implementation to the .cc file. https://codereview.chromium.org/14268009/diff/24001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/14268009/diff/24001/chrome/chrome_tests.gypi#... chrome/chrome_tests.gypi:1220: 'browser/chromeos/policy/device_policy_cros_browsertest.cc', These should be in alphabetical order.
Thanks! https://codereview.chromium.org/14268009/diff/24001/chrome/browser/chromeos/p... File chrome/browser/chromeos/policy/device_policy_cros_browsertest.h (right): https://codereview.chromium.org/14268009/diff/24001/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browsertest.h:5: #ifndef CHROME_BROWSER_CHROMEOS_POLICY_DEVICE_POLICY_CROS_BROWSERTEST_H_ On 2013/04/19 18:11:55, Alexei Svitkine wrote: > Rename this to device_policy_cros_browser_test.h/cc, similar to > cros_in_process_browser_test.h. (underscore between 'browser' and 'test') > > (Since this isn't actually a browsertest test file, but a base class for other > tests.) Done. https://codereview.chromium.org/14268009/diff/24001/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browsertest.h:26: virtual ~DevicePolicyCrosBrowserTest() {} On 2013/04/19 18:11:55, Alexei Svitkine wrote: > Move the implementation to the .cc file. Done. https://codereview.chromium.org/14268009/diff/24001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/14268009/diff/24001/chrome/chrome_tests.gypi#... chrome/chrome_tests.gypi:1220: 'browser/chromeos/policy/device_policy_cros_browsertest.cc', On 2013/04/19 18:11:55, Alexei Svitkine wrote: > These should be in alphabetical order. Done.
LGTM!
satorux: Please take a look at the login/ part for OWNERS approval.
chrome/browser/chromeos/login/ lgtm https://codereview.chromium.org/14268009/diff/13001/chrome/browser/chromeos/p... File chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc (right): https://codereview.chromium.org/14268009/diff/13001/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc:45: .Times(AnyNumber()); On 2013/04/19 17:58:25, Mathieu Perreault wrote: > On 2013/04/19 12:04:13, satorux1 wrote: > > On 2013/04/19 11:46:20, satorux1 wrote: > > > On 2013/04/19 10:37:00, Mattias Nissler wrote: > > > > I think these are just for silencing gmock? I don't think we should have > to > > > put > > > > these into each and every test fixture using MockDBusThreadManager. You > > might > > > > want to check with satorux@ on how this is supposed to be handled. > > > > > > > > > We are in the process of removing gmock from chromeos/dbus: crbug.com/223061 > > > > > > A recent patch from keybuk@ https://codereview.chromium.org/13927010/ > > > implemented fakes instead of mocks > > > > Sorry. my previous comment didn't answer your questions. I'm curious why image > > burner is involved here. Looks completely unrelated. Maybe we should just > remove > > the mock image burner class if it's not used meaningfully > > It doesn't seem used, let me know if you want me to remove it from the codebase. > For now I will leave these expectations. That's fine. haruki@ will be removing unused mocks.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/14268009/24002
Retried try job too often on mac_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Thanks for splitting out the test support code. LGTM after fixing the nits. https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... File chrome/browser/chromeos/policy/device_policy_cros_browser_test.cc (right): https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.cc:16: #include "chromeos/dbus/fake_session_manager_client.h" no need to repeat #includes already present in the header (people disagree about that this, chrome/browser/chromeos/policy generally doesn't include twice) https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.cc:29: DevicePolicyCrosBrowserTest::DevicePolicyCrosBrowserTest() : nit: break before colon, indent following line by 4. https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... File chrome/browser/chromeos/policy/device_policy_cros_browser_test.h (right): https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.h:11: #include "chrome/browser/chromeos/policy/proto/chrome_device_policy.pb.h" not needed? https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.h:12: #include "chrome/browser/metrics/variations/variations_service.h" not needed https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.h:13: #include "chrome/common/chrome_paths.h" not needed? https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.h:14: #include "chrome/test/base/testing_browser_process.h" not needed / forward-declare? https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.h:16: #include "chromeos/dbus/mock_dbus_thread_manager.h" forward-declare? https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.h:28: virtual void SetUpInProcessBrowserTestFixture() OVERRIDE; #include "base/compiler_specific.h" https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.h:30: void InstallOwnerKey(); Needs docs: What it does and when to call. https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.h:32: void RefreshDevicePolicy(); ditto https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.h:62: base::ScopedTempDir temp_dir_; #include "base/files/scoped_temp_dir.h" https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.h:64: DISALLOW_COPY_AND_ASSIGN(DevicePolicyCrosBrowserTest); #include "base/basictypes.h"
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/14268009/46002
Thanks! https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... File chrome/browser/chromeos/policy/device_policy_cros_browser_test.cc (right): https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.cc:16: #include "chromeos/dbus/fake_session_manager_client.h" On 2013/04/22 10:07:43, Mattias Nissler wrote: > no need to repeat #includes already present in the header (people disagree about > that this, chrome/browser/chromeos/policy generally doesn't include twice) Done. https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.cc:29: DevicePolicyCrosBrowserTest::DevicePolicyCrosBrowserTest() : On 2013/04/22 10:07:43, Mattias Nissler wrote: > nit: break before colon, indent following line by 4. Done. https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... File chrome/browser/chromeos/policy/device_policy_cros_browser_test.h (right): https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.h:11: #include "chrome/browser/chromeos/policy/proto/chrome_device_policy.pb.h" On 2013/04/22 10:07:43, Mattias Nissler wrote: > not needed? Done. https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.h:12: #include "chrome/browser/metrics/variations/variations_service.h" On 2013/04/22 10:07:43, Mattias Nissler wrote: > not needed Done. https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.h:13: #include "chrome/common/chrome_paths.h" On 2013/04/22 10:07:43, Mattias Nissler wrote: > not needed? Done. https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.h:14: #include "chrome/test/base/testing_browser_process.h" On 2013/04/22 10:07:43, Mattias Nissler wrote: > not needed / forward-declare? Done. https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.h:16: #include "chromeos/dbus/mock_dbus_thread_manager.h" On 2013/04/22 10:07:43, Mattias Nissler wrote: > forward-declare? Done. https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.h:28: virtual void SetUpInProcessBrowserTestFixture() OVERRIDE; On 2013/04/22 10:07:43, Mattias Nissler wrote: > #include "base/compiler_specific.h" Done. https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.h:30: void InstallOwnerKey(); On 2013/04/22 10:07:43, Mattias Nissler wrote: > Needs docs: What it does and when to call. Done. https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.h:32: void RefreshDevicePolicy(); On 2013/04/22 10:07:43, Mattias Nissler wrote: > ditto Done. https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.h:62: base::ScopedTempDir temp_dir_; On 2013/04/22 10:07:43, Mattias Nissler wrote: > #include "base/files/scoped_temp_dir.h" Done. https://codereview.chromium.org/14268009/diff/24002/chrome/browser/chromeos/p... chrome/browser/chromeos/policy/device_policy_cros_browser_test.h:64: DISALLOW_COPY_AND_ASSIGN(DevicePolicyCrosBrowserTest); On 2013/04/22 10:07:43, Mattias Nissler wrote: > #include "base/basictypes.h" Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/14268009/51004
Message was sent while issue was closed.
Change committed as 195533 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
