Correct padding and focus ring for frameless constrained window dialog

Correct padding and focus ring for frameless constrained window dialog: http://i.imgur.com/IQbR9.png

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

[Mike Wittman, 2012-10-08 17:03:45]

+cc:Sailesh for the ConstrainedWindow layout constant changes.

[sail, 2012-10-08 17:19:12]

http://codereview.chromium.org/11077006/diff/1/chrome/browser/ui/constrained_...
File chrome/browser/ui/constrained_window.h (right):

http://codereview.chromium.org/11077006/diff/1/chrome/browser/ui/constrained_...
chrome/browser/ui/constrained_window.h:25: static const int kCloseButtonPadding
= 4; // Padding around the close button.
Could you reword this comment. Currently it doesn't explain how to use this
value.

[Peter Kasting, 2012-10-08 17:23:58]

(1) When you list multiple reviewers you always need to say exactly what you
want each to review.
(2) The last line of your CL description doesn't belong in the CL description. 
The R= lines are weird too, I've never seen this convention.

[Mike Wittman, 2012-10-08 21:34:19]

http://codereview.chromium.org/11077006/diff/1/chrome/browser/ui/constrained_...
File chrome/browser/ui/constrained_window.h (right):

http://codereview.chromium.org/11077006/diff/1/chrome/browser/ui/constrained_...
chrome/browser/ui/constrained_window.h:25: static const int kCloseButtonPadding
= 4; // Padding around the close button.
I think it would be helpful to create an ASCII art diagram to document the
layout constants.  It's likely to be difficult to understand solely via text
descriptions.

http://codereview.chromium.org/11077006/diff/1/chrome/browser/ui/views/constr...
File chrome/browser/ui/views/constrained_window_frame_simple.cc (right):

http://codereview.chromium.org/11077006/diff/1/chrome/browser/ui/views/constr...
chrome/browser/ui/views/constrained_window_frame_simple.cc:114: void
ConstrainedWindowFrameSimple::SetHeaderView(HeaderViews* header_views)
The custom header padding needs to be applied here also.

http://codereview.chromium.org/11077006/diff/1/chrome/browser/ui/views/constr...
chrome/browser/ui/views/constrained_window_frame_simple.cc:207:
header_size.height() + ConstrainedWindow::kRowPadding;
This should also include ConstrainedWindow::kVerticalPadding to account for the
bottom part of the frame.

http://codereview.chromium.org/11077006/diff/1/ui/views/controls/button/chrom...
File ui/views/controls/button/chrome_style.cc (right):

http://codereview.chromium.org/11077006/diff/1/ui/views/controls/button/chrom...
ui/views/controls/button/chrome_style.cc:278: rect, kFocusRingRadius,
kFocusRingRadius, paint);
Is there a reason to use SkCanvas::drawRoundRect rather than
gfx::Canvas::DrawRoundRect?  If not, use gfx::Canvas::DrawRoundRect; if so,
convert kFocusRingRadius to SkScalar using SkIntToScalar.

[please use gerrit instead, 2012-10-09 18:38:09]

Made the padding implementation more clear.
Added ASCII art comments for spacing.

https://codereview.chromium.org/11077006/diff/1/chrome/browser/ui/constrained...
File chrome/browser/ui/constrained_window.h (right):

https://codereview.chromium.org/11077006/diff/1/chrome/browser/ui/constrained...
chrome/browser/ui/constrained_window.h:25: static const int kCloseButtonPadding
= 4; // Padding around the close button.
On 2012/10/08 17:19:12, sail wrote:
> Could you reword this comment. Currently it doesn't explain how to use this
> value.

Done.

https://codereview.chromium.org/11077006/diff/1/chrome/browser/ui/constrained...
chrome/browser/ui/constrained_window.h:25: static const int kCloseButtonPadding
= 4; // Padding around the close button.
On 2012/10/08 21:34:19, Mike Wittman wrote:
> I think it would be helpful to create an ASCII art diagram to document the
> layout constants.  It's likely to be difficult to understand solely via text
> descriptions.

I admit that kCloseButtonPadding comment is confusing. Drew an ASCII diagram :)

