views: implement width snapping for DialogDelegateViews

views: implement width snapping for DialogDelegateViews

This change:
1) Adds ViewsDelegate::GetSnappedDialogWidth, which snaps a
   dialog width up to the next allowable size;
2) Adds DialogDelegateView::GetPreferredSize, which snaps widths
   using ViewsDelegate::GetSnappedDialogWidth;
3) Forbids subclasses of DialogDelegateView to override GetPreferredSize;
4) Adds DialogDelegateView::GetUnsnappedPreferredSize to allow dialogs to
   express their preferred size before snapping;
5) Converts all existing overrides of GetPreferredSize by subclasses of
   DialogDelegateView to instead override GetUnsnappedPreferredSize

See also: https://codereview.chromium.org/2750063002/

BUG=635173

[Elly Fong-Jones, 2017-03-29 20:14:12]

Description was changed from

==========
views: implement width snapping for DialogDelegateViews

This change:
1) Adds ViewsDelegate::GetSnappedDialogWidth, which snaps a
   dialog width up to the next allowable size;
2) Adds DialogDelegateView::GetPreferredSize, which snaps widths
   using ViewsDelegate::GetSnappedDialogWidth;
3) Forbids subclasses of DialogDelegateView to override GetPreferredSize;
4) Adds DialogDelegateView::GetUnsnappedPreferredSize to allow dialogs to
   express their preferred size before snapping;
5) Converts all existing overrides of GetPreferredSize by subclasses of
   DialogDelegateView to instead override GetUnsnappedPreferredSize

See also: https://codereview.chromium.org/2750063002/

BUG=635173
==========

to

==========
views: implement width snapping for DialogDelegateViews

This change:
1) Adds ViewsDelegate::GetSnappedDialogWidth, which snaps a
   dialog width up to the next allowable size;
2) Adds DialogDelegateView::GetPreferredSize, which snaps widths
   using ViewsDelegate::GetSnappedDialogWidth;
3) Forbids subclasses of DialogDelegateView to override GetPreferredSize;
4) Adds DialogDelegateView::GetUnsnappedPreferredSize to allow dialogs to
   express their preferred size before snapping;
5) Converts all existing overrides of GetPreferredSize by subclasses of
   DialogDelegateView to instead override GetUnsnappedPreferredSize

See also: https://codereview.chromium.org/2750063002/

BUG=635173
==========

[Elly Fong-Jones, 2017-03-29 20:14:12]

ellyjones@chromium.org changed reviewers:
+ pkasting@chromium.org

[Elly Fong-Jones, 2017-03-29 20:14:21]

pkasting: ptal? :)

[Elly Fong-Jones, 2017-03-29 20:36:47]

The CQ bit was checked by ellyjones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 20:37:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 20:49:33]

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

[commit-bot: I haz the power, 2017-03-29 20:49:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Kasting, 2017-03-30 00:35:36]

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc (right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc:248: // Must
hardcode a width so the label knows where to wrap.
Nit: I suggest filing a followup bug on computing label preferred sizes more
like was suggested in our discussion, and adding a TODO here to fix this.

It seems like simply hardcoding a number for the entire dialog is poor in this
case anyway, as the string is not necessarily going to wrap in all (or even
most?) languages, and we should be computing something more fine-grained.

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc:250: return
gfx::Size(kWidth, GetHeightForWidth(kWidth));
This line exposes a problem I didn't fully think through, and it happens in
multiple dialogs.

Let's say a dialog wants to be 330 px wide, and it computes a 200 px height for
that width.  With this CL, we'll snap the width to 448 px and return a preferred
size of (448, 200).  But this may leave a bad-looking amount of blank space at
the bottom of the dialog if GetHeightForWidth(448) would have returned something
< 200.

I think we should do something like this:

* Change GetUnsnappedPreferredSize() to GetPreferredWidth() and return an int.
* Implement DialogDelegateView::GetPreferredSize() by snapping
GetPreferredWidth() and then calling GetHeightForWidth() to compute the height.
* Ensure GetHeightForWidth() will do the Right Thing on all dialogs (it probably
already does for most).

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/boo...
File chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc (right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc:141:
IDS_EDITBOOKMARK_DIALOG_HEIGHT_LINES));
Hmm.  This isn't the right way to size this dialog -- if we have a localized
size at all, it should only apply to something like a treeview, not the
surrounding stuff.  And maybe not even that.

