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

chrome/browser/ui/toolbar/toolbar_actions_bar.cc

 // Copyright 2014 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/toolbar/toolbar_actions_bar.h"
 
 #include "base/auto_reset.h"
 #include "base/location.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/single_thread_task_runner.h"
@@ skipping 285 matching lines @@
       size_t rindex = actions.size() - index - 1;
       std::rotate(actions.rbegin() + rindex,
                   actions.rbegin() + rindex + 1,
                   actions.rend() - visible + 1);
     }
   }
 
   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
   // actions, as they rely on the extension system to function.
   if (!model_ || !model_->extensions_initialized())
     return;
 
   {
     // TODO(robliao): Remove ScopedTracker below once https://crbug.com/463337
     // is fixed.
@@ skipping 49 matching lines @@
 
     for (size_t i = 0; i < toolbar_actions_.size(); ++i)
       delegate_->AddViewForAction(toolbar_actions_[i], i);
   }
 
   // Once the actions are created, we should animate the changes.
   suppress_animation_ = false;
 
   // 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.
     scoped_ptr<extensions::ExtensionMessageBubbleController> controller =
         ExtensionMessageBubbleFactory(browser_->profile()).GetController();
     if (controller) {
       base::ThreadTaskRunnerHandle::Get()->PostTask(
           FROM_HERE, base::Bind(&ToolbarActionsBar::MaybeShowExtensionBubble,
                                 weak_ptr_factory_.GetWeakPtr(),
                                 base::Passed(controller.Pass())));
@@ skipping 271 matching lines @@
     // We may already be at the right size (this can happen frequently with
     // overflow, where we have a fixed width, and in tests, where we skip
     // animations). If this is the case, we still need to Redraw(), because the
     // icons within the toolbar may have changed (e.g. if we removed one
     // action and added a different one in quick succession).
     delegate_->Redraw(false);
   }
 }
 
 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
   // only the new actions.
   DeleteActions();
   CreateActions();
   // Resize the delegate. We suppress the chevron so that we don't risk showing
   // it only for the duration of the animation.
   ResizeDelegate(gfx::Tween::LINEAR, true);
 }
@@ skipping 61 matching lines @@
   for (ToolbarActionViewController* action : toolbar_actions_) {
     if (action->GetId() == id)
       return action;
   }
   return nullptr;
 }
 
 content::WebContents* ToolbarActionsBar::GetCurrentWebContents() {
   return browser_->tab_strip_model()->GetActiveWebContents();
 }