Add an extension override bubble and warning box for proxy extensions.

chrome/browser/extensions/extension_message_bubble_controller_unittest.cc

Index: chrome/browser/extensions/extension_message_bubble_controller_unittest.cc
diff --git a/chrome/browser/extensions/extension_message_bubble_controller_unittest.cc b/chrome/browser/extensions/extension_message_bubble_controller_unittest.cc
index 77181b7ab6ae5de98ab1ae8bdb537418f2dd3981..bc9f8ee36ebe296d48734ddd16f90e4384a9b70c 100644
--- a/chrome/browser/extensions/extension_message_bubble_controller_unittest.cc
+++ b/chrome/browser/extensions/extension_message_bubble_controller_unittest.cc
@@ -10,6 +10,7 @@
 #include "chrome/browser/extensions/extension_message_bubble.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/ntp_overridden_bubble_controller.h"
+#include "chrome/browser/extensions/proxy_overridden_bubble_controller.h"
 #include "chrome/browser/extensions/settings_api_bubble_controller.h"
 #include "chrome/browser/extensions/suspicious_extension_bubble_controller.h"
 #include "chrome/browser/extensions/test_extension_system.h"
@@ -158,6 +159,31 @@ class TestNtpOverriddenBubbleController
   }
 };
 
+// A test class for the ProxyOverriddenBubbleController.
+class TestProxyOverriddenBubbleController
+    : public ProxyOverriddenBubbleController,
+      public TestDelegate {
+ public:
+  explicit TestProxyOverriddenBubbleController(Profile* profile)
+      : ProxyOverriddenBubbleController(profile) {
+  }
+
+  virtual void OnBubbleAction() OVERRIDE {
+    ++action_button_callback_count_;
+    ProxyOverriddenBubbleController::OnBubbleAction();

[Devlin, 2014/05/20 17:26:31]

So much of this code is duplicated, but I can't think of any better way off the
top of my head.  I'll think about it for a bit.

[Finnur, 2014/05/21 16:30:32]

The only way I could think of was to move these counts into the parent class and
add accessors. But unfortunately, that would add code to Chrome that is only
useful for testing, so it seemed a bit wasteful...

+  }
+
+  virtual void OnBubbleDismiss() OVERRIDE {
+    ++dismiss_button_callback_count_;
+    ProxyOverriddenBubbleController::OnBubbleDismiss();
+  }
+
+  virtual void OnLinkClicked() OVERRIDE {
+    ++link_click_callback_count_;
+    ProxyOverriddenBubbleController::OnLinkClicked();
+  }
+};
+
 // A fake bubble used for testing the controller. Takes an action that specifies
 // what should happen when the bubble is "shown" (the bubble is actually not
 // shown, the corresponding action is taken immediately).
@@ -288,6 +314,22 @@ class ExtensionMessageBubbleTest : public testing::Test {
     service_->AddExtension(builder.Build().get());
   }
 
+  void LoadExtensionOverridingProxy(const std::string& index,
+                                    const std::string& id,
+                                    Manifest::Location location) {
+    extensions::ExtensionBuilder builder;

[Devlin, 2014/05/20 17:26:31]

no extensions:: prefix.

[Finnur, 2014/05/21 16:30:32]

Indeed.

+    builder.SetManifest(extensions::DictionaryBuilder()
+                            .Set("name", std::string("Extension " + index))
+                            .Set("version", "1.0")
+                            .Set("manifest_version", 2)
+                            .Set("permissions",
+                                 extensions::ListBuilder().Append("proxy")));
+
+    builder.SetLocation(location);
+    builder.SetID(id);
+    service_->AddExtension(builder.Build().get());

[Devlin, 2014/05/20 17:26:31]

We'll get a ton of fun errors if these extensions just fail to install.  How
about:

testing::AssertionResult LoadExtensionOverridingProxy() {
  ...
  if (!ExtensionRegistry::Get(profile())->enabled_extensions().GetByID(id))
    return testing::AssertionFailure() << "Could not install extension: " << id;
  return testing::AssertionSuccess();
}

TEST_F(...) {
  ...
  ASSERT_TRUE(LoadExtensionOverridingProxy("1", kId1, Manifest::UNPACKED));
  ASSERT_TRUE(LoadExtensionOverridingProxy("2", kId2, Manifest::UNPACKED));
  ASSERT_TRUE(LoadExtensionOverridingProxy("3", kId3, Manifest::UNPACKED));
  ...
}

[Finnur, 2014/05/21 16:30:32]

Done.

+  }
+
   void Init() {
     // The two lines of magical incantation required to get the extension
     // service to work inside a unit test and access the extension prefs.
@@ -723,4 +765,102 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_NtpOverriddenControllerTest) {
   service_->UninstallExtension(kId3, false, NULL);
 }
 
