mac: Refactor tab strip layout logic.

mac: Refactor tab strip layout logic.

This CL has no intended behavioral changes.

Moved the logic from browser_window_controller to browser_window_layout. Add
test coverage for tab strip layout.

BUG=

Committed: https://crrev.com/d7e0bdda84dad75cd4dbba57dfc89932671ec6bf
Cr-Commit-Position: refs/heads/master@{#297570}

[erikchen, 2014-09-26 01:27:34]

erikchen@chromium.org changed reviewers:
+ rsesek@chromium.org

[erikchen, 2014-09-26 01:27:35]

rsesek: Please review.

[Robert Sesek, 2014-09-26 16:55:55]

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller_private.h (right):

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller_private.h:47: -
(void)applyTabStripLayout:(chrome::TabStripLayout)layout;
const&

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:788: if ([self
shouldShowAvatar]) {
This looks like new code here. I'm not following where this came from/where this
was done originally.

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_layout.h (right):

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_layout.h:45: BOOL shouldUseNewAvatar;
Does shouldShowAvatar have to be YES for this to be YES?

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_layout.h:46: // The size of the avatar
button.
What is this if shouldShowAvatar is NO?

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_layout.h:77: CGFloat leftIndent;
What are these idnenting?

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_layout.h:79: BOOL addWindowControls;
Comment.

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_layout.mm (right):

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_layout.mm:173: memset(&layout, 0,
sizeof(layout));
Can you not do = {0} or = {} instead? Otherwise, IWYU.

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_layout.mm:185:
parameters_.inAnyFullscreen && base::mac::IsOSYosemiteOrLater();
What about when not in screen?

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_layout.mm:220: // Calculate the right
indentation.  The default indentation built into the
nit: preference is for one space between sentences.

[erikchen, 2014-09-26 17:37:28]

Patchset #2 (id:20001) has been deleted

[erikchen, 2014-09-26 17:39:06]

rsesek: PTAL

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller_private.h (right):

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller_private.h:47: -
(void)applyTabStripLayout:(chrome::TabStripLayout)layout;
On 2014/09/26 16:55:54, Robert Sesek wrote:
> const&

Done.

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:788: if ([self
shouldShowAvatar]) {
On 2014/09/26 16:55:54, Robert Sesek wrote:
> This looks like new code here. I'm not following where this came from/where
this
> was done originally.

These are all the parameters used to compute layout in the method previously
called -layoutTabStripAtMaxY:...

I've just pulled them out and moved them here.

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_layout.h (right):

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_layout.h:45: BOOL shouldUseNewAvatar;
On 2014/09/26 16:55:54, Robert Sesek wrote:
> Does shouldShowAvatar have to be YES for this to be YES?

I've updated the comments to indicate that this parameter only needs to be
defined if shouldShowAvatar == YES.

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_layout.h:46: // The size of the avatar
button.
On 2014/09/26 16:55:54, Robert Sesek wrote:
> What is this if shouldShowAvatar is NO?

ditto

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_layout.h:77: CGFloat leftIndent;
On 2014/09/26 16:55:54, Robert Sesek wrote:
> What are these idnenting?

Added comments. The indent is for the controls of the TabStripController.

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_layout.h:79: BOOL addWindowControls;
On 2014/09/26 16:55:54, Robert Sesek wrote:
> Comment.

Done. "Whether the TabStripController needs to add custom traffic light
buttons."

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_layout.mm (right):

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_layout.mm:173: memset(&layout, 0,
sizeof(layout));
On 2014/09/26 16:55:55, Robert Sesek wrote:
> Can you not do = {0} or = {} instead? Otherwise, IWYU.

= {} will correctly zero initialize the struct. I went with that.

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_layout.mm:185:
parameters_.inAnyFullscreen && base::mac::IsOSYosemiteOrLater();
On 2014/09/26 16:55:55, Robert Sesek wrote:
> What about when not in screen?

I don't understand your question. My best interpretation is s/screen/fullscreen,
in which case this logic is still correct (its the same logic as before the
refactor).

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_layout.mm:220: // Calculate the right
indentation.  The default indentation built into the
On 2014/09/26 16:55:55, Robert Sesek wrote:
> nit: preference is for one space between sentences.

Done. (This was a copy paste).

[Robert Sesek, 2014-09-26 19:06:23]

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_layout.mm (right):

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_layout.mm:185:
parameters_.inAnyFullscreen && base::mac::IsOSYosemiteOrLater();
On 2014/09/26 17:39:06, erikchen wrote:
> On 2014/09/26 16:55:55, Robert Sesek wrote:
> > What about when not in screen?
> 
> I don't understand your question. My best interpretation is
s/screen/fullscreen,
> in which case this logic is still correct (its the same logic as before the
> refactor).

Sorry, yes meant fullscreen. The naming of the member |addWindowControls|
implies that window controls won't be added unless you're in fullscreen on
Yosemite. Maybe rename the method to be |addWindowControlsInFullScreen| (which
was the name of the old local variable)?

https://codereview.chromium.org/607723002/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_layout.h (right):

https://codereview.chromium.org/607723002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_layout.h:40: // button doesn't exist.
Only needs to be defined if hasTabStrip == YES.
nit: instead of ==, use "is".

https://codereview.chromium.org/607723002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_layout.h:40: // button doesn't exist.
Only needs to be defined if hasTabStrip == YES.
"Only needs to be defined" is not accurate; they're all defined by virtue of
being struct members. "Only set {when,if}" is probably a better phrase.

[erikchen, 2014-09-26 20:53:00]

Patchset #3 (id:60001) has been deleted

[erikchen, 2014-09-26 20:55:59]

rsesek: PTAL

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_layout.mm (right):

https://codereview.chromium.org/607723002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_layout.mm:185:
parameters_.inAnyFullscreen && base::mac::IsOSYosemiteOrLater();
On 2014/09/26 19:06:23, Robert Sesek wrote:
> On 2014/09/26 17:39:06, erikchen wrote:
> > On 2014/09/26 16:55:55, Robert Sesek wrote:
> > > What about when not in screen?
> > 
> > I don't understand your question. My best interpretation is
> s/screen/fullscreen,
> > in which case this logic is still correct (its the same logic as before the
> > refactor).
> 
> Sorry, yes meant fullscreen. The naming of the member |addWindowControls|
> implies that window controls won't be added unless you're in fullscreen on
> Yosemite. Maybe rename the method to be |addWindowControlsInFullScreen| (which
> was the name of the old local variable)?

I renamed all of the fields, properties, and methods to use the nomenclature
"customWindowControls", to better differentiate the controls from the default
window controls.

https://codereview.chromium.org/607723002/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_layout.h (right):

https://codereview.chromium.org/607723002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_layout.h:40: // button doesn't exist.
Only needs to be defined if hasTabStrip == YES.
On 2014/09/26 19:06:23, Robert Sesek wrote:
> "Only needs to be defined" is not accurate; they're all defined by virtue of
> being struct members. "Only set {when,if}" is probably a better phrase.

Changed to "Only needs to be set when hasTabStrip is YES"

[Robert Sesek, 2014-09-29 18:57:22]

https://codereview.chromium.org/607723002/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_layout.mm (right):

https://codereview.chromium.org/607723002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_layout.mm:26: const CGFloat
kAvatarRightOffset = 4;
nit: blank line after

https://codereview.chromium.org/607723002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_layout.mm:166: if
(!parameters_.hasTabStrip) {
Would it make sense to zero output_.tabStripLayout in this case?

[erikchen, 2014-09-29 22:16:33]

https://codereview.chromium.org/607723002/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_layout.mm (right):

https://codereview.chromium.org/607723002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_layout.mm:26: const CGFloat
kAvatarRightOffset = 4;
On 2014/09/29 18:57:22, Robert Sesek wrote:
> nit: blank line after

Done.

https://codereview.chromium.org/607723002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_layout.mm:166: if
(!parameters_.hasTabStrip) {
On 2014/09/29 18:57:22, Robert Sesek wrote:
> Would it make sense to zero output_.tabStripLayout in this case?

All of output_ is zero-ed at line 53. Added the appropriate header to satisfy
IWYU.

[Robert Sesek, 2014-09-30 13:46:16]

LGTM

[erikchen, 2014-09-30 21:41:14]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-09-30 21:42:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/607723002/100001

[commit-bot: I haz the power, 2014-09-30 22:00:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-30 22:00:02]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/62730)

[erikchen, 2014-09-30 22:33:29]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-09-30 22:35:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/607723002/140001

[commit-bot: I haz the power, 2014-09-30 23:19:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-30 23:19:27]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[erikchen, 2014-10-01 00:06:07]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-10-01 00:07:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/607723002/160001

[commit-bot: I haz the power, 2014-10-01 01:17:32]

Committed patchset #7 (id:160001) as edf40d2f41bf0739d8943b44943034062dca14f4

[commit-bot: I haz the power, 2014-10-01 01:18:09]

Patchset 7 (id:??) landed as
https://crrev.com/d7e0bdda84dad75cd4dbba57dfc89932671ec6bf
Cr-Commit-Position: refs/heads/master@{#297570}