Cleanup to keep panel from manipulating its panel strip assignment directly.

chrome/browser/ui/panels/docked_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/docked_panel_strip.h"
 
 #include <algorithm>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 68 matching lines @@
   gfx::Rect old_area = display_area_;
   display_area_ = display_area;
 
   if (panels_.empty())
     return;
 
   RefreshLayout();
 }
 
 void DockedPanelStrip::AddPanel(Panel* panel) {
-  DCHECK_EQ(this, panel->panel_strip());
+  DCHECK_NE(this, panel->panel_strip());
+  panel->SetPanelStrip(this);
 
   // 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.
     // Prevent layout refresh when panel is removed from this strip.
     disable_layout_refresh_ = true;
-    PanelStrip* overflow_strip = panel_manager_->overflow_strip();
     int x;
     while ((x = GetRightMostAvailablePosition() - width) < display_area_.x()) {
       DCHECK(!panels_.empty());
-      panels_.back()->MoveToStrip(overflow_strip);
+      panel_manager_->ChangePanelLayout(panels_.back(),
+                                        PanelStrip::IN_OVERFLOW);

[Andrei, 2012/03/01 00:08:50]

I like this change, and I can see how you are trying to abstract away the
existence of the overflow strip.

Yet I would like it to go further of course. I think all of this logic belongs
to the panel manager (who would be the only entity ever calling AddPanel). I
would add a CanFitPanel() method here and move this logic to
ChangePanelLayout(), which calls AddPanel(), and not the other way around.

Would that be in line with your thinking? And would it be too much to ask? :)

[jennb, 2012/03/01 20:52:58]

Ended up leaving this, though moved into a subroutine. Seems right for the dock
strip to decide when it is too full. Drag controller may end up adding panels.
Didn't want every place that added a panel to have to also add "make space"
logic.

     }
     disable_layout_refresh_ = false;
 
     Panel::ExpansionState expansion_state_to_restore;
     if (panel->expansion_state() == Panel::EXPANDED) {
       expansion_state_to_restore = Panel::EXPANDED;
     } else {
       if (are_titlebars_up_ || panel->IsDrawingAttention()) {
         expansion_state_to_restore = Panel::TITLE_ONLY;
         height = panel->TitleOnlyHeight();
@@ skipping 75 matching lines @@
 
 int DockedPanelStrip::StartingRightPosition() const {
   return display_area_.right();
 }
 
 int DockedPanelStrip::GetRightMostAvailablePosition() const {
   return panels_.empty() ? StartingRightPosition() :
       (panels_.back()->GetBounds().x() - kPanelsHorizontalSpacing);
 }
 
-bool DockedPanelStrip::RemovePanel(Panel* panel) {
+void DockedPanelStrip::RemovePanel(Panel* panel) {
+  DCHECK_EQ(this, panel->panel_strip());
   if (panel->has_temporary_layout()) {
     panels_in_temporary_layout_.erase(panel);
-    return true;
+    return;
   }
 
   Panels::iterator iter = find(panels_.begin(), panels_.end(), panel);
-  if (iter == panels_.end())
-    return false;
+  DCHECK(iter != panels_.end());
 
   if (panel->expansion_state() != Panel::EXPANDED)
     DecrementMinimizedPanels();
 
   // Removing an element from the list will invalidate the iterator that refers
-  // to it. So we need to check if the dragging panel iterators are affected.
-  bool update_original_dragging_iterator_after_erase = false;
-  if (dragging_panel_original_iterator_ != panels_.end()) {
-    if (dragging_panel_current_iterator_ == iter) {
-      // If the dragging panel is being removed, set both dragging iterators to
-      // the end of the list.
-      dragging_panel_current_iterator_ = dragging_panel_original_iterator_ =
-          panels_.end();
-    } else if (dragging_panel_original_iterator_ == iter) {
-      // If |dragging_panel_original_iterator_| refers to the element being
-      // removed, set the flag so that the iterator can be updated to next
-      // element.
-      update_original_dragging_iterator_after_erase = true;
-    }
-  }
+  // to it. So we need to update the iterator in that case.
+  DCHECK(dragging_panel_current_iterator_ != iter);
+  bool update_iterator_after_erase =
+      (dragging_panel_original_iterator_ == iter);
 
   iter = panels_.erase(iter);
 
-  if (update_original_dragging_iterator_after_erase)
+  if (update_iterator_after_erase)
     dragging_panel_original_iterator_ = iter;
 
   if (!disable_layout_refresh_)
     RefreshLayout();

[Andrei, 2012/03/01 00:08:50]

If AddPanel() sets panel->SetPanelStrip() to this, RemovePanel() should probably
set it to NULL? Then on add we could DCHECK that is IS null, meaning that it has
been properly removed from it's previous parent first.

[jennb, 2012/03/01 00:30:58]

Agree. I actually tried it and undid the change. Setting to NULL led to crashes
because the panel is left with a NULL panel strip and we don't have guards in
place to ensure the strip is non-NULL before use. There are timing holes where
panel is removed from the strip, but it's destructor has not been called yet.

-
-  return true;
 }
 
 bool DockedPanelStrip::CanShowPanelAsActive(const Panel* panel) const {
   // Panels with temporary layout cannot be shown as active.
   return !panel->has_temporary_layout();
 }
 
 bool DockedPanelStrip::CanDragPanel(const Panel* panel) const {
   // Only the panels having temporary layout can't be dragged.
   return !panel->has_temporary_layout();
@@ skipping 442 matching lines @@
       new_bounds.set_y(
           GetBottomPositionForExpansionState(panel->expansion_state()) -
               new_bounds.height());
       panel->SetPanelBounds(new_bounds);
     }
 
     rightmost_position = x - kPanelsHorizontalSpacing;
   }
 
   // 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.

[Andrei, 2012/03/01 00:08:50]

This is higher-level logic affecting multiple strips and panels that somehow is
implemented in the docked panel strip. Do you think we could move it to the
panel manager? I ask for a lot, I know :)
(Here is an idea): panels may go to overflow with a delay, but they always come
back instantly. Maybe we can run this logic (moving things from overflow back to
docked) asynchronously with a delay as well? After a layout refresh, we post a
task to bring some panels from overflow... This way we'll make sure panels do
not jump from one strip to another while we do something else (unless we ask
them to), and it is easy to move the code to the manager where it belongs. What
do you think???

[jennb, 2012/03/01 20:52:58]

I simplified the logic that bumps panels to overflow. Kept it in dock strip
though for same reason as AddPanel - that it's ok for dock to know when it's too
full.

I moved the logic for moving panels out of overflow to PanelManager. Agreed with
your reasoning on manipulating the overflow strip directly here. Did not use a
post task to avoid scenarios of another panel being created/activated before the
task fires, as that would mess up the ordering of the panels.

-  OverflowPanelStrip* overflow_strip = panel_manager_->overflow_strip();
   if (panel_iter != panels_.end()) {
     // Prevent layout refresh when panel is removed from this strip.
     disable_layout_refresh_ = true;
 
     // Keep track of panels to move to overflow in a separate storage since
     // removing it from the list will invalidate the iterator.
     std::vector<Panel*> panels_to_move_to_overflow;
     for (; panel_iter != panels_.end(); ++panel_iter)
       panels_to_move_to_overflow.push_back(*panel_iter);
 
     // Move panels to overflow in reverse to maintain their order.
     for (std::vector<Panel*>::reverse_iterator iter =
              panels_to_move_to_overflow.rbegin();
          iter != panels_to_move_to_overflow.rend(); ++iter) {
-      (*iter)->MoveToStrip(overflow_strip);
+      panel_manager_->ChangePanelLayout(*iter, PanelStrip::IN_OVERFLOW);
     }
 
     disable_layout_refresh_ = false;
   } else {
     // Attempt to add more panels from overflow to the strip.
+    OverflowPanelStrip* overflow_strip = panel_manager_->overflow_strip();
     Panel* overflow_panel;
     while ((overflow_panel = overflow_strip->first_panel()) &&
            GetRightMostAvailablePosition() >=
                display_area_.x() + overflow_panel->restored_size().width()) {
-      overflow_panel->MoveToStrip(this);
+      panel_manager_->ChangePanelLayout(overflow_panel, PanelStrip::DOCKED);
     }
   }
 }
 
 void DockedPanelStrip::DelayedMovePanelToOverflow(Panel* panel) {

[Andrei, 2012/03/01 00:08:50]

This can be easily moved to the manager I think.

[jennb, 2012/03/01 20:52:58]

Ended up keeping this here. This provides a guard against the panel being closed
before the timer invokes this method.

   if (panels_in_temporary_layout_.erase(panel)) {
       DCHECK(panel->has_temporary_layout());
-      panel->MoveToStrip(panel_manager_->overflow_strip());
+      panel_manager_->ChangePanelLayout(panel, PanelStrip::IN_OVERFLOW);
   }
 }
 
 void DockedPanelStrip::CloseAll() {
   // This should only be called at the end of tests to clean up.
   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();
 }