https://codereview.chromium.org/11077006/diff/1/chrome/browser/ui/views/const...
File chrome/browser/ui/views/constrained_window_frame_simple.cc (right):

https://codereview.chromium.org/11077006/diff/1/chrome/browser/ui/views/const...
chrome/browser/ui/views/constrained_window_frame_simple.cc:114: void
ConstrainedWindowFrameSimple::SetHeaderView(HeaderViews* header_views)
On 2012/10/08 21:34:19, Mike Wittman wrote:
> The custom header padding needs to be applied here also.

One of my upcoming CLs gives the ability to set custom margins through the
dialog delegate. I think that will address your concern.

https://codereview.chromium.org/11077006/diff/1/chrome/browser/ui/views/const...
chrome/browser/ui/views/constrained_window_frame_simple.cc:207:
header_size.height() + ConstrainedWindow::kRowPadding;
On 2012/10/08 21:34:19, Mike Wittman wrote:
> This should also include ConstrainedWindow::kVerticalPadding to account for
the
> bottom part of the frame.

Done.

https://codereview.chromium.org/11077006/diff/1/ui/views/controls/button/chro...
File ui/views/controls/button/chrome_style.cc (right):

https://codereview.chromium.org/11077006/diff/1/ui/views/controls/button/chro...
ui/views/controls/button/chrome_style.cc:278: rect, kFocusRingRadius,
kFocusRingRadius, paint);
On 2012/10/08 21:34:19, Mike Wittman wrote:
> Is there a reason to use SkCanvas::drawRoundRect rather than
> gfx::Canvas::DrawRoundRect?  If not, use gfx::Canvas::DrawRoundRect; if so,
> convert kFocusRingRadius to SkScalar using SkIntToScalar.

I am using SkCanvas::drawRoundRect to be able to inset by 1/2 pixel to align the
stroke with pixel centers, just like in
ChromeStyleTextButtonBorderPainter::Paint().

[please use gerrit instead, 2012-10-09 18:48:16]

pkasting@: I've removed the nonstandard R= and +email lines from the changelist
description. Below is the description of what needs to be reviewed by each
person.

pkasting@: Please review the following files, where I set the correct padding
for the frameless constrained window dialogs.
 - chrome/browser/ui/constrained_window.h
 - chrome/browser/ui/views/constrained_window_frame_simple.h
 - chrome/browser/ui/views/constrained_window_frame_simple.cc
 - chrome/browser/ui/views/tab_modal_confirm_dialog_views.cc

ben@: Please review the changes to the following file, where I makde the
button-focus-border 1px wide and blue.
 - ui/views/controls/button/chrome_style.cc

ben@: Please review the changes to the following file, where I account for the
message font descent in dialog boxes. This gets us to 20px distance from the
base of the message font to the buttons at the bottom. If we do not account for
the font descent, then we have 20px to the descent of the font, which is extra
3px of space.
 - ui/views/controls/message_box_view.cc

wittman@: You have been working on the frameless constrained window dialogs in
Views. Thus your review of everything will be helpfull, too.

[please use gerrit instead, 2012-10-09 18:51:10]

sail@: The changes to the constants in the following file might affect your work
on frameless constrained window dialogs in Cocoa:
 - chrome/browser/ui/constrained_window.h

[sail, 2012-10-09 18:54:57]

lgtm

[Peter Kasting, 2012-10-09 19:31:09]

https://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/constrai...
File chrome/browser/ui/constrained_window.h (right):

https://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/constrai...
chrome/browser/ui/constrained_window.h:62: static const int kRowPadding = 15;
How can these values be guaranteed to be correct across platforms and fonts?  It
seems like we'd have to calculate these dynamically to ensure the sizings you
want.

https://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/constrai...
chrome/browser/ui/constrained_window.h:70: // close button has an invisible 6px
border.
Is this border so the button will have a larger hit area?  Because if not it'd
be clearer if we could eliminate the border somehow.

https://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/views/co...
File chrome/browser/ui/views/constrained_window_frame_simple.cc (right):

https://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/views/co...
chrome/browser/ui/views/constrained_window_frame_simple.cc:99: views::View*
viewToPad,
Nit: Use underscores