I suggest adding a note on
https://bugs.chromium.org/p/chromium/issues/detail?id=651652 that
BookmarkEditorView needs its sizing computed in a better way in the show_tree_
case, and then leaving a TODO in here referencing that.

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/con...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/con...
chrome/browser/ui/views/content_setting_bubble_contents.cc:199: }
Ugh.  This is all a mess.  Can you add a TODO here referencing bug 649650, to do
this clamping more correctly?

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/cre...
File chrome/browser/ui/views/create_application_shortcut_view.cc (right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/cre...
chrome/browser/ui/views/create_application_shortcut_view.cc:324: // TODO(evanm):
should this use IDS_CREATE_SHORTCUTS_DIALOG_WIDTH_CHARS?
Yes, or that should be removed, because it's unused currently.

But really this is another case where we should be computing the unsnapped width
in a more granular way.  There's no tracking bug in the spreadsheet for
Create[Url,Chrome]ApplicationShortcutView (which is this is parent of); I'd file
one, add it to the sheet, then add a note in that bug about this and a TODO here
referencing the bug.

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/cre...
chrome/browser/ui/views/create_application_shortcut_view.cc:327: kDialogWidth);
This is just GetHeightForWidth().

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/des...
File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc
(right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/des...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:282:
static const size_t kDialogViewWidth = 600;
Another case that needs a bug in the spreadsheet and a TODO here referencing
that bug

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/dev...
File chrome/browser/ui/views/device_chooser_content_view.cc (right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/dev...
chrome/browser/ui/views/device_chooser_content_view.cc:115: return
gfx::Size(402, 320);
This width/height should probably be based on the font rather than exact pixel
values (i.e. be a localized contents size).  The TODO for this can reference bug
610430.

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/chooser_dialog_view.cc (right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/chooser_dialog_view.cc:78:
client->set_minimum_size(gfx::Size(402, 320));
I think this line may not be needed at all.  If it's needed, we should
understand why, and where it should live, and in any case the size here
shouldn't be copy-and-pasted from DeviceChooserContentView (e.g. it should just
obtain the desired size from that class).  Another case where the TODO can
reference bug 610430.

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_popup.h (right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_popup.h:72: // views::View
overrides.
This class doesn't directly subclass View or DialogDelegateView, so really all
those overrides belong with the BubbleDialogDelegateView overrides.

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/har...
File chrome/browser/ui/views/harmony/harmony_layout_delegate.cc (right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/har...
chrome/browser/ui/views/harmony/harmony_layout_delegate.cc:84: const int
kSizes[] = {320, 448, 512};
Nit: Can probably just inline this into the next line?

  for (int size : {320, 448, 512}) {

Or does this not compile?

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/har...
File chrome/browser/ui/views/harmony/layout_delegate.h (right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/har...
chrome/browser/ui/views/harmony/layout_delegate.h:107: // Returns the smallest
allowable width for a dialog |width| points wide.
Nit: "...for a dialog that wants a width of at least |width|."

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/imp...
File chrome/browser/ui/views/importer/import_lock_dialog_view.h (right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/imp...
chrome/browser/ui/views/importer/import_lock_dialog_view.h:30: // views::View:
Nit: This doesn't directly subclass View or DialogDelegate, put all three
override groups together

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/pas...
File chrome/browser/ui/views/passwords/account_chooser_dialog_view.h (right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/pas...
chrome/browser/ui/views/passwords/account_chooser_dialog_view.h:47: //
DialogDelegateView
Nit: Combine this group with the two above (class does not subclass
WidgetDelegate or DialogDelegate directly)

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/pas...
File chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.h
(right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/pas...
chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.h:39: //
views::DialogDelegateView
Nit: Combine with two sections above

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/payment_request_dialog_view.cc (right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_dialog_view.cc:205: return
gfx::Size(450, 450);
Another case that needs a bug filed

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/payment_request_dialog_view.h (right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_dialog_view.h:89: //
views::DialogDelegateView
Nit: Combine with section below

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/per...
File chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc
(right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/per...
chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:156:
gfx::Size GetUnsnappedPreferredSize() const override;
Nit: Leave as part of the below list

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/per...
chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:313: //
TODO(estade): bubbles should default to this width.
Nit: This TODO can go away.  In fact I think this whole override can maybe go
away.

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/per...
chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:314: const
int kWidth = 320 - GetInsets().width();
I don't understand why this wants to subtract GetInsets().  Are the insets going
to be outdented on the preferred size when computing the final visible size?  If
so that's kind of lame, because that means we need to subtract off the insets in
DialogDelegateView after computing the snapped width.  But I'm hoping this is
just wrong.

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/pro...
File chrome/browser/ui/views/profiles/user_manager_view.cc (right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/pro...
chrome/browser/ui/views/profiles/user_manager_view.cc:76: return
switches::UsePasswordSeparatedSigninFlow()
The cases in this file really should also have bugs and TODOs on sizing them
programmatically...

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/pro...
File chrome/browser/ui/views/profiles/user_manager_view.h (right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/pro...
chrome/browser/ui/views/profiles/user_manager_view.h:122: //
views::DialogDelegateView:
Nit: Combine with above section

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/tas...
File chrome/browser/ui/views/task_manager_view.cc (right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/tas...
chrome/browser/ui/views/task_manager_view.cc:154: return gfx::Size(460, 270);
This should probably be using a localized size instead of a pixel value (no bug
yet on file)

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/tra...
File chrome/browser/ui/views/translate/translate_bubble_view.cc (right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/tra...
chrome/browser/ui/views/translate/translate_bubble_view.cc:245: int width = 0;
Isn't this the case you said could just be eliminated since it should be handled
by the layout manager?

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/tra...
File chrome/browser/ui/views/translate/translate_bubble_view.h (right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/tra...
chrome/browser/ui/views/translate/translate_bubble_view.h:96: //
views::DialogDelegateView methods.
Nit: Combine with three sections above, label LocationBarDelegateView

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/val...
File chrome/browser/ui/views/validation_message_bubble_view.h (right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/val...
chrome/browser/ui/views/validation_message_bubble_view.h:32: gfx::Size
GetUnsnappedPreferredSize() const override;
Nit: Leave in below list

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/web...
File chrome/browser/ui/views/webshare/webshare_target_picker_view.h (right):

https://codereview.chromium.org/2785683003/diff/1/chrome/browser/ui/views/web...
chrome/browser/ui/views/webshare/webshare_target_picker_view.h:46: gfx::Size
GetUnsnappedPreferredSize() const override;
Nit: Combine with two sections below

https://codereview.chromium.org/2785683003/diff/1/ui/views/bubble/info_bubble.cc
File ui/views/bubble/info_bubble.cc (right):

https://codereview.chromium.org/2785683003/diff/1/ui/views/bubble/info_bubble...
ui/views/bubble/info_bubble.cc:91: return
BubbleDialogDelegateView::GetPreferredSize();
I think this causes an infinite loop.  Has to be View::GetPreferredSize(), I
believe?

https://codereview.chromium.org/2785683003/diff/1/ui/views/bubble/info_bubble.h
File ui/views/bubble/info_bubble.h (right):

https://codereview.chromium.org/2785683003/diff/1/ui/views/bubble/info_bubble...
ui/views/bubble/info_bubble.h:37: gfx::Size GetUnsnappedPreferredSize() const
override;
Nit: Leave in above list

https://codereview.chromium.org/2785683003/diff/1/ui/views/bubble/tray_bubble...
File ui/views/bubble/tray_bubble_view.h (right):

https://codereview.chromium.org/2785683003/diff/1/ui/views/bubble/tray_bubble...
ui/views/bubble/tray_bubble_view.h:134: // Overridden from
views::DialogDelegateView.
Nit: Combine this and below list into the one above

https://codereview.chromium.org/2785683003/diff/1/ui/views/window/dialog_dele...
File ui/views/window/dialog_delegate.h (right):

https://codereview.chromium.org/2785683003/diff/1/ui/views/window/dialog_dele...
ui/views/window/dialog_delegate.h:141: virtual gfx::Size
GetUnsnappedPreferredSize() const;
Nit: Comment