|
|
Created:
8 years, 9 months ago by jennb Modified:
8 years, 9 months ago CC:
chromium-reviews, Dmitry Lomov (no reviews), Dmitry Titov, jianli, dcheng Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCleanup to keep panel from manipulating its panel strip assignment directly.
Only PanelManager adds/removes panels from panel strips.
Only exception is in tests.
BUG=None
TEST=Existing tests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124806
Patch Set 1 #
Total comments: 17
Patch Set 2 : feedback changes #Patch Set 3 : removed unneeded includes #
Total comments: 14
Patch Set 4 : more logic moved to panel manager #Patch Set 5 : removed deleted method from .h #
Total comments: 3
Patch Set 6 : more feedback changes #
Total comments: 12
Patch Set 7 : addressed final nits #Messages
Total messages: 18 (0 generated)
OK, here's a bunch of comments. Please feel free to disagree :) Also some of them probably go beyond what you planned for this patch, so it's alright to leave those out. http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/detach... File chrome/browser/ui/panels/detached_panel_strip.cc (right): http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/detach... chrome/browser/ui/panels/detached_panel_strip.cc:44: panels_.erase(panel); Same comment about SetPaneStrip() as for the docked strip. http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/docked... File chrome/browser/ui/panels/docked_panel_strip.cc (right): http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/docked... chrome/browser/ui/panels/docked_panel_strip.cc:111: PanelStrip::IN_OVERFLOW); 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? :) http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/docked... chrome/browser/ui/panels/docked_panel_strip.cc:232: RefreshLayout(); 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. http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/docked... chrome/browser/ui/panels/docked_panel_strip.cc:695: // resize/removal of a panel may affect how many panels fit in the strip. 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??? http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/docked... chrome/browser/ui/panels/docked_panel_strip.cc:726: void DockedPanelStrip::DelayedMovePanelToOverflow(Panel* panel) { This can be easily moved to the manager I think. http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/overfl... File chrome/browser/ui/panels/overflow_panel_strip.cc (right): http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/overfl... chrome/browser/ui/panels/overflow_panel_strip.cc:140: UpdateOverflowIndicatorCount(); Same comment about SetPanelStrip() as for the docked strip http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/overfl... chrome/browser/ui/panels/overflow_panel_strip.cc:166: panel_manager_->ChangePanelLayout(panel, PanelStrip::DOCKED); Ideally making sure a panel CAN be activated before calling this should be the job of the caller. I'll put it this way: changing panel layout for a panel is a system-wide side effect. Are the callers always prepared and aware of the fact that, by calling ActivatePanel(), they are making a change that reaches far beyond this panel? Of course I am not sure how costly it would be to change :) http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/overfl... chrome/browser/ui/panels/overflow_panel_strip.cc:177: panel_manager_->ChangePanelLayout(panel, PanelStrip::DOCKED); Same as above. http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/panel.cc File chrome/browser/ui/panels/panel.cc (right): http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/panel.... chrome/browser/ui/panels/panel.cc:151: void Panel::SetPanelStrip(PanelStrip* new_strip) { Yes!!! http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/panel_... File chrome/browser/ui/panels/panel_overflow_browsertest.cc (right): http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_overflow_browsertest.cc:652: panel1->SetPanelStrip(NULL); That would nto be needed if RemovePanel() set it to NULL automatically.
No new revision yet. A few answers below. I'm working on moving the inter-strip logic in docked_panel_strip to panel_manager. I think it can be done. http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/docked... File chrome/browser/ui/panels/docked_panel_strip.cc (right): http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/docked... chrome/browser/ui/panels/docked_panel_strip.cc:232: RefreshLayout(); On 2012/03/01 00:08:50, Andrei wrote: > 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. 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. http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/overfl... File chrome/browser/ui/panels/overflow_panel_strip.cc (right): http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/overfl... chrome/browser/ui/panels/overflow_panel_strip.cc:166: panel_manager_->ChangePanelLayout(panel, PanelStrip::DOCKED); On 2012/03/01 00:08:50, Andrei wrote: > Ideally making sure a panel CAN be activated before calling this should be the > job of the caller. > I'll put it this way: changing panel layout for a panel is a system-wide side > effect. Are the callers always prepared and aware of the fact that, by calling > ActivatePanel(), they are making a change that reaches far beyond this panel? > > Of course I am not sure how costly it would be to change :) For this activation path, the panel can always be activated. This is how a minimized panel gets restored to expanded mode, or an overflow panel gets moved back to the dock strip. One usage of this is when extensions want to give focus to a panel. http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/overfl... chrome/browser/ui/panels/overflow_panel_strip.cc:177: panel_manager_->ChangePanelLayout(panel, PanelStrip::DOCKED); On 2012/03/01 00:08:50, Andrei wrote: > Same as above. Ditto above; this is always allowed. http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/panel_... File chrome/browser/ui/panels/panel_overflow_browsertest.cc (right): http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_overflow_browsertest.cc:652: panel1->SetPanelStrip(NULL); On 2012/03/01 00:08:50, Andrei wrote: > That would nto be needed if RemovePanel() set it to NULL automatically. Agree. This is the test that led me to try setting NULL in RemovePanel(). I opted to add this one line rather than adding guards against NULL everywhere that panel strip is accessed from the panel.
On Wed, Feb 29, 2012 at 4:30 PM, <jennb@chromium.org> wrote: > No new revision yet. A few answers below. I'm working on moving the > inter-strip > logic in docked_panel_strip to panel_manager. I think it can be done. > > Wow! :) > > > http://codereview.chromium.**org/9560002/diff/1/chrome/** > browser/ui/panels/docked_**panel_strip.cc<http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/docked_panel_strip.cc> > File chrome/browser/ui/panels/**docked_panel_strip.cc (right): > > http://codereview.chromium.**org/9560002/diff/1/chrome/** > browser/ui/panels/docked_**panel_strip.cc#newcode232<http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/docked_panel_strip.cc#newcode232> > chrome/browser/ui/panels/**docked_panel_strip.cc:232: RefreshLayout(); > > On 2012/03/01 00:08:50, Andrei wrote: > >> 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. >> > > 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. OK, maybe not now, but it would be nice to have such guards, along with a bit that tells us that the panel is in limbo between strips. It is scary having panels claiming to be in a strip that does not recognize them. > > > http://codereview.chromium.**org/9560002/diff/1/chrome/** > browser/ui/panels/overflow_**panel_strip.cc<http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/overflow_panel_strip.cc> > File chrome/browser/ui/panels/**overflow_panel_strip.cc (right): > > http://codereview.chromium.**org/9560002/diff/1/chrome/** > browser/ui/panels/overflow_**panel_strip.cc#newcode166<http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/overflow_panel_strip.cc#newcode166> > chrome/browser/ui/panels/**overflow_panel_strip.cc:166: > panel_manager_->**ChangePanelLayout(panel, PanelStrip::DOCKED); > On 2012/03/01 00:08:50, Andrei wrote: > >> Ideally making sure a panel CAN be activated before calling this >> > should be the > >> job of the caller. >> I'll put it this way: changing panel layout for a panel is a >> > system-wide side > >> effect. Are the callers always prepared and aware of the fact that, by >> > calling > >> ActivatePanel(), they are making a change that reaches far beyond this >> > panel? > > Of course I am not sure how costly it would be to change :) >> > > For this activation path, the panel can always be activated. This is how > a minimized panel gets restored to expanded mode, or an overflow panel > gets moved back to the dock strip. One usage of this is when extensions > want to give focus to a panel. I would think of having two methods, ActivatePanel() and ActivatePanelInternal() -- the former to be called exclusively by outside clients and the latter to be used in the panels code. > > > http://codereview.chromium.**org/9560002/diff/1/chrome/** > browser/ui/panels/overflow_**panel_strip.cc#newcode177<http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/overflow_panel_strip.cc#newcode177> > chrome/browser/ui/panels/**overflow_panel_strip.cc:177: > panel_manager_->**ChangePanelLayout(panel, PanelStrip::DOCKED); > On 2012/03/01 00:08:50, Andrei wrote: > >> Same as above. >> > > Ditto above; this is always allowed. > > > http://codereview.chromium.**org/9560002/diff/1/chrome/** > browser/ui/panels/panel_**overflow_browsertest.cc<http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/panel_overflow_browsertest.cc> > File chrome/browser/ui/panels/**panel_overflow_browsertest.cc (right): > > http://codereview.chromium.**org/9560002/diff/1/chrome/** > browser/ui/panels/panel_**overflow_browsertest.cc#**newcode652<http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/panel_overflow_browsertest.cc#newcode652> > chrome/browser/ui/panels/**panel_overflow_browsertest.cc:**652: > panel1->SetPanelStrip(NULL); > On 2012/03/01 00:08:50, Andrei wrote: > >> That would nto be needed if RemovePanel() set it to NULL >> > automatically. > > Agree. This is the test that led me to try setting NULL in > RemovePanel(). I opted to add this one line rather than adding guards > against NULL everywhere that panel strip is accessed from the panel. > > http://codereview.chromium.**org/9560002/<http://codereview.chromium.org/9560... >
I didn't quite give you everything you wanted, but in exchange, I added the set_panel_strip(NULL) to RemovePanel() and added guards. The imbalance with AddPanel was getting to my OCD sensors. Also cleaned up Panel::IsMinimized to stop digging into strip logic. Didn't touch Activate() vs ActivateInternal() in this patch. When the OS activates a panel, it doesn't use Activate(), rather it tells the panel "you've already been activated" which uses a different code path, so the current code should be fine. http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/docked... File chrome/browser/ui/panels/docked_panel_strip.cc (right): http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/docked... chrome/browser/ui/panels/docked_panel_strip.cc:111: PanelStrip::IN_OVERFLOW); On 2012/03/01 00:08:50, Andrei wrote: > 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? :) 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. http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/docked... chrome/browser/ui/panels/docked_panel_strip.cc:695: // resize/removal of a panel may affect how many panels fit in the strip. On 2012/03/01 00:08:50, Andrei wrote: > 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??? 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. http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/docked... chrome/browser/ui/panels/docked_panel_strip.cc:726: void DockedPanelStrip::DelayedMovePanelToOverflow(Panel* panel) { On 2012/03/01 00:08:50, Andrei wrote: > This can be easily moved to the manager I think. Ended up keeping this here. This provides a guard against the panel being closed before the timer invokes this method.
On 2012/03/01 20:52:58, jennb wrote: > I didn't quite give you everything you wanted, but in exchange, I added the > set_panel_strip(NULL) to RemovePanel() and added guards. The imbalance with > AddPanel was getting to my OCD sensors. > It would feel too surreal if I got everything I wanted as soon as I asked for it :) In any case, it looks like the stated goal -- "keep panel from manipulating its strip" -- has been reached, so it LGTM. How do you feel about splitting the PanelStrip interface into two parts and giving the panels only one part, the one that does not include any non-const methods? That would make sure the policy is kept and does not deteriorate. As for "keeping panel strips from manipulating other panel strips", we can come back to it later, right? ;) BTW by "keep from manipulating" I mean not only where the logic is implemented, but also when it is executed. In other words, I fight for removal of "side effects" as much as possible. When a method on a panel is called, it should affect only this panel; if it's called on the strip, it can change this strip and panels within it, etc. If a call to a panel or panel strip can change *anything*, I suddenly get a feeling of working with a bunch of globals... (sorry for this little rant :) > Also cleaned up Panel::IsMinimized to stop digging into strip logic. > > Didn't touch Activate() vs ActivateInternal() in this patch. When the OS > activates a panel, it doesn't use Activate(), rather it tells the panel "you've > already been activated" which uses a different code path, so the current code > should be fine. > > http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/docked... > File chrome/browser/ui/panels/docked_panel_strip.cc (right): > > http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/docked... > chrome/browser/ui/panels/docked_panel_strip.cc:111: PanelStrip::IN_OVERFLOW); > On 2012/03/01 00:08:50, Andrei wrote: > > 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? :) > > Ended up leaving this, though moved into a subroutine. Seems right for the dock > strip to decide when it is too full. Decide it needs space -- yes of course, take system-wide actions because of it -- I would avoid it if possible. > Drag controller may end up adding panels. > Didn't want every place that added a panel to have to also add "make space" > logic. > Everyone would need to go through a method on the PanelManager, instead of talking directly to the strips. But as I said, we can come back to this at a later time. > http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/docked... > chrome/browser/ui/panels/docked_panel_strip.cc:695: // resize/removal of a panel > may affect how many panels fit in the strip. > On 2012/03/01 00:08:50, Andrei wrote: > > 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??? > > 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. Ditto. > > 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. > I do not see why it should. Can moving panels between the docked and overflow strips be made to preserve ordering? Ideally the task "examine space in the docked strip and move panels to/from overflow as needed" should not break any invariants so it could be allowed to run at any time. Do you think it's possible? > http://codereview.chromium.org/9560002/diff/1/chrome/browser/ui/panels/docked... > chrome/browser/ui/panels/docked_panel_strip.cc:726: void > DockedPanelStrip::DelayedMovePanelToOverflow(Panel* panel) { > On 2012/03/01 00:08:50, Andrei wrote: > > This can be easily moved to the manager I think. > > Ended up keeping this here. This provides a guard against the panel being closed > before the timer invokes this method.
https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... File chrome/browser/ui/panels/docked_panel_strip.cc (right): https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... chrome/browser/ui/panels/docked_panel_strip.cc:700: panel_manager_->MovePanelsOutOfOverflowIfCanFit(); MovePanelsToOverflow and panel_manager_->MovePanelsOutOfOverflowIfCanFit seem not to be consistent. It might be better to put both in either PanelManager or DockedPanelStrip. Also, do we also need to disable disable_layout_refresh_ for the latter one? https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... chrome/browser/ui/panels/docked_panel_strip.cc:719: void DockedPanelStrip::EnsureAvailableSpace(int width) { This seems to be quite similar to MovePanelsToOverflow. Anyway to merge these two? https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... File chrome/browser/ui/panels/docked_panel_strip.h (right): https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... chrome/browser/ui/panels/docked_panel_strip.h:96: bool CanFitPanel(Panel* panel); nit: need to add const modifier. https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... chrome/browser/ui/panels/docked_panel_strip.h:117: // Move all panels after |overflow_point|, inclusive, to overflow. Not sure I understand what overflow_point means? Probably sth like: void MovePanelsToOverflowUntil(Panel* last_panel_to_move_to_overflow) https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... File chrome/browser/ui/panels/panel.cc (right): https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... chrome/browser/ui/panels/panel.cc:207: if (!panel_strip_) It seems to be quite a burden to check panel_strip_ for each possible panel methods. Any way to avoid this? https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... File chrome/browser/ui/panels/panel_manager.cc (right): https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... chrome/browser/ui/panels/panel_manager.cc:203: switch (new_layout) { Probably better to set the target strip in switch/case and then call target_strip->Add. https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... File chrome/browser/ui/panels/panel_manager.h (right): https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... chrome/browser/ui/panels/panel_manager.h:66: void ChangePanelLayout(Panel* panel, PanelStrip::Type new_layout); The name ChangePanelLayout seems to get easily confused with the panel layout, which means how controls are layout in the panel. Will MovePanelToStrip sounds better?
https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... File chrome/browser/ui/panels/docked_panel_strip.cc (right): https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... chrome/browser/ui/panels/docked_panel_strip.cc:700: panel_manager_->MovePanelsOutOfOverflowIfCanFit(); On 2012/03/02 00:11:41, jianli wrote: > MovePanelsToOverflow and panel_manager_->MovePanelsOutOfOverflowIfCanFit seem > not to be consistent. It might be better to put both in either PanelManager or > DockedPanelStrip. Also, do we also need to disable disable_layout_refresh_ for > the latter one? Moved the former to PanelManager as well. Adding panels to the dock does not cause a layout refresh but I'll disable layout refresh for it anyways so that all operations that affect multiple panels at once consistently disable refreshes. https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... chrome/browser/ui/panels/docked_panel_strip.cc:719: void DockedPanelStrip::EnsureAvailableSpace(int width) { On 2012/03/02 00:11:41, jianli wrote: > This seems to be quite similar to MovePanelsToOverflow. Anyway to merge these > two? Not easily. In this one, the stopping point is not known in advance. I moved this to panel manager as well, with the expectation that drag controller will go through panel manager to add panels to the dock strip. https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... File chrome/browser/ui/panels/docked_panel_strip.h (right): https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... chrome/browser/ui/panels/docked_panel_strip.h:96: bool CanFitPanel(Panel* panel); On 2012/03/02 00:11:41, jianli wrote: > nit: need to add const modifier. Done. https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... chrome/browser/ui/panels/docked_panel_strip.h:117: // Move all panels after |overflow_point|, inclusive, to overflow. On 2012/03/02 00:11:41, jianli wrote: > Not sure I understand what overflow_point means? Probably sth like: > void MovePanelsToOverflowUntil(Panel* last_panel_to_move_to_overflow) Done. https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... File chrome/browser/ui/panels/panel.cc (right): https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... chrome/browser/ui/panels/panel.cc:207: if (!panel_strip_) On 2012/03/02 00:11:41, jianli wrote: > It seems to be quite a burden to check panel_strip_ for each possible panel > methods. Any way to avoid this? It's a pain but as these are all entry points for accessing a panel, it has to be checked at every point. https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... File chrome/browser/ui/panels/panel_manager.cc (right): https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... chrome/browser/ui/panels/panel_manager.cc:203: switch (new_layout) { On 2012/03/02 00:11:41, jianli wrote: > Probably better to set the target strip in switch/case and then call > target_strip->Add. Done. https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... File chrome/browser/ui/panels/panel_manager.h (right): https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/p... chrome/browser/ui/panels/panel_manager.h:66: void ChangePanelLayout(Panel* panel, PanelStrip::Type new_layout); On 2012/03/02 00:11:41, jianli wrote: > The name ChangePanelLayout seems to get easily confused with the panel layout, > which means how controls are layout in the panel. Will MovePanelToStrip sounds > better? I was trying to avoid Strip in the name, but oh well. Done.
lgtm https://chromiumcodereview.appspot.com/9560002/diff/9018/chrome/browser/ui/pa... File chrome/browser/ui/panels/panel_manager.cc (right): https://chromiumcodereview.appspot.com/9560002/diff/9018/chrome/browser/ui/pa... chrome/browser/ui/panels/panel_manager.cc:228: void PanelManager::EnsurePanelFitsInDock(Panel* panel) { nit: EnsurePanelFitsInDockedStrip? https://chromiumcodereview.appspot.com/9560002/diff/9018/chrome/browser/ui/pa... chrome/browser/ui/panels/panel_manager.cc:232: MovePanelToStrip(docked_strip_->last_panel(), PanelStrip::IN_OVERFLOW); We know that we start from last docked panel and move it into overflow. When DockedPanelStrip::RemovePanel is called, we still search the whole list to remove it. Could we be smarter to deal with this more efficiently? https://chromiumcodereview.appspot.com/9560002/diff/9018/chrome/browser/ui/pa... chrome/browser/ui/panels/panel_manager.cc:243: MovePanelToStrip(bumped_panel, PanelStrip::IN_OVERFLOW); ditto.
Still lgtm. I am thinking we should probably move EnsurePanelFitsInDock, MovePanelsToOverflow, MovePanelsOutOfOverflow from PanelManager back to DockedPanelStrip so that we can avoid exposing last_panel, enable_layout_refresh in DockedPanelStrip. First, MovePanelsToOverflow and MovePanelsOutOfOverflow are only needed by DockedPanelStrip and putting them in PanelManager could potentially make them abused by others. Second, enable_layout_refresh is only needed by these 3 methods and we do not want to open it for other purpose. On Thu, Mar 1, 2012 at 6:26 PM, <jianli@chromium.org> wrote: > lgtm > > > > > https://chromiumcodereview.**appspot.com/9560002/diff/9018/** > chrome/browser/ui/panels/**panel_manager.cc<https://chromiumcodereview.appspot.com/9560002/diff/9018/chrome/browser/ui/panels/panel_manager.cc> > File chrome/browser/ui/panels/**panel_manager.cc (right): > > https://chromiumcodereview.**appspot.com/9560002/diff/9018/** > chrome/browser/ui/panels/**panel_manager.cc#newcode228<https://chromiumcodereview.appspot.com/9560002/diff/9018/chrome/browser/ui/panels/panel_manager.cc#newcode228> > chrome/browser/ui/panels/**panel_manager.cc:228: void > PanelManager::**EnsurePanelFitsInDock(Panel* panel) { > nit: EnsurePanelFitsInDockedStrip? > > https://chromiumcodereview.**appspot.com/9560002/diff/9018/** > chrome/browser/ui/panels/**panel_manager.cc#newcode232<https://chromiumcodereview.appspot.com/9560002/diff/9018/chrome/browser/ui/panels/panel_manager.cc#newcode232> > chrome/browser/ui/panels/**panel_manager.cc:232: > MovePanelToStrip(docked_strip_**->last_panel(), PanelStrip::IN_OVERFLOW); > We know that we start from last docked panel and move it into overflow. > When DockedPanelStrip::RemovePanel is called, we still search the whole > list to remove it. Could we be smarter to deal with this more > efficiently? > > https://chromiumcodereview.**appspot.com/9560002/diff/9018/** > chrome/browser/ui/panels/**panel_manager.cc#newcode243<https://chromiumcodereview.appspot.com/9560002/diff/9018/chrome/browser/ui/panels/panel_manager.cc#newcode243> > chrome/browser/ui/panels/**panel_manager.cc:243: > MovePanelToStrip(bumped_panel, PanelStrip::IN_OVERFLOW); > ditto. > > https://chromiumcodereview.**appspot.com/9560002/<https://chromiumcodereview.... >
On 2012/03/02 03:02:28, jianli wrote: > Still lgtm. > > I am thinking we should probably > move EnsurePanelFitsInDock, MovePanelsToOverflow, MovePanelsOutOfOverflow > from PanelManager back to DockedPanelStrip so that we can avoid > exposing last_panel, enable_layout_refresh in DockedPanelStrip. Exposing last_panel in docked strip is no different than exposing first_panel in overflow strip. I have an idea for getting rid of enable_layout_refresh_ entirely. We can also optimize RemovePanel by having it iterate backwards to optimize the common case of removing from the end. > First, MovePanelsToOverflow and MovePanelsOutOfOverflow are only needed by > DockedPanelStrip and putting them in PanelManager could potentially make > them abused by others. Second, enable_layout_refresh is only needed by > these 3 methods and we do not want to open it for other purpose. I prefer that the strips don't have to interact with each other directly. I'll continue working on this some more tomorrow. Jenn > > > On Thu, Mar 1, 2012 at 6:26 PM, <mailto:jianli@chromium.org> wrote: > > > lgtm > > > > > > > > > > https://chromiumcodereview.**appspot.com/9560002/diff/9018/** > > > chrome/browser/ui/panels/**panel_manager.cc<https://chromiumcodereview.appspot.com/9560002/diff/9018/chrome/browser/ui/panels/panel_manager.cc> > > File chrome/browser/ui/panels/**panel_manager.cc (right): > > > > https://chromiumcodereview.**appspot.com/9560002/diff/9018/** > > > chrome/browser/ui/panels/**panel_manager.cc#newcode228<https://chromiumcodereview.appspot.com/9560002/diff/9018/chrome/browser/ui/panels/panel_manager.cc#newcode228> > > chrome/browser/ui/panels/**panel_manager.cc:228: void > > PanelManager::**EnsurePanelFitsInDock(Panel* panel) { > > nit: EnsurePanelFitsInDockedStrip? > > > > https://chromiumcodereview.**appspot.com/9560002/diff/9018/** > > > chrome/browser/ui/panels/**panel_manager.cc#newcode232<https://chromiumcodereview.appspot.com/9560002/diff/9018/chrome/browser/ui/panels/panel_manager.cc#newcode232> > > chrome/browser/ui/panels/**panel_manager.cc:232: > > MovePanelToStrip(docked_strip_**->last_panel(), PanelStrip::IN_OVERFLOW); > > We know that we start from last docked panel and move it into overflow. > > When DockedPanelStrip::RemovePanel is called, we still search the whole > > list to remove it. Could we be smarter to deal with this more > > efficiently? > > > > https://chromiumcodereview.**appspot.com/9560002/diff/9018/** > > > chrome/browser/ui/panels/**panel_manager.cc#newcode243<https://chromiumcodereview.appspot.com/9560002/diff/9018/chrome/browser/ui/panels/panel_manager.cc#newcode243> > > chrome/browser/ui/panels/**panel_manager.cc:243: > > MovePanelToStrip(bumped_panel, PanelStrip::IN_OVERFLOW); > > ditto. > > > > > https://chromiumcodereview.**appspot.com/9560002/%3Chttps://chromiumcoderevie...> > >
> > > First, MovePanelsToOverflow and MovePanelsOutOfOverflow are only needed by >> DockedPanelStrip and putting them in PanelManager could potentially make >> them abused by others. Second, enable_layout_refresh is only needed by >> these 3 methods and we do not want to open it for other purpose. >> > > I prefer that the strips don't have to interact with each other directly. > > The reason can be exactly the scenario we discussed with Jenn yesterday: what if you are dragging a panel in the docking strip and the strip decides to move it to overflow at the same time? If the interaction goes through the panel manager, it has ample opportunity to notify the drag controller, or to stop / change the course of action entirely.
patch set 6 changes: 1. removed need for disabling layout refreshes in dock strip by using a guard in PanelManager 2.optimized docked strip's RemovePanel for the common case of removing the last panel. (Reminded again why storing iterators along with a container is BAD. Fortunately, Jian will be removing that in his dragging patch.) 3. moved logic for deciding if docked strip needs to overflow back inside AddPanel and reworked logic so we only need MovePanelsToOverflow. Jenn
lgtm https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/p... File chrome/browser/ui/panels/docked_panel_strip.cc (right): https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/p... chrome/browser/ui/panels/docked_panel_strip.cc:681: // Dock is too full. nit: Insufficient space for the requested width. Bump panels to the overflow. https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/p... chrome/browser/ui/panels/docked_panel_strip.cc:682: Panel* last_panel_to_send_to_overflow; nit: Seems not need to define this variable outside for loop. https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/p... chrome/browser/ui/panels/docked_panel_strip.cc:688: // Found where panel fits. nit: comment can be removed. https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/p... File chrome/browser/ui/panels/docked_panel_strip.h (right): https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/p... chrome/browser/ui/panels/docked_panel_strip.h:131: // |width| of panel to be fitted into the dock strip. nit: better rephrase the comment to make it more like sentence. https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/p... chrome/browser/ui/panels/docked_panel_strip.h:134: int FitPanelInStrip(int width); nit: FitPanelWithWidth? so that the name is consistent with CanFitPanel. Or, CanFitPanel => HasSufficientSpaceForPanel FitPanelInStrip => EnsureSufficientSpaceForPanel https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/p... File chrome/browser/ui/panels/panel_manager.h (right): https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/p... chrome/browser/ui/panels/panel_manager.h:219: // True only while moving panels to overflow. nit: Please comment why we need this flag, like we do not want to move the overflow panel that is bumped from the docked strip back to the strip.
https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/p... File chrome/browser/ui/panels/docked_panel_strip.cc (right): https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/p... chrome/browser/ui/panels/docked_panel_strip.cc:681: // Dock is too full. On 2012/03/02 22:25:32, jianli wrote: > nit: Insufficient space for the requested width. Bump panels to the overflow. Done. https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/p... chrome/browser/ui/panels/docked_panel_strip.cc:682: Panel* last_panel_to_send_to_overflow; On 2012/03/02 22:25:32, jianli wrote: > nit: Seems not need to define this variable outside for loop. Slight efficiency to avoid stack manipulation each time through the loop. https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/p... chrome/browser/ui/panels/docked_panel_strip.cc:688: // Found where panel fits. On 2012/03/02 22:25:32, jianli wrote: > nit: comment can be removed. Done. https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/p... File chrome/browser/ui/panels/docked_panel_strip.h (right): https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/p... chrome/browser/ui/panels/docked_panel_strip.h:131: // |width| of panel to be fitted into the dock strip. On 2012/03/02 22:25:32, jianli wrote: > nit: better rephrase the comment to make it more like sentence. Done. https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/p... chrome/browser/ui/panels/docked_panel_strip.h:134: int FitPanelInStrip(int width); On 2012/03/02 22:25:32, jianli wrote: > nit: FitPanelWithWidth? > > so that the name is consistent with CanFitPanel. > > Or, CanFitPanel => HasSufficientSpaceForPanel > FitPanelInStrip => EnsureSufficientSpaceForPanel Done. https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/p... File chrome/browser/ui/panels/panel_manager.h (right): https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/p... chrome/browser/ui/panels/panel_manager.h:219: // True only while moving panels to overflow. On 2012/03/02 22:25:32, jianli wrote: > nit: Please comment why we need this flag, like we do not want to move the > overflow panel that is bumped from the docked strip back to the strip. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jennb@chromium.org/9560002/24005
> > https://chromiumcodereview.**appspot.com/9560002/diff/** > 20001/chrome/browser/ui/**panels/docked_panel_strip.cc#**newcode682<https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/panels/docked_panel_strip.cc#newcode682> > chrome/browser/ui/panels/**docked_panel_strip.cc:682: Panel* > last_panel_to_send_to_**overflow; > On 2012/03/02 22:25:32, jianli wrote: > >> nit: Seems not need to define this variable outside for loop. >> > > Slight efficiency to avoid stack manipulation each time through the > > loop. > > I am quite sure that with modern compilers there is no difference in the resulting code.
Change committed as 124806 |