Improve settings override bubble to indicate policy installed extensions

chrome/browser/ui/extensions/extension_message_bubble_bridge.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/extensions/extension_message_bubble_bridge.h"
 
 #include <utility>
 
 #include "chrome/browser/extensions/extension_message_bubble_controller.h"
+#include "chrome/browser/profiles/profile.h"
+#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)
     : controller_(std::move(controller)) {}
 
 ExtensionMessageBubbleBridge::~ExtensionMessageBubbleBridge() {}
 
 bool ExtensionMessageBubbleBridge::ShouldShow() {
   return controller_->ShouldShow();
 }
@@ skipping 47 matching lines @@
       break;
     }
     case CLOSE_EXECUTE:
       controller_->OnBubbleAction();
       break;
     case CLOSE_LEARN_MORE:
       controller_->OnLinkClicked();
       break;
   }
 }
+
+std::unique_ptr<ToolbarActionsBarBubbleDelegate::ExtraViewInfo>

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

When I remove the "ToolbarActionsBarBubbleDelegate::" from the declaration, I
get a compiler error. 

C++ question: Why must it be left here, but can be left out when using the
ExtraViewInfo struct within the method?

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

Within the method, you are within the scope of the class, and therefore
(normally) don't need to specify - it's the same reason you can (normally)
access methods or members without the class specification.  Outside of the
class, though, the compiler wouldn't know which struct you're referring to, so
it needs the full specification.

*normally - sometimes, you might want to refer to a parent class's
implementation, member, etc - then you would have to use the class
specification.

[catmullings, 2016/08/31 00:03:30]

Done.

