Improve settings override bubble to indicate policy installed extensions

chrome/browser/ui/extensions/extension_message_bubble_bridge.cc

Index: chrome/browser/ui/extensions/extension_message_bubble_bridge.cc
diff --git a/chrome/browser/ui/extensions/extension_message_bubble_bridge.cc b/chrome/browser/ui/extensions/extension_message_bubble_bridge.cc
index 0611f3269d583a1a51f63197a16a0cf12b245a94..f5c206ebef2ff5471f3194a0ce44a44ae4d80be0 100644
--- a/chrome/browser/ui/extensions/extension_message_bubble_bridge.cc
+++ b/chrome/browser/ui/extensions/extension_message_bubble_bridge.cc
@@ -7,6 +7,13 @@
 #include <utility>
 
 #include "chrome/browser/extensions/extension_message_bubble_controller.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h"

[Devlin, 2016/08/02 20:34:05]

This is included in the .h; no need for it here.

[catmullings, 2016/08/13 00:35:52]

Done.

+#include "extensions/browser/extension_registry.h"
+#include "grit/components_scaled_resources.h"
+#include "grit/generated_resources.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/resource/resource_bundle.h"
 
 ExtensionMessageBubbleBridge::ExtensionMessageBubbleBridge(
     std::unique_ptr<extensions::ExtensionMessageBubbleController> controller)
@@ -74,3 +81,40 @@ void ExtensionMessageBubbleBridge::OnBubbleClosed(CloseAction action) {
       break;
   }
 }
+
+std::unique_ptr<ToolbarActionsBarBubbleDelegate::ExtraViewInfo>
+ExtensionMessageBubbleBridge::GetExtraViewInfo() {
+  extensions::ExtensionIdList list = controller_->GetExtensionIdList();

[Devlin, 2016/08/02 20:34:05]

const & to avoid a copy.

[catmullings, 2016/08/13 00:35:52]

Done.

+
+  if (list.empty())
+    return NULL;
+
+  std::unique_ptr<ToolbarActionsBarBubbleDelegate::ExtraViewInfo>

[Devlin, 2016/08/02 20:34:05]

nit: since this is in the scope of the bridge class, which implements
ToolbarActionsBarBubbleDelegate, we can actually just refer to this as
"ExtraViewInfo"

[catmullings, 2016/08/13 00:35:52]

Done.

+      extra_view_info(nullptr);
+
+  extensions::ExtensionRegistry* registry =
+      extensions::ExtensionRegistry::Get(controller_->profile());
+  for (const std::string& id : list) {
+    const extensions::Extension* extension = registry->GetExtensionById(
+        id, extensions::ExtensionRegistry::EVERYTHING);

[Devlin, 2016/08/02 20:34:05]

I think we should only be worried about enabled extensions here.

[catmullings, 2016/08/13 00:35:52]

With the changes below, this is no longer relevant, right?

Done.

+    if (!extension &&
+        !extensions::Manifest::IsPolicyLocation(extension->location())) {
+      continue;
+    }
+    extra_view_info =
+        std::unique_ptr<ToolbarActionsBarBubbleDelegate::ExtraViewInfo>(
+            new ToolbarActionsBarBubbleDelegate::ExtraViewInfo);
+    extra_view_info->icon = new views::ImageView();
+    extra_view_info->icon->SetImage(
+        ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
+            IDR_OMNIBOX_HTTPS_POLICY_WARNING));
+
+    extra_view_info->text =
+        l10n_util::GetStringUTF16(IDS_EXTENSIONS_INSTALLED_BY_ADMIN);
+
+    // TODO: Stop after first policy extension encountered. Figure out how

[Devlin, 2016/08/02 20:34:05]

For this first version, I think it's reasonable to *only* show the policy
indication on warnings that are for a specific extension.  It's strictly better
than the current version, and it ensures we never e.g. imply everything is
policy installed when only one extension is.

My recommendation would be:
- Add a method on ExtensionMessageBubbleDelegate for CanBePolicyInstalled() (not
attached to that name - feel free to come up with something better) that returns
true for the bubbles we're concerned about (proxy, settings, new tab page).
- In the case of those bubbles, there should only ever be a single extension. In
that case, we can here do something like:
DCHECK_EQ(1u, list.size());
const extensions::Extension* extension =
extensions::ExtensionRegistry::Get(controller_->profile())->enabled_extensions().GetByID(list[0]);
if (extensions::Manifest::IsPolicyLocation(extension->location())) {
  ... extra view stuff.
}

WDYT?

[catmullings, 2016/08/13 00:35:52]

I don't mind the name CanBePolicyInstalled(); it describes the expected
functionality. I named it IsPolicyInstallable() instead, but open to
alternatives, including your suggested name. 
Why do you suggest checking DCHECK_EQ(1u, list.size()); ? Wouldn't it be
possible that list.size > 1?
Done.

[Devlin, 2016/08/22 20:59:41]

With the bubbles that this is relevant for, we should only ever be warning about
a single extension.  Which is important, because otherwise it's unclear what the
UI is (i.e., how to say "Some of these N extensions are policy-installed, others
aren't.")

+    // handle multiple policy extensions.
+    break;
+  }
+  return extra_view_info;

[Devlin, 2016/08/02 20:34:05]

We still need to handle the case where the extension isn't policy installed and
we should show the learn more link.

[catmullings, 2016/08/13 00:35:52]

Done.

+}