Improve settings override bubble to indicate policy installed extensions

chrome/browser/extensions/extension_message_bubble_bridge_unittest.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.

[Devlin, 2016/09/26 20:13:09]

test files should (normally) be near the files they test.  So this should go in
chrome/browser/ui/extensions/, near where the extension_message_bubble_bridge
file is.

[catmullings, 2016/10/06 18:24:19]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/command_line.h"
+#include "base/logging.h"
+#include "base/path_service.h"
+#include "base/values.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_service_test_with_install.h"
+#include "chrome/browser/extensions/extension_web_ui_override_registrar.h"
+#include "chrome/browser/extensions/ntp_overridden_bubble_delegate.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/extensions/extension_message_bubble_bridge.h"
+#include "chrome/browser/ui/toolbar/test_toolbar_actions_bar_bubble_delegate.h"
+#include "chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h"
+#include "chrome/browser/ui/toolbar/toolbar_actions_model.h"
+#include "chrome/browser/ui/toolbar/toolbar_actions_model_factory.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/grit/generated_resources.h"
+#include "chrome/test/base/test_browser_window.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/grit/components_scaled_resources.h"
+#include "components/keyed_service/core/keyed_service.h"
+#include "components/strings/grit/components_strings.h"
+#include "content/public/browser/browser_context.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extension_system.h"
+#include "extensions/common/extension_builder.h"
+#include "extensions/common/manifest.h"
+#include "extensions/common/value_builder.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/base/l10n/l10n_util.h"
+
+namespace {
+
+std::unique_ptr<KeyedService> BuildOverrideRegistrar(
+    content::BrowserContext* context) {
+  return base::MakeUnique<extensions::ExtensionWebUIOverrideRegistrar>(context);
+}
+
+std::unique_ptr<KeyedService> BuildToolbarModel(
+    content::BrowserContext* context) {
+  return base::MakeUnique<ToolbarActionsModel>(
+      Profile::FromBrowserContext(context),
+      extensions::ExtensionPrefs::Get(context));
+}
+
+}  // namespace
+
+namespace extensions {

[Devlin, 2016/09/26 20:13:09]

nit: \n

[catmullings, 2016/10/06 18:24:19]

Done.

+class ExtensionMessageBubbleBridgeUnitTest
+    : public ExtensionServiceTestWithInstall {
+ public:
+  ExtensionMessageBubbleBridgeUnitTest() {}
+  ~ExtensionMessageBubbleBridgeUnitTest() override {}
+  Browser* browser() { return browser_.get(); }
+
+  // test_data/extensions.
+  base::FilePath test_data_dir_;

[Devlin, 2016/09/26 20:13:09]

Why do we need this instead of ExtensionServiceTestBase::data_dir_?

[catmullings, 2016/10/06 18:24:19]

Cool. Didn't know that ExtensionServiceTestBase::data_dir_ referred to the test
data dir.

Done.

+
+ private:
+  void SetUp() override {
+    ExtensionServiceTestWithInstall::SetUp();
+    InitializeEmptyExtensionService();
+
+    browser_window_.reset(new TestBrowserWindow());
+    Browser::CreateParams params(profile());
+    params.type = Browser::TYPE_TABBED;
+    params.window = browser_window_.get();
+    browser_.reset(new Browser(params));
+
+    extensions::ExtensionWebUIOverrideRegistrar::GetFactoryInstance()

[Devlin, 2016/09/26 20:13:09]

no need for extensions:: prefix

[catmullings, 2016/10/06 18:24:19]

How do you know that the extensions:: prefix can be dropped?

More generally, how does one know when or when not to include it? I see the
extensions:: prefix in many places where the "namespace extensions {}" is used.

[Devlin, 2016/10/10 15:27:28]

As a rule of thumb, you don't need to use the extensions:: prefix any time you
are in the namespace extensions {} block.  Code that does is often because the
namespace was added afterwards and no one updated the innards.  There are a few
exceptions to this if you have conflicting types in different namespaces (you
can often see this when code specifies "::switches", e.g.), but that's pretty
rare.

[catmullings, 2016/10/12 18:58:58]

My guess would be that ExtensionWebUIOverrideRegistrar::GetFactoryInstance()
wouldn't clash with anything in this file under the extensions namespace. 

[catmullings, 2016/10/12 18:58:58]

Done.

+        ->SetTestingFactory(browser()->profile(), &BuildOverrideRegistrar);
+    extensions::ExtensionWebUIOverrideRegistrar::GetFactoryInstance()->Get(
+        browser()->profile());
+    ToolbarActionsModelFactory::GetInstance()->SetTestingFactory(
+        browser()->profile(), &BuildToolbarModel);
+
+    PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir_);
+    test_data_dir_ = test_data_dir_.AppendASCII("extensions");
+  }
+
+  void TearDown() override {
+    browser_.reset();
+    browser_window_.reset();
+    ExtensionServiceTestWithInstall::TearDown();
+  }
+
+  std::unique_ptr<TestBrowserWindow> browser_window_;
+  std::unique_ptr<Browser> browser_;
+
+  DISALLOW_COPY_AND_ASSIGN(ExtensionMessageBubbleBridgeUnitTest);
+};
+
+TEST_F(ExtensionMessageBubbleBridgeUnitTest,
+       TestGetExtraViewInfoMethodWithNormalSettingsOverrideExtension) {
+  base::FilePath fp(test_data_dir_.AppendASCII("api_test/override/newtab/"));
+  EXPECT_NE(nullptr, PackAndInstallCRX(fp, INSTALL_NEW));
+
+  std::unique_ptr<ExtensionMessageBubbleController> ntp_bubble_controller(
+      new ExtensionMessageBubbleController(
+          new NtpOverriddenBubbleDelegate(browser()->profile()), browser()));
+
+  ASSERT_EQ(1U, ntp_bubble_controller->GetExtensionList().size());
+
+  std::unique_ptr<ToolbarActionsBarBubbleDelegate> bridge(
+      new ExtensionMessageBubbleBridge(std::move(ntp_bubble_controller)));
+
+  std::unique_ptr<ToolbarActionsBarBubbleDelegate::ExtraViewInfo>
+      extra_view_info = bridge->GetExtraViewInfo();
+
+  ASSERT_EQ(-1, extra_view_info->resource_id);

[Devlin, 2016/09/26 20:13:09]

prefer EXPECT_EQ in cases like this where a failure wouldn't invalidate the rest
of the test or cause a crash.

[catmullings, 2016/10/06 18:24:19]

Done.

+  ASSERT_EQ(l10n_util::GetStringUTF16(IDS_LEARN_MORE), extra_view_info->text);
+  ASSERT_EQ(true, extra_view_info->is_text_linked);
+}
+
+TEST_F(ExtensionMessageBubbleBridgeUnitTest,
+       TestGetExtraViewInfoMethodWithPolicyInstalledSettingsOverrideExtension) {
+  base::FilePath fp(test_data_dir_.AppendASCII("api_test/override/newtab/"));
+  EXPECT_NE(nullptr, PackAndInstallCRXWithLocation(
+                         fp, Manifest::EXTERNAL_POLICY, INSTALL_NEW));
+
+  std::unique_ptr<ExtensionMessageBubbleController> ntp_bubble_controller(
+      new ExtensionMessageBubbleController(
+          new NtpOverriddenBubbleDelegate(browser()->profile()), browser()));
+
+  ASSERT_EQ(1U, ntp_bubble_controller->GetExtensionList().size());
+
+  std::unique_ptr<ToolbarActionsBarBubbleDelegate> bridge(
+      new ExtensionMessageBubbleBridge(std::move(ntp_bubble_controller)));
+
+  std::unique_ptr<ToolbarActionsBarBubbleDelegate::ExtraViewInfo>
+      extra_view_info = bridge->GetExtraViewInfo();
+
+  extra_view_info = bridge->GetExtraViewInfo();
+
+  ASSERT_EQ(IDR_OMNIBOX_HTTPS_POLICY_WARNING, extra_view_info->resource_id);
+  ASSERT_EQ(l10n_util::GetStringUTF16(IDS_EXTENSIONS_INSTALLED_BY_ADMIN),
+            extra_view_info->text);
+  ASSERT_EQ(false, extra_view_info->is_text_linked);
+}
+
+TEST_F(ExtensionMessageBubbleBridgeUnitTest,
+       TestGetActionButtonTextWithNormalSettingsOverrideExtension) {
+  base::FilePath fp(test_data_dir_.AppendASCII("api_test/override/newtab/"));
+  EXPECT_NE(nullptr, PackAndInstallCRX(fp, INSTALL_NEW));
+
+  std::unique_ptr<ExtensionMessageBubbleController> ntp_bubble_controller(
+      new ExtensionMessageBubbleController(
+          new NtpOverriddenBubbleDelegate(browser()->profile()), browser()));
+
+  ASSERT_EQ(1U, ntp_bubble_controller->GetExtensionList().size());
+
+  std::unique_ptr<ToolbarActionsBarBubbleDelegate> bridge(
+      new ExtensionMessageBubbleBridge(std::move(ntp_bubble_controller)));
+
+  ASSERT_EQ(
+      l10n_util::GetStringUTF16(IDS_EXTENSION_CONTROLLED_RESTORE_SETTINGS),
+      bridge->GetActionButtonText());

[Devlin, 2016/09/26 20:13:09]

Given the only change between this and the first test is this assertion, I think
we can probably just add this assertion there (and similar for the following
test).

[catmullings, 2016/10/06 18:24:19]

Done.

+}
+
+TEST_F(ExtensionMessageBubbleBridgeUnitTest,
+       TestGetActionButtonTextWithPolicyInstalledSettingsOverrideExtension) {
+  base::FilePath fp(test_data_dir_.AppendASCII("api_test/override/newtab/"));
+  EXPECT_NE(nullptr, PackAndInstallCRXWithLocation(
+                         fp, Manifest::EXTERNAL_POLICY, INSTALL_NEW));
+
+  std::unique_ptr<ExtensionMessageBubbleController> ntp_bubble_controller(
+      new ExtensionMessageBubbleController(
+          new NtpOverriddenBubbleDelegate(browser()->profile()), browser()));
+
+  ASSERT_EQ(1U, ntp_bubble_controller->GetExtensionList().size());
+
+  std::unique_ptr<ToolbarActionsBarBubbleDelegate> bridge(
+      new ExtensionMessageBubbleBridge(std::move(ntp_bubble_controller)));
+
+  ASSERT_EQ(base::string16(), bridge->GetActionButtonText());
+}
+}  // namespace extensions