https://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/views/co...
chrome/browser/ui/views/constrained_window_frame_simple.cc:134:
ConstrainedWindow::kHorizontalPadding) {
Instead of constructing an Insets member, do something like:

  set_border(Border::CreateEmptyBorder(T, L, B, R));

This will actually reserve this space for you so you don't need to discount for
it everywhere, which in turn should make your HeaderLayout class and the rest of
the code here simpler.

https://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/views/co...
chrome/browser/ui/views/constrained_window_frame_simple.cc:138:
client_insets_.right());
Nit: Indent even

https://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/views/co...
chrome/browser/ui/views/constrained_window_frame_simple.cc:197: gfx::Size
header_size =
Nit: Simpler:

  gfx::Rect bounds(GetContentsBounds());
  if (header_views_->header)
    bounds.Inset(0, header_views_->header->GetPreferredSize().height(), 0, 0);
  return bounds;

This assumes you follow the border suggestion above.

https://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/views/co...
chrome/browser/ui/views/constrained_window_frame_simple.cc:214: gfx::Size
header_size =
Nit: Simplify by rewriting like the function above and making use of something
like:

  bounds.Inset(-GetInsets());

[Mike Wittman, 2012-10-09 22:55:46]

This change makes the layout logic a lot clearer.

https://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/views/co...
File chrome/browser/ui/views/constrained_window_frame_simple.cc (right):

https://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/views/co...
chrome/browser/ui/views/constrained_window_frame_simple.cc:134:
ConstrainedWindow::kHorizontalPadding) {
On 2012/10/09 19:31:10, Peter Kasting wrote:
> Instead of constructing an Insets member, do something like:
> 
>   set_border(Border::CreateEmptyBorder(T, L, B, R));
> 
> This will actually reserve this space for you so you don't need to discount
for
> it everywhere, which in turn should make your HeaderLayout class and the rest
of
> the code here simpler.

This is good advice, but if you go this route please verify that it works OK on
Windows.  I've run into some issues getting borders to coexist properly with a
window mask on that platform.

https://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/views/co...
File chrome/browser/ui/views/constrained_window_frame_simple.h (right):

https://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/views/co...
chrome/browser/ui/views/constrained_window_frame_simple.h:74: gfx::Insets
client_insets_;
For clarity, make this const.

https://codereview.chromium.org/11077006/diff/9001/ui/views/controls/button/c...
File ui/views/controls/button/chrome_style.cc (right):

https://codereview.chromium.org/11077006/diff/9001/ui/views/controls/button/c...
ui/views/controls/button/chrome_style.cc:278: rect, kFocusRingRadius,
kFocusRingRadius, paint);
kFocusRingRadius should be converted to SkScalar before passing to
drawRoundRect.

https://codereview.chromium.org/11077006/diff/9001/ui/views/controls/message_...
File ui/views/controls/message_box_view.cc (right):

https://codereview.chromium.org/11077006/diff/9001/ui/views/controls/message_...
ui/views/controls/message_box_view.cc:282: layout->AddPaddingRow(0,
inter_row_vertical_spacing_ - descent);
This should be behind the flag.  Also, is this class used in non-constrained
window contexts?  If so, and we don't want to adopt this layout universally,
you'll need to make this configurable and plumb the setting from the relevant
dialogs.

[Ben Goodger (Google), 2012-10-10 16:11:54]

https://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/constrai...
File chrome/browser/ui/constrained_window.h (right):

https://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/constrai...
chrome/browser/ui/constrained_window.h:21: // The kTitleTopPadding is the
padding between the top of the window and the
I would tend to put all of this in a file separate from the constrained window
API (constrained_window_constants.h?). it's huge and distracting. Also I think
if you come up with clear variable names the description/diagrams are probably
unnecessary.

[please use gerrit instead, 2012-10-10 19:12:57]

Addressing reviewers comments inline. The code should be simpler.

sail@: Do we need to change CSS for WebUI to take into account the updated close
buttons?

http://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/constrain...
File chrome/browser/ui/constrained_window.h (right):