+// The feature this is meant to test is only implemented on Windows.
+#if defined(OS_WIN)
+#define MAYBE_ProxyOverriddenControllerTest ProxyOverriddenControllerTest
+#else
+#define MAYBE_ProxyOverriddenControllerTest DISABLED_ProxyOverriddenControllerTest
+#endif
+
+TEST_F(ExtensionMessageBubbleTest, MAYBE_ProxyOverriddenControllerTest) {
+  Init();
+  extensions::ExtensionPrefs* prefs =

[Devlin, 2014/05/20 17:26:31]

No extensions:: prefix.

[Finnur, 2014/05/21 16:30:32]

Done.

+      extensions::ExtensionPrefs::Get(profile());
+  // Load two extensions overriding proxy and one overriding something

[Devlin, 2014/05/20 17:26:31]

It looks like all three are overriding proxy - am I missing something?

[Finnur, 2014/05/21 16:30:32]

Yes, the last one is overriding the StartPage.

[Devlin, 2014/05/21 17:00:39]

D'oh.  Long function names + monospace font + same number of characters = hard
to see the difference.  My bad.

+  // unrelated (to check for interference). Extension 2 should still win
+  // on the proxy setting.
+  LoadExtensionOverridingProxy("1", kId1, Manifest::UNPACKED);

[Devlin, 2014/05/20 17:26:31]

nit: Why pass in location if it's always unpacked?

[Finnur, 2014/05/21 16:30:32]

Readability (location not buried inside the function itself) and consistency
with other LoadExtension* functions (for some the location matters a great
deal).

[Devlin, 2014/05/21 17:00:39]

Fair enough. :)

+  LoadExtensionOverridingProxy("2", kId2, Manifest::UNPACKED);
+  LoadExtensionOverridingStart("3", kId3, Manifest::UNPACKED);
+
+  scoped_ptr<TestProxyOverriddenBubbleController> controller(
+      new TestProxyOverriddenBubbleController(profile()));
+
+  // The list will contain one enabled unpacked extension (ext 2).
+  EXPECT_TRUE(controller->ShouldShow(kId2));

[Devlin, 2014/05/20 17:26:31]

To be safe, maybe an EXPECT_FALSE(controller->ShouldShow(<non-overriding
extension id>))

[Finnur, 2014/05/21 16:30:32]

Done.

+  std::vector<base::string16> override_extensions =
+      controller->GetExtensionList();
+  ASSERT_EQ(1U, override_extensions.size());
+  EXPECT_TRUE(base::ASCIIToUTF16("Extension 2") ==

[Devlin, 2014/05/20 17:26:31]

Why not
EXPECT_EQ(base::ASCIIToUTF16("Extension 2"), override_extensions[0]);
?

It seems like converting to a base::string16, then comparing against the c_str()
is very odd.

[Finnur, 2014/05/21 16:30:32]

I got failures on other platforms when I tried that initially, but I just tested
on Linux and it seems to work. Let me change this instance and if it works I'll
fix the others.

[Devlin, 2014/05/21 17:00:39]

If there are compiler errors, feel free to change it back, but please add a
comment for me one month from now. :)

+              override_extensions[0].c_str());
+  EXPECT_EQ(0U, controller->link_click_count());
+  EXPECT_EQ(0U, controller->dismiss_click_count());
+  EXPECT_EQ(0U, controller->action_click_count());
+
+  // Simulate showing the bubble and dismissing it.
+  FakeExtensionMessageBubble bubble;
+  bubble.set_action_on_show(
+      FakeExtensionMessageBubble::BUBBLE_ACTION_CLICK_DISMISS_BUTTON);
+  EXPECT_TRUE(controller->ShouldShow(kId2));

[Devlin, 2014/05/20 17:26:31]

This was already checked on line 790, right?

[Finnur, 2014/05/21 16:30:32]

Done.

+  controller->Show(&bubble);
+  EXPECT_EQ(0U, controller->link_click_count());
+  EXPECT_EQ(0U, controller->action_click_count());
+  EXPECT_EQ(1U, controller->dismiss_click_count());
+  // No extension should have become disabled.
+  EXPECT_TRUE(service_->GetExtensionById(kId1, false) != NULL);

