Cleanup: Remove pointless GetInsets() override. Simplify |container_size_| t...

chrome/browser/views/browser_actions_container.cc

 // Copyright (c) 2010 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/views/browser_actions_container.h"
 
 #include "app/l10n_util.h"
 #include "app/resource_bundle.h"
 #include "app/slide_animation.h"
 #include "base/stl_util-inl.h"
@@ skipping 60 matching lines @@
 
 // The margin above the chevron.
 static const int kChevronTopMargin = 9;
 
 // The margin to the right of the chevron.
 static const int kChevronRightMargin = 4;
 
 // Width for the resize area.
 static const int kResizeAreaWidth = 4;
 
-// Width of the drop indicator.
-static const int kDropIndicatorWidth = 2;
-
-// Color of the drop indicator.
-static const SkColor kDropIndicatorColor = SK_ColorBLACK;
-
 // The x offset for the drop indicator (how much we shift it by).
 static const int kDropIndicatorOffsetLtr = 3;
 static const int kDropIndicatorOffsetRtl = 9;
 
 }  // namespace.
 
 // Static.
 bool BrowserActionsContainer::disable_animations_during_testing_ = false;
 
 ////////////////////////////////////////////////////////////////////////////////
 // BrowserActionButton
 
 BrowserActionButton::BrowserActionButton(Extension* extension,
                                          BrowserActionsContainer* panel)