http://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/constrain...
chrome/browser/ui/constrained_window.h:21: // The kTitleTopPadding is the
padding between the top of the window and the
On 2012/10/10 16:11:54, Ben Goodger (Google) wrote:
> I would tend to put all of this in a file separate from the constrained window
> API (constrained_window_constants.h?). it's huge and distracting. Also I think
> if you come up with clear variable names the description/diagrams are probably
> unnecessary.

Kenmoore@ has asked to use regular padding without mentioning the x-heights and
baselines of fonts. This means that we do not need to have the diagrams any
more. I am removing them.

http://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/constrain...
chrome/browser/ui/constrained_window.h:62: static const int kRowPadding = 15;
On 2012/10/09 19:31:10, Peter Kasting wrote:
> How can these values be guaranteed to be correct across platforms and fonts? 
It
> seems like we'd have to calculate these dynamically to ensure the sizings you
> want.

The author of the mocks (kenmoore@) has asked us to use regular padding as we
have done before and ignore the ascents, descents, x-heights, and baselines. If
fonts change in the future or vary across platforms such that x-height becoms
19px instead of 20px, it is not a disaster.

http://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/constrain...
chrome/browser/ui/constrained_window.h:70: // close button has an invisible 6px
border.
On 2012/10/09 19:31:10, Peter Kasting wrote:
> Is this border so the button will have a larger hit area?  Because if not it'd
> be clearer if we could eliminate the border somehow.

The border is so the button has a larger hit area. Kenmoore@ says that the
larger button was originally motivated by touch-friendliness. The UX folks have
since changed their minds and have provided us with new (smaller) x button,
which I am checking in as part of this CL now.

http://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/views/con...
File chrome/browser/ui/views/constrained_window_frame_simple.cc (right):

http://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/views/con...
chrome/browser/ui/views/constrained_window_frame_simple.cc:99: views::View*
viewToPad,
On 2012/10/09 19:31:10, Peter Kasting wrote:
> Nit: Use underscores

I've removed this method entirely in favor of set_border().

http://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/views/con...
chrome/browser/ui/views/constrained_window_frame_simple.cc:134:
ConstrainedWindow::kHorizontalPadding) {
On 2012/10/09 22:55:46, Mike Wittman wrote:
> On 2012/10/09 19:31:10, Peter Kasting wrote:
> > Instead of constructing an Insets member, do something like:
> > 
> >   set_border(Border::CreateEmptyBorder(T, L, B, R));
> > 
> > This will actually reserve this space for you so you don't need to discount
> for
> > it everywhere, which in turn should make your HeaderLayout class and the
rest
> of
> > the code here simpler.
> 
> This is good advice, but if you go this route please verify that it works OK
on
> Windows.  I've run into some issues getting borders to coexist properly with a
> window mask on that platform.

wittman@: Please try the latest patch set on your Windows build machine.

http://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/views/con...
chrome/browser/ui/views/constrained_window_frame_simple.cc:134:
ConstrainedWindow::kHorizontalPadding) {
On 2012/10/09 19:31:10, Peter Kasting wrote:
> Instead of constructing an Insets member, do something like:
> 
>   set_border(Border::CreateEmptyBorder(T, L, B, R));
> 
> This will actually reserve this space for you so you don't need to discount
for
> it everywhere, which in turn should make your HeaderLayout class and the rest
of
> the code here simpler.

I am using set_border now, but I want to keep |client_insets_| member, because
ConstrainedWindow will need to override the insets to be {0, 0, 0, 0}.

http://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/views/con...
chrome/browser/ui/views/constrained_window_frame_simple.cc:138:
client_insets_.right());
On 2012/10/09 19:31:10, Peter Kasting wrote:
> Nit: Indent even

Done.

http://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/views/con...
chrome/browser/ui/views/constrained_window_frame_simple.cc:197: gfx::Size
header_size =
On 2012/10/09 19:31:10, Peter Kasting wrote:
> Nit: Simpler:
> 
>   gfx::Rect bounds(GetContentsBounds());
>   if (header_views_->header)
>     bounds.Inset(0, header_views_->header->GetPreferredSize().height(), 0, 0);
>   return bounds;
> 
> This assumes you follow the border suggestion above.

Done.

