fix GridLayout::CreatePanel for new style dialogs

fix GridLayout::CreatePanel for new style dialogs

BUG=231523
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194688

[msw, 2013-04-15 22:48:37]

https://codereview.chromium.org/13925019/diff/3001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/task_manager_view.cc (right):

https://codereview.chromium.org/13925019/diff/3001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/task_manager_view.cc:501: (new_style ?
views::kPanelHorizMarginNewStyle : views::kPanelHorizMargin);
nit: make a local for this margin value to use here and below.

https://codereview.chromium.org/13925019/diff/3001/ui/views/layout/grid_layou...
File ui/views/layout/grid_layout.cc (right):

https://codereview.chromium.org/13925019/diff/3001/ui/views/layout/grid_layou...
ui/views/layout/grid_layout.cc:680: int horizontal_margin =
DialogDelegate::UseNewStyle() ?
I'm don't think we should change the default GridLayout margin. Other individual
host Views should probably call SetInsets() with their desired values.

[Evan Stade, 2013-04-15 23:52:53]

https://codereview.chromium.org/13925019/diff/3001/ui/views/layout/grid_layou...
File ui/views/layout/grid_layout.cc (right):

https://codereview.chromium.org/13925019/diff/3001/ui/views/layout/grid_layou...
ui/views/layout/grid_layout.cc:680: int horizontal_margin =
DialogDelegate::UseNewStyle() ?
On 2013/04/15 22:48:37, msw wrote:
> I'm don't think we should change the default GridLayout margin. Other
individual
> host Views should probably call SetInsets() with their desired values.

this is not for all GridLayouts. It's only for "panels", i.e. GridLayouts
created via CreatePanel. Only dialogs should be using this function.

[Evan Stade, 2013-04-16 00:20:37]

TaskManagerView fixed.

https://codereview.chromium.org/13925019/diff/3001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/task_manager_view.cc (right):

https://codereview.chromium.org/13925019/diff/3001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/task_manager_view.cc:501: (new_style ?
views::kPanelHorizMarginNewStyle : views::kPanelHorizMargin);
On 2013/04/15 22:48:37, msw wrote:
> nit: make a local for this margin value to use here and below.

Done.

[msw, 2013-04-16 00:53:21]

LGTM with a couple nits. Please post before/after pics.

https://codereview.chromium.org/13925019/diff/3001/ui/views/layout/grid_layou...
File ui/views/layout/grid_layout.cc (right):

https://codereview.chromium.org/13925019/diff/3001/ui/views/layout/grid_layou...
ui/views/layout/grid_layout.cc:680: int horizontal_margin =
DialogDelegate::UseNewStyle() ?
On 2013/04/15 23:52:53, Evan Stade wrote:
> On 2013/04/15 22:48:37, msw wrote:
> > I'm don't think we should change the default GridLayout margin. Other
> individual
> > host Views should probably call SetInsets() with their desired values.
> 
> this is not for all GridLayouts. It's only for "panels", i.e. GridLayouts
> created via CreatePanel. Only dialogs should be using this function.

Ah, I didn't realize that. Then this seems reasonable.

https://codereview.chromium.org/13925019/diff/10001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/task_manager_view.cc (right):

https://codereview.chromium.org/13925019/diff/10001/chrome/browser/ui/views/t...
chrome/browser/ui/views/task_manager_view.cc:506: parent_bounds.bottom() -
size.height() - vertical_margin;
nit: fits on the line above.

https://codereview.chromium.org/13925019/diff/10001/chrome/browser/ui/views/t...
chrome/browser/ui/views/task_manager_view.cc:525: views::kPanelVertMargin);
nit: fits on the line above.

[Evan Stade, 2013-04-16 01:39:39]

+sky

some of these are pretty hard to trigger, like the ImportLockDialogView. Before
and after in both new and old style would be 4 screenshots per dialog. There are
many, many dialogs that this CL could potentially affect (taking into account
all the ones that use CreatePanel or MessageBoxView). So posting screenshots
would be a very time-consuming process. Is there any dialog in particular you
would like to see screenshots for? I think the easiest thing to do would be to
download and apply this patch, then trigger all the dialogs (or a sufficient
subset of them).

