Improve <adview> implementation and add tests.

chrome/browser/extensions/ad_view_browsertest.cc

Index: chrome/browser/extensions/ad_view_browsertest.cc
diff --git a/chrome/browser/extensions/ad_view_browsertest.cc b/chrome/browser/extensions/ad_view_browsertest.cc
index 4a70524d7600e530925692b884fdcae56a545410..e5430e89393ee343f3ff799495beb702c4115900 100644
--- a/chrome/browser/extensions/ad_view_browsertest.cc
+++ b/chrome/browser/extensions/ad_view_browsertest.cc
@@ -5,6 +5,9 @@
 #include "chrome/browser/extensions/extension_test_message_listener.h"
 #include "chrome/browser/extensions/platform_app_browsertest_util.h"
 #include "chrome/common/chrome_switches.h"
+#include "content/test/net/url_request_prepackaged_interceptor.h"
+#include "net/url_request/url_fetcher.h"
+
 
 class AdViewTest : public extensions::PlatformAppBrowserTest {
  protected:
@@ -15,28 +18,130 @@ class AdViewTest : public extensions::PlatformAppBrowserTest {
   }
 };
 
-IN_PROC_BROWSER_TEST_F(AdViewTest, LoadEventIsCalled) {
+IN_PROC_BROWSER_TEST_F(AdViewTest, LoadCommitEventIsCalled) {
   ASSERT_TRUE(StartTestServer());
 
   ExtensionTestMessageListener listener("guest-loaded", false);
-  LoadAndLaunchPlatformApp("ad_view/load_event");
+  LoadAndLaunchPlatformApp("ad_view/loadcommit_event");
   ASSERT_TRUE(listener.WaitUntilSatisfied());
 }
 
-IN_PROC_BROWSER_TEST_F(AdViewTest, AdNetworkIsLoaded) {
+IN_PROC_BROWSER_TEST_F(AdViewTest, LoadAbortEventIsCalled) {
   ASSERT_TRUE(StartTestServer());
 
-  ExtensionTestMessageListener listener("ad-network-loaded", false);
-  LoadAndLaunchPlatformApp("ad_view/ad_network_loaded");
+  ExtensionTestMessageListener listener("loadabort-received", false);
+  LoadAndLaunchPlatformApp("ad_view/loadabort_event");
+  ASSERT_TRUE(listener.WaitUntilSatisfied());
+}
+
+IN_PROC_BROWSER_TEST_F(AdViewTest, CommitMessageFromAdNetwork) {
+  ASSERT_TRUE(StartTestServer());
+
+  ExtensionTestMessageListener listener("onloadcommit-ack-message", false);
+  LoadAndLaunchPlatformApp("ad_view/onloadcommit_ack");
   ASSERT_TRUE(listener.WaitUntilSatisfied());
 }
 
 // This test currently fails on trybots because it requires new binary file
 // (image315.png). The test will be enabled once the binary files are committed.

[benwells, 2013/03/27 21:43:05]

Is this comment out of date now?

[rpaquay, 2013/03/27 22:57:25]

Yes, I removed it. Note that I actually expect this CL to fail on the trybots
because it seems binary files (image315.png in this case) are not processed
correctly during initial commit. So, I will probably have to re-add the
"DISABLED_" prefix and the comment above, and the submit another CL to enable it
back...

-IN_PROC_BROWSER_TEST_F(AdViewTest, DISABLED_DisplayFirstAd) {
+IN_PROC_BROWSER_TEST_F(AdViewTest, DisplayFirstAd) {
   ASSERT_TRUE(StartTestServer());
 
   ExtensionTestMessageListener listener("ad-displayed", false);
   LoadAndLaunchPlatformApp("ad_view/display_first_ad");
   ASSERT_TRUE(listener.WaitUntilSatisfied());
 }
+
+IN_PROC_BROWSER_TEST_F(AdViewTest, PropertiesAreInSyncWithAttributes) {
+  ASSERT_TRUE(StartTestServer());
+
+  ExtensionTestMessageListener listener("test-finished", false);
+  LoadAndLaunchPlatformApp("ad_view/properties_exposed");
+  ASSERT_TRUE(listener.WaitUntilSatisfied());
+}
+
+IN_PROC_BROWSER_TEST_F(AdViewTest, AdViewPermissionIsRequired) {
+  ASSERT_TRUE(StartTestServer());
+
+  ExtensionTestMessageListener listener("test-finished", false);
+  LoadAndLaunchPlatformApp("ad_view/permission_required");
+  ASSERT_TRUE(listener.WaitUntilSatisfied());
+}
+
+IN_PROC_BROWSER_TEST_F(AdViewTest, ChangeAdNetworkValue) {
+  ASSERT_TRUE(StartTestServer());
+
+  ExtensionTestMessageListener listener("test-finished", false);
+  LoadAndLaunchPlatformApp("ad_view/change_ad_network");
+  ASSERT_TRUE(listener.WaitUntilSatisfied());
+}
+
+
+class AdViewNoSrcTest : public extensions::PlatformAppBrowserTest {
+ protected:
+  virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
+    extensions::PlatformAppBrowserTest::SetUpCommandLine(command_line);
+    command_line->AppendSwitch(switches::kEnableAdview);
+    //Note: The "kEnableAdviewSrcAttribute" flag is not here!
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(AdViewNoSrcTest, InvalidAdNetworkIsIgnored) {
+  // The point of this test is to ensure an invalid (i.e. not whitelisted)

[benwells, 2013/03/27 21:43:05]

These comments are helpful. Could you add them to the other tests as well?

[rpaquay, 2013/03/27 22:57:25]

Done.

+  // value of "ad-network" attribute is ignored.
+  ASSERT_TRUE(StartTestServer());
+
+  ExtensionTestMessageListener listener("test-finished", false);
+  LoadAndLaunchPlatformApp("ad_view/invalid_ad_network");
+  ASSERT_TRUE(listener.WaitUntilSatisfied());
+}
+
+IN_PROC_BROWSER_TEST_F(AdViewNoSrcTest, EnableAdviewSrcAttributeFlagRequired) {
+  // The point of this test is to ensure there are no HTTP request made
+  // to a non-whitelisted ad-network if the "src" attribute is set
+  // on an <adview> element.
+  ASSERT_TRUE(StartTestServer());
+
+  ExtensionTestMessageListener listener("test-finished", false);
+  LoadAndLaunchPlatformApp("ad_view/src_flag_required");
+  ASSERT_TRUE(listener.WaitUntilSatisfied());
+}
+
+IN_PROC_BROWSER_TEST_F(AdViewNoSrcTest, SrcNotExposed) {
+  base::FilePath file_path = test_data_dir_
+    .AppendASCII("platform_apps")
+    .AppendASCII("ad_view/src_not_exposed")
+    .AppendASCII("ad_network_fake_website.html");
+
+  // Note: The following URL is identical to the whitelisted url
+  //       for "admob" (see ad_view.js).
+  GURL url = GURL("https://admob-sdk.doubleclick.net/chromeapps");
+  std::string scheme = url.scheme();
+  std::string hostname = url.host();
+
+  content::URLRequestPrepackagedInterceptor interceptor(scheme, hostname);

[benwells, 2013/03/27 21:43:05]

Why does this test use an interceptor and the others a test server?

[rpaquay, 2013/03/27 22:57:25]

Because I thought it would be good to have a test verifying the behavior
end-2-end when using an ad-network from the whitelist (i.e. *not* using the
special test flag). Since the URL specified in the whitelist is arbitrary, I
needed to use the interceptor mechanism as opposed to the local http server.

[benwells, 2013/03/28 00:16:48]

That makes sense. I'm wondering if using the interceptor is preferable for all
tests, would it be less flaky do you think?

+  interceptor.SetResponse(url, file_path);
+
+  ExtensionTestMessageListener listener("test-finished", false);
+  LoadAndLaunchPlatformApp("ad_view/src_not_exposed");
+  ASSERT_TRUE(listener.WaitUntilSatisfied());
+  ASSERT_EQ(1, interceptor.GetHitCount());
+}
+
+class AdViewNotEnabledTest : public extensions::PlatformAppBrowserTest {
+ protected:
+  virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
+    extensions::PlatformAppBrowserTest::SetUpCommandLine(command_line);
+    //Note: The "kEnableAdview" flag is not here!
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(AdViewNotEnabledTest, EnableAdviewFlagRequired) {
+  // The point of this test is to ensure the <adview> tag does not do anything
+  // when the "kEnableAdview" flag is not set.
+  ASSERT_TRUE(StartTestServer());
+
+  ExtensionTestMessageListener listener("test-finished", false);
+  LoadAndLaunchPlatformApp("ad_view/flag_required");
+  ASSERT_TRUE(listener.WaitUntilSatisfied());
+}