http://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/views/con...
chrome/browser/ui/views/constrained_window_frame_simple.cc:214: gfx::Size
header_size =
On 2012/10/09 19:31:10, Peter Kasting wrote:
> Nit: Simplify by rewriting like the function above and making use of something
> like:
> 
>   bounds.Inset(-GetInsets());

Done.

http://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/views/con...
File chrome/browser/ui/views/constrained_window_frame_simple.h (right):

http://codereview.chromium.org/11077006/diff/9001/chrome/browser/ui/views/con...
chrome/browser/ui/views/constrained_window_frame_simple.h:74: gfx::Insets
client_insets_;
On 2012/10/09 22:55:46, Mike Wittman wrote:
> For clarity, make this const.

Done.

http://codereview.chromium.org/11077006/diff/9001/ui/views/controls/button/ch...
File ui/views/controls/button/chrome_style.cc (right):

http://codereview.chromium.org/11077006/diff/9001/ui/views/controls/button/ch...
ui/views/controls/button/chrome_style.cc:278: rect, kFocusRingRadius,
kFocusRingRadius, paint);
On 2012/10/09 22:55:46, Mike Wittman wrote:
> kFocusRingRadius should be converted to SkScalar before passing to
> drawRoundRect.
> 

Done.

http://codereview.chromium.org/11077006/diff/9001/ui/views/controls/message_b...
File ui/views/controls/message_box_view.cc (right):

http://codereview.chromium.org/11077006/diff/9001/ui/views/controls/message_b...
ui/views/controls/message_box_view.cc:282: layout->AddPaddingRow(0,
inter_row_vertical_spacing_ - descent);
On 2012/10/09 22:55:46, Mike Wittman wrote:
> This should be behind the flag.  Also, is this class used in non-constrained
> window contexts?  If so, and we don't want to adopt this layout universally,
> you'll need to make this configurable and plumb the setting from the relevant
> dialogs.

The word from UX (kenmoore@) is to adjust the regular spacing instead of
accounting for font descents, ascents, x-heights, and baselines.

[sail, 2012-10-10 19:18:00]

On 2012/10/10 19:12:57, Rouslan Solomakhin wrote:
> Addressing reviewers comments inline. The code should be simpler.
> 
> sail@: Do we need to change CSS for WebUI to take into account the updated
close
> buttons?

Yes, you'll need to add a pressed state. Also, is the size of the image
different? If so you'll either have to ask the UX people to redo the icons or
make sure that the WebUI still looks correct.
Also, you'll probably want a browser/resources/ OWNERS reviewer.

[please use gerrit instead, 2012-10-10 19:24:00]

arv@: Please review the changes under chrome/browser/resources. The UX folks
have given us the update x.png and x-hover.png. They have also given us a new
file: x-pressed.png.

[arv (Not doing code reviews), 2012-10-10 19:29:22]

LGTM rubberstamp for the images

[Peter Kasting, 2012-10-10 20:22:26]

http://codereview.chromium.org/11077006/diff/7004/chrome/browser/ui/views/con...
File chrome/browser/ui/views/constrained_window_frame_simple.cc (right):

http://codereview.chromium.org/11077006/diff/7004/chrome/browser/ui/views/con...
chrome/browser/ui/views/constrained_window_frame_simple.cc:112:
client_insets_(ConstrainedWindow::kClientTopPadding,
I didn't totally grasp why you wanted this member.  You said "ConstrainedWindow
will need to override the insets".  How is that going to happen, since there
doesn't currently seem to be any access to this member?  It seems like the right
way would be for the ConstrainedWindow to delete the border, or specify that it
shouldn't be added in the first place, maybe?

http://codereview.chromium.org/11077006/diff/7004/chrome/browser/ui/views/con...
chrome/browser/ui/views/constrained_window_frame_simple.cc:119:
client_insets_.right());
It seems like the header shouldn't have to worry about these insets since the
border should already be taking care of them.

[please use gerrit instead, 2012-10-11 00:34:57]

pkasting@: I've modified the header to be significantly simpler, like you
suggested.

csilv@: Please review the changes to
chrome/browser/resources/shared/css/overlay.css to adjust the padding around the
new close buttons.

