Implemented ForceMaximizeBrowserWindowOnFirstRun policy, added unit test and browser test.

chrome/browser/chromeos/policy/login_policy_base_test.h

+// 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.
+
+#ifndef CHROME_BROWSER_CHROMEOS_POLICY_LOGIN_POLICY_BASE_TEST_H_
+#define CHROME_BROWSER_CHROMEOS_POLICY_LOGIN_POLICY_BASE_TEST_H_
+
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "chrome/browser/chromeos/login/test/oobe_base_test.h"
+#include "chrome/browser/policy/test/local_policy_test_server.h"

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

Nit: A forward-declaration is sufficient here. Move the include to the
implementation file instead.

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

Done.

+
+namespace policy {
+
+class LoginPolicyBaseTest : public chromeos::OobeBaseTest {

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

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.

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

Renamed. But why we have OobeBaseTest, not OobeTestBase?

[bartfab (slow), 2015/04/01 14:22:41]

Yeah, that one is definitely poorly named :(.

+ protected:
+  LoginPolicyBaseTest();
+  ~LoginPolicyBaseTest() override;

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

Nit: No need to override the destructor.

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

It will not be compiled without override.

[bartfab (slow), 2015/04/01 14:22:41]

I mean no need to add a destructor at all. You are inheriting one already.

[peletskyi, 2015/04/01 18:55:45]

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.

[bartfab (slow), 2015/04/02 08:44:27]

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.

+
+  void SetUp() override;

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

Nit: Add // chromeos::OobeBaseTest:

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

Done.

+  void SetUpCommandLine(base::CommandLine* command_line) override;
+  void SetUpOnMainThread() override;
+  void SetupGaiaServerWithAccessTokens();

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

Nit 1: Add blank line above.
Nit 2: s/Setup/SetUp/
Nit 3: Make this private.

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

Done.

+  void SetMergeSessionParams(const std::string& email);

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

Nit 1: #include <string>
Nit 2: Make this private.

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

Done.

+  void SkipToLoginScreen();
+  void LogIn(const std::string& user_id, const std::string& password);
+  void SetServerPolicy();

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

Nit: Make this private.

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

Done.

+  virtual std::string GetPolicy() const = 0;
+
+  base::FilePath policy_file_path() const;

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

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.

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

(1,2) done.
(3) It is used twice :). 

+  scoped_ptr<LocalPolicyTestServer> test_server_;

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

Nit: Make this private if it is not used by any derived classes.

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

Done.

+
+  base::ScopedTempDir temp_dir_;

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

Nit 1: Make this private if it is not used by any derived classes.
Nit 2: #include "base/files/scoped_temp_dir.h"

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

Done.

+
+  static const char kAccountPassword[];

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

Nit: There is no need for the base class to define a dummy password as the base
class never actually refers to it.

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

Yes, but it is needed in both derived classes and thus can be placed here to
avoid code duplication.

+  static const char kAccountId[];

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

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.

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

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.

[bartfab (slow), 2015/04/01 14:22:41]

Acknowledged.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(LoginPolicyBaseTest);
+};
+
+}  // namespace policy
+#endif  // CHROME_BROWSER_CHROMEOS_POLICY_LOGIN_POLICY_BASE_TEST_H_