Move IN_OVERFLOW from Panel::ExpansionState to new enum.

chrome/browser/ui/panels/panel_strip.cc

 // Copyright (c) 2012 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/panels/panel_strip.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/logging.h"
@@ skipping 63 matching lines @@
   gfx::Rect old_area = display_area_;
   display_area_ = new_area;
 
   if (panels_.empty())
     return;
 
   Rearrange();
 }
 
 void PanelStrip::AddPanel(Panel* panel) {
-  DCHECK_NE(Panel::IN_OVERFLOW, panel->expansion_state());
+  DCHECK_NE(Panel::IN_OVERFLOW, panel->layout_state());
 
   // Always update limits, even for exiting panels, in case the maximums changed
   // while panel was out of the strip.
   int max_panel_width = GetMaxPanelWidth();
   int max_panel_height = GetMaxPanelHeight();
   panel->SetSizeRange(gfx::Size(kPanelMinWidth, kPanelMinHeight),
                       gfx::Size(max_panel_width, max_panel_height));
 
   gfx::Size restored_size = panel->restored_size();
   int height = restored_size.height();
   int width = restored_size.width();
 
   if (panel->initialized()) {
     // Bump panels in the strip to make room for this panel.
     int x;
     while ((x = GetRightMostAvailablePosition() - width) < display_area_.x()) {
       DCHECK(!panels_.empty());
-      panels_.back()->SetExpansionState(Panel::IN_OVERFLOW);
+      panels_.back()->SetLayoutState(Panel::IN_OVERFLOW);
     }
     int y = display_area_.bottom() - height;
     panel->SetPanelBounds(gfx::Rect(x, y, width, height));
+
+    if (panel->expansion_state() == Panel::TITLE_ONLY ||
+        panel->expansion_state() == Panel::MINIMIZED)
+      IncrementMinimizedPanels();
   } else {
     // Initialize the newly created panel. Does not bump any panels from strip.
     if (height == 0 && width == 0) {
       // Auto resizable is enabled only if no initial size is provided.
       panel->SetAutoResizable(true);
     } else {
       if (height == 0)
         height = width / kPanelDefaultWidthToHeightRatio;
       if (width == 0)
         width = height * kPanelDefaultWidthToHeightRatio;
@@ skipping 70 matching lines @@
   // If we're in the process of dragging, delay the removal.
   if (dragging_panel_index_ != kInvalidPanelIndex) {
     panels_pending_to_remove_.push_back(panel);
     return true;
   }
 
   DoRemove(panel);
 
   // Don't rearrange the strip if a panel is being moved from the panel strip
   // to the overflow strip.
-  if (panel->expansion_state() != Panel::IN_OVERFLOW)
+  if (panel->layout_state() == Panel::DOCKED)
     Rearrange();
 
   return true;
 }
 
 void PanelStrip::DelayedRemove() {
   for (size_t i = 0; i < panels_pending_to_remove_.size(); ++i)
     DoRemove(panels_pending_to_remove_[i]);
   panels_pending_to_remove_.clear();
   Rearrange();
@@ skipping 137 matching lines @@
   } else {
     panels_[dragging_panel_index_]->SetPanelBounds(
         dragging_panel_bounds_);
   }
 
   dragging_panel_index_ = kInvalidPanelIndex;
 
   DelayedRemove();
 }
 
-void PanelStrip::OnPanelExpansionStateChanged(Panel* panel) {
+void PanelStrip::OnPanelLayoutStateChanged(Panel* panel,
+                                          Panel::LayoutState old_state) {
+  Panel::LayoutState layout_state = panel->layout_state();
+  switch (layout_state) {
+    case Panel::DOCKED:
+      if (old_state == Panel::IN_OVERFLOW) {
+        panel_manager_->panel_overflow_strip()->Remove(panel);
+        AddPanel(panel);
+        panel->SetAppIconVisibility(true);
+        panel->set_draggable(true);
+        UpdateBoundsPerExpansionState(panel);
+      }
+      break;
+    case Panel::DETACHED:
+    case Panel::IN_OVERFLOW:
+      DCHECK_EQ(Panel::DOCKED, old_state);
+      break;
+    default:
+      NOTREACHED();
+      break;
+  }
+}
+
+void PanelStrip::OnPanelExpansionStateChanged(Panel* panel,
+                                              Panel::ExpansionState old_state) {
+  DCHECK_EQ(Panel::DOCKED, panel->layout_state());

[jennb, 2012/01/19 23:44:09]

This dcheck may fail. I saw code that called SetExpansionState(EXPANDED)
followed by SetLayoutState(DOCKED). If you really want this dcheck, it should be
in SetExpansionState() instead.

[jianli, 2012/01/20 01:16:07]

It will not fail since PanelManager::OnPanelExpansionStateChanged will only call
this when the layout state is DOCKED. Calling SetExpansionState(...) before
SetLayoutState(DOCKED) for overflow panel will only update the internal state
without doing any further work, like changing the minimized panel count and
updating the bounds. The further work mentioned above will be performed only
when we flip the layout state from IN_OVERFLOW to DOCKED.

[jennb, 2012/01/20 02:01:54]

Updating the expansion state (a piece of layout data that is 'owned' by the
panel_strip) when the panel is not docked and not telling the panel_strip seems
error prone. What about changing the order so that SetLayoutState(DOCKED) is
always called before SetExpansionState(EXPANDED)?

+
+  switch (panel->expansion_state()) {
+    case Panel::EXPANDED:
+      DecrementMinimizedPanels();
+      break;
+    case Panel::TITLE_ONLY:
+    case Panel::MINIMIZED:
+      if (old_state == Panel::EXPANDED)
+        IncrementMinimizedPanels();
+      break;
+    default:
+      NOTREACHED();
+      break;
+  }
+
+  UpdateBoundsPerExpansionState(panel);
+}
+
+void PanelStrip::UpdateBoundsPerExpansionState(Panel* panel) {
   gfx::Size size = panel->restored_size();
   Panel::ExpansionState expansion_state = panel->expansion_state();
-  Panel::ExpansionState old_state = panel->old_expansion_state();
-  if (old_state == Panel::IN_OVERFLOW) {
-    panel_manager_->panel_overflow_strip()->Remove(panel);
-    AddPanel(panel);
-    panel->SetAppIconVisibility(true);
-    panel->set_draggable(true);
-  }
   switch (expansion_state) {
     case Panel::EXPANDED:
-      if (old_state == Panel::TITLE_ONLY || old_state == Panel::MINIMIZED)
-        DecrementMinimizedPanels();
       break;
     case Panel::TITLE_ONLY:
       size.set_height(panel->TitleOnlyHeight());
-      if (old_state == Panel::EXPANDED || old_state == Panel::IN_OVERFLOW)
-        IncrementMinimizedPanels();
       break;
     case Panel::MINIMIZED:
       size.set_height(Panel::kMinimizedPanelHeight);
-      if (old_state == Panel::EXPANDED || old_state == Panel::IN_OVERFLOW)
-        IncrementMinimizedPanels();
-      break;
-    case Panel::IN_OVERFLOW:
-      if (old_state == Panel::TITLE_ONLY || old_state == Panel::MINIMIZED)
-        DecrementMinimizedPanels();
       break;
     default:
       NOTREACHED();
       break;
   }
 
   int bottom = GetBottomPositionForExpansionState(expansion_state);
   gfx::Rect bounds = panel->GetBounds();
   panel->SetPanelBounds(
       gfx::Rect(bounds.right() - size.width(),
@@ skipping 34 matching lines @@
   if (new_height < panel->min_size().height())
     new_height = panel->min_size().height();
 
   // Update restored size.
   gfx::Size new_size(new_width, new_height);
   if (new_size != panel->restored_size())
     panel->set_restored_size(new_size);
 
   // Only need to adjust bounds height when panel is expanded.
   gfx::Rect bounds = panel->GetBounds();
-  Panel::ExpansionState expansion_state = panel->expansion_state();
   if (new_height != bounds.height() &&
-      expansion_state == Panel::EXPANDED) {
+      panel->layout_state() == Panel::DOCKED &&

[jennb, 2012/01/19 23:44:09]

Instead of adding DOCKED checks in this and the next section, add 

if (!DOCKED) return;

 at line 464.

[jianli, 2012/01/20 01:16:07]

We also need to call Rearrange if an overflow panel is shrunken. Rewrote the
code here.

+      panel->expansion_state() == Panel::EXPANDED) {
     bounds.set_y(bounds.bottom() - new_height);
     bounds.set_height(new_height);
   }
 
   // Only need to adjust width if panel is in the panel strip.
   int delta_x = bounds.width() - new_width;
-  if (delta_x != 0 && expansion_state != Panel::IN_OVERFLOW) {
+  if (delta_x != 0 && panel->layout_state() == Panel::DOCKED) {
     bounds.set_width(new_width);
     bounds.set_x(bounds.x() + delta_x);
   }
 
   if (bounds != panel->GetBounds())
     panel->SetPanelBounds(bounds);
 
   // Only need to rearrange if panel's width changed.
   if (delta_x != 0)
     Rearrange();
@@ skipping 194 matching lines @@
 
   // TODO(jianli): remove the guard when overflow support is enabled on other
   // platforms. http://crbug.com/105073
 #if defined(OS_WIN)
   // Add/remove panels from/to overflow. A change in work area or the
   // resize/removal of a panel may affect how many panels fit in the strip.
   if (panel_index < panels_.size()) {
     // Move panels to overflow in reverse to maintain their order.
     for (size_t overflow_index = panels_.size() - 1;
          overflow_index >= panel_index; --overflow_index)
-      panels_[overflow_index]->SetExpansionState(Panel::IN_OVERFLOW);
+      panels_[overflow_index]->SetLayoutState(Panel::IN_OVERFLOW);
   } else {
     // Attempt to add more panels from overflow to the strip.
     PanelOverflowStrip* overflow_strip = panel_manager_->panel_overflow_strip();
     Panel* overflow_panel;
     while ((overflow_panel = overflow_strip->first_panel()) &&
            GetRightMostAvailablePosition() >=
                display_area_.x() + overflow_panel->restored_size().width()) {
       // We need to get back to the previous expansion state.
       Panel::ExpansionState expansion_state_to_restore =
-          overflow_panel->old_expansion_state();
+          overflow_panel->expansion_state();
       if (expansion_state_to_restore == Panel::MINIMIZED ||

[jennb, 2012/01/19 23:44:09]

Change this to !EXPANDED? Then we don't need two checks.

[jianli, 2012/01/20 01:16:07]

Done.

           expansion_state_to_restore == Panel::TITLE_ONLY) {
         expansion_state_to_restore = are_titlebars_up_ ? Panel::TITLE_ONLY
                                                        : Panel::MINIMIZED;
       }
       overflow_panel->SetExpansionState(expansion_state_to_restore);
+      overflow_panel->SetLayoutState(Panel::DOCKED);
     }
   }
 #endif
 }
 
 void PanelStrip::DelayedMovePanelToOverflow(Panel* panel) {
   if (panels_in_temporary_layout_.erase(panel)) {
-      DCHECK(panel->has_temporary_layout());
-      panel->SetExpansionState(Panel::IN_OVERFLOW);
+    DCHECK(panel->has_temporary_layout());
+    panel->SetLayoutState(Panel::IN_OVERFLOW);
   }
 }
 
 void PanelStrip::RemoveAll() {
   // This should only be called at the end of tests to clean up.
   DCHECK(dragging_panel_index_ == kInvalidPanelIndex);
   DCHECK(panels_in_temporary_layout_.empty());
 
   // Make a copy of the iterator as closing panels can modify the vector.
   Panels panels_copy = panels_;
 
   // Start from the bottom to avoid reshuffling.
   for (Panels::reverse_iterator iter = panels_copy.rbegin();
        iter != panels_copy.rend(); ++iter)
     (*iter)->Close();
 }
 
 bool PanelStrip::is_dragging_panel() const {
   return dragging_panel_index_ != kInvalidPanelIndex;
 }