Implement browser test for It2Me

chrome/test/remoting/it2me_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 "chrome/test/remoting/remote_desktop_browsertest.h"
+
+namespace remoting {
+
+class It2MeBrowserTest : public RemoteDesktopBrowserTest {
+ protected:
+  void SetUpTestForIt2Me();
+  std::string GetAccessCode(content::WebContents* contents);
+};
+
+void It2MeBrowserTest::SetUpTestForIt2Me() {
+  VerifyInternetAccess();
+  Install();
+  LaunchChromotingApp(false);
+  Auth();
+  LoadScript(app_web_content(), FILE_PATH_LITERAL("browser_test.js"));

[Jamie, 2015/01/06 22:18:51]

This is basically the same code as RemoteDesktopBrowserTest::SetUpTestForMe2Me.
I think you could rename that method, make it expand both sections (not just the
Me2Me section) and use it for IT2Me tests as well. If there's a reason why we
need separate methods, then they should at least be implemented by the same
class.

[kelvinp, 2015/01/09 22:21:30]

Removed.

+}
+
+std::string It2MeBrowserTest::GetAccessCode(content::WebContents* contents) {
+  base::Value* return_value = nullptr;
+  RunJavaScriptTest(contents, "GET_ACCESS_CODE", "{}", &return_value);

[Jamie, 2015/01/06 22:18:51]

GET_ACCESS_CODE is not a test in the same sense that CONNECT_IT2ME is, so it
seems strange to use RunJavaScriptTest to run it. Is there anything about
RunScriptAndExtractString that prevents you using that here?

It's also dependent on it2me_browser_test.js being loaded, which is not ideal in
a helper function. SetUpTestForIt2Me method should probable load the file that
defines it (whether that be it2me_browser_test.js or another file dedicated to
helper functions).

[kelvinp, 2015/01/09 22:21:29]

Discussed offline

+  scoped_ptr<base::Value> owned_value(return_value);
+  std::string access_code;
+  owned_value->GetAsString(&access_code);
+  return access_code;
+}
+
+IN_PROC_BROWSER_TEST_F(It2MeBrowserTest, MANUAL_Connect) {
+  SetUpTestForIt2Me();
+
+  content::WebContents* helpee_content = app_web_content();
+  LoadScript(helpee_content, FILE_PATH_LITERAL("it2me_browser_test.js"));
+
+  LaunchChromotingApp(false /* defer_start*/, NEW_FOREGROUND_TAB);

[Jamie, 2015/01/06 22:18:51]

Does NEW_FOREGROUND_TAB work for apps v2?

[Jamie, 2015/01/06 22:18:51]

We haven't documented the |false| parameter anywhere else, so I don't think it's
necessary here. If you want to keep it, you should add a space before */ for
consistency with other comments.

[kelvinp, 2015/01/09 22:21:29]

Good point.  Comment added and modified the code to always use NEW_WINDOW for v2
app.

[kelvinp, 2015/01/09 22:21:30]

Done.

+  content::WebContents* helper_content = active_web_contents();

[Jamie, 2015/01/06 22:18:51]

Would it be possible to return the web contents from LaunchChromotingApp? I
think this code would be clearer if it looked like:

  helper_content = LaunchChromotingApp(...);
  LoadScript(helper_content,...);

But I don't know if that's feasible given the current implementation.

There's also duplication here between SetUpTestFor* and the code that sets up
the new tab (both start the app and load the same JS files). Ideally, we'd be
able to share that code.

[kelvinp, 2015/01/09 22:21:30]

Done.

+  LoadScript(helper_content, FILE_PATH_LITERAL("browser_test.js"));
+  LoadScript(helper_content, FILE_PATH_LITERAL("it2me_browser_test.js"));
+
+  RunJavaScriptTest(helper_content, "CONNECT_IT2ME", "{"
+    "accessCode: '" + GetAccessCode(helpee_content) + "'"
+  "}");
+
+  Cleanup();
+}

[Jamie, 2015/01/06 22:18:51]

Can you also add a test to make sure that trying to connect with an invalid
access code doesn't work?

[kelvinp, 2015/01/09 22:21:30]

Done.

+
+}  // namespace remoting