Account for minimum tabstrip width in opaque browser frame.

Account for minimum tabstrip width in opaque browser frame.

BUG=99330
TEST=Under opaque theme, resize window really small. The minimum width shouldn't cause tabstrip elements to be cutoff.

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

[Peter Kasting, 2011-10-06 20:31:32]

http://codereview.chromium.org/8172010/diff/3001/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

http://codereview.chromium.org/8172010/diff/3001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:312: width() -
GetBoundsForTabStrip(tabstrip).width() + min_tabstrip_width;
This worries me because it is flexible.  For example, it might allow a Chrome
window with no avatar showing to shrink to a small size, but then if we show an
avatar for multiprofile or incognito, the window will be too small.  It seems
like we can solve this by
(a) Moving this calculation up into the code just above that's already
accounting for the other things in the frame here -- we just need to know how
the tabstrip is spaced relative to the caption buttons, and
(b) Adding something about the avatar to that calculation as well.

[Peter Kasting, 2012-06-13 19:34:35]

Alexei, did you want to try implementing my comment here?  This change seems to
have died.

[Alexei Svitkine (slow), 2012-06-13 19:37:40]

On 2012/06/13 19:34:35, Peter Kasting wrote:
> Alexei, did you want to try implementing my comment here?  This change seems
to
> have died.

Been swamped with other work and haven't had a chance to revisit this. :(

[Alexei Svitkine (slow), 2012-07-10 20:13:13]

.

[Alexei Svitkine (slow), 2012-07-10 20:17:30]

Hey Peter,

I've taken another stab at this and have changed the code to hopefully address
your concerns. It now includes the avatar icon size in the minimum width
calculation.

I've had to refactor GetBoundsForTabStrip() to add a helper function
GetBoundsForTabStripAndAvatarArea(), but the logic there is unchanged.

I've tested this under both RTL and LTR.

Please have another look.

[Peter Kasting, 2012-07-10 20:34:27]

LGTM with one substantive question

http://codereview.chromium.org/8172010/diff/7006/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

http://codereview.chromium.org/8172010/diff/7006/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:225: if
(browser_view()->ShouldShowAvatar()) {
Nit: Simpler:

  // Exclude avatar region, if present.
  if (browser_view()->ShouldShowAvatar())
    bounds.Inset(avatar_bounds_.width() + kAvatarRightSpacing, 0, 0, 0);

http://codereview.chromium.org/8172010/diff/7006/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:275:
min_tabstrip_width + browser_view()->GetOTRAvatarIcon().width();
Don't you also need kAvatarRightSpacing in here?

http://codereview.chromium.org/8172010/diff/7006/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:276:
min_size.set_width(std::max(min_tabstrip_area_width, min_size.width()));
Tiny nit: For consistency with the max() call above, reverse these two args.

http://codereview.chromium.org/8172010/diff/7006/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:581: const int
available_width =
Nit: OK to drop parens on these next two statements

http://codereview.chromium.org/8172010/diff/7006/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:588: std::max(0,
tabstrip_width),
Nit: Also OK to indent 4 and combine next line with this one

[Alexei Svitkine (slow), 2012-07-10 20:51:47]

http://codereview.chromium.org/8172010/diff/7006/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

http://codereview.chromium.org/8172010/diff/7006/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:225: if
(browser_view()->ShouldShowAvatar()) {
On 2012/07/10 20:34:27, Peter Kasting wrote:
> Nit: Simpler:
> 
>   // Exclude avatar region, if present.
>   if (browser_view()->ShouldShowAvatar())
>     bounds.Inset(avatar_bounds_.width() + kAvatarRightSpacing, 0, 0, 0);

That doesn't work, since that would be adding (avatar_bounds_.width() +
kAvatarRightSpacing) to the existing |x| value, instead of replacing it. It
would need to take the difference of the two if it were to pass it to Inset(),
at which point it's not really simpler.

http://codereview.chromium.org/8172010/diff/7006/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:275:
min_tabstrip_width + browser_view()->GetOTRAvatarIcon().width();
On 2012/07/10 20:34:27, Peter Kasting wrote:
> Don't you also need kAvatarRightSpacing in here?

You're right. Done.

http://codereview.chromium.org/8172010/diff/7006/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:276:
min_size.set_width(std::max(min_tabstrip_area_width, min_size.width()));
On 2012/07/10 20:34:27, Peter Kasting wrote:
> Tiny nit: For consistency with the max() call above, reverse these two args.

Done.

http://codereview.chromium.org/8172010/diff/7006/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:581: const int
available_width =
On 2012/07/10 20:34:27, Peter Kasting wrote:
> Nit: OK to drop parens on these next two statements

Done.