https://codereview.chromium.org/13925019/diff/10001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/task_manager_view.cc (right):

https://codereview.chromium.org/13925019/diff/10001/chrome/browser/ui/views/t...
chrome/browser/ui/views/task_manager_view.cc:525: views::kPanelVertMargin);
On 2013/04/16 00:53:21, msw wrote:
> nit: fits on the line above.

Done.

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/c...
File chrome/browser/ui/views/confirm_bubble_views.cc (right):

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/c...
chrome/browser/ui/views/confirm_bubble_views.cc:18: views::GridLayout* layout =
views::GridLayout::CreatePanel(this);
this is a behavioral change, but I think it's an improvement.

https://codereview.chromium.org/13925019/diff/23002/ui/views/controls/message...
File ui/views/controls/message_box_view.h (left):

https://codereview.chromium.org/13925019/diff/23002/ui/views/controls/message...
ui/views/controls/message_box_view.h:51: int top_inset;
NB: nothing was using this it seems.

[msw, 2013-04-16 01:58:24]

Screenshots of one or two example dialogs would be nice.
(perhaps just after pics, if before+after is untenable)
(perhaps just new-style, if both old+new is untenable)

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/chromeos/p...
File chrome/browser/chromeos/power/idle_action_warning_dialog_view.cc (right):

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/chromeos/p...
chrome/browser/chromeos/power/idle_action_warning_dialog_view.cc:60: if
(DialogDelegate::UseNewStyle()) {
nit: I prefer the pattern of setting a local first instead:
const int horizontal_margin = DialogDelegate::UseNewStyle() ? ... : ...

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/c...
File chrome/browser/ui/views/confirm_bubble_views.cc (right):

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/c...
chrome/browser/ui/views/confirm_bubble_views.cc:18: views::GridLayout* layout =
views::GridLayout::CreatePanel(this);
On 2013/04/16 01:39:39, Evan Stade wrote:
> this is a behavioral change, but I think it's an improvement.

A screenshot would help :)

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc
(right):

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:208: width
+= views::kPanelHorizMargin * 3;
Shouldn't this depend on DialogDelegate::UseNewStyle() like below?

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:227: x +=
views::kPanelHorizMargin;
Should this optionally use views::kButtonHEdgeMarginNew?

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:237: x +=
views::kPanelHorizMargin;
Should this optionally use views::kButtonHEdgeMarginNew?

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right):

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:309: return
DialogDelegate::UseNewStyle();
I don't think this matters (not checked in DialogDelegate's BubbleFrameView).

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/i...
File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right):

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/i...
chrome/browser/ui/views/importer/import_lock_dialog_view.cc:63: if
(DialogDelegate::UseNewStyle()) {
nit: I prefer the pattern of setting a local first instead:
const int horizontal_margin = DialogDelegate::UseNewStyle() ? ... : ...

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/t...
File chrome/browser/ui/views/task_manager_view.cc (right):

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/t...
chrome/browser/ui/views/task_manager_view.cc:500: int horizontal_margin =
nit: const

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/t...
chrome/browser/ui/views/task_manager_view.cc:502: int vertical_margin =
nit: const

https://codereview.chromium.org/13925019/diff/23002/ui/views/layout/grid_layo...
File ui/views/layout/grid_layout.cc (right):

https://codereview.chromium.org/13925019/diff/23002/ui/views/layout/grid_layo...
ui/views/layout/grid_layout.cc:680: int horizontal_margin =
DialogDelegate::UseNewStyle() ?
nit: const

https://codereview.chromium.org/13925019/diff/23002/ui/views/layout/grid_layo...
File ui/views/layout/grid_layout.h (right):

https://codereview.chromium.org/13925019/diff/23002/ui/views/layout/grid_layo...
ui/views/layout/grid_layout.h:112: const gfx::Insets& insets() { return insets_;
}
I didn't see where this is used; please remove if unused.

[Evan Stade, 2013-04-16 19:58:25]

Scott, sorry for lack of context. I added you for OWNERS review of pretty much
the whole CL. The comment about screenshots was directed at msw@.

I have added some screenshots to the bug.

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/chromeos/p...
File chrome/browser/chromeos/power/idle_action_warning_dialog_view.cc (right):

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/chromeos/p...
chrome/browser/chromeos/power/idle_action_warning_dialog_view.cc:60: if
(DialogDelegate::UseNewStyle()) {
On 2013/04/16 01:58:24, msw wrote:
> nit: I prefer the pattern of setting a local first instead:
> const int horizontal_margin = DialogDelegate::UseNewStyle() ? ... : ...

Normally I do as well, except that this should change soon. It will be easier
(and create a cleaner changelog) to just delete half of the this block.

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/c...
File chrome/browser/ui/views/confirm_bubble_views.cc (right):

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/c...
chrome/browser/ui/views/confirm_bubble_views.cc:18: views::GridLayout* layout =
views::GridLayout::CreatePanel(this);
On 2013/04/16 01:58:24, msw wrote:
> On 2013/04/16 01:39:39, Evan Stade wrote:
> > this is a behavioral change, but I think it's an improvement.
> 
> A screenshot would help :)

Done.

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc
(right):

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:208: width
+= views::kPanelHorizMargin * 3;
On 2013/04/16 01:58:24, msw wrote:
> Shouldn't this depend on DialogDelegate::UseNewStyle() like below?

Done.

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:227: x +=
views::kPanelHorizMargin;
On 2013/04/16 01:58:24, msw wrote:
> Should this optionally use views::kButtonHEdgeMarginNew?

actually it should use a different constant (fixed)

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:237: x +=
views::kPanelHorizMargin;
On 2013/04/16 01:58:24, msw wrote:
> Should this optionally use views::kButtonHEdgeMarginNew?

ditto

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right):

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:309: return
DialogDelegate::UseNewStyle();
On 2013/04/16 01:58:24, msw wrote:
> I don't think this matters (not checked in DialogDelegate's BubbleFrameView).

