Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
964503002:
Implemented ForceMaximizeBrowserWindowOnFirstRun policy, added unit test and browser test. (Closed)
|
Created:
5 years, 9 months ago by peletskyi Modified:
5 years, 8 months ago CC:
chromium-reviews, tdanderson+views_chromium.org, sadrul, nkostylev+watch_chromium.org, tfarina, asvitkine+watch_chromium.org, oshima+watch_chromium.org, kalyank, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplemented ForceMaximizeBrowserWindowOnFirstRun policy, added unit test and browser test.
BUG=356285
Committed: https://crrev.com/da0bbb16cc00553245e806aa8c5adc4c38f7a9ed
Cr-Commit-Position: refs/heads/master@{#324244}
Patch Set 1 #Patch Set 2 : Removed wrong added files #Patch Set 3 : Cleaned chrome/chrome_tests.gypi #Patch Set 4 : Fixed description #
Total comments: 198
Patch Set 5 : Fix after review #
Total comments: 120
Patch Set 6 : Fixed remarks from Bartosz #
Total comments: 2
Patch Set 7 : Added JSONWriter and removed depricated function. #
Total comments: 27
Patch Set 8 : Now we use scoped_ptr #
Total comments: 7
Patch Set 9 : #
Total comments: 2
Patch Set 10 : Renamed ForceMaximizeOnFirstRun #Patch Set 11 : #Messages
Total messages: 33 (9 generated)
peletskyi@chromium.org changed reviewers: + bartfab@chromium.org
Hi Bartosz, can you take a look to my changes? Thanks, Oleksandr
Sorry, I really forgot to publish my comments last night. https://codereview.chromium.org/964503002/diff/60001/ash/shell/shell_delegate... File ash/shell/shell_delegate_impl.cc (right): https://codereview.chromium.org/964503002/diff/60001/ash/shell/shell_delegate... ash/shell/shell_delegate_impl.cc:185: return true; Nit: This dummy delegate should return the default setting, which is |false| in this case. https://codereview.chromium.org/964503002/diff/60001/ash/shell_delegate.h File ash/shell_delegate.h (right): https://codereview.chromium.org/964503002/diff/60001/ash/shell_delegate.h#new... ash/shell_delegate.h:86: // Returns true if window must be maximized at the first run. Nit: You should make it explicit that this applies to the very first window ever shown only. How about: "Returns true if the first window shown on first run should be unconditionally maximized, overriding the heuristic that normally chooses the window size. https://codereview.chromium.org/964503002/diff/60001/ash/shell_delegate.h#new... ash/shell_delegate.h:87: virtual bool IsFirstTimeMaximized() const = 0; Nit: How about ForceMaximizeOnFirstRun()? https://codereview.chromium.org/964503002/diff/60001/ash/test/test_shell_dele... File ash/test/test_shell_delegate.h (right): https://codereview.chromium.org/964503002/diff/60001/ash/test/test_shell_dele... ash/test/test_shell_delegate.h:72: void SetFirstTimeMaximized(bool maximized) { Nit: If you rename IsFirstTimeMaximized(), rename this method and the member accordingly. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner.cc File ash/wm/window_positioner.cc (right): https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner.cc:268: // Policy to maximize window on the first run works only for Chrome OS. Nit: No need for this comment. Ash is cross-plaform and will honor whatever the delegate tells it to do. The fact that |true| is only possible on Chrome OS is irrelevant to ash. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner.cc:269: const bool maximized_first_run = 1: Nit: How about s/maximized_first_run/maximize_first_window_on_first_run/ for consistency? https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner.cc:271: // When using "small screens" we want to always open in full screen mode, Nit: How about "We want to always open maximized on "small screens" or when policy tells us to?" https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner.cc:276: maximized_first_run) && Where is the code that ensures we only enforce this policy for the *first* window, i.e., we do not enforce it again when all windows are closed and a new one gets opened? https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner.cc:278: if (show_state_in == ui::SHOW_STATE_DEFAULT && set_maximized) { Nit: You should rearrange this so that |set_maximized| is not calculated when it is not needed: if (show_state_in == ui::SHOW_STATE_DEFAULT) { const bool bool set_maximized = ...; if (set_maximized) *show_state_out = ui::SHOW_STATE_MAXIMIZED; } https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... File ash/wm/window_positioner_unittest.cc (right): https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:150: TEST_F(WindowPositionerTest, FirstRunMaximizeWindow) { Nit: The test names do not make it clear what combination of conditions is tested by each. I think you should tweak the test names a bit and also add short comments that explain what is being tested. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:155: test::TestShellDelegate* d = Nit: Avoid abbreviations like |d|. Better use |delegate|. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:159: ash::WindowPositioner::GetBoundsAndShowStateForNewWindow( Nit: s/ash::// https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:160: Shell::GetScreen(), NULL, false, ui::SHOW_STATE_DEFAULT, &bounds_in_out, Nit: s/NULL/nullptr/ https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:167: // Set width of the screen wider than limit. Nit: What limit? https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:168: const int kWidth = ash::WindowPositioner::GetForceMaximizedWidthLimit() + 100; Nit 1: This is not really a constant if it is calculated based on a method's return value, hence s/kWidth/width/. Nit 2: s/ash::// https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:169: const std::string kResolution = std::to_string(kWidth) + "x300"; //"1466x300" 1: Nit: s/kResolution/resolution/ for the same reason as above. 2: Nit: #include <string> 3: std::to_string() is not allowed. You cannot use C++11 library functions. 4: Nit: s|//|// | https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:171: gfx::Rect bounds_in_out(0, 0, 320, 240); // random bounds Nit: The bounds are "random" in all the tests. Why do you need to document it here, but not in the other tests? I would just remove the comment. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:174: test::TestShellDelegate* d = As above: s/d/delegate/ https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:178: ash::WindowPositioner::GetBoundsAndShowStateForNewWindow( Nit: s/ash::// https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:179: Shell::GetScreen(), NULL, false, ui::SHOW_STATE_DEFAULT, &bounds_in_out, Nit: s/NULL/nullptr/ https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:186: // Set width of the screen narrower than limit. Nit: What limit? https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:187: const int kWidth = ash::WindowPositioner::GetForceMaximizedWidthLimit() - 100; Nit: As above: s/kWidth/width/ https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:188: const std::string kResolution = std::to_string(kWidth) + "x300"; //"1266x300" 1: Nit: s/kResolution/resolution/ 3: std::to_string() is not allowed. You cannot use C++11 library functions. 4: Nit: s|//|// | https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:190: gfx::Rect bounds_in_out(0, 0, 320, 240); // random bounds Nit: As above, I think you can drop this comment. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:193: test::TestShellDelegate* d = As above: s/d/delegate/ https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:197: ash::WindowPositioner::GetBoundsAndShowStateForNewWindow( Nit: s/ash::// https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:198: Shell::GetScreen(), NULL, false, ui::SHOW_STATE_DEFAULT, &bounds_in_out, Nit: s/NULL/nullptr/ https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:203: You do not seem to have any test that verifies a subsequent windows (i.e. not the first window on first login) are *not* affected by the policy. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc (right): https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:6: #include "ash/display/display_manager.h" Nit: Add blank line above. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:13: #include "ui/base/base_window.h" Nit: Not used. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:17: class ForceMaximizeBrowserWindowOnFirstRunTest : public LoginPolicyBaseTest { Nit: How about s/BrowserWindow// for consistency? You can also shorten the file name a bit to match :). https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:18: std::string GetPolicy() const override { Nit: Add public: or protected: https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:19: return std::string( Nit: Do you really need the explicit std::string() construction here? https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:20: "{" It seems odd for |LoginPolicyBaseTest| to rely on individual tests to duplicate this boilerplate JSON. IMHO, the boilerplate should live in |LoginPolicyBaseTest| with individual tests just filling in the mandatory and recommended sections. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:32: }; Nit: Add DISALLOW_COPY_AND_ASSIGN(ForceMaximizeBrowserWindowOnFirstRunTest). https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:34: IN_PROC_BROWSER_TEST_F(ForceMaximizeBrowserWindowOnFirstRunTest, StartSession) { Nit 1: #include "testing/gtest/include/gtest/gtest.h" Nit 2: If you add a couple more test cases, you probably want to rename this one to something like "MaximizeOnFirstRun." https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:35: // Set width of the screen wider than limit. By this condition without 1: Nit: What limit? 2: Add a test that verifies when the policy is set to |false|, the window is not maximized. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:38: const int kWidth = ash::WindowPositioner::GetForceMaximizedWidthLimit() + 100; Nit: This is not really a constant if it is calculated based on a method's return value, hence s/kWidth/width/. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:39: const std::string kResolution = std::to_string(kWidth) + "x300"; //"1466x300" 1: Nit: s/kResolution/resolution/ for the same reason as above. 2: std::to_string() is not allowed. You cannot use C++11 library functions. 3: Nit: s|//|// | https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:40: ash::DisplayManager* display_manager = Nit 1: const pointer. Nit 2: #include "ash/display/display_manager.h" https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:41: ash::Shell::GetInstance()->display_manager(); Nit: #include "ash/shell.h" https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:48: // Check that the browser window is maximized Nit 1: s/the/the first/ Nit 2: Add full stop at the end. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:49: BrowserList* browser_list = Nit: const pointer. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:50: BrowserList::GetInstance(chrome::HOST_DESKTOP_TYPE_ASH); Nit: #include "chrome/browser/ui/host_desktop.h" https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:52: Browser* browser = browser_list->get(0); Nit: const pointer. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:56: Can you add a test that verifies on *second* login, the policy is ignored? You can chain logins by having a test case X and a test case PRE_X. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:57: } // namespace policy Nit: Add blank line below. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/login_policy_base_test.cc (right): https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:5: #include "login_policy_base_test.h" Nit 1: Add the path to this file. Nit 2: Add a blank line below. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:30: namespace policy { Nit: Move this higher up so that the anonymous namespace is also part of |namespace policy|. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:33: const char LoginPolicyBaseTest::kAccountId[] = "dla1@example.com"; Nit: Why are you using "dla" here? Is this base meant for device-local account tests only? https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:46: test_server_.reset(new LocalPolicyTestServer(policy_file_path())); Nit: #include "chrome/browser/policy/test/local_policy_test_server.h" https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:53: command_line->AppendSwitchASCII(policy::switches::kDeviceManagementUrl, Nit: #include "components/policy/core/common/policy_switches.h" https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:54: test_server_->GetServiceURL().spec()); Nit: #include "url/gurl.h" https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:89: chromeos::WizardController::SkipPostLoginScreensForTesting(); Nit: #include "chrome/browser/chromeos/login/wizard_controller.h" https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:90: chromeos::WizardController* wizard_controller = Nit: const pointer. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:93: wizard_controller->SkipToLoginForTesting(chromeos::LoginScreenContext()); Nit: #include "chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h" https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:95: content::WindowedNotificationObserver( Nit: #include "content/public/test/test_utils.h" https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:96: chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE, Nit: #include "chrome/browser/chrome_notification_types.h" https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:102: GetLoginDisplay()->ShowSigninScreenForCreds(user_id, password); Nit: #include "chrome/browser/chromeos/login/ui/webui_login_display.h" https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:110: std::string pol = GetPolicy(); Nit: Avoid abbreviations like |pol|. You can actually just fold this into the next line. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:111: const std::string policy = base::StringPrintf( You clearly have very specific requirements for the format of GetPolicy()'s return value. Document these in the header file. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:115: base::WriteFile(policy_file_path(), policy.data(), policy.size()); Nit: #include "base/files/file_util.h" https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:120: return temp_dir_.path().AppendASCII("policy.json"); Nit: #include "base/files/file_path.h" https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/login_policy_base_test.h (right): https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:11: #include "chrome/browser/policy/test/local_policy_test_server.h" Nit: A forward-declaration is sufficient here. Move the include to the implementation file instead. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:15: class LoginPolicyBaseTest : public chromeos::OobeBaseTest { Nit 1: This is not a base test, it is a *test base*. Swap around the two terms in the class and file names. Nit 2: Briefly document what this base is to be used for. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:18: ~LoginPolicyBaseTest() override; Nit: No need to override the destructor. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:20: void SetUp() override; Nit: Add // chromeos::OobeBaseTest: https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:23: void SetupGaiaServerWithAccessTokens(); Nit 1: Add blank line above. Nit 2: s/Setup/SetUp/ Nit 3: Make this private. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:24: void SetMergeSessionParams(const std::string& email); Nit 1: #include <string> Nit 2: Make this private. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:27: void SetServerPolicy(); Nit: Make this private. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:30: base::FilePath policy_file_path() const; 1: If a method is not inlined, it should not be named in hacker_style. 2: Nit: Forward-declare |base::FilePath|. 3: Nit: This method is actually used in a single place only. Just inline its body there and remove the method altogether. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:31: scoped_ptr<LocalPolicyTestServer> test_server_; Nit: Make this private if it is not used by any derived classes. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:33: base::ScopedTempDir temp_dir_; Nit 1: Make this private if it is not used by any derived classes. Nit 2: #include "base/files/scoped_temp_dir.h" https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:35: static const char kAccountPassword[]; Nit: There is no need for the base class to define a dummy password as the base class never actually refers to it. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:36: static const char kAccountId[]; I think it would be better to expose this as a method rather than a static string. Otherwise, derived test classes suddenly refer to a magic constant named |kAccountId| and you have no idea where it came from. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc (left): https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:12: #include "base/macros.h" Nit: Keep this. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:21: #include "chrome/browser/ui/host_desktop.h" Nit: Still used. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:162: DISALLOW_COPY_AND_ASSIGN(UserCloudPolicyManagerTest); Nit: Keep this. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc (right): https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:6: #include "chrome/browser/chromeos/policy/login_policy_base_test.h" Nit: Add a blank line above. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:14: std::string GetPolicy() const override { Nit: Mark this as public: or protected:. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/prefs/bro... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/964503002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.cc:513: Nit: Remove the blank line. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.cc:514: registry->RegisterBooleanPref( Please move this to chromeos::Preferences::RegisterProfilePrefs(). There is no precedent for registering specific preferences here. It is always delegsted to helper methods. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/ui/ash/ch... File chrome/browser/ui/ash/chrome_shell_delegate.cc (right): https://codereview.chromium.org/964503002/diff/60001/chrome/browser/ui/ash/ch... chrome/browser/ui/ash/chrome_shell_delegate.cc:95: if (ProfileManager::GetActiveUserProfile()) { The documentation for this method says that you should "avoid using it at all cost" :). It is also states that for guest, you will get a "suitable" profile, whatever that may mean. I think a better implementation is: const user_manager::User* const user = user_manager::UserManager::Get()->GetActiveUser(); if (!user) return false; return chromeos::ProfileHelper::Get()->GetProfileByUser(user)-> GetPrefs()->GetBoolean(prefs::kForceMaximizeBrowserWindowOnFirstRun); https://codereview.chromium.org/964503002/diff/60001/chrome/browser/ui/ash/ch... chrome/browser/ui/ash/chrome_shell_delegate.cc:96: return ProfileManager::GetActiveUserProfile()->GetPrefs()->GetBoolean( Nit: #include "chrome/browser/profiles/profile.h" https://codereview.chromium.org/964503002/diff/60001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/964503002/diff/60001/chrome/common/pref_names... chrome/common/pref_names.cc:835: // This boolean controls if browser window is maximized on the first run. Nit: How about: "This boolean controls whether the first window shown on first run should be unconditionally maximized, overriding the heuristic that normally chooses the window size." https://codereview.chromium.org/964503002/diff/60001/chrome/common/pref_names... chrome/common/pref_names.cc:836: const char kForceMaximizeBrowserWindowOnFirstRun[] = "ui.first_run_maximized"; Nit: How about s/first_run_maximized/force_maximize_on_first_run/ for consistency? https://codereview.chromium.org/964503002/diff/60001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/964503002/diff/60001/chrome/common/pref_names... chrome/common/pref_names.h:275: extern const char kForceMaximizeBrowserWindowOnFirstRun[]; Nit: How about s/BrowserWindow//? It seems unnecessary. https://codereview.chromium.org/964503002/diff/60001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/964503002/diff/60001/components/policy/resour... components/policy/resources/policy_templates.json:7136: 'name': 'ForceMaximizeBrowserWindowOnFirstRun', Nit: How about s/BrowserWindow//? It seems unnecessary. https://codereview.chromium.org/964503002/diff/60001/components/policy/resour... components/policy/resources/policy_templates.json:7139: 'supported_on': ['chrome_os:41-'], Nit: s/41/43/ https://codereview.chromium.org/964503002/diff/60001/components/policy/resour... components/policy/resources/policy_templates.json:7146: 'caption': '''Maximize browser window on the first run''', Nit 1: s/browser/the first browser/ Nit 2: s/the // https://codereview.chromium.org/964503002/diff/60001/components/policy/resour... components/policy/resources/policy_templates.json:7147: 'desc': '''If this policy is set to true <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> will be launched in maximized mode on the first run or on each login in case of public sessions. Nit 1: s/true/true,/ Nit 2: s/be launched in maximized mode on the/unconditionally maximize the the first window shown on/ Nit 3: I think we can remove the " or on each login in case of public sessions bit." It is not just public sessions that behave this way. Ephemeral user sessions are also always considered to be doing a "first login." https://codereview.chromium.org/964503002/diff/60001/components/policy/resour... components/policy/resources/policy_templates.json:7148: If this policy is set to false or not configured <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> will be opened in default mode (maximized in case if screen width less than 1366 in case of first run or as in previous run).''', Nit 1: s/configured/configured,/ Nit 2: I think you can simplify this to: "a heuristic will decide whether to maximize the first window shown, based on the screen size." There is no need to refer to a "previous run" here as by definition, we are looking at first runs only. https://codereview.chromium.org/964503002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/964503002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:45942: + <int value="290" label="Force maximize browser window on first run"/> Please sync the label with policy_templates.json.
PTAL https://codereview.chromium.org/964503002/diff/60001/ash/shell/shell_delegate... File ash/shell/shell_delegate_impl.cc (right): https://codereview.chromium.org/964503002/diff/60001/ash/shell/shell_delegate... ash/shell/shell_delegate_impl.cc:185: return true; On 2015/03/12 11:54:43, bartfab wrote: > Nit: This dummy delegate should return the default setting, which is |false| in > this case. Done. https://codereview.chromium.org/964503002/diff/60001/ash/shell_delegate.h File ash/shell_delegate.h (right): https://codereview.chromium.org/964503002/diff/60001/ash/shell_delegate.h#new... ash/shell_delegate.h:86: // Returns true if window must be maximized at the first run. On 2015/03/12 11:54:43, bartfab wrote: > Nit: You should make it explicit that this applies to the very first window ever > shown only. How about: > > "Returns true if the first window shown on first run should be unconditionally > maximized, overriding the heuristic that normally chooses the window size. Done. https://codereview.chromium.org/964503002/diff/60001/ash/shell_delegate.h#new... ash/shell_delegate.h:87: virtual bool IsFirstTimeMaximized() const = 0; On 2015/03/12 11:54:43, bartfab wrote: > Nit: How about ForceMaximizeOnFirstRun()? Done. https://codereview.chromium.org/964503002/diff/60001/ash/test/test_shell_dele... File ash/test/test_shell_delegate.h (right): https://codereview.chromium.org/964503002/diff/60001/ash/test/test_shell_dele... ash/test/test_shell_delegate.h:72: void SetFirstTimeMaximized(bool maximized) { On 2015/03/12 11:54:43, bartfab wrote: > Nit: If you rename IsFirstTimeMaximized(), rename this method and the member > accordingly. Done. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner.cc File ash/wm/window_positioner.cc (right): https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner.cc:268: // Policy to maximize window on the first run works only for Chrome OS. On 2015/03/12 11:54:43, bartfab wrote: > Nit: No need for this comment. Ash is cross-plaform and will honor whatever the > delegate tells it to do. The fact that |true| is only possible on Chrome OS is > irrelevant to ash. Done. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner.cc:269: const bool maximized_first_run = On 2015/03/12 11:54:43, bartfab wrote: > 1: Nit: How about s/maximized_first_run/maximize_first_window_on_first_run/ for > consistency? Done. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner.cc:271: // When using "small screens" we want to always open in full screen mode, On 2015/03/12 11:54:43, bartfab wrote: > Nit: How about "We want to always open maximized on "small screens" or when > policy tells us to?" Done. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner.cc:278: if (show_state_in == ui::SHOW_STATE_DEFAULT && set_maximized) { On 2015/03/12 11:54:43, bartfab wrote: > Nit: You should rearrange this so that |set_maximized| is not calculated when it > is not needed: > > if (show_state_in == ui::SHOW_STATE_DEFAULT) { > const bool bool set_maximized = ...; > if (set_maximized) > *show_state_out = ui::SHOW_STATE_MAXIMIZED; > } Done. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... File ash/wm/window_positioner_unittest.cc (right): https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:150: TEST_F(WindowPositionerTest, FirstRunMaximizeWindow) { On 2015/03/12 11:54:44, bartfab wrote: > Nit: The test names do not make it clear what combination of conditions is > tested by each. I think you should tweak the test names a bit and also add short > comments that explain what is being tested. Done. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:155: test::TestShellDelegate* d = On 2015/03/12 11:54:44, bartfab wrote: > Nit: Avoid abbreviations like |d|. Better use |delegate|. Done. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:159: ash::WindowPositioner::GetBoundsAndShowStateForNewWindow( On 2015/03/12 11:54:44, bartfab wrote: > Nit: s/ash::// Done. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:160: Shell::GetScreen(), NULL, false, ui::SHOW_STATE_DEFAULT, &bounds_in_out, On 2015/03/12 11:54:44, bartfab wrote: > Nit: s/NULL/nullptr/ Done. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:167: // Set width of the screen wider than limit. On 2015/03/12 11:54:44, bartfab wrote: > Nit: What limit? Done. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:168: const int kWidth = ash::WindowPositioner::GetForceMaximizedWidthLimit() + 100; On 2015/03/12 11:54:44, bartfab wrote: > Nit 1: This is not really a constant if it is calculated based on a method's > return value, hence s/kWidth/width/. > Nit 2: s/ash::// Done. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:169: const std::string kResolution = std::to_string(kWidth) + "x300"; //"1466x300" On 2015/03/12 11:54:44, bartfab wrote: > 1: Nit: s/kResolution/resolution/ for the same reason as above. > 2: Nit: #include <string> > 3: std::to_string() is not allowed. You cannot use C++11 library functions. > 4: Nit: s|//|// | Done. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:174: test::TestShellDelegate* d = On 2015/03/12 11:54:44, bartfab wrote: > As above: s/d/delegate/ Done. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:178: ash::WindowPositioner::GetBoundsAndShowStateForNewWindow( On 2015/03/12 11:54:44, bartfab wrote: > Nit: s/ash::// Done. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:179: Shell::GetScreen(), NULL, false, ui::SHOW_STATE_DEFAULT, &bounds_in_out, On 2015/03/12 11:54:44, bartfab wrote: > Nit: s/NULL/nullptr/ Done. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:186: // Set width of the screen narrower than limit. On 2015/03/12 11:54:44, bartfab wrote: > Nit: What limit? Done. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:187: const int kWidth = ash::WindowPositioner::GetForceMaximizedWidthLimit() - 100; On 2015/03/12 11:54:44, bartfab wrote: > Nit: As above: s/kWidth/width/ Done. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:188: const std::string kResolution = std::to_string(kWidth) + "x300"; //"1266x300" On 2015/03/12 11:54:44, bartfab wrote: > 1: Nit: s/kResolution/resolution/ > 3: std::to_string() is not allowed. You cannot use C++11 library functions. > 4: Nit: s|//|// | Done. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:193: test::TestShellDelegate* d = On 2015/03/12 11:54:44, bartfab wrote: > As above: s/d/delegate/ Done. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:197: ash::WindowPositioner::GetBoundsAndShowStateForNewWindow( On 2015/03/12 11:54:44, bartfab wrote: > Nit: s/ash::// Done. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:198: Shell::GetScreen(), NULL, false, ui::SHOW_STATE_DEFAULT, &bounds_in_out, On 2015/03/12 11:54:44, bartfab wrote: > Nit: s/NULL/nullptr/ Done. https://codereview.chromium.org/964503002/diff/60001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:203: On 2015/03/12 11:54:44, bartfab wrote: > You do not seem to have any test that verifies a subsequent windows (i.e. not > the first window on first login) are *not* affected by the policy. Added to browser tests. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc (right): https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:6: #include "ash/display/display_manager.h" On 2015/03/12 11:54:45, bartfab wrote: > Nit: Add blank line above. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:13: #include "ui/base/base_window.h" On 2015/03/12 11:54:45, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:17: class ForceMaximizeBrowserWindowOnFirstRunTest : public LoginPolicyBaseTest { On 2015/03/12 11:54:45, bartfab wrote: > Nit: How about s/BrowserWindow// for consistency? You can also shorten the file > name a bit to match :). Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:17: class ForceMaximizeBrowserWindowOnFirstRunTest : public LoginPolicyBaseTest { On 2015/03/12 11:54:45, bartfab wrote: > Nit: How about s/BrowserWindow// for consistency? You can also shorten the file > name a bit to match :). Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:18: std::string GetPolicy() const override { On 2015/03/12 11:54:45, bartfab wrote: > Nit: Add public: or protected: Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:19: return std::string( On 2015/03/12 11:54:45, bartfab wrote: > Nit: Do you really need the explicit std::string() construction here? Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:20: "{" On 2015/03/12 11:54:45, bartfab wrote: > It seems odd for |LoginPolicyBaseTest| to rely on individual tests to duplicate > this boilerplate JSON. IMHO, the boilerplate should live in > |LoginPolicyBaseTest| with individual tests just filling in the mandatory and > recommended sections. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:32: }; On 2015/03/12 11:54:45, bartfab wrote: > Nit: Add DISALLOW_COPY_AND_ASSIGN(ForceMaximizeBrowserWindowOnFirstRunTest). Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:34: IN_PROC_BROWSER_TEST_F(ForceMaximizeBrowserWindowOnFirstRunTest, StartSession) { On 2015/03/12 11:54:45, bartfab wrote: > Nit 1: #include "testing/gtest/include/gtest/gtest.h" > Nit 2: If you add a couple more test cases, you probably want to rename this one > to something like "MaximizeOnFirstRun." Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:35: // Set width of the screen wider than limit. By this condition without On 2015/03/12 11:54:45, bartfab wrote: > 1: Nit: What limit? > 2: Add a test that verifies when the policy is set to |false|, the window is not > maximized. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:35: // Set width of the screen wider than limit. By this condition without On 2015/03/12 11:54:45, bartfab wrote: > 1: Nit: What limit? > 2: Add a test that verifies when the policy is set to |false|, the window is not > maximized. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:38: const int kWidth = ash::WindowPositioner::GetForceMaximizedWidthLimit() + 100; On 2015/03/12 11:54:45, bartfab wrote: > Nit: This is not really a constant if it is calculated based on a method's > return value, hence s/kWidth/width/. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:39: const std::string kResolution = std::to_string(kWidth) + "x300"; //"1466x300" On 2015/03/12 11:54:45, bartfab wrote: > 1: Nit: s/kResolution/resolution/ for the same reason as above. > 2: std::to_string() is not allowed. You cannot use C++11 library functions. > 3: Nit: s|//|// | Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:40: ash::DisplayManager* display_manager = On 2015/03/12 11:54:45, bartfab wrote: > Nit 1: const pointer. > Nit 2: #include "ash/display/display_manager.h" Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:41: ash::Shell::GetInstance()->display_manager(); On 2015/03/12 11:54:45, bartfab wrote: > Nit: #include "ash/shell.h" Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:48: // Check that the browser window is maximized On 2015/03/12 11:54:45, bartfab wrote: > Nit 1: s/the/the first/ > Nit 2: Add full stop at the end. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:49: BrowserList* browser_list = On 2015/03/12 11:54:45, bartfab wrote: > Nit: const pointer. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:50: BrowserList::GetInstance(chrome::HOST_DESKTOP_TYPE_ASH); On 2015/03/12 11:54:45, bartfab wrote: > Nit: #include "chrome/browser/ui/host_desktop.h" Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:52: Browser* browser = browser_list->get(0); On 2015/03/12 11:54:45, bartfab wrote: > Nit: const pointer. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:56: On 2015/03/12 11:54:45, bartfab wrote: > Can you add a test that verifies on *second* login, the policy is ignored? You > can chain logins by having a test case X and a test case PRE_X. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:56: On 2015/03/12 11:54:45, bartfab wrote: > Can you add a test that verifies on *second* login, the policy is ignored? You > can chain logins by having a test case X and a test case PRE_X. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc:57: } // namespace policy On 2015/03/12 11:54:45, bartfab wrote: > Nit: Add blank line below. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/login_policy_base_test.cc (right): https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:5: #include "login_policy_base_test.h" On 2015/03/12 11:54:46, bartfab wrote: > Nit 1: Add the path to this file. > Nit 2: Add a blank line below. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:30: namespace policy { On 2015/03/12 11:54:46, bartfab wrote: > Nit: Move this higher up so that the anonymous namespace is also part of > |namespace policy|. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:33: const char LoginPolicyBaseTest::kAccountId[] = "dla1@example.com"; On 2015/03/12 11:54:46, bartfab wrote: > Nit: Why are you using "dla" here? Is this base meant for device-local account > tests only? It was here before my changes, but in another file... Have no idea what does this mean. :) https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:46: test_server_.reset(new LocalPolicyTestServer(policy_file_path())); On 2015/03/12 11:54:46, bartfab wrote: > Nit: #include "chrome/browser/policy/test/local_policy_test_server.h" Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:53: command_line->AppendSwitchASCII(policy::switches::kDeviceManagementUrl, On 2015/03/12 11:54:46, bartfab wrote: > Nit: #include "components/policy/core/common/policy_switches.h" Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:54: test_server_->GetServiceURL().spec()); On 2015/03/12 11:54:46, bartfab wrote: > Nit: #include "url/gurl.h" Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:89: chromeos::WizardController::SkipPostLoginScreensForTesting(); On 2015/03/12 11:54:46, bartfab wrote: > Nit: #include "chrome/browser/chromeos/login/wizard_controller.h" Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:90: chromeos::WizardController* wizard_controller = On 2015/03/12 11:54:46, bartfab wrote: > Nit: const pointer. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:93: wizard_controller->SkipToLoginForTesting(chromeos::LoginScreenContext()); On 2015/03/12 11:54:46, bartfab wrote: > Nit: #include "chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h" Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:95: content::WindowedNotificationObserver( On 2015/03/12 11:54:46, bartfab wrote: > Nit: #include "content/public/test/test_utils.h" Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:96: chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE, On 2015/03/12 11:54:46, bartfab wrote: > Nit: #include "chrome/browser/chrome_notification_types.h" Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:102: GetLoginDisplay()->ShowSigninScreenForCreds(user_id, password); On 2015/03/12 11:54:46, bartfab wrote: > Nit: #include "chrome/browser/chromeos/login/ui/webui_login_display.h" Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:102: GetLoginDisplay()->ShowSigninScreenForCreds(user_id, password); On 2015/03/12 11:54:46, bartfab wrote: > Nit: #include "chrome/browser/chromeos/login/ui/webui_login_display.h" Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:110: std::string pol = GetPolicy(); On 2015/03/12 11:54:46, bartfab wrote: > Nit: Avoid abbreviations like |pol|. You can actually just fold this into the > next line. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:111: const std::string policy = base::StringPrintf( On 2015/03/12 11:54:46, bartfab wrote: > You clearly have very specific requirements for the format of GetPolicy()'s > return value. Document these in the header file. No need for this any longer https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:115: base::WriteFile(policy_file_path(), policy.data(), policy.size()); On 2015/03/12 11:54:46, bartfab wrote: > Nit: #include "base/files/file_util.h" Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:120: return temp_dir_.path().AppendASCII("policy.json"); On 2015/03/12 11:54:46, bartfab wrote: > Nit: #include "base/files/file_path.h" Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/login_policy_base_test.h (right): https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:11: #include "chrome/browser/policy/test/local_policy_test_server.h" On 2015/03/12 11:54:47, bartfab wrote: > Nit: A forward-declaration is sufficient here. Move the include to the > implementation file instead. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:15: class LoginPolicyBaseTest : public chromeos::OobeBaseTest { On 2015/03/12 11:54:46, bartfab wrote: > Nit 1: This is not a base test, it is a *test base*. Swap around the two terms > in the class and file names. > Nit 2: Briefly document what this base is to be used for. Renamed. But why we have OobeBaseTest, not OobeTestBase? https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:18: ~LoginPolicyBaseTest() override; On 2015/03/12 11:54:46, bartfab wrote: > Nit: No need to override the destructor. It will not be compiled without override. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:20: void SetUp() override; On 2015/03/12 11:54:47, bartfab wrote: > Nit: Add // chromeos::OobeBaseTest: Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:23: void SetupGaiaServerWithAccessTokens(); On 2015/03/12 11:54:47, bartfab wrote: > Nit 1: Add blank line above. > Nit 2: s/Setup/SetUp/ > Nit 3: Make this private. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:24: void SetMergeSessionParams(const std::string& email); On 2015/03/12 11:54:47, bartfab wrote: > Nit 1: #include <string> > Nit 2: Make this private. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:27: void SetServerPolicy(); On 2015/03/12 11:54:46, bartfab wrote: > Nit: Make this private. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:30: base::FilePath policy_file_path() const; On 2015/03/12 11:54:46, bartfab wrote: > 1: If a method is not inlined, it should not be named in hacker_style. > 2: Nit: Forward-declare |base::FilePath|. > 3: Nit: This method is actually used in a single place only. Just inline its > body there and remove the method altogether. (1,2) done. (3) It is used twice :). https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:31: scoped_ptr<LocalPolicyTestServer> test_server_; On 2015/03/12 11:54:47, bartfab wrote: > Nit: Make this private if it is not used by any derived classes. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:33: base::ScopedTempDir temp_dir_; On 2015/03/12 11:54:47, bartfab wrote: > Nit 1: Make this private if it is not used by any derived classes. > Nit 2: #include "base/files/scoped_temp_dir.h" Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:35: static const char kAccountPassword[]; On 2015/03/12 11:54:47, bartfab wrote: > Nit: There is no need for the base class to define a dummy password as the base > class never actually refers to it. Yes, but it is needed in both derived classes and thus can be placed here to avoid code duplication. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:36: static const char kAccountId[]; On 2015/03/12 11:54:47, bartfab wrote: > I think it would be better to expose this as a method rather than a static > string. Otherwise, derived test classes suddenly refer to a magic constant named > |kAccountId| and you have no idea where it came from. What is the advantage? In case with method they will refer to a magic method... And behind this method will be just |return kAccountId;|. I think it will increase complexity of the code without clear advantages. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc (left): https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:12: #include "base/macros.h" On 2015/03/12 11:54:47, bartfab wrote: > Nit: Keep this. Done. Added DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:21: #include "chrome/browser/ui/host_desktop.h" On 2015/03/12 11:54:47, bartfab wrote: > Nit: Still used. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:162: DISALLOW_COPY_AND_ASSIGN(UserCloudPolicyManagerTest); On 2015/03/12 11:54:47, bartfab wrote: > Nit: Keep this. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc (right): https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:6: #include "chrome/browser/chromeos/policy/login_policy_base_test.h" On 2015/03/12 11:54:47, bartfab wrote: > Nit: Add a blank line above. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:14: std::string GetPolicy() const override { On 2015/03/12 11:54:47, bartfab wrote: > Nit: Mark this as public: or protected:. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/prefs/bro... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/964503002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.cc:513: On 2015/03/12 11:54:47, bartfab wrote: > Nit: Remove the blank line. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.cc:514: registry->RegisterBooleanPref( On 2015/03/12 11:54:47, bartfab wrote: > Please move this to chromeos::Preferences::RegisterProfilePrefs(). There is no > precedent for registering specific preferences here. It is always delegsted to > helper methods. Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/ui/ash/ch... File chrome/browser/ui/ash/chrome_shell_delegate.cc (right): https://codereview.chromium.org/964503002/diff/60001/chrome/browser/ui/ash/ch... chrome/browser/ui/ash/chrome_shell_delegate.cc:95: if (ProfileManager::GetActiveUserProfile()) { On 2015/03/12 11:54:47, bartfab wrote: > The documentation for this method says that you should "avoid using it at all > cost" :). It is also states that for guest, you will get a "suitable" profile, > whatever that may mean. I think a better implementation is: > > const user_manager::User* const user = > user_manager::UserManager::Get()->GetActiveUser(); > if (!user) > return false; > return chromeos::ProfileHelper::Get()->GetProfileByUser(user)-> > GetPrefs()->GetBoolean(prefs::kForceMaximizeBrowserWindowOnFirstRun); Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/ui/ash/ch... chrome/browser/ui/ash/chrome_shell_delegate.cc:96: return ProfileManager::GetActiveUserProfile()->GetPrefs()->GetBoolean( On 2015/03/12 11:54:47, bartfab wrote: > Nit: #include "chrome/browser/profiles/profile.h" Done. https://codereview.chromium.org/964503002/diff/60001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/964503002/diff/60001/chrome/common/pref_names... chrome/common/pref_names.cc:835: // This boolean controls if browser window is maximized on the first run. On 2015/03/12 11:54:47, bartfab wrote: > Nit: How about: > > "This boolean controls whether the first window shown on first run should be > unconditionally maximized, overriding the heuristic that normally chooses the > window size." Done. https://codereview.chromium.org/964503002/diff/60001/chrome/common/pref_names... chrome/common/pref_names.cc:836: const char kForceMaximizeBrowserWindowOnFirstRun[] = "ui.first_run_maximized"; On 2015/03/12 11:54:47, bartfab wrote: > Nit: How about s/first_run_maximized/force_maximize_on_first_run/ for > consistency? Done. https://codereview.chromium.org/964503002/diff/60001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/964503002/diff/60001/chrome/common/pref_names... chrome/common/pref_names.h:275: extern const char kForceMaximizeBrowserWindowOnFirstRun[]; On 2015/03/12 11:54:47, bartfab wrote: > Nit: How about s/BrowserWindow//? It seems unnecessary. Done. https://codereview.chromium.org/964503002/diff/60001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/964503002/diff/60001/components/policy/resour... components/policy/resources/policy_templates.json:7136: 'name': 'ForceMaximizeBrowserWindowOnFirstRun', On 2015/03/12 11:54:48, bartfab wrote: > Nit: How about s/BrowserWindow//? It seems unnecessary. Done. https://codereview.chromium.org/964503002/diff/60001/components/policy/resour... components/policy/resources/policy_templates.json:7139: 'supported_on': ['chrome_os:41-'], On 2015/03/12 11:54:48, bartfab wrote: > Nit: s/41/43/ Done. https://codereview.chromium.org/964503002/diff/60001/components/policy/resour... components/policy/resources/policy_templates.json:7146: 'caption': '''Maximize browser window on the first run''', On 2015/03/12 11:54:48, bartfab wrote: > Nit 1: s/browser/the first browser/ > Nit 2: s/the // Done. https://codereview.chromium.org/964503002/diff/60001/components/policy/resour... components/policy/resources/policy_templates.json:7147: 'desc': '''If this policy is set to true <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> will be launched in maximized mode on the first run or on each login in case of public sessions. On 2015/03/12 11:54:48, bartfab wrote: > Nit 1: s/true/true,/ > Nit 2: s/be launched in maximized mode on the/unconditionally maximize the the > first window shown on/ > Nit 3: I think we can remove the " or on each login in case of public sessions > bit." It is not just public sessions that behave this way. Ephemeral user > sessions are also always considered to be doing a "first login." Done. https://codereview.chromium.org/964503002/diff/60001/components/policy/resour... components/policy/resources/policy_templates.json:7148: If this policy is set to false or not configured <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> will be opened in default mode (maximized in case if screen width less than 1366 in case of first run or as in previous run).''', On 2015/03/12 11:54:48, bartfab wrote: > Nit 1: s/configured/configured,/ > Nit 2: I think you can simplify this to: "a heuristic will decide whether to > maximize the first window shown, based on the screen size." There is no need to > refer to a "previous run" here as by definition, we are looking at first runs > only. Done. https://codereview.chromium.org/964503002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/964503002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:45942: + <int value="290" label="Force maximize browser window on first run"/> On 2015/03/12 11:54:48, bartfab wrote: > Please sync the label with policy_templates.json. Done.
https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/login_policy_base_test.cc (right): https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:33: const char LoginPolicyBaseTest::kAccountId[] = "dla1@example.com"; On 2015/03/18 13:28:12, peletskyi wrote: > On 2015/03/12 11:54:46, bartfab wrote: > > Nit: Why are you using "dla" here? Is this base meant for device-local account > > tests only? > > It was here before my changes, but in another file... Have no idea what does > this mean. :) Device-local accounts are public sessions and single-app kiosk sessions. Every other account is not device-local. If it does not break things, please change the accunt ID to something more generic, e.g. "user@example.com". https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:90: chromeos::WizardController* wizard_controller = On 2015/03/18 13:28:11, peletskyi wrote: > On 2015/03/12 11:54:46, bartfab wrote: > > Nit: const pointer. > > Done. No, not done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/login_policy_base_test.h (right): https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:15: class LoginPolicyBaseTest : public chromeos::OobeBaseTest { On 2015/03/18 13:28:12, peletskyi wrote: > On 2015/03/12 11:54:46, bartfab wrote: > > Nit 1: This is not a base test, it is a *test base*. Swap around the two terms > > in the class and file names. > > Nit 2: Briefly document what this base is to be used for. > > Renamed. But why we have OobeBaseTest, not OobeTestBase? Yeah, that one is definitely poorly named :(. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:18: ~LoginPolicyBaseTest() override; On 2015/03/18 13:28:12, peletskyi wrote: > On 2015/03/12 11:54:46, bartfab wrote: > > Nit: No need to override the destructor. > > It will not be compiled without override. I mean no need to add a destructor at all. You are inheriting one already. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:36: static const char kAccountId[]; On 2015/03/18 13:28:12, peletskyi wrote: > On 2015/03/12 11:54:47, bartfab wrote: > > I think it would be better to expose this as a method rather than a static > > string. Otherwise, derived test classes suddenly refer to a magic constant > named > > |kAccountId| and you have no idea where it came from. > > What is the advantage? In case with method they will refer to a magic method... > And behind this method will be just |return kAccountId;|. I think it will > increase complexity of the code without clear advantages. Acknowledged. https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... File ash/wm/window_positioner_unittest.cc (left): https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:5: #include "ash/wm/window_positioner.h" This should still be the first include. https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... File ash/wm/window_positioner_unittest.cc (right): https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:153: // resolutions of full screen one can set "ForceMaximizeBrowserWindowOnFirstRun" Nit: s/of full screen/,/ https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:159: const std::string resolution = base::IntToString(width) + "x300"; Nit: #include "base/strings/string_number_conversions.h" https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:161: gfx::Rect bounds_in_out(0, 0, 320, 240); // random bounds Nit: s/random bounds/Random bounds./ (we always format comments as sentences, even if they are not :). https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:164: test::TestShellDelegate* delegate = Nit: const pointer. https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:181: gfx::Rect bounds_in_out(0, 0, 320, 240); // random bounds Nit: s/random bounds/Random bounds./ https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:184: test::TestShellDelegate* delegate = Nit: const pointer. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc (right): https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:4: #include <string> Nit: Add a blank line above. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:24: ForceMaximizeOnFirstRunTest() : LoginPolicyTestBase(){}; Nit: s/{};/ {}/ https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:32: // is *NOT* maximized by default. Nit: The sentence is a bit hard to parse. How about something like "Set a screen resolution for which the first browser window will not be maximized by default?" https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:37: ash::DisplayManager* display_manager = Nit: const pointer. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:44: DISALLOW_COPY_AND_ASSIGN(ForceMaximizeOnFirstRunTest); Nit: #include "base/macros.h" https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:53: const BrowserList* browser_list = Nit: const pointer. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:56: Browser* browser = browser_list->get(0); Nit: const pointer. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:60: browser->window()->Restore(); Nit: Add a comment that you need to un-maximize the window here as its state will be carried forward to the next window you open. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:63: // Create the second window and check if it is not affected by policy. Nit 1: a/the/a/ Nit 2: s/if/that/ https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:64: Browser* browser1 = CreateBrowser(ProfileManager::GetActiveUserProfile()); 1: As with chrome_shell_delegate.cc, the documentation for this method says that you should "avoid using it at all cost." 2: Nit: const pointer. Alternatively, make |browser| not be a const pointer and simply reset |browser| to the new window here. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:71: content::WindowedNotificationObserver( Nit: #include "content/public/test/test_utils.h" https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:72: chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE, Nit: #include "chrome/browser/chrome_notification_types.h" https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:73: content::NotificationService::AllSources()).Wait(); Nit: #include "content/public/browser/notification_service.h" https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:74: Nit: For consistency with the first test, remove the blank line. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:77: Browser* browser = CreateBrowser(ProfileManager::GetActiveUserProfile()); Nit: const pointer. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:84: ForceMaximizetPolicyFalseTest() : ForceMaximizeOnFirstRunTest(){}; Nit: s/{};/ {}/ https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:85: std::string GetMandatoryPolicies() const override { Nit: Add a blank line above. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:95: Nit: For consistency with the first test, remove the blank line. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:99: const BrowserList* browser_list = Nit: Const pointer. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:102: const Browser* browser = browser_list->get(0); Nit: Const pointer. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/login_policy_test_base.cc (right): https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_test_base.cc:6: #include "base/files/file_path.h" Nit: Move this to the header file. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_test_base.cc:42: std::string ret = 1: Construct a base::DictionaryValue and use base::JSONWriter to generate the string instead of constructing JSON manually. 2: Nit: Avoid abbreviations like |ret| 3: Nit: const. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/login_policy_test_base.h (right): https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_test_base.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. Nit: s/2014/2015/ https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_test_base.h:8: #include <string> Nit: Add blank line below. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_test_base.h:22: // This class can be used for the tests when policy needs to be set prior login. Nit: How about: "This class can be used to implement tests which need policy to be set prior to login." https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_test_base.h:33: virtual std::string GetMandatoryPolicies() const; Rather than expecting tests to manually construct JSON entries, please have these methods fill in a base::DictionaryValue instead. You can then add these dictionaries to an overall dictionary which holds all policy and finally serialize the whole thing. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_test_base.h:46: base::FilePath PolicyFilePath() const; Nit: When declaring a member of this type (not a pointer or reference), a forward-declaration is not sufficient. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_test_base.h:55: #endif // CHROME_BROWSER_CHROMEOS_POLICY_LOGIN_POLICY_TEST_BASE_H_ Nit: Add a blank line above. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc (right): https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:4: #include <string> Nit: Add a blank line above. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:17: UserCloudPolicyManagerTest() : LoginPolicyTestBase(){}; Nit: s/{};/ {}/ https://codereview.chromium.org/964503002/diff/80001/chrome/browser/ui/ash/ch... File chrome/browser/ui/ash/chrome_shell_delegate.cc (right): https://codereview.chromium.org/964503002/diff/80001/chrome/browser/ui/ash/ch... chrome/browser/ui/ash/chrome_shell_delegate.cc:10: #include "base/prefs/pref_service.h" Nit: Move this to the #if defined(OS_CHROMEOS) section. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/ui/ash/ch... chrome/browser/ui/ash/chrome_shell_delegate.cc:12: #include "chrome/browser/chromeos/profiles/profile_helper.h" Nit: Move this to the #if defined(OS_CHROMEOS) section. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/ui/ash/ch... chrome/browser/ui/ash/chrome_shell_delegate.cc:14: #include "chrome/browser/profiles/profile.h" Nit: Move this to the #if defined(OS_CHROMEOS) section. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/ui/ash/ch... chrome/browser/ui/ash/chrome_shell_delegate.cc:23: #include "chrome/common/pref_names.h" Nit: Move this to the #if defined(OS_CHROMEOS) section. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/ui/ash/ch... chrome/browser/ui/ash/chrome_shell_delegate.cc:97: const user_manager::User* const user = Nit: #include "components/user_manager/user.h" https://codereview.chromium.org/964503002/diff/80001/chrome/browser/ui/ash/ch... chrome/browser/ui/ash/chrome_shell_delegate.cc:99: if (user) Nit: Add curly braces. https://codereview.chromium.org/964503002/diff/80001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/964503002/diff/80001/chrome/common/pref_names... chrome/common/pref_names.cc:820: // This boolean controls whether the first window shown on first run should be Nit: Add blank line above. https://codereview.chromium.org/964503002/diff/80001/chrome/common/pref_names... chrome/common/pref_names.cc:823: const char kForceMaximizeOnFirstRun[] = Nit: This fits on one line. https://codereview.chromium.org/964503002/diff/80001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/964503002/diff/80001/components/policy/resour... components/policy/resources/policy_templates.json:126: # For your editing convenience: highest ID currently used: 295 Nit: Update this to 296. https://codereview.chromium.org/964503002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/964503002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:47632: + <int value="296" label="Force maximize browser window on first run"/> Nit: Please synchronize this with the caption in policy_templates.json.
peletskyi@chromium.org changed reviewers: + isherman@chromium.org, oshima@chromium.org, sky@chromium.org
Hi all, please take a look to the changes. I've fixed all the remarks from Bartosz (except two, which I need to discuss with him tomorrow). Thanks :) https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/login_policy_base_test.cc (right): https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:33: const char LoginPolicyBaseTest::kAccountId[] = "dla1@example.com"; On 2015/04/01 14:22:41, bartfab wrote: > On 2015/03/18 13:28:12, peletskyi wrote: > > On 2015/03/12 11:54:46, bartfab wrote: > > > Nit: Why are you using "dla" here? Is this base meant for device-local > account > > > tests only? > > > > It was here before my changes, but in another file... Have no idea what does > > this mean. :) > > Device-local accounts are public sessions and single-app kiosk sessions. Every > other account is not device-local. If it does not break things, please change > the accunt ID to something more generic, e.g. mailto:"user@example.com". Done. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:90: chromeos::WizardController* wizard_controller = On 2015/04/01 14:22:41, bartfab wrote: > On 2015/03/18 13:28:11, peletskyi wrote: > > On 2015/03/12 11:54:46, bartfab wrote: > > > Nit: const pointer. > > > > Done. > > No, not done. Can not be done, because method SkipToLoginForTesting() is not const. https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/login_policy_base_test.h (right): https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:18: ~LoginPolicyBaseTest() override; On 2015/04/01 14:22:41, bartfab wrote: > On 2015/03/18 13:28:12, peletskyi wrote: > > On 2015/03/12 11:54:46, bartfab wrote: > > > Nit: No need to override the destructor. > > > > It will not be compiled without override. > > I mean no need to add a destructor at all. You are inheriting one already. The compiler writes: ../../chrome/browser/chromeos/policy/login_policy_base_test.h:15:1: error: [chromium-style] Complex class/struct needs an explicit out-of-line destructor. Let's leave it. https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... File ash/wm/window_positioner_unittest.cc (left): https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:5: #include "ash/wm/window_positioner.h" On 2015/04/01 14:22:41, bartfab wrote: > This should still be the first include. Done. https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... File ash/wm/window_positioner_unittest.cc (right): https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:153: // resolutions of full screen one can set "ForceMaximizeBrowserWindowOnFirstRun" On 2015/04/01 14:22:41, bartfab wrote: > Nit: s/of full screen/,/ Done. https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:159: const std::string resolution = base::IntToString(width) + "x300"; On 2015/04/01 14:22:41, bartfab wrote: > Nit: #include "base/strings/string_number_conversions.h" Done. https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:161: gfx::Rect bounds_in_out(0, 0, 320, 240); // random bounds On 2015/04/01 14:22:41, bartfab wrote: > Nit: s/random bounds/Random bounds./ (we always format comments as sentences, > even if they are not :). Done. https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:164: test::TestShellDelegate* delegate = On 2015/04/01 14:22:41, bartfab wrote: > Nit: const pointer. Method SetForceMaximizeOnFirstRun is not const. https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:181: gfx::Rect bounds_in_out(0, 0, 320, 240); // random bounds On 2015/04/01 14:22:41, bartfab wrote: > Nit: s/random bounds/Random bounds./ Done. https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:184: test::TestShellDelegate* delegate = On 2015/04/01 14:22:41, bartfab wrote: > Nit: const pointer. Method SetForceMaximizeOnFirstRun is not const. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc (right): https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:4: #include <string> On 2015/04/01 14:22:42, bartfab wrote: > Nit: Add a blank line above. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:24: ForceMaximizeOnFirstRunTest() : LoginPolicyTestBase(){}; On 2015/04/01 14:22:42, bartfab wrote: > Nit: s/{};/ {}/ Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:32: // is *NOT* maximized by default. On 2015/04/01 14:22:42, bartfab wrote: > Nit: The sentence is a bit hard to parse. How about something like "Set a screen > resolution for which the first browser window will not be maximized by default?" Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:37: ash::DisplayManager* display_manager = On 2015/04/01 14:22:42, bartfab wrote: > Nit: const pointer. ash::test::DisplayManagerTestApi takes non-const pointer. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:44: DISALLOW_COPY_AND_ASSIGN(ForceMaximizeOnFirstRunTest); On 2015/04/01 14:22:42, bartfab wrote: > Nit: #include "base/macros.h" Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:53: const BrowserList* browser_list = On 2015/04/01 14:22:42, bartfab wrote: > Nit: const pointer. Already const https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:56: Browser* browser = browser_list->get(0); On 2015/04/01 14:22:42, bartfab wrote: > Nit: const pointer. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:60: browser->window()->Restore(); On 2015/04/01 14:22:41, bartfab wrote: > Nit: Add a comment that you need to un-maximize the window here as its state > will be carried forward to the next window you open. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:63: // Create the second window and check if it is not affected by policy. On 2015/04/01 14:22:42, bartfab wrote: > Nit 1: a/the/a/ > Nit 2: s/if/that/ Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:71: content::WindowedNotificationObserver( On 2015/04/01 14:22:41, bartfab wrote: > Nit: #include "content/public/test/test_utils.h" Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:72: chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE, On 2015/04/01 14:22:42, bartfab wrote: > Nit: #include "chrome/browser/chrome_notification_types.h" Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:73: content::NotificationService::AllSources()).Wait(); On 2015/04/01 14:22:42, bartfab wrote: > Nit: #include "content/public/browser/notification_service.h" Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:74: On 2015/04/01 14:22:42, bartfab wrote: > Nit: For consistency with the first test, remove the blank line. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:77: Browser* browser = CreateBrowser(ProfileManager::GetActiveUserProfile()); On 2015/04/01 14:22:42, bartfab wrote: > Nit: const pointer. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:84: ForceMaximizetPolicyFalseTest() : ForceMaximizeOnFirstRunTest(){}; On 2015/04/01 14:22:42, bartfab wrote: > Nit: s/{};/ {}/ Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:85: std::string GetMandatoryPolicies() const override { On 2015/04/01 14:22:42, bartfab wrote: > Nit: Add a blank line above. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:99: const BrowserList* browser_list = On 2015/04/01 14:22:42, bartfab wrote: > Nit: Const pointer. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:102: const Browser* browser = browser_list->get(0); On 2015/04/01 14:22:42, bartfab wrote: > Nit: Const pointer. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/login_policy_test_base.cc (right): https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_test_base.cc:6: #include "base/files/file_path.h" On 2015/04/01 14:22:42, bartfab wrote: > Nit: Move this to the header file. Why? https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_test_base.cc:42: std::string ret = On 2015/04/01 14:22:42, bartfab wrote: > 1: Construct a base::DictionaryValue and use base::JSONWriter to generate the > string instead of constructing JSON manually. > 2: Nit: Avoid abbreviations like |ret| > 3: Nit: const. 2,3 Done. 1. I think the code will be rather complicated with jsonwriter. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/login_policy_test_base.h (right): https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_test_base.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/04/01 14:22:42, bartfab wrote: > Nit: s/2014/2015/ Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_test_base.h:8: #include <string> On 2015/04/01 14:22:43, bartfab wrote: > Nit: Add blank line below. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_test_base.h:22: // This class can be used for the tests when policy needs to be set prior login. On 2015/04/01 14:22:43, bartfab wrote: > Nit: How about: "This class can be used to implement tests which need policy to > be set prior to login." Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_test_base.h:46: base::FilePath PolicyFilePath() const; On 2015/04/01 14:22:43, bartfab wrote: > Nit: When declaring a member of this type (not a pointer or reference), a > forward-declaration is not sufficient. Following styleguide we should use like forward declaration https://www.chromium.org/developers/coding-style/cpp-dos-and-donts https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_test_base.h:55: #endif // CHROME_BROWSER_CHROMEOS_POLICY_LOGIN_POLICY_TEST_BASE_H_ On 2015/04/01 14:22:43, bartfab wrote: > Nit: Add a blank line above. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc (right): https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:4: #include <string> On 2015/04/01 14:22:43, bartfab wrote: > Nit: Add a blank line above. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:17: UserCloudPolicyManagerTest() : LoginPolicyTestBase(){}; On 2015/04/01 14:22:43, bartfab wrote: > Nit: s/{};/ {}/ Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/ui/ash/ch... File chrome/browser/ui/ash/chrome_shell_delegate.cc (right): https://codereview.chromium.org/964503002/diff/80001/chrome/browser/ui/ash/ch... chrome/browser/ui/ash/chrome_shell_delegate.cc:10: #include "base/prefs/pref_service.h" On 2015/04/01 14:22:43, bartfab wrote: > Nit: Move this to the #if defined(OS_CHROMEOS) section. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/ui/ash/ch... chrome/browser/ui/ash/chrome_shell_delegate.cc:12: #include "chrome/browser/chromeos/profiles/profile_helper.h" On 2015/04/01 14:22:43, bartfab wrote: > Nit: Move this to the #if defined(OS_CHROMEOS) section. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/ui/ash/ch... chrome/browser/ui/ash/chrome_shell_delegate.cc:14: #include "chrome/browser/profiles/profile.h" On 2015/04/01 14:22:43, bartfab wrote: > Nit: Move this to the #if defined(OS_CHROMEOS) section. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/ui/ash/ch... chrome/browser/ui/ash/chrome_shell_delegate.cc:23: #include "chrome/common/pref_names.h" On 2015/04/01 14:22:43, bartfab wrote: > Nit: Move this to the #if defined(OS_CHROMEOS) section. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/ui/ash/ch... chrome/browser/ui/ash/chrome_shell_delegate.cc:97: const user_manager::User* const user = On 2015/04/01 14:22:43, bartfab wrote: > Nit: #include "components/user_manager/user.h" Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/ui/ash/ch... chrome/browser/ui/ash/chrome_shell_delegate.cc:99: if (user) On 2015/04/01 14:22:43, bartfab wrote: > Nit: Add curly braces. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/964503002/diff/80001/chrome/common/pref_names... chrome/common/pref_names.cc:820: // This boolean controls whether the first window shown on first run should be On 2015/04/01 14:22:43, bartfab wrote: > Nit: Add blank line above. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/common/pref_names... chrome/common/pref_names.cc:823: const char kForceMaximizeOnFirstRun[] = On 2015/04/01 14:22:43, bartfab wrote: > Nit: This fits on one line. Done. https://codereview.chromium.org/964503002/diff/80001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/964503002/diff/80001/components/policy/resour... components/policy/resources/policy_templates.json:126: # For your editing convenience: highest ID currently used: 295 On 2015/04/01 14:22:43, bartfab wrote: > Nit: Update this to 296. 298 :) https://codereview.chromium.org/964503002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/964503002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:47632: + <int value="296" label="Force maximize browser window on first run"/> On 2015/04/01 14:22:43, bartfab wrote: > Nit: Please synchronize this with the caption in policy_templates.json. Done.
histograms.xml lgtm -- I'm assuming that's what you wanted me to look at?
https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/login_policy_base_test.cc (right): https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:90: chromeos::WizardController* wizard_controller = On 2015/04/01 18:55:45, peletskyi wrote: > On 2015/04/01 14:22:41, bartfab wrote: > > On 2015/03/18 13:28:11, peletskyi wrote: > > > On 2015/03/12 11:54:46, bartfab wrote: > > > > Nit: const pointer. > > > > > > Done. > > > > No, not done. > > Can not be done, because method SkipToLoginForTesting() is not const. I asked for a const pointer, not a pointer to const :). Try: chromeos::WizardController* const wizard_controller = https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/login_policy_base_test.h (right): https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.h:18: ~LoginPolicyBaseTest() override; On 2015/04/01 18:55:45, peletskyi wrote: > On 2015/04/01 14:22:41, bartfab wrote: > > On 2015/03/18 13:28:12, peletskyi wrote: > > > On 2015/03/12 11:54:46, bartfab wrote: > > > > Nit: No need to override the destructor. > > > > > > It will not be compiled without override. > > > > I mean no need to add a destructor at all. You are inheriting one already. > > The compiler writes: > ../../chrome/browser/chromeos/policy/login_policy_base_test.h:15:1: error: > [chromium-style] Complex class/struct needs an explicit out-of-line destructor. > Let's leave it. Sure, let's leave it. I am still surprise it insists on this as there already is an inherited, explicit out-of-line destructor. But so be it, we cannot win against a zealous style-checker bot. https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... File ash/wm/window_positioner_unittest.cc (right): https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:164: test::TestShellDelegate* delegate = On 2015/04/01 18:55:45, peletskyi wrote: > On 2015/04/01 14:22:41, bartfab wrote: > > Nit: const pointer. > > Method SetForceMaximizeOnFirstRun is not const. Const pointer, not pointer to const. https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:184: test::TestShellDelegate* delegate = On 2015/04/01 18:55:45, peletskyi wrote: > On 2015/04/01 14:22:41, bartfab wrote: > > Nit: const pointer. > > Method SetForceMaximizeOnFirstRun is not const. Const pointer, not pointer to const. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc (right): https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:37: ash::DisplayManager* display_manager = On 2015/04/01 18:55:46, peletskyi wrote: > On 2015/04/01 14:22:42, bartfab wrote: > > Nit: const pointer. > > ash::test::DisplayManagerTestApi takes non-const pointer. It takes a pointer to non-const, but it does not care whether the pointer itself is const or not. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:53: const BrowserList* browser_list = On 2015/04/01 18:55:46, peletskyi wrote: > On 2015/04/01 14:22:42, bartfab wrote: > > Nit: const pointer. > > Already const This is a pointer to const. Make it a const pointer as well: const BrowserList* const browser_list = https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:56: Browser* browser = browser_list->get(0); On 2015/04/01 18:55:45, peletskyi wrote: > On 2015/04/01 14:22:42, bartfab wrote: > > Nit: const pointer. > > Done. Nice to see that it can be a pointer to coinst. Please also make it a const pointer :). https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:64: Browser* browser1 = CreateBrowser(ProfileManager::GetActiveUserProfile()); On 2015/04/01 14:22:41, bartfab wrote: > 1: As with chrome_shell_delegate.cc, the documentation for this method says that > you should "avoid using it at all cost." > 2: Nit: const pointer. Alternatively, make |browser| not be a const pointer and > simply reset |browser| to the new window here. 1: This was not not addressed. 2: Please make |browser1| a const pointer to const. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:77: Browser* browser = CreateBrowser(ProfileManager::GetActiveUserProfile()); On 2015/04/01 18:55:46, peletskyi wrote: > On 2015/04/01 14:22:42, bartfab wrote: > > Nit: const pointer. > > Done. Please make this a const pointer to const. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:99: const BrowserList* browser_list = On 2015/04/01 18:55:46, peletskyi wrote: > On 2015/04/01 14:22:42, bartfab wrote: > > Nit: Const pointer. > > Done. Please make this a const pointer to const. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:102: const Browser* browser = browser_list->get(0); On 2015/04/01 18:55:46, peletskyi wrote: > On 2015/04/01 14:22:42, bartfab wrote: > > Nit: Const pointer. > > Done. Please make this a const pointer to const. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/login_policy_test_base.cc (right): https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_test_base.cc:6: #include "base/files/file_path.h" On 2015/04/01 18:55:46, peletskyi wrote: > On 2015/04/01 14:22:42, bartfab wrote: > > Nit: Move this to the header file. > > Why? Because I thought that you needed the full type in the header. Now that I stand corrected, this LGTM. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_test_base.cc:42: std::string ret = On 2015/04/01 18:55:46, peletskyi wrote: > On 2015/04/01 14:22:42, bartfab wrote: > > 1: Construct a base::DictionaryValue and use base::JSONWriter to generate the > > string instead of constructing JSON manually. > > 2: Nit: Avoid abbreviations like |ret| > > 3: Nit: const. > > 2,3 Done. > 1. I think the code will be rather complicated with jsonwriter. Re 1: Why would it be complicated? You could even do it like this: * Store the initial JSON structure as an explicit string constant. * Parse this structure into a DictionaryValue. * Add any data you need, such as the policy user, mandatory and recommended policies. * Write back to JSON. It should all be no more than 20 lines of code. Re 3: No, not done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/login_policy_test_base.h (right): https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_test_base.h:33: virtual std::string GetMandatoryPolicies() const; On 2015/04/01 14:22:43, bartfab wrote: > Rather than expecting tests to manually construct JSON entries, please have > these methods fill in a base::DictionaryValue instead. You can then add these > dictionaries to an overall dictionary which holds all policy and finally > serialize the whole thing. I still think it is the right thing to do this. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_test_base.h:46: base::FilePath PolicyFilePath() const; On 2015/04/01 18:55:46, peletskyi wrote: > On 2015/04/01 14:22:43, bartfab wrote: > > Nit: When declaring a member of this type (not a pointer or reference), a > > forward-declaration is not sufficient. > > Following styleguide we should use like forward declaration > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts Nice catch. I was not aware that for return types, forward declarations are sufficient. LGTM. https://codereview.chromium.org/964503002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/964503002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:47632: + <int value="296" label="Force maximize browser window on first run"/> On 2015/04/01 18:55:47, peletskyi wrote: > On 2015/04/01 14:22:43, bartfab wrote: > > Nit: Please synchronize this with the caption in policy_templates.json. > > Done. No, not done. https://codereview.chromium.org/964503002/diff/100001/ash/shell/shell_delegat... File ash/shell/shell_delegate_impl.cc (right): https://codereview.chromium.org/964503002/diff/100001/ash/shell/shell_delegat... ash/shell/shell_delegate_impl.cc:184: bool ShellDelegateImpl::ForceMaximizeOnFirstRun() const { You have a merge error here. The method appears three times now.
peletskyi@chromium.org changed reviewers: - oshima@chromium.org, sky@chromium.org
Hope now it looks better :) https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/login_policy_base_test.cc (right): https://codereview.chromium.org/964503002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_base_test.cc:90: chromeos::WizardController* wizard_controller = On 2015/04/02 08:44:27, bartfab wrote: > On 2015/04/01 18:55:45, peletskyi wrote: > > On 2015/04/01 14:22:41, bartfab wrote: > > > On 2015/03/18 13:28:11, peletskyi wrote: > > > > On 2015/03/12 11:54:46, bartfab wrote: > > > > > Nit: const pointer. > > > > > > > > Done. > > > > > > No, not done. > > > > Can not be done, because method SkipToLoginForTesting() is not const. > > I asked for a const pointer, not a pointer to const :). Try: > > chromeos::WizardController* const wizard_controller = Done. https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... File ash/wm/window_positioner_unittest.cc (right): https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:164: test::TestShellDelegate* delegate = On 2015/04/02 08:44:28, bartfab wrote: > On 2015/04/01 18:55:45, peletskyi wrote: > > On 2015/04/01 14:22:41, bartfab wrote: > > > Nit: const pointer. > > > > Method SetForceMaximizeOnFirstRun is not const. > > Const pointer, not pointer to const. Done. https://codereview.chromium.org/964503002/diff/80001/ash/wm/window_positioner... ash/wm/window_positioner_unittest.cc:184: test::TestShellDelegate* delegate = On 2015/04/02 08:44:28, bartfab wrote: > On 2015/04/01 18:55:45, peletskyi wrote: > > On 2015/04/01 14:22:41, bartfab wrote: > > > Nit: const pointer. > > > > Method SetForceMaximizeOnFirstRun is not const. > > Const pointer, not pointer to const. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc (right): https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:37: ash::DisplayManager* display_manager = On 2015/04/02 08:44:28, bartfab wrote: > On 2015/04/01 18:55:46, peletskyi wrote: > > On 2015/04/01 14:22:42, bartfab wrote: > > > Nit: const pointer. > > > > ash::test::DisplayManagerTestApi takes non-const pointer. > > It takes a pointer to non-const, but it does not care whether the pointer itself > is const or not. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:53: const BrowserList* browser_list = On 2015/04/02 08:44:28, bartfab wrote: > On 2015/04/01 18:55:46, peletskyi wrote: > > On 2015/04/01 14:22:42, bartfab wrote: > > > Nit: const pointer. > > > > Already const > > This is a pointer to const. Make it a const pointer as well: > > const BrowserList* const browser_list = Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:56: Browser* browser = browser_list->get(0); On 2015/04/02 08:44:28, bartfab wrote: > On 2015/04/01 18:55:45, peletskyi wrote: > > On 2015/04/01 14:22:42, bartfab wrote: > > > Nit: const pointer. > > > > Done. > > Nice to see that it can be a pointer to coinst. Please also make it a const > pointer :). Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:64: Browser* browser1 = CreateBrowser(ProfileManager::GetActiveUserProfile()); On 2015/04/02 08:44:28, bartfab wrote: > On 2015/04/01 14:22:41, bartfab wrote: > > 1: As with chrome_shell_delegate.cc, the documentation for this method says > that > > you should "avoid using it at all cost." > > 2: Nit: const pointer. Alternatively, make |browser| not be a const pointer > and > > simply reset |browser| to the new window here. > > 1: This was not not addressed. > 2: Please make |browser1| a const pointer to const. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:77: Browser* browser = CreateBrowser(ProfileManager::GetActiveUserProfile()); On 2015/04/02 08:44:28, bartfab wrote: > On 2015/04/01 18:55:46, peletskyi wrote: > > On 2015/04/01 14:22:42, bartfab wrote: > > > Nit: const pointer. > > > > Done. > > Please make this a const pointer to const. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:95: On 2015/04/01 14:22:42, bartfab wrote: > Nit: For consistency with the first test, remove the blank line. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:99: const BrowserList* browser_list = On 2015/04/02 08:44:28, bartfab wrote: > On 2015/04/01 18:55:46, peletskyi wrote: > > On 2015/04/01 14:22:42, bartfab wrote: > > > Nit: Const pointer. > > > > Done. > > Please make this a const pointer to const. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:102: const Browser* browser = browser_list->get(0); On 2015/04/02 08:44:28, bartfab wrote: > On 2015/04/01 18:55:46, peletskyi wrote: > > On 2015/04/01 14:22:42, bartfab wrote: > > > Nit: Const pointer. > > > > Done. > > Please make this a const pointer to const. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/login_policy_test_base.cc (right): https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_test_base.cc:42: std::string ret = On 2015/04/02 08:44:28, bartfab wrote: > On 2015/04/01 18:55:46, peletskyi wrote: > > On 2015/04/01 14:22:42, bartfab wrote: > > > 1: Construct a base::DictionaryValue and use base::JSONWriter to generate > the > > > string instead of constructing JSON manually. > > > 2: Nit: Avoid abbreviations like |ret| > > > 3: Nit: const. > > > > 2,3 Done. > > 1. I think the code will be rather complicated with jsonwriter. > > Re 1: Why would it be complicated? You could even do it like this: > > * Store the initial JSON structure as an explicit string constant. > * Parse this structure into a DictionaryValue. > * Add any data you need, such as the policy user, mandatory and recommended > policies. > * Write back to JSON. > > It should all be no more than 20 lines of code. > > Re 3: No, not done. Done. https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/login_policy_test_base.h (right): https://codereview.chromium.org/964503002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/login_policy_test_base.h:33: virtual std::string GetMandatoryPolicies() const; On 2015/04/02 08:44:28, bartfab wrote: > On 2015/04/01 14:22:43, bartfab wrote: > > Rather than expecting tests to manually construct JSON entries, please have > > these methods fill in a base::DictionaryValue instead. You can then add these > > dictionaries to an overall dictionary which holds all policy and finally > > serialize the whole thing. > > I still think it is the right thing to do this. Done. https://codereview.chromium.org/964503002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/964503002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:47632: + <int value="296" label="Force maximize browser window on first run"/> On 2015/04/02 08:44:28, bartfab wrote: > On 2015/04/01 18:55:47, peletskyi wrote: > > On 2015/04/01 14:22:43, bartfab wrote: > > > Nit: Please synchronize this with the caption in policy_templates.json. > > > > Done. > > No, not done. Hm, on my machine it is 298... https://codereview.chromium.org/964503002/diff/100001/ash/shell/shell_delegat... File ash/shell/shell_delegate_impl.cc (right): https://codereview.chromium.org/964503002/diff/100001/ash/shell/shell_delegat... ash/shell/shell_delegate_impl.cc:184: bool ShellDelegateImpl::ForceMaximizeOnFirstRun() const { On 2015/04/02 08:44:28, bartfab wrote: > You have a merge error here. The method appears three times now. Done.
peletskyi@chromium.org changed reviewers: + oshima@chromium.org, sky@chromium.org
oshima@ and sky@ can you please review changes in: ash/ chrome/browser/chromeos/preferences.cc chrome/browser/ui/ash/chrome_shell_delegate.* Thanks :)
lgtm https://codereview.chromium.org/964503002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/964503002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:47632: + <int value="296" label="Force maximize browser window on first run"/> On 2015/04/02 12:53:29, peletskyi wrote: > On 2015/04/02 08:44:28, bartfab wrote: > > On 2015/04/01 18:55:47, peletskyi wrote: > > > On 2015/04/01 14:22:43, bartfab wrote: > > > > Nit: Please synchronize this with the caption in policy_templates.json. > > > > > > Done. > > > > No, not done. > > Hm, on my machine it is 298... Yes, the number is corect. What I meant was that the "label" here does not match the "caption" in policy_templates.json. Unless there is good reason to deviate form it, they should be the same. https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc (right): https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:33: base::DictionaryValue* const dict = new base::DictionaryValue; Nit 1: #include "base/values.h" Nit 2: Use a scoped_ptr. https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:74: const user_manager::User* const user = The code is fine as it is but if you want to make it shorter, feel free to fold these 5 lines into 1. You do not need to store all the temporaries in explicit variables unless you want to. Up to you. If you stick with five lines, it would be nice to move this to a helper, something like: const Browser* OpenNewBrowserWindow() { ... } https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:75: user_manager::UserManager::Get()->GetActiveUser(); Nit: #include "components/user_manager/user.h" https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:76: Profile* const profile = Nit: #include "chrome/browser/profiles/profile.h" https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:104: base::DictionaryValue* const dict = new base::DictionaryValue; Nit: Use a scoped_ptr. https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/login_policy_test_base.cc (right): https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/login_policy_test_base.cc:39: std::string GetPolicy(base::DictionaryValue* mandatory, Nit: Pass |mandatory| and |recommended| as scoped_ptrs. https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/login_policy_test_base.cc:43: base::DictionaryValue* policy_type_dict = new base::DictionaryValue; Nit 1: Use a scoped_ptr. Nit 2: #include "base/values.h" https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/login_policy_test_base.cc:47: base::ListValue* managed_users_list = new base::ListValue; Nit: Use a scoped_ptr. https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/login_policy_test_base.h (right): https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/login_policy_test_base.h:35: virtual base::DictionaryValue* GetMandatoryPoliciesValue() const; Nit 1: Forward-declare base::DictionaryValue. Nit 2: When passing ownership, always use a scoped_ptr. https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc (right): https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:21: base::ListValue* list = new base::ListValue; Nit 1: Use a scoped_ptr. Nit 2: #include "base/values.h" https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:25: base::DictionaryValue* const dict = new base::DictionaryValue; Nit: Use a scoped_ptr. https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:26: dict->Set("RestoreOnStartupURLs", list); Nit: You could improve the existing test a bit here by switching from hard-coded strings to the corresponding policy constants, as done in your new test.
Done https://codereview.chromium.org/964503002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/964503002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:47632: + <int value="296" label="Force maximize browser window on first run"/> On 2015/04/02 13:29:31, bartfab wrote: > On 2015/04/02 12:53:29, peletskyi wrote: > > On 2015/04/02 08:44:28, bartfab wrote: > > > On 2015/04/01 18:55:47, peletskyi wrote: > > > > On 2015/04/01 14:22:43, bartfab wrote: > > > > > Nit: Please synchronize this with the caption in policy_templates.json. > > > > > > > > Done. > > > > > > No, not done. > > > > Hm, on my machine it is 298... > > Yes, the number is corect. What I meant was that the "label" here does not match > the "caption" in policy_templates.json. Unless there is good reason to deviate > form it, they should be the same. Done. https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc (right): https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:74: const user_manager::User* const user = On 2015/04/02 13:29:31, bartfab (OOO till 7th April) wrote: > The code is fine as it is but if you want to make it shorter, feel free to fold > these 5 lines into 1. You do not need to store all the temporaries in explicit > variables unless you want to. Up to you. If you stick with five lines, it would > be nice to move this to a helper, something like: > > const Browser* OpenNewBrowserWindow() { > ... > } I'd prefer helper :) https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:75: user_manager::UserManager::Get()->GetActiveUser(); On 2015/04/02 13:29:31, bartfab (OOO till 7th April) wrote: > Nit: #include "components/user_manager/user.h" Already here https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:76: Profile* const profile = On 2015/04/02 13:29:31, bartfab (OOO till 7th April) wrote: > Nit: #include "chrome/browser/profiles/profile.h" Done. https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:104: base::DictionaryValue* const dict = new base::DictionaryValue; On 2015/04/02 13:29:31, bartfab (OOO till 7th April) wrote: > Nit: Use a scoped_ptr. Done. https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/login_policy_test_base.cc (right): https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/login_policy_test_base.cc:39: std::string GetPolicy(base::DictionaryValue* mandatory, On 2015/04/02 13:29:31, bartfab (OOO till 7th April) wrote: > Nit: Pass |mandatory| and |recommended| as scoped_ptrs. Done. https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/login_policy_test_base.cc:43: base::DictionaryValue* policy_type_dict = new base::DictionaryValue; On 2015/04/02 13:29:31, bartfab (OOO till 7th April) wrote: > Nit 1: Use a scoped_ptr. > Nit 2: #include "base/values.h" Done. https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/login_policy_test_base.cc:47: base::ListValue* managed_users_list = new base::ListValue; On 2015/04/02 13:29:31, bartfab (OOO till 7th April) wrote: > Nit: Use a scoped_ptr. Done. https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/login_policy_test_base.h (right): https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/login_policy_test_base.h:35: virtual base::DictionaryValue* GetMandatoryPoliciesValue() const; On 2015/04/02 13:29:31, bartfab (OOO till 7th April) wrote: > Nit 1: Forward-declare base::DictionaryValue. > Nit 2: When passing ownership, always use a scoped_ptr. Done. https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc (right): https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:21: base::ListValue* list = new base::ListValue; On 2015/04/02 13:29:32, bartfab (OOO till 7th April) wrote: > Nit 1: Use a scoped_ptr. > Nit 2: #include "base/values.h" Done. https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:25: base::DictionaryValue* const dict = new base::DictionaryValue; On 2015/04/02 13:29:32, bartfab (OOO till 7th April) wrote: > Nit: Use a scoped_ptr. Done. https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:26: dict->Set("RestoreOnStartupURLs", list); On 2015/04/02 13:29:32, bartfab (OOO till 7th April) wrote: > Nit: You could improve the existing test a bit here by switching from hard-coded > strings to the corresponding policy constants, as done in your new test. Done.
Still LGTM. https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc (right): https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:75: user_manager::UserManager::Get()->GetActiveUser(); On 2015/04/02 15:24:50, peletskyi wrote: > On 2015/04/02 13:29:31, bartfab (OOO till 7th April) wrote: > > Nit: #include "components/user_manager/user.h" > > Already here Sorry, I meant "components/user_manager/user_manager.h", which is missing. https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc (right): https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:26: dict->Set("RestoreOnStartupURLs", list); On 2015/04/02 15:24:51, peletskyi wrote: > On 2015/04/02 13:29:32, bartfab (OOO till 7th April) wrote: > > Nit: You could improve the existing test a bit here by switching from > hard-coded > > strings to the corresponding policy constants, as done in your new test. > > Done. What you did is great. I had missed it that there is a constant you can use instead of the magic "4". What I actually meant was to replace the "RestoreOnStartupURLs" and "RestoreOnStartup" strings with constants though, which would be a further improvement to the code. https://codereview.chromium.org/964503002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc (right): https://codereview.chromium.org/964503002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:54: const Browser* OpenNewBrowserWindow() { Nit: There is no need for this to be a member of ForceMaximizeOnFirstRunTest. It does not access any members. https://codereview.chromium.org/964503002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/login_policy_test_base.cc (right): https://codereview.chromium.org/964503002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/policy/login_policy_test_base.cc:99: return scoped_ptr<base::DictionaryValue>(new base::DictionaryValue); Nit: You can use make_scoped_ptr() to make this a bit shorter. https://codereview.chromium.org/964503002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/policy/login_policy_test_base.cc:104: return scoped_ptr<base::DictionaryValue>(new base::DictionaryValue); Nit: You can use make_scoped_ptr() to make this a bit shorter.
PTAL if you want to :) https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc (right): https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:75: user_manager::UserManager::Get()->GetActiveUser(); On 2015/04/07 11:37:16, bartfab wrote: > On 2015/04/02 15:24:50, peletskyi wrote: > > On 2015/04/02 13:29:31, bartfab (OOO till 7th April) wrote: > > > Nit: #include "components/user_manager/user.h" > > > > Already here > > Sorry, I meant "components/user_manager/user_manager.h", which is missing. Done. https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc (right): https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:26: dict->Set("RestoreOnStartupURLs", list); On 2015/04/07 11:37:16, bartfab wrote: > On 2015/04/02 15:24:51, peletskyi wrote: > > On 2015/04/02 13:29:32, bartfab (OOO till 7th April) wrote: > > > Nit: You could improve the existing test a bit here by switching from > > hard-coded > > > strings to the corresponding policy constants, as done in your new test. > > > > Done. > > What you did is great. I had missed it that there is a constant you can use > instead of the magic "4". What I actually meant was to replace the > "RestoreOnStartupURLs" and "RestoreOnStartup" strings with constants though, > which would be a further improvement to the code. Done. https://codereview.chromium.org/964503002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc (right): https://codereview.chromium.org/964503002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:54: const Browser* OpenNewBrowserWindow() { On 2015/04/07 11:37:17, bartfab wrote: > Nit: There is no need for this to be a member of ForceMaximizeOnFirstRunTest. It > does not access any members. We refer to CreateBrowser() https://codereview.chromium.org/964503002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/login_policy_test_base.cc (right): https://codereview.chromium.org/964503002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/policy/login_policy_test_base.cc:99: return scoped_ptr<base::DictionaryValue>(new base::DictionaryValue); On 2015/04/07 11:37:17, bartfab wrote: > Nit: You can use make_scoped_ptr() to make this a bit shorter. Done. https://codereview.chromium.org/964503002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/policy/login_policy_test_base.cc:104: return scoped_ptr<base::DictionaryValue>(new base::DictionaryValue); On 2015/04/07 11:37:17, bartfab wrote: > Nit: You can use make_scoped_ptr() to make this a bit shorter. Done.
On 2015/04/07 13:14:29, peletskyi wrote: > PTAL if you want to :) > > https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... > File > chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc > (right): > > https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... > chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:75: > user_manager::UserManager::Get()->GetActiveUser(); > On 2015/04/07 11:37:16, bartfab wrote: > > On 2015/04/02 15:24:50, peletskyi wrote: > > > On 2015/04/02 13:29:31, bartfab (OOO till 7th April) wrote: > > > > Nit: #include "components/user_manager/user.h" > > > > > > Already here > > > > Sorry, I meant "components/user_manager/user_manager.h", which is missing. > > Done. > > https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... > File > chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc > (right): > > https://codereview.chromium.org/964503002/diff/120001/chrome/browser/chromeos... > chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:26: > dict->Set("RestoreOnStartupURLs", list); > On 2015/04/07 11:37:16, bartfab wrote: > > On 2015/04/02 15:24:51, peletskyi wrote: > > > On 2015/04/02 13:29:32, bartfab (OOO till 7th April) wrote: > > > > Nit: You could improve the existing test a bit here by switching from > > > hard-coded > > > > strings to the corresponding policy constants, as done in your new test. > > > > > > Done. > > > > What you did is great. I had missed it that there is a constant you can use > > instead of the magic "4". What I actually meant was to replace the > > "RestoreOnStartupURLs" and "RestoreOnStartup" strings with constants though, > > which would be a further improvement to the code. > > Done. > > https://codereview.chromium.org/964503002/diff/140001/chrome/browser/chromeos... > File > chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc > (right): > > https://codereview.chromium.org/964503002/diff/140001/chrome/browser/chromeos... > chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:54: > const Browser* OpenNewBrowserWindow() { > On 2015/04/07 11:37:17, bartfab wrote: > > Nit: There is no need for this to be a member of ForceMaximizeOnFirstRunTest. > It > > does not access any members. > > We refer to CreateBrowser() > > https://codereview.chromium.org/964503002/diff/140001/chrome/browser/chromeos... > File chrome/browser/chromeos/policy/login_policy_test_base.cc (right): > > https://codereview.chromium.org/964503002/diff/140001/chrome/browser/chromeos... > chrome/browser/chromeos/policy/login_policy_test_base.cc:99: return > scoped_ptr<base::DictionaryValue>(new base::DictionaryValue); > On 2015/04/07 11:37:17, bartfab wrote: > > Nit: You can use make_scoped_ptr() to make this a bit shorter. > > Done. > > https://codereview.chromium.org/964503002/diff/140001/chrome/browser/chromeos... > chrome/browser/chromeos/policy/login_policy_test_base.cc:104: return > scoped_ptr<base::DictionaryValue>(new base::DictionaryValue); > On 2015/04/07 11:37:17, bartfab wrote: > > Nit: You can use make_scoped_ptr() to make this a bit shorter. > > Done. Hi oshima, friendly ping
LGTM. No further nits. https://codereview.chromium.org/964503002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc (right): https://codereview.chromium.org/964503002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc:54: const Browser* OpenNewBrowserWindow() { On 2015/04/07 13:14:29, peletskyi wrote: > On 2015/04/07 11:37:17, bartfab wrote: > > Nit: There is no need for this to be a member of ForceMaximizeOnFirstRunTest. > It > > does not access any members. > > We refer to CreateBrowser() Acknowledged.
https://codereview.chromium.org/964503002/diff/160001/ash/shell/shell_delegat... File ash/shell/shell_delegate_impl.h (right): https://codereview.chromium.org/964503002/diff/160001/ash/shell/shell_delegat... ash/shell/shell_delegate_impl.h:42: bool ForceMaximizeOnFirstRun() const override; Can you follow the same pattern as other methods (IsXxx) ?
PTAL https://codereview.chromium.org/964503002/diff/160001/ash/shell/shell_delegat... File ash/shell/shell_delegate_impl.h (right): https://codereview.chromium.org/964503002/diff/160001/ash/shell/shell_delegat... ash/shell/shell_delegate_impl.h:42: bool ForceMaximizeOnFirstRun() const override; On 2015/04/07 16:52:15, oshima wrote: > Can you follow the same pattern as other methods (IsXxx) ? Done.
ash lgtm
ash lgtm
The CQ bit was checked by peletskyi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, bartfab@chromium.org Link to the patchset: https://codereview.chromium.org/964503002/#ps180001 (title: "Renamed ForceMaximizeOnFirstRun")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/964503002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by peletskyi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, isherman@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/964503002/#ps200001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/964503002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/da0bbb16cc00553245e806aa8c5adc4c38f7a9ed Cr-Commit-Position: refs/heads/master@{#324244} |
