Add webview testAPIMethodExistence to app_shell_browsertests.

extensions/browser/guest_view/web_view/web_view_browsertests.cc

Index: extensions/browser/guest_view/web_view/web_view_browsertests.cc
diff --git a/extensions/browser/guest_view/web_view/web_view_browsertests.cc b/extensions/browser/guest_view/web_view/web_view_browsertests.cc
new file mode 100644
index 0000000000000000000000000000000000000000..ed95297c3fa82abbb81a7c3f3337af341d26bfdc
--- /dev/null
+++ b/extensions/browser/guest_view/web_view/web_view_browsertests.cc
@@ -0,0 +1,155 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.

[Yoyo Zhou, 2014/09/19 00:26:03]

Even if you have multiple tests, this file should be named
web_view_browsertest.cc. But various platforms don't like files with the same
basename. Can you come up with something better (non-plural)?

[lfg, 2014/09/22 17:44:27]

What about web_view_apitest.cc ?

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/path_service.h"
+#include "base/strings/stringprintf.h"
+#include "content/public/test/browser_test_utils.h"
+#include "content/public/test/test_utils.h"
+#include "extensions/browser/app_window/app_window.h"
+#include "extensions/browser/app_window/app_window_registry.h"
+#include "extensions/browser/extension_host.h"
+#include "extensions/browser/guest_view/guest_view_manager.h"
+#include "extensions/browser/guest_view/guest_view_manager_factory.h"
+#include "extensions/browser/process_manager.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/extension_paths.h"
+#include "extensions/shell/browser/shell_extension_system.h"
+#include "extensions/shell/test/shell_test.h"
+#include "extensions/test/extension_test_message_listener.h"
+
+namespace {
+
+class TestGuestViewManager : public extensions::GuestViewManager {

[Yoyo Zhou, 2014/09/19 00:26:03]

This looks very similar to the other web_view_browsertest.cc. Is there a plan to
avoid this sort of redundancy? Add some TODOs here and there if necessary.

[lfg, 2014/09/22 17:44:27]

They are the same. I've added a TODO to merge them together. The idea is that as
soon as app_shell_browsertests is running on all platforms we should be able to
migrate most of the tests out of chrome browsertests.

+ public:
+  explicit TestGuestViewManager(content::BrowserContext* context)
+      : GuestViewManager(context),
+        seen_guest_removed_(false),
+        web_contents_(NULL) {}
+
+  content::WebContents* WaitForGuestCreated() {
+    if (web_contents_)
+      return web_contents_;
+
+    created_message_loop_runner_ = new content::MessageLoopRunner;
+    created_message_loop_runner_->Run();
+    return web_contents_;
+  }
+
+  void WaitForGuestDeleted() {
+    if (seen_guest_removed_)
+      return;
+
+    deleted_message_loop_runner_ = new content::MessageLoopRunner;
+    deleted_message_loop_runner_->Run();
+  }
+
+ private:
+  // GuestViewManager override:
+  virtual void AddGuest(int guest_instance_id,
+                        content::WebContents* guest_web_contents) OVERRIDE {
+    extensions::GuestViewManager::AddGuest(guest_instance_id,
+                                           guest_web_contents);
+    web_contents_ = guest_web_contents;
+    seen_guest_removed_ = false;
+
+    if (created_message_loop_runner_.get())
+      created_message_loop_runner_->Quit();
+  }
+
+  virtual void RemoveGuest(int guest_instance_id) OVERRIDE {
+    extensions::GuestViewManager::RemoveGuest(guest_instance_id);
+    web_contents_ = NULL;
+    seen_guest_removed_ = true;
+
+    if (deleted_message_loop_runner_.get())
+      deleted_message_loop_runner_->Quit();
+  }
+
+  bool seen_guest_removed_;
+  content::WebContents* web_contents_;
+  scoped_refptr<content::MessageLoopRunner> created_message_loop_runner_;
+  scoped_refptr<content::MessageLoopRunner> deleted_message_loop_runner_;
+};
+
+// Test factory for creating test instances of GuestViewManager.
+class TestGuestViewManagerFactory : public extensions::GuestViewManagerFactory {
+ public:
+  TestGuestViewManagerFactory() : test_guest_view_manager_(NULL) {}
+
+  virtual ~TestGuestViewManagerFactory() {}
+
+  virtual extensions::GuestViewManager* CreateGuestViewManager(
+      content::BrowserContext* context) OVERRIDE {
+    return GetManager(context);
+  }
+
+  TestGuestViewManager* GetManager(content::BrowserContext* context) {
+    if (!test_guest_view_manager_) {
+      test_guest_view_manager_ = new TestGuestViewManager(context);
+    }
+    return test_guest_view_manager_;
+  }
+
+ private:
+  TestGuestViewManager* test_guest_view_manager_;

[Yoyo Zhou, 2014/09/19 00:26:04]

scoped_ptr? (How does this get deleted?)

[lfg, 2014/09/22 17:44:27]

Good question, I copied this code from the other file and didn't really think
about it.

So, after some investigation, this function gets called from
GuestViewManager::FromBrowserContext, where it gets assigned to a linked_ptr
that takes care of deleting it on destruction.

It doesn't look like this is safe to return the same pointer more than once, as
the ownership is transferred, so I've added some comments and changed the
function to assert if its called twice.

+
+  DISALLOW_COPY_AND_ASSIGN(TestGuestViewManagerFactory);
+};
+
+}  // namespace
+
+namespace extensions {
+
+// This class intercepts download request from the guest.
+class WebViewTest : public AppShellTest {
+ protected:
+  void TestHelper(const std::string& test_name,

[Yoyo Zhou, 2014/09/19 00:26:03]

This name isn't very descriptive. It could be better, like RunTest.

[lfg, 2014/09/22 17:44:27]

Done.

+                  const std::string& app_location) {
+    std::cout << "Launching test " << test_name << " from " << app_location

[Yoyo Zhou, 2014/09/19 00:26:03]

remove debug comments or change them to VLOG/DVLOG

[lfg, 2014/09/22 17:44:27]

Forgot those. Removed.

+              << std::endl;
+
+    base::FilePath test_data_dir;
+    PathService::Get(extensions::DIR_TEST_DATA, &test_data_dir);
+    test_data_dir = test_data_dir.AppendASCII(app_location.c_str());
+
+    bool loaded = extension_system_->LoadApp(test_data_dir);
+    ASSERT_TRUE(loaded);

[Yoyo Zhou, 2014/09/19 00:26:03]

inline with previous line

[lfg, 2014/09/22 17:44:27]

Done.

+    extension_system_->LaunchApp();
+
+    ExtensionTestMessageListener launch_listener("LAUNCHED", false);
+    ASSERT_TRUE(launch_listener.WaitUntilSatisfied());
+
+    const AppWindowRegistry::AppWindowList& app_window_list =
+        AppWindowRegistry::Get(browser_context_)->app_windows();
+    if (app_window_list.empty()) {
+      LOG(ERROR) << "UNABLE TO FIND EMBEDDER WEB CONTENTS.";

[Yoyo Zhou, 2014/09/19 00:26:04]

All caps doesn't seem necessary.

[lfg, 2014/09/22 17:44:27]

I've changed it to a DCHECK, as per fsamuel@ suggestion.

+      return;
+    }
+    content::WebContents* embedder_web_contents =

[Fady Samuel, 2014/09/18 23:22:14]

Can we add a DCHECK above to verify there is exactly one app window?

[lfg, 2014/09/22 17:44:27]

Done.

+        (*app_window_list.begin())->web_contents();
+
+    ExtensionTestMessageListener done_listener("TEST_PASSED", false);
+    done_listener.set_failure_message("TEST_FAILED");
+    if (!content::ExecuteScript(
+            embedder_web_contents,
+            base::StringPrintf("runTest('%s')", test_name.c_str()))) {
+      LOG(ERROR) << "UNABLE TO START TEST.";
+      return;
+    }
+    ASSERT_TRUE(done_listener.WaitUntilSatisfied());
+  }
+
+  WebViewTest() {
+    extensions::GuestViewManager::set_factory_for_testing(&factory_);
+  }
+
+ private:
+  TestGuestViewManagerFactory factory_;
+};
+
+IN_PROC_BROWSER_TEST_F(WebViewTest, Shim_TestAPIMethodExistence) {

[Yoyo Zhou, 2014/09/19 00:26:03]

Shim looks like it's in a strange place in this name. Why is it here?

[lfg, 2014/09/22 17:44:27]

I originally copied from the chrome browsertests. Removed it now.

+  TestHelper("testAPIMethodExistence", "web_view/shim");
+}
+
+}  // namespace extensions