regardless, it seems correct, and is less confusing than what was there before.

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/t...
File chrome/browser/ui/views/task_manager_view.cc (right):

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/t...
chrome/browser/ui/views/task_manager_view.cc:500: int horizontal_margin =
On 2013/04/16 01:58:24, msw wrote:
> nit: const

Done.

https://codereview.chromium.org/13925019/diff/23002/chrome/browser/ui/views/t...
chrome/browser/ui/views/task_manager_view.cc:502: int vertical_margin =
On 2013/04/16 01:58:24, msw wrote:
> nit: const

Done.

https://codereview.chromium.org/13925019/diff/23002/ui/views/layout/grid_layo...
File ui/views/layout/grid_layout.cc (right):

https://codereview.chromium.org/13925019/diff/23002/ui/views/layout/grid_layo...
ui/views/layout/grid_layout.cc:680: int horizontal_margin =
DialogDelegate::UseNewStyle() ?
On 2013/04/16 01:58:24, msw wrote:
> nit: const

Done.

https://codereview.chromium.org/13925019/diff/23002/ui/views/layout/grid_layo...
File ui/views/layout/grid_layout.h (right):

https://codereview.chromium.org/13925019/diff/23002/ui/views/layout/grid_layo...
ui/views/layout/grid_layout.h:112: const gfx::Insets& insets() { return insets_;
}
On 2013/04/16 01:58:24, msw wrote:
> I didn't see where this is used; please remove if unused.

oh yea. Not used any more. Removed.

[msw, 2013-04-16 21:19:09]

LGTM, thanks!

[sky, 2013-04-16 22:01:02]

LGTM

[Evan Stade, 2013-04-17 21:43:15]

Committed patchset #7 manually as r194688 (presubmit successful).