Constrained Windows Cocoa: Part 4 Refactor ConstrainedWindowSupport

Constrained Windows Cocoa: Part 4 Refactor ConstrainedWindowSupport

Currently ConstrainedWindowMac and TabStripController are tightly coupled. This makes it harder to add the new constrained window UI along side the old UI.

To simplify things this CL refactors ConstrainedWindowSupport to be much simpler.

BUG=BUG=134584
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154832

[Robert Sesek, 2012-08-31 21:22:36]

Seems like mostly plumbing. Maybe add Avi for the GTM stuff.

https://chromiumcodereview.appspot.com/10871082/diff/1/chrome/browser/ui/brow...
File chrome/browser/ui/browser_window.h (right):

https://chromiumcodereview.appspot.com/10871082/diff/1/chrome/browser/ui/brow...
chrome/browser/ui/browser_window.h:338: virtual bool GetIsShowingInstant() = 0;
I'm not a fan of this name. I think the "Get" is unnecessary "IsX()" methods can
easily be seen to return bool. Second, "ShowingInstant" what? Maybe
IsInstantTabShowing() or something like that.

https://chromiumcodereview.appspot.com/10871082/diff/1/chrome/browser/ui/cons...
File chrome/browser/ui/constrained_window.h (right):

https://chromiumcodereview.appspot.com/10871082/diff/1/chrome/browser/ui/cons...
chrome/browser/ui/constrained_window.h:47: virtual bool
GetCanShowConstrainedWindow();
I think the "Get" here is unnecessary. Just CanShowConstraintedWindow() implies
that it returns a bool.

[sail, 2012-08-31 21:26:25]

+avi for the old GTM constrained windows code

[Avi (use Gerrit), 2012-08-31 23:29:58]

This looks reasonable. Nico was the one who hooked that up; I'm the expert in
the sheet controller's internals, but I don't know about how it was integrated.

[sail, 2012-09-02 22:43:11]

rebased and addressed review comments

https://chromiumcodereview.appspot.com/10871082/diff/1/chrome/browser/ui/brow...
File chrome/browser/ui/browser_window.h (right):

https://chromiumcodereview.appspot.com/10871082/diff/1/chrome/browser/ui/brow...
chrome/browser/ui/browser_window.h:338: virtual bool GetIsShowingInstant() = 0;
On 2012/08/31 21:22:36, rsesek wrote:
> I'm not a fan of this name. I think the "Get" is unnecessary "IsX()" methods
can
> easily be seen to return bool. Second, "ShowingInstant" what? Maybe
> IsInstantTabShowing() or something like that.

Done.

https://chromiumcodereview.appspot.com/10871082/diff/1/chrome/browser/ui/cons...
File chrome/browser/ui/constrained_window.h (right):

https://chromiumcodereview.appspot.com/10871082/diff/1/chrome/browser/ui/cons...
chrome/browser/ui/constrained_window.h:47: virtual bool
GetCanShowConstrainedWindow();
On 2012/08/31 21:22:36, rsesek wrote:
> I think the "Get" here is unnecessary. Just CanShowConstraintedWindow()
implies
> that it returns a bool.

Done.

[Nico, 2012-09-04 15:56:03]

The changes in this CL lgtm

https://chromiumcodereview.appspot.com/10871082/diff/7001/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (left):

https://chromiumcodereview.appspot.com/10871082/diff/7001/chrome/browser/ui/c...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2088: // We use the
TabContentsController's view in |swapInTabAtIndex|, so we have
This comment is still true, right? Move it to the new place.

If I remember correctly, the reason why this code is in this place is because
this view hierarchy belongs to the tab structure, so I thought it should be in
some tab class, not in some unrelated class.

[Robert Sesek, 2012-09-04 16:45:22]

LGTM

https://chromiumcodereview.appspot.com/10871082/diff/7001/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://chromiumcodereview.appspot.com/10871082/diff/7001/chrome/browser/ui/c...
chrome/browser/ui/cocoa/browser_window_controller.h:330: -
(BOOL)isInstantTabShowing;
Thanks for renaming the Cocoa, too.

[sail, 2012-09-04 17:04:22]

http://codereview.chromium.org/10871082/diff/7001/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (left):

http://codereview.chromium.org/10871082/diff/7001/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2088: // We use the
TabContentsController's view in |swapInTabAtIndex|, so we have
On 2012/09/04 15:56:03, Nico wrote:
> This comment is still true, right? Move it to the new place.
> 
> If I remember correctly, the reason why this code is in this place is because
> this view hierarchy belongs to the tab structure, so I thought it should be in
> some tab class, not in some unrelated class.

Done.
I view hierarchy related code into a helper function in tab_strip_controller.mm.

[commit-bot: I haz the power, 2012-09-04 17:51:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10871082/3004

[commit-bot: I haz the power, 2012-09-04 22:50:19]

Change committed as 154832