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

chrome/browser/ui/panels/docked_panel_strip.cc

Index: chrome/browser/ui/panels/docked_panel_strip.cc
diff --git a/chrome/browser/ui/panels/docked_panel_strip.cc b/chrome/browser/ui/panels/docked_panel_strip.cc
index d4cd227cac19fd76788d5eb3f63be9239b8b3f14..4b5dcb946517725e91a8b77e1ad1c938ae796553 100644
--- a/chrome/browser/ui/panels/docked_panel_strip.cc
+++ b/chrome/browser/ui/panels/docked_panel_strip.cc
@@ -86,7 +86,8 @@ void DockedPanelStrip::SetDisplayArea(const gfx::Rect& display_area) {
 }
 
 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.
@@ -103,11 +104,11 @@ void DockedPanelStrip::AddPanel(Panel* panel) {
     // 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;
 
@@ -203,45 +204,32 @@ int DockedPanelStrip::GetRightMostAvailablePosition() const {
       (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 {
@@ -705,7 +693,6 @@ void DockedPanelStrip::RefreshLayout() {
 
   // 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;
@@ -720,17 +707,18 @@ void DockedPanelStrip::RefreshLayout() {
     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);
     }
   }
 }
@@ -738,7 +726,7 @@ void DockedPanelStrip::RefreshLayout() {
 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);
   }
 }