Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(292)

Issue 9560002: Cleanup to keep panel from manipulating its panel strip assignment directly. (Closed)

Created:
8 years, 9 months ago by jennb
Modified:
8 years, 9 months ago
Reviewers:
Andrei, jianli
CC:
chromium-reviews, Dmitry Lomov (no reviews), Dmitry Titov, jianli, dcheng
Visibility:
Public.

Description

Cleanup 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+216 lines, -148 lines) Patch
M chrome/browser/ui/panels/detached_panel_browsertest.cc View 1 2 3 4 chunks +10 lines, -13 lines 0 comments Download
M chrome/browser/ui/panels/detached_panel_strip.h View 1 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/ui/panels/detached_panel_strip.cc View 1 2 chunks +12 lines, -3 lines 0 comments Download
M chrome/browser/ui/panels/docked_panel_strip.h View 1 2 3 4 5 6 6 chunks +12 lines, -5 lines 0 comments Download
M chrome/browser/ui/panels/docked_panel_strip.cc View 1 2 3 4 5 6 9 chunks +66 lines, -76 lines 0 comments Download
M chrome/browser/ui/panels/overflow_panel_strip.h View 1 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/ui/panels/overflow_panel_strip.cc View 1 2 3 6 chunks +17 lines, -13 lines 0 comments Download
M chrome/browser/ui/panels/panel.h View 1 1 chunk +2 lines, -3 lines 0 comments Download
M chrome/browser/ui/panels/panel.cc View 1 11 chunks +15 lines, -26 lines 0 comments Download
M chrome/browser/ui/panels/panel_manager.h View 1 2 3 4 5 6 3 chunks +14 lines, -0 lines 0 comments Download
M chrome/browser/ui/panels/panel_manager.cc View 1 2 3 4 5 5 chunks +59 lines, -3 lines 0 comments Download
M chrome/browser/ui/panels/panel_overflow_browsertest.cc View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/panels/panel_strip.h View 1 2 chunks +3 lines, -2 lines 0 comments Download

Messages

Total messages: 18 (0 generated)
jennb
8 years, 9 months ago (2012-02-29 22:32:53 UTC) #1
Andrei
OK, here's a bunch of comments. Please feel free to disagree :) Also some of ...
8 years, 9 months ago (2012-03-01 00:08:50 UTC) #2
jennb
No new revision yet. A few answers below. I'm working on moving the inter-strip logic ...
8 years, 9 months ago (2012-03-01 00:30:58 UTC) #3
aburago_google.com
On Wed, Feb 29, 2012 at 4:30 PM, <jennb@chromium.org> wrote: > No new revision yet. ...
8 years, 9 months ago (2012-03-01 00:48:01 UTC) #4
jennb
I didn't quite give you everything you wanted, but in exchange, I added the set_panel_strip(NULL) ...
8 years, 9 months ago (2012-03-01 20:52:58 UTC) #5
Andrei
On 2012/03/01 20:52:58, jennb wrote: > I didn't quite give you everything you wanted, but ...
8 years, 9 months ago (2012-03-01 23:32:28 UTC) #6
jianli
https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/panels/docked_panel_strip.cc File chrome/browser/ui/panels/docked_panel_strip.cc (right): https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/panels/docked_panel_strip.cc#newcode700 chrome/browser/ui/panels/docked_panel_strip.cc:700: panel_manager_->MovePanelsOutOfOverflowIfCanFit(); MovePanelsToOverflow and panel_manager_->MovePanelsOutOfOverflowIfCanFit seem not to be consistent. ...
8 years, 9 months ago (2012-03-02 00:11:41 UTC) #7
jennb
https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/panels/docked_panel_strip.cc File chrome/browser/ui/panels/docked_panel_strip.cc (right): https://chromiumcodereview.appspot.com/9560002/diff/10001/chrome/browser/ui/panels/docked_panel_strip.cc#newcode700 chrome/browser/ui/panels/docked_panel_strip.cc:700: panel_manager_->MovePanelsOutOfOverflowIfCanFit(); On 2012/03/02 00:11:41, jianli wrote: > MovePanelsToOverflow and ...
8 years, 9 months ago (2012-03-02 01:26:32 UTC) #8
jianli
lgtm 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 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 chrome/browser/ui/panels/panel_manager.cc:232: ...
8 years, 9 months ago (2012-03-02 02:26:09 UTC) #9
jianli
Still lgtm. I am thinking we should probably move EnsurePanelFitsInDock, MovePanelsToOverflow, MovePanelsOutOfOverflow from PanelManager back ...
8 years, 9 months ago (2012-03-02 03:02:28 UTC) #10
jennb
On 2012/03/02 03:02:28, jianli wrote: > Still lgtm. > > I am thinking we should ...
8 years, 9 months ago (2012-03-02 04:40:02 UTC) #11
aburago_google.com
> > > First, MovePanelsToOverflow and MovePanelsOutOfOverflow are only needed by >> DockedPanelStrip and putting ...
8 years, 9 months ago (2012-03-02 16:53:24 UTC) #12
jennb
patch set 6 changes: 1. removed need for disabling layout refreshes in dock strip by ...
8 years, 9 months ago (2012-03-02 20:03:10 UTC) #13
jianli
lgtm https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/panels/docked_panel_strip.cc File chrome/browser/ui/panels/docked_panel_strip.cc (right): https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/panels/docked_panel_strip.cc#newcode681 chrome/browser/ui/panels/docked_panel_strip.cc:681: // Dock is too full. nit: Insufficient space ...
8 years, 9 months ago (2012-03-02 22:25:32 UTC) #14
jennb
https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/panels/docked_panel_strip.cc File chrome/browser/ui/panels/docked_panel_strip.cc (right): https://chromiumcodereview.appspot.com/9560002/diff/20001/chrome/browser/ui/panels/docked_panel_strip.cc#newcode681 chrome/browser/ui/panels/docked_panel_strip.cc:681: // Dock is too full. On 2012/03/02 22:25:32, jianli ...
8 years, 9 months ago (2012-03-02 22:44:37 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jennb@chromium.org/9560002/24005
8 years, 9 months ago (2012-03-02 22:45:01 UTC) #16
Andrei
> > 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 ...
8 years, 9 months ago (2012-03-02 22:49:19 UTC) #17
commit-bot: I haz the power
8 years, 9 months ago (2012-03-03 01:11:27 UTC) #18
Change committed as 124806

Powered by Google App Engine
This is Rietveld 408576698