+ExtensionMessageBubbleBridge::GetExtraViewInfo() {
+  const extensions::ExtensionIdList& list = controller_->GetExtensionIdList();
+
+  if (list.empty())

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

Can this happen?

[catmullings, 2016/08/31 00:03:30]

W/o digging too deep into the stack, yes, that line can return true if the
following happens:

https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_mess...

[Devlin, 2016/08/31 18:46:45]

But if that's true, ShouldShow() returns false, which means we should never get
so far as constructing the bubble.  Right?

[catmullings, 2016/09/06 19:16:23]

Done.

+    return nullptr;
+
+  DCHECK_EQ(1u, list.size());

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

This isn't quite true, right?  Some bubbles can have more than one extension. 
This should go within the block for IsPolicyInstallable() (name pending)

[catmullings, 2016/08/31 00:03:30]

Yup, originally I had planned to put it in IsPolicyInstallable(), but
GetExtensionIdList() is a method on the controller class, not the delegate. 

The delegate class doesn't have a similar a GetExtensionIdList method because it
only contains a single extension_id as a private member. Given that there would
only be a single extension_id associated with a delegate, is it necessary to
enforce the DCHECK_EQ then?

[Devlin, 2016/08/31 18:46:45]

I don't quite follow.  A bubble can be shown for multiple extensions - take a
look at the DevModeBubbleDelegate.  The list returned from GetExtensionIdList()
is constructed by looping over the set of extensions and adding any for which
ShouldIncludeExtension() is true - which can be >1.  Certain delegates, such as
the settings override bubbles, are restricted to a single extension - but that's
a property of the specific delegate, rather than a generic delegate or the
bubble controller on the whole.  Does that make sense?

[catmullings, 2016/09/06 19:16:22]

Fixed. Responding to explain and justify the change. 

Moved the DCHECK_EQ to inside the code block under the (temporarily named)
IsPolicyInstallable() if-statement check below. In that case, we would only hit
the DCHECK_EQ if IsPolicyInstallable() is true, which is the expected case for
delegates/bubbles like Settings API, NTP, and proxy. Settings override delegates
are are restricted to a single extension, so we can assert that list.size() ==
1. 

Done.

+  const extensions::Extension* extension =
+      extensions::ExtensionRegistry::Get(controller_->profile())
+          ->enabled_extensions()
+          .GetByID(list[0]);
+
+  if (!extension)
+    return nullptr;
+
+  std::unique_ptr<ExtraViewInfo> extra_view_info(new ExtraViewInfo);

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

nit: It doesn't really matter with classes, but I slightly prefer explicit
construction (i.e., new ExtraViewInfo() - note the added parens)

[catmullings, 2016/08/31 00:03:30]

When/why is explicit construction better than using a smart pointer?
Particularly, in this case.

[Devlin, 2016/08/31 18:46:45]

They're orthogonal.  Smart ptrs are ownership, but you still here construct the
object with "new ExtraViewInfo".  Explicit construction refers to the difference
between
new Foo
and
new Foo()
both of which can be used in conjunction with a smart ptr (i.e., it would just
be std::unique_ptr<Foo> foo(new Foo())).

There's a decent stack overflow answer explaining the difference between those
here:
http://stackoverflow.com/questions/620137/do-the-parentheses-after-the-type-n...

Now, in our code base, it doesn't usually make much difference, because we very
rarely allow/encourage the use of compiler-generated constructors.  But it's
just a bit cleaner, IMO, to avoid the risk of leaving members uninitialized.

All that said, we've also very recently (last week?) started preferring
MakeUnique, so it might be best to just use that here.

[catmullings, 2016/09/06 19:16:22]

Done.

+
+  if (controller_->delegate()->IsPolicyInstallable() &&

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

Equivalent to CanBePolicyInstalled().

I didn't quite understand your code review comment wrt when to invoke this. I
assumed it should be here, but it feels redundant, considering the next check on
line 103 extensions::Manifest::IsPolicyLocation(extension->location()). 

Was this how you intended the check to be used?
Why is this extra check for delegate()->IsPolicyInstallable() included, instead
of only having the one on line 103?

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

CanBePolicyInstalled/IsPolicyInstallable aren't great names for what we're
trying to capture (I threw it up there as a strawman).  At a high level, we only
want to show this extra decoration for specific bubbles - ones where we expect
that there could be policy-installed extensions, and where there's only one
controlling extension.  The method on the delegate would be to indicate that the
bubble is of that type.  We don't want to just deduce that on the offchance that
another bubble sometimes satisfies that criteria, but isn't guaranteed to.  Does
that make sense?

[catmullings, 2016/08/31 00:03:30]

What you say makes sense, but I'm don't understand why the name 
CanBePolicyInstalled/IsPolicyInstallable doesn't capture this rationale.

Particularly when you say the following:
Isn't that the purpose of saying "can be" policy installed? 

[Devlin, 2016/08/31 18:46:45]

From reading the name, "CanBePolicyInstalled" implies one thing - that an
extension for which the bubble is being shown could possibly be installed by
enterprise policy.  But it doesn't imply that there's only a single controlling
extension, which, for this UI, is important.

Concrete example:
Potentially, "suspicious" (not in the webstore) extensions might be installed by
policy, but there could be >1 of them, some of which are policy-installed and
some of which aren't.  "CanBePolicyInstalled" would return true for that - they
could be policy installed - but we don't want to show the additional decoration
in that case.

[catmullings, 2016/09/06 19:16:22]

Done.

+      extensions::Manifest::IsPolicyLocation(extension->location())) {
+    extra_view_info->resource_id = IDR_OMNIBOX_HTTPS_POLICY_WARNING;
+    extra_view_info->text =
+        l10n_util::GetStringUTF16(IDS_EXTENSIONS_INSTALLED_BY_ADMIN);
+    extra_view_info->is_text_linked = false;
+  } else {
+    extra_view_info->resource_id = -1;

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

See my question/comment in
chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h

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

Let's have defaults like this handled by an ExtraViewInfo ctor.

[catmullings, 2016/08/31 00:03:30]

Done.

+    extra_view_info->text = controller_->delegate()->GetLearnMoreLabel();
+    extra_view_info->is_text_linked = true;
+  }
+
+  return extra_view_info;
+}