Implemented ForceMaximizeBrowserWindowOnFirstRun policy, added unit test and browser test.

chrome/browser/chromeos/policy/force_maximize_browser_window_on_first_run_chromeos_browsertest.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <string>
+#include "ash/display/display_manager.h"

[bartfab (slow), 2015/03/12 11:54:45]

Nit: Add blank line above.

[peletskyi, 2015/03/18 13:28:10]

Done.

+#include "ash/test/display_manager_test_api.h"
+#include "ash/wm/window_positioner.h"
+#include "chrome/browser/chromeos/policy/login_policy_base_test.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "ui/base/base_window.h"

[bartfab (slow), 2015/03/12 11:54:45]

Nit: Not used.

[peletskyi, 2015/03/18 13:28:11]

Done.

+
+namespace policy {
+
+class ForceMaximizeBrowserWindowOnFirstRunTest : public LoginPolicyBaseTest {

[bartfab (slow), 2015/03/12 11:54:45]

Nit: How about s/BrowserWindow// for consistency? You can also shorten the file
name a bit to match :).

[peletskyi, 2015/03/18 13:28:11]

Done.

[peletskyi, 2015/03/18 13:28:11]

Done.

+  std::string GetPolicy() const override {

[bartfab (slow), 2015/03/12 11:54:45]

Nit: Add public: or protected:

[peletskyi, 2015/03/18 13:28:11]

Done.

+    return std::string(

[bartfab (slow), 2015/03/12 11:54:45]

Nit: Do you really need the explicit std::string() construction here?

[peletskyi, 2015/03/18 13:28:10]

Done.

+        "{"

[bartfab (slow), 2015/03/12 11:54:45]

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.

[peletskyi, 2015/03/18 13:28:11]

Done.

+        "  \"%s\": {"
+        "    \"mandatory\": {"
+        "      \"ForceMaximizeBrowserWindowOnFirstRun\": true"
+        "    },"
+        "    \"recommended\": {}"
+        "  },"
+        "  \"managed_users\": [ \"*\" ],"
+        "  \"policy_user\": \"%s\","
+        "  \"current_key_index\": 0"
+        "}");
+  }
+};

[bartfab (slow), 2015/03/12 11:54:45]

Nit: Add DISALLOW_COPY_AND_ASSIGN(ForceMaximizeBrowserWindowOnFirstRunTest).

[peletskyi, 2015/03/18 13:28:10]

Done.

+
+IN_PROC_BROWSER_TEST_F(ForceMaximizeBrowserWindowOnFirstRunTest, StartSession) {

[bartfab (slow), 2015/03/12 11:54:45]

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."

[peletskyi, 2015/03/18 13:28:10]

Done.

+  // Set width of the screen wider than limit. By this condition without

[bartfab (slow), 2015/03/12 11:54:45]

1: Nit: What limit?
2: Add a test that verifies when the policy is set to |false|, the window is not
maximized.

[peletskyi, 2015/03/18 13:28:11]

Done.

[peletskyi, 2015/03/18 13:28:11]

Done.

+  // policy ForceMaximizeBrowserWindowOnFirstRun the browser window should
+  // not be maximized.
+  const int kWidth = ash::WindowPositioner::GetForceMaximizedWidthLimit() + 100;

[bartfab (slow), 2015/03/12 11:54:45]

Nit: This is not really a constant if it is calculated based on a method's
return value, hence s/kWidth/width/.

[peletskyi, 2015/03/18 13:28:11]

Done.

+  const std::string kResolution = std::to_string(kWidth) + "x300";  //"1466x300"

[bartfab (slow), 2015/03/12 11:54:45]

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|//|// |

[peletskyi, 2015/03/18 13:28:10]

Done.

+  ash::DisplayManager* display_manager =

[bartfab (slow), 2015/03/12 11:54:45]

Nit 1: const pointer.
Nit 2: #include "ash/display/display_manager.h"

[peletskyi, 2015/03/18 13:28:11]

Done.

+      ash::Shell::GetInstance()->display_manager();

[bartfab (slow), 2015/03/12 11:54:45]

Nit: #include "ash/shell.h"

[peletskyi, 2015/03/18 13:28:11]

Done.

+  ash::test::DisplayManagerTestApi display_manager_test_api(display_manager);
+  display_manager_test_api.UpdateDisplay(kResolution);
+
+  SkipToLoginScreen();
+  LogIn(kAccountId, kAccountPassword);
+
+  // Check that the browser window is maximized

[bartfab (slow), 2015/03/12 11:54:45]

Nit 1: s/the/the first/
Nit 2: Add full stop at the end.

[peletskyi, 2015/03/18 13:28:10]

Done.

+  BrowserList* browser_list =

[bartfab (slow), 2015/03/12 11:54:45]

Nit: const pointer.

[peletskyi, 2015/03/18 13:28:11]

Done.

+      BrowserList::GetInstance(chrome::HOST_DESKTOP_TYPE_ASH);

[bartfab (slow), 2015/03/12 11:54:45]

Nit: #include "chrome/browser/ui/host_desktop.h"

[peletskyi, 2015/03/18 13:28:11]

Done.

+  EXPECT_EQ(1U, browser_list->size());
+  Browser* browser = browser_list->get(0);

[bartfab (slow), 2015/03/12 11:54:45]

Nit: const pointer.

[peletskyi, 2015/03/18 13:28:10]

Done.

+  ASSERT_TRUE(browser);
+  EXPECT_TRUE(browser->window()->IsMaximized());
+}
+

[bartfab (slow), 2015/03/12 11:54:45]

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.

[peletskyi, 2015/03/18 13:28:10]

Done.

[peletskyi, 2015/03/18 13:28:10]

Done.

+}  // namespace policy

[bartfab (slow), 2015/03/12 11:54:45]

Nit: Add blank line below.

[peletskyi, 2015/03/18 13:28:11]

Done.