-    : ALLOW_THIS_IN_INITIALIZER_LIST(MenuButton(this, L"", NULL, false)),
+    : ALLOW_THIS_IN_INITIALIZER_LIST(
+          MenuButton(this, std::wstring(), NULL, false)),
       browser_action_(extension->browser_action()),
       extension_(extension),
       ALLOW_THIS_IN_INITIALIZER_LIST(tracker_(this)),
       showing_context_menu_(false),
       panel_(panel) {
   set_alignment(TextButton::ALIGN_CENTER);
 
   // No UpdateState() here because View hierarchy not setup yet. Our parent
   // should call UpdateState() after creation.
 
@@ skipping 19 matching lines @@
 
 void BrowserActionButton::Destroy() {
   if (showing_context_menu_) {
     context_menu_menu_->CancelMenu();
     MessageLoop::current()->DeleteSoon(FROM_HERE, this);
   } else {
     delete this;
   }
 }
 
-gfx::Insets BrowserActionButton::GetInsets() const {
-  static gfx::Insets zero_inset;
-  return zero_inset;
+void BrowserActionButton::ButtonPressed(views::Button* sender,
+                                        const views::Event& event) {
+  panel_->OnBrowserActionExecuted(this, false);

[Finnur, 2010/07/27 09:02:23]

nit: Doesn't the style guide say you should document what you are passing in
when you have simple bool/int params?

[Peter Kasting, 2010/07/27 18:24:43]

The precise style guide text is:

"When you pass in NULL, boolean, or literal integer values to functions, you
should consider adding a comment about what they are, or make your code
self-documenting by using constants."

Thus it's suggested, but not required.  There are a couple reasons I removed the
"// inspect_with_devtools" comments in this file.

First, they didn't seem to add clarity.  In some cases, like the one here, the
comment looked like a comment on the whole function call rather than on one
argument, which was confusing.  And even in the other cases,
"inspect_with_devtools" on its own is an argument name, not an explanation, and
is meaningless without going and studying the original function definition.  If
someone has to do that anyway, adding the arg name here helps little.

Second, in some cases (though not this particular one), the result was
noticeably less noisy, because in addition to the comment disappearing, things
could beome single-line, and then sometimes lose surrounding braces too.

Third, we don't annotate almost any other instance of bool, int, or NULL
arguments to functions, in this or any other file in the codebase, so it felt
inconsistent.

If you think these added a lot of clarity, though, I can restore them.  Let me
know.

[Finnur, 2010/07/27 20:13:17]

OK

 }
 
-void BrowserActionButton::ButtonPressed(views::Button* sender,
-    const views::Event& event) {
-  panel_->OnBrowserActionExecuted(this, false);  // inspect_with_devtools
-}
-
-void BrowserActionButton::OnImageLoaded(
-    SkBitmap* image, ExtensionResource resource, int index) {
+void BrowserActionButton::OnImageLoaded(SkBitmap* image,
+                                        ExtensionResource resource,
+                                        int index) {
   if (image)
     default_icon_ = *image;
 
   // Call back to UpdateState() because a more specific icon might have been set
   // while the load was outstanding.
   UpdateState();
 }
 
 void BrowserActionButton::UpdateState() {
   int tab_id = panel_->GetCurrentTabId();
@@ skipping 11 matching lines @@
   if (name.empty())
     name = UTF8ToWide(extension()->name());
   SetTooltipText(name);
   SetAccessibleName(name);
   GetParent()->SchedulePaint();
 }
 
 void BrowserActionButton::Observe(NotificationType type,
                                   const NotificationSource& source,
                                   const NotificationDetails& details) {
-  if (type == NotificationType::EXTENSION_BROWSER_ACTION_UPDATED) {
-    UpdateState();
-    // The browser action may have become visible/hidden so we need to make
-    // sure the state gets updated.
-    panel_->OnBrowserActionVisibilityChanged();
-  } else {
-    NOTREACHED() << L"Received unexpected notification";
-  }
+  DCHECK(type == NotificationType::EXTENSION_BROWSER_ACTION_UPDATED);
+  UpdateState();
+  // The browser action may have become visible/hidden so we need to make
+  // sure the state gets updated.
+  panel_->OnBrowserActionVisibilityChanged();
 }
 
 bool BrowserActionButton::IsPopup() {
   int tab_id = panel_->GetCurrentTabId();
-  if (tab_id < 0) {
-    NOTREACHED() << "Button is not on a specific tab.";
-    return false;
-  }
+  DCHECK_GE(tab_id, 0);
   return browser_action_->HasPopup(tab_id);
 }
 
 GURL BrowserActionButton::GetPopupUrl() {
   int tab_id = panel_->GetCurrentTabId();
-  if (tab_id < 0) {
-    NOTREACHED() << "Button is not on a specific tab.";
-    GURL empty_url;
-    return empty_url;
-  }
+  DCHECK_GE(tab_id, 0);
   return browser_action_->GetPopupUrl(tab_id);
 }
 
 bool BrowserActionButton::Activate() {
-  if (IsPopup()) {
-    panel_->OnBrowserActionExecuted(this, false);  // |inspect_with_devtools|.
+  if (!IsPopup())
+    return true;
 
-    // TODO(erikkay): Run a nested modal loop while the mouse is down to
-    // enable menu-like drag-select behavior.
+  panel_->OnBrowserActionExecuted(this, false);  // |inspect_with_devtools|.
 
-    // The return value of this method is returned via OnMousePressed.
-    // We need to return false here since we're handing off focus to another
-    // widget/view, and true will grab it right back and try to send events
-    // to us.
-    return false;
-  }
-  return true;
+  // TODO(erikkay): Run a nested modal loop while the mouse is down to
+  // enable menu-like drag-select behavior.
+
+  // The return value of this method is returned via OnMousePressed.
+  // We need to return false here since we're handing off focus to another
+  // widget/view, and true will grab it right back and try to send events
+  // to us.
+  return false;
 }
 
 bool BrowserActionButton::OnMousePressed(const views::MouseEvent& e) {
-  if (e.IsRightMouseButton()) {
-    // Get the top left point of this button in screen coordinates.
-    gfx::Point point = gfx::Point(0, 0);
-    ConvertPointToScreen(this, &point);
+  if (!e.IsRightMouseButton()) {
+    return IsPopup() ?
+        MenuButton::OnMousePressed(e) : TextButton::OnMousePressed(e);
+  }
 
-    // Make the menu appear below the button.
-    point.Offset(0, height());
+  // Get the top left point of this button in screen coordinates.
+  gfx::Point point = gfx::Point(0, 0);
+  ConvertPointToScreen(this, &point);
 
-    ShowContextMenu(point, true);
-    return false;
-  } else if (IsPopup()) {
-    return MenuButton::OnMousePressed(e);
-  }
-  return TextButton::OnMousePressed(e);
+  // Make the menu appear below the button.
+  point.Offset(0, height());
+
+  ShowContextMenu(point, true);
+  return false;
 }
 
 void BrowserActionButton::OnMouseReleased(const views::MouseEvent& e,
                                           bool canceled) {
   if (IsPopup() || showing_context_menu_) {
     // TODO(erikkay) this never actually gets called (probably because of the
     // loss of focus).
     MenuButton::OnMouseReleased(e, canceled);
   } else {
     TextButton::OnMouseReleased(e, canceled);
   }
 }
 
 bool BrowserActionButton::OnKeyReleased(const views::KeyEvent& e) {
-  if (IsPopup())
-    return MenuButton::OnKeyReleased(e);
-  return TextButton::OnKeyReleased(e);
+  return IsPopup() ?
+      MenuButton::OnKeyReleased(e) : TextButton::OnKeyReleased(e);
 }
 
 void BrowserActionButton::OnMouseExited(const views::MouseEvent& e) {
   if (IsPopup() || showing_context_menu_)
     MenuButton::OnMouseExited(e);
   else
     TextButton::OnMouseExited(e);
 }
 
 void BrowserActionButton::ShowContextMenu(const gfx::Point& p,
                                           bool is_mouse_gesture) {
   showing_context_menu_ = true;
   SetButtonPushed();
 
   // Reconstructs the menu every time because the menu's contents are dynamic.
-  context_menu_contents_ = new ExtensionContextMenuModel(
-      extension(), panel_->browser(), panel_);
+  context_menu_contents_ =
+      new ExtensionContextMenuModel(extension(), panel_->browser(), panel_);
   context_menu_menu_.reset(new views::Menu2(context_menu_contents_.get()));
   context_menu_menu_->RunContextMenuAt(p);
 
   SetButtonNotPushed();
   showing_context_menu_ = false;
 }
 
 void BrowserActionButton::SetButtonPushed() {
   SetState(views::CustomButton::BS_PUSHED);
   menu_visible_ = true;
@@ skipping 27 matching lines @@
   button_->Destroy();
 }
 
 gfx::Canvas* BrowserActionView::GetIconWithBadge() {
   int tab_id = panel_->GetCurrentTabId();
 
   SkBitmap icon = button_->extension()->browser_action()->GetIcon(tab_id);
   if (icon.isNull())
     icon = button_->default_icon();
 
-  gfx::Canvas* canvas =
-      new gfx::CanvasSkia(icon.width(), icon.height(), false);
+  gfx::Canvas* canvas = new gfx::CanvasSkia(icon.width(), icon.height(), false);
   canvas->DrawBitmapInt(icon, 0, 0);
 
   if (tab_id >= 0) {
-    gfx::Rect bounds =
-        gfx::Rect(icon.width(), icon.height() + kControlVertOffset);
-    button_->extension()->browser_action()->PaintBadge(canvas,
-                                                       bounds, tab_id);
+    gfx::Rect bounds(icon.width(), icon.height() + kControlVertOffset);
+    button_->extension()->browser_action()->PaintBadge(canvas, bounds, tab_id);
   }
 
   return canvas;
 }
 
 bool BrowserActionView::GetAccessibleRole(AccessibilityTypes::Role* role) {
   DCHECK(role);
   *role = AccessibilityTypes::ROLE_GROUPING;
   return true;
 }
 
 void BrowserActionView::Layout() {
   button_->SetBounds(0, kControlVertOffset, width(), kButtonSize);
 }
 
 void BrowserActionView::PaintChildren(gfx::Canvas* canvas) {
   View::PaintChildren(canvas);
   ExtensionAction* action = button()->browser_action();
   int tab_id = panel_->GetCurrentTabId();
-  if (tab_id < 0)
-    return;
-
-  action->PaintBadge(canvas, gfx::Rect(width(), height()), tab_id);
+  if (tab_id >= 0)
+    action->PaintBadge(canvas, gfx::Rect(width(), height()), tab_id);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // BrowserActionsContainer
 
-BrowserActionsContainer::BrowserActionsContainer(
-    Browser* browser, View* owner_view)
+BrowserActionsContainer::BrowserActionsContainer(Browser* browser,
+                                                 View* owner_view)
     : profile_(browser->profile()),
       browser_(browser),
       owner_view_(owner_view),
       popup_(NULL),
       popup_button_(NULL),
       model_(NULL),
       chevron_(NULL),
       overflow_menu_(NULL),
       suppress_chevron_(false),
       resize_amount_(0),
       animation_target_size_(0),
       drop_indicator_position_(-1),
       ALLOW_THIS_IN_INITIALIZER_LIST(task_factory_(this)),
       ALLOW_THIS_IN_INITIALIZER_LIST(show_menu_task_factory_(this)) {
   SetID(VIEW_ID_BROWSER_ACTION_TOOLBAR);
 
   if (profile_->GetExtensionsService()) {
     model_ = profile_->GetExtensionsService()->toolbar_model();
     model_->AddObserver(this);
   }
+
   resize_animation_.reset(new SlideAnimation(this));
   resize_area_ = new views::ResizeArea(this);
   resize_area_->SetAccessibleName(l10n_util::GetString(IDS_ACCNAME_SEPARATOR));
   AddChildView(resize_area_);
 
   // TODO(glen): Come up with a new bitmap for the chevron.
   ResourceBundle& rb = ResourceBundle::GetSharedInstance();
   SkBitmap* chevron_image = rb.GetBitmapNamed(IDR_BOOKMARK_BAR_CHEVRONS);
   chevron_ = new views::MenuButton(NULL, std::wstring(), this, false);
   chevron_->SetVisible(false);
   chevron_->SetIcon(*chevron_image);
   chevron_->SetAccessibleName(
       l10n_util::GetString(IDS_ACCNAME_EXTENSIONS_CHEVRON));
   // Chevron contains >> that should point left in LTR locales.
   chevron_->EnableCanvasFlippingForRTLUI(true);
   AddChildView(chevron_);
 
-  if (!profile_->GetPrefs()->HasPrefPath(prefs::kExtensionToolbarSize)) {
+  if (model_ &&
+      !profile_->GetPrefs()->HasPrefPath(prefs::kExtensionToolbarSize)) {
     // Migration code to the new VisibleIconCount pref.
     // TODO(mpcomplete): remove this after users are upgraded to 5.0.
     int predefined_width =
         profile_->GetPrefs()->GetInteger(prefs::kBrowserActionContainerWidth);
     if (predefined_width != 0) {
-      int icon_width = (kButtonSize + kBrowserActionButtonPadding);
-      if (model_) {
-        model_->SetVisibleIconCount(
-            (predefined_width - WidthOfNonIconArea()) / icon_width);
-      }
+      model_->SetVisibleIconCount((predefined_width - WidthOfNonIconArea()) /
+          (kButtonSize + kBrowserActionButtonPadding));
     }
   }
-
   if (model_ && model_->extensions_initialized())
     SetContainerWidth();
 
   SetAccessibleName(l10n_util::GetString(IDS_ACCNAME_EXTENSIONS));
 }
 
 BrowserActionsContainer::~BrowserActionsContainer() {
   if (model_)
     model_->RemoveObserver(this);
   StopShowFolderDropMenuTimer();
   HidePopup();
   DeleteBrowserActionViews();
 }
 
 // Static.
 void BrowserActionsContainer::RegisterUserPrefs(PrefService* prefs) {
   prefs->RegisterIntegerPref(prefs::kBrowserActionContainerWidth, 0);
 }
 
 int BrowserActionsContainer::GetCurrentTabId() const {
   TabContents* tab_contents = browser_->GetSelectedTabContents();
-  if (!tab_contents)
-    return -1;
-
-  return tab_contents->controller().session_id().id();
+  return tab_contents ? tab_contents->controller().session_id().id() : -1;
 }
 
 BrowserActionView* BrowserActionsContainer::GetBrowserActionView(
     ExtensionAction* action) {
-  for (BrowserActionViews::iterator iter =
-       browser_action_views_.begin(); iter != browser_action_views_.end();
-       ++iter) {
+  for (BrowserActionViews::iterator iter = browser_action_views_.begin();
+       iter != browser_action_views_.end(); ++iter) {
     if ((*iter)->button()->browser_action() == action)
       return *iter;
   }
-
   return NULL;
 }
 
 void BrowserActionsContainer::RefreshBrowserActionViews() {
   for (size_t i = 0; i < browser_action_views_.size(); ++i)
     browser_action_views_[i]->button()->UpdateState();
 }
 
-void BrowserActionsContainer::CloseOverflowMenu() {
-  if (overflow_menu_)
-    overflow_menu_->CancelMenu();
-}
-
-void BrowserActionsContainer::StopShowFolderDropMenuTimer() {
-  show_menu_task_factory_.RevokeAll();
-}
-
-void BrowserActionsContainer::StartShowFolderDropMenuTimer() {
-  int delay = View::GetMenuShowDelay();
-  MessageLoop::current()->PostDelayedTask(FROM_HERE,
-      show_menu_task_factory_.NewRunnableMethod(
-          &BrowserActionsContainer::ShowDropFolder),
-      delay);
-}
-
-void BrowserActionsContainer::ShowDropFolder() {
-  DCHECK(!overflow_menu_);
-  SetDropIndicator(-1);
-  overflow_menu_ = new BrowserActionOverflowMenuController(
-      this, chevron_, browser_action_views_, VisibleBrowserActions());
-  overflow_menu_->set_observer(this);
-  overflow_menu_->RunMenu(GetWindow()->GetNativeWindow(), true);
-}
-
-void BrowserActionsContainer::SetDropIndicator(int x_pos) {
-  if (drop_indicator_position_ != x_pos) {
-    drop_indicator_position_ = x_pos;
-    SchedulePaint();
-  }
-}
-
 void BrowserActionsContainer::CreateBrowserActionViews() {
   DCHECK(browser_action_views_.empty());
   if (!model_)
     return;
 
-  for (ExtensionList::iterator iter = model_->begin();
-       iter != model_->end(); ++iter) {
+  for (ExtensionList::iterator iter = model_->begin(); iter != model_->end();
+       ++iter) {
     if (!ShouldDisplayBrowserAction(*iter))
       continue;
 
     BrowserActionView* view = new BrowserActionView(*iter, this);
     browser_action_views_.push_back(view);
     AddChildView(view);
   }
 }
 
 void BrowserActionsContainer::DeleteBrowserActionViews() {
   if (!browser_action_views_.empty()) {
     for (size_t i = 0; i < browser_action_views_.size(); ++i)
       RemoveChildView(browser_action_views_[i]);
     STLDeleteContainerPointers(browser_action_views_.begin(),
                                browser_action_views_.end());
     browser_action_views_.clear();
   }
 }
 
 void BrowserActionsContainer::OnBrowserActionVisibilityChanged() {
-  SetVisible(browser_action_views_.size() > 0);
+  SetVisible(!browser_action_views_.empty());
   owner_view_->Layout();
   owner_view_->SchedulePaint();
 }
 
-void BrowserActionsContainer::HidePopup() {
-  if (popup_)
-    popup_->Close();
-}
-
-void BrowserActionsContainer::TestExecuteBrowserAction(int index) {
-  BrowserActionButton* button = browser_action_views_[index]->button();
-  OnBrowserActionExecuted(button, false);  // |inspect_with_devtools|.
-}
-
-void BrowserActionsContainer::TestSetIconVisibilityCount(size_t icons) {
-  chevron_->SetVisible(icons < browser_action_views_.size());
-  container_size_.set_width(IconCountToWidth(icons));
-  Layout();
-  SchedulePaint();
+size_t BrowserActionsContainer::VisibleBrowserActions() const {
+  size_t visible_actions = 0;
+  for (size_t i = 0; i < browser_action_views_.size(); ++i) {
+    if (browser_action_views_[i]->IsVisible())
+      ++visible_actions;
+  }
+  return visible_actions;
 }
 
 void BrowserActionsContainer::OnBrowserActionExecuted(
-    BrowserActionButton* button, bool inspect_with_devtools) {
+    BrowserActionButton* button,
+    bool inspect_with_devtools) {
   ExtensionAction* browser_action = button->browser_action();
 
   // Popups just display.  No notification to the extension.
   // TODO(erikkay): should there be?
-  if (button->IsPopup()) {
-    // If we're showing the same popup, just hide it and return.
-    bool same_showing = popup_ && button == popup_button_;
-
-    // Always hide the current popup, even if it's not the same.
-    // Only one popup should be visible at a time.
-    HidePopup();
-
-    if (same_showing)
-      return;
-
-    // We can get the execute event for browser actions that are not visible,
-    // since buttons can be activated from the overflow menu (chevron). In that
-    // case we show the popup as originating from the chevron.
-    View* reference_view = button->GetParent()->IsVisible() ? button : chevron_;
-    gfx::Point origin;
-    View::ConvertPointToScreen(reference_view, &origin);
-    gfx::Rect rect = reference_view->bounds();
-    rect.set_x(origin.x());
-    rect.set_y(origin.y());
-
-    gfx::NativeWindow frame_window =
-        browser_->window()->GetNativeHandle();
-    BubbleBorder::ArrowLocation arrow_location = base::i18n::IsRTL() ?
-        BubbleBorder::TOP_LEFT : BubbleBorder::TOP_RIGHT;
-
-    popup_ = ExtensionPopup::Show(button->GetPopupUrl(), browser_,
-        browser_->profile(), frame_window, rect, arrow_location,
-        true,  // Activate the popup window.
-        inspect_with_devtools,
-        ExtensionPopup::BUBBLE_CHROME,
-        this);  // ExtensionPopupDelegate

[Finnur, 2010/07/27 09:02:23]

nit: same question here (comment).

-    popup_button_ = button;
-    popup_button_->SetButtonPushed();
-
+  if (!button->IsPopup()) {
+    ExtensionBrowserEventRouter::GetInstance()->BrowserActionExecuted(
+        profile_, browser_action->extension_id(), browser_);
     return;
   }
 
-  // Otherwise, we send the action to the extension.
-  ExtensionBrowserEventRouter::GetInstance()->BrowserActionExecuted(
-      profile_, browser_action->extension_id(), browser_);
+  // If we're showing the same popup, just hide it and return.
+  bool same_showing = popup_ && button == popup_button_;
+
+  // Always hide the current popup, even if it's not the same.
+  // Only one popup should be visible at a time.
+  HidePopup();
+
+  if (same_showing)
+    return;
+
+  // We can get the execute event for browser actions that are not visible,
+  // since buttons can be activated from the overflow menu (chevron). In that
+  // case we show the popup as originating from the chevron.
+  View* reference_view = button->GetParent()->IsVisible() ? button : chevron_;
+  gfx::Point origin;
+  View::ConvertPointToScreen(reference_view, &origin);
+  gfx::Rect rect = reference_view->bounds();
+  rect.set_origin(origin);
+
+  gfx::NativeWindow frame_window = browser_->window()->GetNativeHandle();
+  BubbleBorder::ArrowLocation arrow_location = base::i18n::IsRTL() ?
+      BubbleBorder::TOP_LEFT : BubbleBorder::TOP_RIGHT;
+
+  popup_ = ExtensionPopup::Show(button->GetPopupUrl(), browser_,
+      browser_->profile(), frame_window, rect, arrow_location, true,
+      inspect_with_devtools, ExtensionPopup::BUBBLE_CHROME, this);

[Finnur, 2010/07/27 09:02:23]

nit: same question here (comments deleted).

+  popup_button_ = button;
+  popup_button_->SetButtonPushed();
 }
 
 gfx::Size BrowserActionsContainer::GetPreferredSize() {
   if (browser_action_views_.empty())
     return gfx::Size(0, 0);
 
   // We calculate the size of the view by taking the current width and
   // subtracting resize_amount_ (the latter represents how far the user is
   // resizing the view or, if animating the snapping, how far to animate it).
   // But we also clamp it to a minimum size and the maximum size, so that the
   // container can never shrink too far or take up more space than it needs. In
   // other words: ContainerMinSize() < width() - resize < ClampTo(MAX).
-  int width = std::max(ContainerMinSize(),
-                       container_size_.width() - resize_amount_);
-  int max_width = ClampToNearestIconCount(-1, false);  // -1 gives max width.
-  width = std::min(width, max_width);
-
-  return gfx::Size(width, kButtonSize);
+  return gfx::Size(std::min(std::max(ContainerMinSize(),
+      container_width_ - resize_amount_), ClampToNearestIconCount(-1, false)),
+      kButtonSize);

[Finnur, 2010/07/27 09:02:23]

I'm not a huge fan of this consolidation. The way it was before was more
readable and easier to debug when you step through it and I don't really see any
benefits (the compiler is going to create unnamed temporaries anyway, right?)...

[Peter Kasting, 2010/07/27 18:24:43]

I partly did this because the final form (after the functional patch coming up
later) is even (slightly) simpler:

  return gfx::Size(std::min(std::max(ContainerMinSize(),
    container_width_ - resize_amount_), IconCountToWidth(-1, false)), 0);

Because it's now two lines, it's easier to read than what's in this patch.

But if you still don't like that, then one possibility would be:

  int resized_width = container_width_ - resize_amount_;
  return gfx::Size(std::min(std::max(ContainerMinSize(), resized_width),
                            IconCountToWidth(-1, false)), 0);

Another would be:

  int clamped width = std::min(
      std::max(ContainerMinSize(), container_width_ - resize_amount_),
      IconCountToWidth(-1, false));
  return gfx::Size(clamped_width, 0);

All of these seem a bit less verbose than the original.  WDYT?

[Finnur, 2010/07/27 20:13:17]

I think the last version (clamped_width) is a decent compromise.

On 2010/07/27 18:24:43, Peter Kasting wrote:

 }
 
 void BrowserActionsContainer::Layout() {
-  if (browser_action_views_.size() == 0) {
+  if (browser_action_views_.empty()) {
     SetVisible(false);
     chevron_->SetVisible(false);
     return;
-  } else {
-    SetVisible(true);
   }
 
+  SetVisible(true);
+
   resize_area_->SetBounds(0, 0, kResizeAreaWidth, height());
   int x = kResizeAreaWidth;
 
   x += base::i18n::IsRTL() ? kHorizontalPaddingRtl : kHorizontalPadding;
 
   // Calculate if all icons fit without showing the chevron. We need to know
   // this beforehand, because showing the chevron will decrease the space that
   // we have to draw the visible ones (ie. if one icon is visible and another
   // doesn't have enough room).
   int last_x_of_icons = x +
       (browser_action_views_.size() * kButtonSize) +
       ((browser_action_views_.size() - 1) *
           kBrowserActionButtonPadding);
 
   gfx::Size sz = GetPreferredSize();
   int max_x = sz.width() - kDividerHorizontalMargin - kChevronRightMargin;
 
   // If they don't all fit, show the chevron (unless suppressed).
-  gfx::Size chevron_size;
   if (last_x_of_icons >= max_x && !suppress_chevron_) {
     chevron_->SetVisible(true);
-    chevron_size = chevron_->GetPreferredSize();
+    gfx::Size chevron_size(chevron_->GetPreferredSize());
     max_x -= chevron_size.width();
     chevron_->SetBounds(width() - chevron_size.width() - kChevronRightMargin,
                         kChevronTopMargin,
                         chevron_size.width(), chevron_size.height());
   } else {
     chevron_->SetVisible(false);
   }
 
   // Now draw the icons for the browser actions in the available space.
   for (size_t i = 0; i < browser_action_views_.size(); ++i) {
@@ skipping 14 matching lines @@
 void BrowserActionsContainer::Paint(gfx::Canvas* canvas) {
   // The one-pixel themed vertical divider to the right of the browser actions.
   int x = base::i18n::IsRTL() ?
       kDividerHorizontalMargin : (width() - kDividerHorizontalMargin);
   DetachableToolbarView::PaintVerticalDivider(
       canvas, x, height(), kDividerVerticalPadding,
       DetachableToolbarView::kEdgeDividerColor,
       DetachableToolbarView::kMiddleDividerColor,
       GetThemeProvider()->GetColor(BrowserThemeProvider::COLOR_TOOLBAR));
 
-  // The two-pixel width drop indicator.
+  // TODO(sky/glen): Instead of using a drop indicator, animate the icons while
+  // dragging (like we do for tab dragging).
   if (drop_indicator_position_ > -1) {
-    x = drop_indicator_position_;
-    int y = kDividerVerticalPadding;
-    gfx::Rect indicator_bounds(x - kDropIndicatorWidth / 2,
-                               y,
-                               kDropIndicatorWidth,
-                               height() - (2 * kDividerVerticalPadding));
+    // The two-pixel width drop indicator.
+    static const int kDropIndicatorWidth = 2;

[Finnur, 2010/07/27 09:02:23]

Is this here because the other one was moved? Just curious...

[Peter Kasting, 2010/07/27 18:24:43]

Since it's only used here it seemed clearer to just declare it here.  This is
also motivated by the fact that, after the upcoming functional change, there are
almost no constants atop this file.

+    gfx::Rect indicator_bounds(
+        drop_indicator_position_ - (kDropIndicatorWidth / 2),
+        kDividerVerticalPadding, kDropIndicatorWidth,
+        height() - (2 * kDividerVerticalPadding));
 
-    // TODO(sky/glen): make me pretty!
+    // Color of the drop indicator.
+    static const SkColor kDropIndicatorColor = SK_ColorBLACK;
     canvas->FillRectInt(kDropIndicatorColor, indicator_bounds.x(),
                         indicator_bounds.y(), indicator_bounds.width(),
                         indicator_bounds.height());
   }
 }
 
 void BrowserActionsContainer::ViewHierarchyChanged(bool is_add,
                                                    views::View* parent,
                                                    views::View* child) {
   // No extensions (e.g., incognito).
   if (!model_)
     return;
 
   if (is_add && child == this) {
     // Initial toolbar button creation and placement in the widget hierarchy.
     // We do this here instead of in the constructor because AddBrowserAction
     // calls Layout on the Toolbar, which needs this object to be constructed
     // before its Layout function is called.
     CreateBrowserActionViews();
   }
 }
 
 bool BrowserActionsContainer::GetDropFormats(
-    int* formats, std::set<OSExchangeData::CustomFormat>* custom_formats) {
+    int* formats,
+    std::set<OSExchangeData::CustomFormat>* custom_formats) {
   custom_formats->insert(BrowserActionDragData::GetBrowserActionCustomFormat());
 
   return true;
 }
 
 bool BrowserActionsContainer::AreDropTypesRequired() {
   return true;
 }
 
 bool BrowserActionsContainer::CanDrop(const OSExchangeData& data) {
   BrowserActionDragData drop_data;
-  if (!drop_data.Read(data))
-    return false;
-  return drop_data.IsFromProfile(profile_);
+  return drop_data.Read(data) ? drop_data.IsFromProfile(profile_) : false;
 }
 
 void BrowserActionsContainer::OnDragEntered(
     const views::DropTargetEvent& event) {
 }
 
 int BrowserActionsContainer::OnDragUpdated(
     const views::DropTargetEvent& event) {
   // First check if we are above the chevron (overflow) menu.
   if (GetViewForPoint(event.location()) == chevron_) {
     if (show_menu_task_factory_.empty() && !overflow_menu_)
       StartShowFolderDropMenuTimer();
     return DragDropTypes::DRAG_MOVE;
-  } else {
-    StopShowFolderDropMenuTimer();
   }
+  StopShowFolderDropMenuTimer();
 
   // Modifying the x value before clamping affects how far you have to drag to
   // get the drop indicator to shift to another position. Modifying after
   // clamping affects where the drop indicator is drawn.
 
   // We add half a button size so that when you drag a button to the right and
   // you are half-way dragging across a button the drop indicator moves from the
   // left of that button to the right of that button.
   int x = event.x() + (kButtonSize / 2) + (2 * kBrowserActionButtonPadding);
   if (chevron_->IsVisible())
@@ skipping 21 matching lines @@
   SchedulePaint();
 }
 
 int BrowserActionsContainer::OnPerformDrop(
     const views::DropTargetEvent& event) {
   BrowserActionDragData data;
   if (!data.Read(event.GetData()))
     return DragDropTypes::DRAG_NONE;
 
   // Make sure we have the same view as we started with.
-  DCHECK(browser_action_views_[data.index()]->button()->extension()->id() ==
-         data.id());
+  DCHECK_EQ(browser_action_views_[data.index()]->button()->extension()->id(),
+            data.id());
   DCHECK(model_);
 
-  Extension* dragging =
-      browser_action_views_[data.index()]->button()->extension();
-
-  int target_x = drop_indicator_position_;
-
   size_t i = 0;
   for (; i < browser_action_views_.size(); ++i) {
     int view_x =
         browser_action_views_[i]->GetBounds(APPLY_MIRRORING_TRANSFORMATION).x();
     if (!browser_action_views_[i]->IsVisible() ||
-        (base::i18n::IsRTL() ? view_x < target_x : view_x >= target_x)) {
+        (base::i18n::IsRTL() ? (view_x < drop_indicator_position_) :
+            (view_x >= drop_indicator_position_))) {
       // We have reached the end of the visible icons or found one that has a
       // higher x position than the drop point.
       break;
     }
   }
 
   // |i| now points to the item to the right of the drop indicator*, which is
   // correct when dragging an icon to the left. When dragging to the right,
   // however, we want the icon being dragged to get the index of the item to
   // the left of the drop indicator, so we subtract one.
   // * Well, it can also point to the end, but not when dragging to the left. :)
   if (i > data.index())
     --i;
 
   if (profile_->IsOffTheRecord())
     i = model_->IncognitoIndexToOriginal(i);
 
-  model_->MoveBrowserAction(dragging, i);
+  model_->MoveBrowserAction(
+      browser_action_views_[data.index()]->button()->extension(), i);
 
   OnDragExited();  // Perform clean up after dragging.
   return DragDropTypes::DRAG_MOVE;
 }
 
 bool BrowserActionsContainer::GetAccessibleRole(
     AccessibilityTypes::Role* role) {
   DCHECK(role);
   *role = AccessibilityTypes::ROLE_GROUPING;
   return true;
 }
 
-void BrowserActionsContainer::MoveBrowserAction(
-    const std::string& extension_id, size_t new_index) {
-  ExtensionsService* service = profile_->GetExtensionsService();
-  if (service) {
-    Extension* extension = service->GetExtensionById(extension_id, false);
-    model_->MoveBrowserAction(extension, new_index);
-    SchedulePaint();
-  }
-}
-
 void BrowserActionsContainer::RunMenu(View* source, const gfx::Point& pt) {
   if (source == chevron_) {
     overflow_menu_ = new BrowserActionOverflowMenuController(
         this, chevron_, browser_action_views_, VisibleBrowserActions());
     overflow_menu_->set_observer(this);
     overflow_menu_->RunMenu(GetWindow()->GetNativeWindow(), false);
   }
 }
 
 void BrowserActionsContainer::WriteDragData(View* sender,
@@ skipping 23 matching lines @@
                                                const gfx::Point& p) {
   return DragDropTypes::DRAG_MOVE;
 }
 
 bool BrowserActionsContainer::CanStartDrag(View* sender,
                                            const gfx::Point& press_pt,
                                            const gfx::Point& p) {
   return true;
 }
 
-int BrowserActionsContainer::ClampToNearestIconCount(
-    int pixelWidth, bool allow_shrink_to_minimum) const {
-  // Calculate the width of one icon.
-  int icon_width = (kButtonSize + kBrowserActionButtonPadding);
+void BrowserActionsContainer::OnResize(int resize_amount, bool done_resizing) {
+  if (!done_resizing) {
+    resize_amount_ = resize_amount;
+    OnBrowserActionVisibilityChanged();
+  } else {
+    // For details on why we do the following see the class comments in the
+    // header.
 
-  // Calculate pixel count for the area not used by the icons.
-  int extras = WidthOfNonIconArea();
+    // Clamp lower limit to 0 and upper limit to the amount that allows enough
+    // room for all icons to show.
+    int new_width = std::max(0, container_width_ - resize_amount);
+    int max_width = ClampToNearestIconCount(-1, false);
+    new_width = std::min(new_width, max_width);
 
-  size_t icon_count = 0u;
-  if (pixelWidth >= 0) {
-    // Caller wants to know how many icons fit within a given space so we start
-    // by subtracting the padding, resize area and dividers.
-    int icon_area = pixelWidth - extras;
-    icon_area = std::max(0, icon_area);
+    // Up until now we've only been modifying the resize_amount, but now it is
+    // time to set the container size to the size we have resized to, but then
+    // animate to the nearest icon count size (or down to min size if no icon).
+    container_width_ = new_width;
+    animation_target_size_ = ClampToNearestIconCount(new_width, true);
+    resize_animation_->Reset();
+    resize_animation_->SetTweenType(Tween::EASE_OUT);
+    resize_animation_->Show();
+  }
+}
 
-    // Make sure we never throw an icon into the chevron menu just because
-    // there isn't enough enough space for the invisible padding around buttons.
-    icon_area += kBrowserActionButtonPadding - 1;
+void BrowserActionsContainer::AnimationProgressed(const Animation* animation) {
+  DCHECK_EQ(resize_animation_.get(), animation);
+  resize_amount_ = static_cast<int>(resize_animation_->GetCurrentValue() *
+      (container_width_ - animation_target_size_));
+  OnBrowserActionVisibilityChanged();
+}
 
-    // Count the number of icons that fit within that area.
-    icon_count = icon_area / icon_width;
+void BrowserActionsContainer::AnimationEnded(const Animation* animation) {
+  container_width_ = animation_target_size_;
+  animation_target_size_ = 0;
+  resize_amount_ = 0;
+  OnBrowserActionVisibilityChanged();
+  suppress_chevron_ = false;
 
-    if (icon_count == 0 && allow_shrink_to_minimum) {
-      extras = ContainerMinSize();  // Allow very narrow width if no icons.
-    } else if (icon_count > browser_action_views_.size()) {
-      // No use allowing more than what we have.
-      icon_count = browser_action_views_.size();
-    }
-  } else {
-    // A negative |pixels| count indicates caller wants to know the max width
-    // that fits all icons;
-    icon_count = browser_action_views_.size();
+  // Don't save the icon count in incognito because there may be fewer icons
+  // in that mode. The result is that the container in a normal window is always
+  // at least as wide as in an incognito window.
+  if (!profile_->IsOffTheRecord())
+    model_->SetVisibleIconCount(VisibleBrowserActions());
+}
+
+void BrowserActionsContainer::NotifyMenuDeleted(
+    BrowserActionOverflowMenuController* controller) {
+  DCHECK(controller == overflow_menu_);
+  overflow_menu_ = NULL;
+}
+
+void BrowserActionsContainer::InspectPopup(ExtensionAction* action) {
+  OnBrowserActionExecuted(GetBrowserActionView(action)->button(), true);
+}
+
+void BrowserActionsContainer::ExtensionPopupIsClosing(ExtensionPopup* popup) {
+  // ExtensionPopup is ref-counted, so we don't need to delete it.
+  DCHECK_EQ(popup_, popup);
+  popup_ = NULL;
+  popup_button_->SetButtonNotPushed();
+  popup_button_ = NULL;
+}
+
+void BrowserActionsContainer::MoveBrowserAction(const std::string& extension_id,
+                                                size_t new_index) {
+  ExtensionsService* service = profile_->GetExtensionsService();
+  if (service) {
+    Extension* extension = service->GetExtensionById(extension_id, false);
+    model_->MoveBrowserAction(extension, new_index);
+    SchedulePaint();
   }
+}
 
-  return extras + (icon_count * icon_width);
+void BrowserActionsContainer::HidePopup() {
+  if (popup_)
+    popup_->Close();
+}
+
+void BrowserActionsContainer::TestExecuteBrowserAction(int index) {
+  BrowserActionButton* button = browser_action_views_[index]->button();
+  OnBrowserActionExecuted(button, false);  // |inspect_with_devtools|.
+}
+
+void BrowserActionsContainer::TestSetIconVisibilityCount(size_t icons) {
+  chevron_->SetVisible(icons < browser_action_views_.size());
+  container_width_ = IconCountToWidth(icons);
+  Layout();
+  SchedulePaint();
 }
 
 void BrowserActionsContainer::BrowserActionAdded(Extension* extension,
                                                  int index) {
 #if defined(DEBUG)
   for (size_t i = 0; i < browser_action_views_.size(); ++i) {
     DCHECK(browser_action_views_[i]->button()->extension() != extension) <<
            "Asked to add a browser action view for an extension that already "
            "exists.";
   }
 #endif
   CloseOverflowMenu();
 
   if (!ShouldDisplayBrowserAction(extension))
     return;
 
+  size_t visible_actions = VisibleBrowserActions();
+
+  // Add the new browser action to the vector and the view hierarchy.
   if (profile_->IsOffTheRecord())
     index = model_->OriginalIndexToIncognito(index);
 
-  // Before we change anything, determine the number of visible browser actions.
-  size_t visible_actions = VisibleBrowserActions();
-
-  // Add the new browser action to the vector and the view hierarchy.
   BrowserActionView* view = new BrowserActionView(extension, this);
   browser_action_views_.insert(browser_action_views_.begin() + index, view);
   AddChildView(index, view);
 
   // If we are still initializing the container, don't bother animating.
   if (!model_->extensions_initialized())
     return;
 
   // For details on why we do the following see the class comments in the
   // header.
@@ skipping 14 matching lines @@
     Animate(Tween::LINEAR, target_size);
   }
 }
 
 void BrowserActionsContainer::BrowserActionRemoved(Extension* extension) {
   CloseOverflowMenu();
 
   if (popup_ && popup_->host()->extension() == extension)
     HidePopup();
 
-  // Before we change anything, determine the number of visible browser
-  // actions.
   size_t visible_actions = VisibleBrowserActions();
-
-  for (BrowserActionViews::iterator iter =
-       browser_action_views_.begin(); iter != browser_action_views_.end();
-       ++iter) {
+  for (BrowserActionViews::iterator iter = browser_action_views_.begin();
+       iter != browser_action_views_.end(); ++iter) {
     if ((*iter)->button()->extension() == extension) {
       RemoveChildView(*iter);
       delete *iter;
       browser_action_views_.erase(iter);
 
       // If the extension is being upgraded we don't want the bar to shrink
       // because the icon is just going to get re-added to the same location.
       if (extension->being_upgraded())
         return;
 
@@ skipping 31 matching lines @@
 }
 
 void BrowserActionsContainer::ModelLoaded() {
   SetContainerWidth();
 }
 
 void BrowserActionsContainer::SetContainerWidth() {
   int visible_actions = model_->GetVisibleIconCount();
   if (visible_actions < 0)  // All icons should be visible.
     visible_actions = model_->size();
-  else
-    chevron_->SetVisible(true);
-  container_size_ = gfx::Size(IconCountToWidth(visible_actions), kButtonSize);
+  chevron_->SetVisible(static_cast<size_t>(visible_actions) < model_->size());
+  container_width_ = IconCountToWidth(visible_actions);
+}
+
+void BrowserActionsContainer::CloseOverflowMenu() {
+  if (overflow_menu_)
+    overflow_menu_->CancelMenu();
+}
+
+void BrowserActionsContainer::StopShowFolderDropMenuTimer() {
+  show_menu_task_factory_.RevokeAll();
+}
+
+void BrowserActionsContainer::StartShowFolderDropMenuTimer() {
+  int delay = View::GetMenuShowDelay();
+  MessageLoop::current()->PostDelayedTask(FROM_HERE,
+      show_menu_task_factory_.NewRunnableMethod(
+          &BrowserActionsContainer::ShowDropFolder),
+      delay);
+}
+
+void BrowserActionsContainer::ShowDropFolder() {
+  DCHECK(!overflow_menu_);
+  SetDropIndicator(-1);
+  overflow_menu_ = new BrowserActionOverflowMenuController(
+      this, chevron_, browser_action_views_, VisibleBrowserActions());
+  overflow_menu_->set_observer(this);
+  overflow_menu_->RunMenu(GetWindow()->GetNativeWindow(), true);
+}
+
+void BrowserActionsContainer::SetDropIndicator(int x_pos) {
+  if (drop_indicator_position_ != x_pos) {
+    drop_indicator_position_ = x_pos;
+    SchedulePaint();
+  }
+}
+
+int BrowserActionsContainer::ClampToNearestIconCount(
+    int pixelWidth,
+    bool allow_shrink_to_minimum) const {
+  // Calculate the width of one icon.
+  int icon_width = (kButtonSize + kBrowserActionButtonPadding);
+
+  // Calculate pixel count for the area not used by the icons.
+  int extras = WidthOfNonIconArea();
+
+  size_t icon_count = 0u;
+  if (pixelWidth >= 0) {
+    // Caller wants to know how many icons fit within a given space so we start
+    // by subtracting the padding, resize area and dividers.
+    int icon_area = pixelWidth - extras;
+    icon_area = std::max(0, icon_area);
+
+    // Make sure we never throw an icon into the chevron menu just because
+    // there isn't enough enough space for the invisible padding around buttons.
+    icon_area += kBrowserActionButtonPadding - 1;
+
+    // Count the number of icons that fit within that area.
+    icon_count = icon_area / icon_width;
+
+    if (icon_count == 0 && allow_shrink_to_minimum) {
+      extras = ContainerMinSize();  // Allow very narrow width if no icons.
+    } else if (icon_count > browser_action_views_.size()) {
+      // No use allowing more than what we have.
+      icon_count = browser_action_views_.size();
+    }
+  } else {
+    // A negative |pixels| count indicates caller wants to know the max width
+    // that fits all icons;
+    icon_count = browser_action_views_.size();
+  }
+
+  return extras + (icon_count * icon_width);
 }
 
 int BrowserActionsContainer::WidthOfNonIconArea() const {
   int chevron_size = (chevron_->IsVisible()) ?
                      chevron_->GetPreferredSize().width() : 0;
   int padding = base::i18n::IsRTL() ?
       kHorizontalPaddingRtl : kHorizontalPadding;
   return kResizeAreaWidth + padding + chevron_size + kChevronRightMargin +
       kDividerHorizontalMargin;
 }
@@ skipping 19 matching lines @@
     resize_animation_->Reset();
     resize_animation_->SetTweenType(tween_type);
     animation_target_size_ = target_size;
     resize_animation_->Show();
   } else {
     animation_target_size_ = target_size;
     AnimationEnded(resize_animation_.get());
   }
 }
 
-size_t BrowserActionsContainer::VisibleBrowserActions() const {
-  size_t visible_actions = 0;
-  for (size_t i = 0; i < browser_action_views_.size(); ++i) {
-    if (browser_action_views_[i]->IsVisible())
-      ++visible_actions;
-  }
-
-  return visible_actions;
-}
-
-void BrowserActionsContainer::OnResize(int resize_amount, bool done_resizing) {
-  if (!done_resizing) {
-    resize_amount_ = resize_amount;
-    OnBrowserActionVisibilityChanged();
-  } else {
-    // For details on why we do the following see the class comments in the
-    // header.
-
-    // Clamp lower limit to 0 and upper limit to the amount that allows enough
-    // room for all icons to show.
-    int new_width = std::max(0, container_size_.width() - resize_amount);
-    int max_width = ClampToNearestIconCount(-1, false);
-    new_width = std::min(new_width, max_width);
-
-    // Up until now we've only been modifying the resize_amount, but now it is
-    // time to set the container size to the size we have resized to, but then
-    // animate to the nearest icon count size (or down to min size if no icon).
-    container_size_.set_width(new_width);
-    animation_target_size_ = ClampToNearestIconCount(new_width, true);
-    resize_animation_->Reset();
-    resize_animation_->SetTweenType(Tween::EASE_OUT);
-    resize_animation_->Show();
-  }
-}
-
-void BrowserActionsContainer::AnimationProgressed(const Animation* animation) {
-  DCHECK(animation == resize_animation_.get());
-
-  double e = resize_animation_->GetCurrentValue();
-  int difference = container_size_.width() - animation_target_size_;
-
-  resize_amount_ = static_cast<int>(e * difference);
-
-  OnBrowserActionVisibilityChanged();
-}
-
-void BrowserActionsContainer::AnimationEnded(const Animation* animation) {
-  container_size_.set_width(animation_target_size_);
-  animation_target_size_ = 0;
-  resize_amount_ = 0;
-  OnBrowserActionVisibilityChanged();
-  suppress_chevron_ = false;
-
-  // Don't save the icon count in incognito because there may be fewer icons
-  // in that mode. The result is that the container in a normal window is always
-  // at least as wide as in an incognito window.
-  if (!profile_->IsOffTheRecord())
-    model_->SetVisibleIconCount(VisibleBrowserActions());
-}
-
-void BrowserActionsContainer::NotifyMenuDeleted(
-    BrowserActionOverflowMenuController* controller) {
-  DCHECK(controller == overflow_menu_);
-  overflow_menu_ = NULL;
-}
-
-void BrowserActionsContainer::InspectPopup(
-    ExtensionAction* action) {
-  OnBrowserActionExecuted(GetBrowserActionView(action)->button(),
-      true);  // |inspect_with_devtools|.
-}
-
-void BrowserActionsContainer::ExtensionPopupIsClosing(ExtensionPopup* popup) {
-  // ExtensionPopup is ref-counted, so we don't need to delete it.
-  DCHECK_EQ(popup_, popup);
-  popup_ = NULL;
-  popup_button_->SetButtonNotPushed();
-  popup_button_ = NULL;
-}
-
 bool BrowserActionsContainer::ShouldDisplayBrowserAction(Extension* extension) {
   // Only display incognito-enabled extensions while in incognito mode.
   return (!profile_->IsOffTheRecord() ||
           profile_->GetExtensionsService()->IsIncognitoEnabled(extension));
 }