[Devlin, 2014/05/20 17:26:31]

Please use ExtensionRegistry::enabled_extensions().GetByID() (we're making an
effort to deprecate ExtensionService::GetExtensionById()).  Also, I think
Registry makes it more clear that we're only checking enabled extensions.

[Finnur, 2014/05/21 16:30:32]

Done.

+  EXPECT_TRUE(service_->GetExtensionById(kId2, false) != NULL);
+  EXPECT_TRUE(service_->GetExtensionById(kId3, false) != NULL);
+  // Only extension 2 should have been acknowledged.
+  EXPECT_FALSE(prefs->HasProxyOverriddenBubbleBeenAcknowledged(kId1));
+  EXPECT_TRUE(prefs->HasProxyOverriddenBubbleBeenAcknowledged(kId2));
+  EXPECT_FALSE(prefs->HasProxyOverriddenBubbleBeenAcknowledged(kId3));
+  // Clean up after ourselves.
+  prefs->SetProxyOverriddenBubbleBeenAcknowledged(kId2, false);
+
+  // Simulate clicking the learn more link to dismiss it.
+  bubble.set_action_on_show(
+      FakeExtensionMessageBubble::BUBBLE_ACTION_CLICK_LINK);
+  controller.reset(new TestProxyOverriddenBubbleController(profile()));
+  EXPECT_TRUE(controller->ShouldShow(kId2));
+  controller->Show(&bubble);
+  EXPECT_EQ(1U, controller->link_click_count());
+  EXPECT_EQ(0U, controller->action_click_count());
+  EXPECT_EQ(0U, controller->dismiss_click_count());
+  // No extension should have become disabled.
+  EXPECT_TRUE(service_->GetExtensionById(kId1, false) != NULL);
+  EXPECT_TRUE(service_->GetExtensionById(kId2, false) != NULL);
+  EXPECT_TRUE(service_->GetExtensionById(kId3, false) != NULL);
+  // Only extension 2 should have been acknowledged.
+  EXPECT_FALSE(prefs->HasProxyOverriddenBubbleBeenAcknowledged(kId1));
+  EXPECT_TRUE(prefs->HasProxyOverriddenBubbleBeenAcknowledged(kId2));
+  EXPECT_FALSE(prefs->HasProxyOverriddenBubbleBeenAcknowledged(kId3));
+  // Clean up after ourselves.
+  prefs->SetProxyOverriddenBubbleBeenAcknowledged(kId2, false);
+
+  // Do it again, but now opt to disable the extension.
+  bubble.set_action_on_show(
+      FakeExtensionMessageBubble::BUBBLE_ACTION_CLICK_ACTION_BUTTON);
+  controller.reset(new TestProxyOverriddenBubbleController(profile()));
+  EXPECT_TRUE(controller->ShouldShow(kId2));
+  override_extensions = controller->GetExtensionList();
+  EXPECT_EQ(1U, override_extensions.size());
+  controller->Show(&bubble);  // Simulate showing the bubble.
+  EXPECT_EQ(0U, controller->link_click_count());
+  EXPECT_EQ(1U, controller->action_click_count());
+  EXPECT_EQ(0U, controller->dismiss_click_count());
+  // Only extension 2 should have become disabled.
+  EXPECT_TRUE(service_->GetExtensionById(kId1, false) != NULL);
+  EXPECT_TRUE(service_->GetExtensionById(kId2, false) == NULL);

[Devlin, 2014/05/20 17:26:31]

ExtensionRegistry will help here, too, because it'd be good to check that it
_only_ got disabled (not uninstalled).

[Finnur, 2014/05/21 16:30:32]

Done.

+  EXPECT_TRUE(service_->GetExtensionById(kId3, false) != NULL);
+  // No extension should have been acknowledged (it got disabled).
+  EXPECT_FALSE(prefs->HasProxyOverriddenBubbleBeenAcknowledged(kId1));
+  EXPECT_FALSE(prefs->HasProxyOverriddenBubbleBeenAcknowledged(kId2));
+  EXPECT_FALSE(prefs->HasProxyOverriddenBubbleBeenAcknowledged(kId3));
+
+  // Clean up after ourselves.
+  service_->UninstallExtension(kId1, false, NULL);
+  service_->UninstallExtension(kId2, false, NULL);
+  service_->UninstallExtension(kId3, false, NULL);
+}
+
 }  // namespace extensions