https://codereview.chromium.org/11077006/diff/7004/chrome/browser/ui/views/co...
File chrome/browser/ui/views/constrained_window_frame_simple.cc (right):

https://codereview.chromium.org/11077006/diff/7004/chrome/browser/ui/views/co...
chrome/browser/ui/views/constrained_window_frame_simple.cc:112:
client_insets_(ConstrainedWindow::kClientTopPadding,
On 2012/10/10 20:22:26, Peter Kasting wrote:
> I didn't totally grasp why you wanted this member.  You said
"ConstrainedWindow
> will need to override the insets".  How is that going to happen, since there
> doesn't currently seem to be any access to this member?  It seems like the
right
> way would be for the ConstrainedWindow to delete the border, or specify that
it
> shouldn't be added in the first place, maybe?

I have not decided how this is going to be done. For now, let's just get rid of
the member.

https://codereview.chromium.org/11077006/diff/7004/chrome/browser/ui/views/co...
chrome/browser/ui/views/constrained_window_frame_simple.cc:119:
client_insets_.right());
On 2012/10/10 20:22:26, Peter Kasting wrote:
> It seems like the header shouldn't have to worry about these insets since the
> border should already be taking care of them.

Done.

[please use gerrit instead, 2012-10-11 00:39:40]

https://codereview.chromium.org/11077006/diff/8007/chrome/browser/ui/views/co...
File chrome/browser/ui/views/constrained_window_views.cc (right):

https://codereview.chromium.org/11077006/diff/8007/chrome/browser/ui/views/co...
chrome/browser/ui/views/constrained_window_views.cc:622:
dialog_client_view->set_background(background);
Adding this back in, because this background is still necessary on Windows.
(Thank you wittman@.)

[csilv, 2012-10-11 16:04:16]

chrome/browser/resources lgtm

[please use gerrit instead, 2012-10-11 16:05:50]

sail@: Please review the changes to constrained_window_alert.mm. I am using
kTitleTopPadding, kClientTopPadding, and kClientBottomPadding instead of
kVerticalPadding.

[sail, 2012-10-11 16:14:50]

constrained_window_alert.mm LGTM

[Peter Kasting, 2012-10-11 18:00:24]

LGTM for ui/views for now.  This code needs an overhaul, we'll do that as a
separate pass.

[Mike Wittman, 2012-10-11 18:03:11]

lgtm on latest changes

[sail, 2012-10-11 18:15:23]

http://codereview.chromium.org/11077006/diff/15008/chrome/browser/ui/cocoa/in...
File chrome/browser/ui/cocoa/intents/web_intent_picker_view_controller.mm
(right):

http://codereview.chromium.org/11077006/diff/15008/chrome/browser/ui/cocoa/in...
chrome/browser/ui/cocoa/intents/web_intent_picker_view_controller.mm:149:
-ConstrainedWindow::kClientBottomPadding);
this will not work
this insets the entire rect (top and bottom).
you'll bounds.origin.y = top_padding
bounds.size.height = NSHeight(innerFrame) - top_padding - bottom_padding

http://codereview.chromium.org/11077006/diff/15008/chrome/browser/ui/cocoa/in...
chrome/browser/ui/cocoa/intents/web_intent_picker_view_controller.mm:169:
ConstrainedWindow::kClientBottomPadding);
same as above

[please use gerrit instead, 2012-10-11 18:41:27]

sail@: Please take a look again. I believe that I have addressed your comments.

https://codereview.chromium.org/11077006/diff/15008/chrome/browser/ui/cocoa/i...
File chrome/browser/ui/cocoa/intents/web_intent_picker_view_controller.mm
(right):

https://codereview.chromium.org/11077006/diff/15008/chrome/browser/ui/cocoa/i...
chrome/browser/ui/cocoa/intents/web_intent_picker_view_controller.mm:149:
-ConstrainedWindow::kClientBottomPadding);
On 2012/10/11 18:15:23, sail wrote:
> this will not work
> this insets the entire rect (top and bottom).
> you'll bounds.origin.y = top_padding
> bounds.size.height = NSHeight(innerFrame) - top_padding - bottom_padding

