[Extensions UI] Highlight toolbar extensions when the redesign bubble is active

chrome/browser/ui/toolbar/toolbar_actions_bar.cc

Index: chrome/browser/ui/toolbar/toolbar_actions_bar.cc
diff --git a/chrome/browser/ui/toolbar/toolbar_actions_bar.cc b/chrome/browser/ui/toolbar/toolbar_actions_bar.cc
index 31600f6974fe2f5f51648ce5fd554cd2bec938fc..bbe755b34f80cbaf9d02446450f9927f7f166699 100644
--- a/chrome/browser/ui/toolbar/toolbar_actions_bar.cc
+++ b/chrome/browser/ui/toolbar/toolbar_actions_bar.cc
@@ -303,6 +303,27 @@ ToolbarActionsBar::GetActions() const {
   return actions;
 }
 
+ToolbarActionsBar::HighlightType ToolbarActionsBar::GetHighlightType() const {
+  // TODO(devlin): This mapping is to avoid platform-specific ui implementations
+  // have a dependency on the ExtensionToolbarModel, but is a little odd. It
+  // would be better to just be able to return the set of images for the
+  // highlight here, but it's hard to do nicely since it's a nine-grid.

[Peter Kasting, 2015/07/23 00:00:12]

Maybe this all becomes better if you return one image instead of nine? 
Certainly this mapping code is ugly (and I'm not sure why depending on the
extension toolbar model is a bad thing anyway).

[Devlin, 2015/07/23 02:43:30]

It could be better with a single image, but see other comment.  It could also be
better if we made the NinePartImageIds struct in ui/base/cocoa/appkit_utils.h
for all platforms, and used that in place of our current int[] image grids for
views (which also has other advantages, like more type safety), because then we
could just return a NinePartImageIds here directly, which would be the best
solution.

As for why the dependency is bad, it's really a point of pride more than
anything else - it took a lot of effort for the view implementations to not
depend at all on the ExtensionToolbarModel, and I didn't want to re-introduce
that dependency just for an enum.  Of course, the fact that it's a point of
pride probably means it's not a good reason. :)

[Peter Kasting, 2015/07/23 05:40:53]

I guess I'd probably ask the same thing with the rest of that work as I'm asking
here: what's the actual benefit?  Presumably there is some or you wouldn't have
done the other work to begin with.  But it seems like platform-specific UI code
depending on cross-platform UI code is OK.  We do that plenty of other places. 
And duplication seems potentially worse than dependency, to me.

[Devlin, 2015/07/23 15:30:13]

Benefit's not substantial enough.  Removed the mapping.

+  HighlightType type = HIGHLIGHT_NONE;
+  if (model_ && model_->extensions_initialized()) {
+    switch (model_->highlight_type()) {
+      case extensions::ExtensionToolbarModel::HIGHLIGHT_NONE:
+        break;  // Already HIGHLIGHT_TYPE_NONE
+      case extensions::ExtensionToolbarModel::HIGHLIGHT_INFO:
+        type = HIGHLIGHT_INFO;
+        break;
+      case extensions::ExtensionToolbarModel::HIGHLIGHT_WARNING:
+        type = HIGHLIGHT_WARNING;
+        break;
+    }
+  }
+  return type;
+}
+
 void ToolbarActionsBar::CreateActions() {
   DCHECK(toolbar_actions_.empty());
   // We wait for the extension system to be initialized before we add any
@@ -372,7 +393,7 @@ void ToolbarActionsBar::CreateActions() {
 
   // CreateActions() can be called multiple times, so we need to make sure we
   // haven't already shown the bubble.
-  if (!checked_extension_bubble_) {
+  if (!checked_extension_bubble_ && !is_highlighting()) {

[Peter Kasting, 2015/07/23 00:00:12]

I'm not totally clear on why this was added...

[Devlin, 2015/07/23 02:43:30]

Extension bubbles can cause a subsection of extensions to be highlighted.  If
the toolbar is already highlighting a different set, we don't want to override
it.

[Peter Kasting, 2015/07/23 05:40:53]

Ahhhh.  Once again, might be worth a descriptive comment.

[Devlin, 2015/07/23 15:30:13]

Done.

     checked_extension_bubble_ = true;
     // CreateActions() can be called as part of the browser window set up, which
     // we need to let finish before showing the actions.
@@ -664,6 +685,8 @@ void ToolbarActionsBar::ResizeDelegate(gfx::Tween::Type tween_type,
 }
 
 void ToolbarActionsBar::OnToolbarHighlightModeChanged(bool is_highlighting) {
+  if (!model_->extensions_initialized())
+    return;
   // It's a bit of a pain that we delete and recreate everything here, but given
   // everything else going on (the lack of highlight, [n] more extensions
   // appearing, etc), it's not worth the extra complexity to create and insert