http://codereview.chromium.org/8172010/diff/7006/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:588: std::max(0,
tabstrip_width),
On 2012/07/10 20:34:27, Peter Kasting wrote:
> Nit: Also OK to indent 4 and combine next line with this one

I feel that ends up being a bit too crowded in this case, so I've left it as is.

[commit-bot: I haz the power, 2012-07-10 20:52:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/8172010/14006

[commit-bot: I haz the power, 2012-07-10 22:44:31]

Try job failure for 8172010-14006 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[Peter Kasting, 2012-07-10 22:47:11]

http://codereview.chromium.org/8172010/diff/7006/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

http://codereview.chromium.org/8172010/diff/7006/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:225: if
(browser_view()->ShouldShowAvatar()) {
On 2012/07/10 20:51:47, Alexei Svitkine wrote:
> On 2012/07/10 20:34:27, Peter Kasting wrote:
> > Nit: Simpler:
> > 
> >   // Exclude avatar region, if present.
> >   if (browser_view()->ShouldShowAvatar())
> >     bounds.Inset(avatar_bounds_.width() + kAvatarRightSpacing, 0, 0, 0);
> 
> That doesn't work, since that would be adding (avatar_bounds_.width() +
> kAvatarRightSpacing) to the existing |x| value, instead of replacing it. It
> would need to take the difference of the two if it were to pass it to Inset(),
> at which point it's not really simpler.

Look carefully: I'm insetting by width(), not by right().  I'm already taking
the "difference" into account, so to speak.

[Alexei Svitkine (slow), 2012-07-11 14:34:42]

On 2012/07/10 22:47:11, Peter Kasting wrote:
>
http://codereview.chromium.org/8172010/diff/7006/chrome/browser/ui/views/fram...
> File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):
> 
>
http://codereview.chromium.org/8172010/diff/7006/chrome/browser/ui/views/fram...
> chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:225: if
> (browser_view()->ShouldShowAvatar()) {
> On 2012/07/10 20:51:47, Alexei Svitkine wrote:
> > On 2012/07/10 20:34:27, Peter Kasting wrote:
> > > Nit: Simpler:
> > > 
> > >   // Exclude avatar region, if present.
> > >   if (browser_view()->ShouldShowAvatar())
> > >     bounds.Inset(avatar_bounds_.width() + kAvatarRightSpacing, 0, 0, 0);
> > 
> > That doesn't work, since that would be adding (avatar_bounds_.width() +
> > kAvatarRightSpacing) to the existing |x| value, instead of replacing it. It
> > would need to take the difference of the two if it were to pass it to
Inset(),
> > at which point it's not really simpler.
> 
> Look carefully: I'm insetting by width(), not by right().  I'm already taking
> the "difference" into account, so to speak.

Ah, I didn't notice that you were suggesting to use width().

Looking at the code some more, this still wouldn't have been correct - the
problem was that GetBoundsForTabStripAndAvatarArea() was actually adding
kTabStripIndent, which isn't part of the avatar sizing/position calculation, so
that replacing the |x| value was giving the right result, but not insetting it.

I've updated the code now to not add kTabStripIndent in
GetBoundsForTabStripAndAvatarArea(), but instead account for it in
GetBoundsForTabStrip().

I've also added kAvatarLeftSpacing to both the min size and the
GetBoundsForTabStrip() calculations.

I believe now it's actually correct. Can you take another look?

[Peter Kasting, 2012-07-11 19:14:51]

LGTM

http://codereview.chromium.org/8172010/diff/14007/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

http://codereview.chromium.org/8172010/diff/14007/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:225: if
(browser_view()->ShouldShowAvatar()) {
Nit: Shorter:

  int space_left_of_tabstrip = browser_view()->ShouldShowAvatar() ?
      (kAvatarLeftSpacing + avatar_bounds_.width() + kAvatarRightSpacing) :
      kTabStripIndent;
  bounds.Inset(space_left_of_tabstrip, 0, 0, 0);

(Comment not necessary)

[Alexei Svitkine (slow), 2012-07-11 19:21:41]

http://codereview.chromium.org/8172010/diff/14007/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

http://codereview.chromium.org/8172010/diff/14007/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:225: if
(browser_view()->ShouldShowAvatar()) {
On 2012/07/11 19:14:51, Peter Kasting wrote:
> Nit: Shorter:
> 
>   int space_left_of_tabstrip = browser_view()->ShouldShowAvatar() ?
>       (kAvatarLeftSpacing + avatar_bounds_.width() + kAvatarRightSpacing) :
>       kTabStripIndent;
>   bounds.Inset(space_left_of_tabstrip, 0, 0, 0);
> 
> (Comment not necessary)

Done.

[commit-bot: I haz the power, 2012-07-11 19:21:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/8172010/20007

[commit-bot: I haz the power, 2012-07-11 20:49:11]

Change committed as 146197