Done.

https://codereview.chromium.org/11077006/diff/15008/chrome/browser/ui/cocoa/i...
chrome/browser/ui/cocoa/intents/web_intent_picker_view_controller.mm:169:
ConstrainedWindow::kClientBottomPadding);
On 2012/10/11 18:15:23, sail wrote:
> same as above

Done.

[sail, 2012-10-11 18:42:34]

web_intent_picker_view_controller.mm LGTM

[please use gerrit instead, 2012-10-11 18:48:11]

On 2012/10/11 18:42:34, sail wrote:
> web_intent_picker_view_controller.mm LGTM

Waiting for a mac try-bot to succeed before committing.

[commit-bot: I haz the power, 2012-10-11 19:09:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/11077006/8010

[commit-bot: I haz the power, 2012-10-11 19:10:00]

Presubmit check for 11077006-8010 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ui/views

Presubmit checks took 1.8s to calculate.

[please use gerrit instead, 2012-10-11 19:13:39]

On 2012/10/11 19:10:00, I haz the power (commit-bot) wrote:
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     ui/views

ben@: Please review the changes to the following file, where I make the
button-focus-border 1px wide and blue, as in http://i.imgur.com/IQbR9.png 
 - ui/views/controls/button/chrome_style.cc

[please use gerrit instead, 2012-10-12 14:51:21]

ben@: A friendly ping...

[Ben Goodger (Google), 2012-10-12 16:48:59]

http://codereview.chromium.org/11077006/diff/8010/chrome/browser/ui/constrain...
File chrome/browser/ui/constrained_window.h (right):

http://codereview.chromium.org/11077006/diff/8010/chrome/browser/ui/constrain...
chrome/browser/ui/constrained_window.h:21: static const int kTitleTopPadding =
15; // Padding above the title.
see my comment in another CL. I think you should move this and all the other
shared style hints to a constrained_window_constants.h file, since it's
unrelated to the interface of creating/showing a constrained window and is in
fact just a shared implementation detail between platforms.

[please use gerrit instead, 2012-10-12 18:56:07]

--------------------------------------
ben@:
I've moved the constants to constrained_window_contants.h. PTLA

--------------------------------------
sail@:
PTLA at chrome/browser/ui/cocoa/*, where I use ConstrainedWindowConstants
instead of ConstrainedWindow for paddings and font styles.

--------------------------------------

http://codereview.chromium.org/11077006/diff/8010/chrome/browser/ui/constrain...
File chrome/browser/ui/constrained_window.h (right):

http://codereview.chromium.org/11077006/diff/8010/chrome/browser/ui/constrain...
chrome/browser/ui/constrained_window.h:21: static const int kTitleTopPadding =
15; // Padding above the title.
On 2012/10/12 16:48:59, Ben Goodger (Google) wrote:
> see my comment in another CL. I think you should move this and all the other
> shared style hints to a constrained_window_constants.h file, since it's
> unrelated to the interface of creating/showing a constrained window and is in
> fact just a shared implementation detail between platforms.

Done.

[sail, 2012-10-12 19:02:06]

cocoa/* LGTM

[Ben Goodger (Google), 2012-10-12 21:19:40]

lgtm

[commit-bot: I haz the power, 2012-10-12 21:28:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/11077006/28001

[commit-bot: I haz the power, 2012-10-12 21:29:04]

Presubmit check for 11077006-28001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright (\(c\) )?(2012|2011|2010|2009|2008|2007|2006|2006-2008) The
Chromium Authors\. All rights reserved\.\n.*? Use of this source code is
governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?:
\*/)?\n
Found a bad license header in these files:
  chrome/browser/ui/constrained_window_constants.h

Presubmit checks took 2.5s to calculate.

[commit-bot: I haz the power, 2012-10-12 21:40:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/11077006/41001

[commit-bot: I haz the power, 2012-10-13 01:41:10]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-13 01:48:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/11077006/41001

[commit-bot: I haz the power, 2012-10-13 04:42:58]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-13 18:48:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/11077006/41001

[commit-bot: I haz the power, 2012-10-13 19:51:10]

Change committed as 161772