views: implement dialog width snapping

ui/views/window/dialog_client_view.cc

Index: ui/views/window/dialog_client_view.cc
diff --git a/ui/views/window/dialog_client_view.cc b/ui/views/window/dialog_client_view.cc
index 5d543a7572efe2b85b8772f293c766cbdb19430f..62dad8b4dfa1cc14c356c7ad3e5b23369bb5f828 100644
--- a/ui/views/window/dialog_client_view.cc
+++ b/ui/views/window/dialog_client_view.cc
@@ -41,12 +41,22 @@ bool ShouldShow(View* view) {
   return view && view->visible();
 }
 
+// Snaps the width of |size| up to the next multiple of |unit|.
+gfx::Size SnapWidthToMultipleOf(gfx::Size size, int unit) {
+  size.Enlarge(unit - 1, 0);

[tapted, 2017/03/15 22:26:11]

I find it a bit weird to manipulate the size in all cases.. E.g. you could do
something like

if (size.width() % unit == 0)
  return size;

size.set_width(size.width() + unit - size.width() % unit);
return size;

[Peter Kasting, 2017/03/21 19:17:03]

FWIW, always manipulating reads OK to me, but in my mind the idiomatic way to do
this is more like:

  size.set_width(((size.width() + unit - 1) / unit) * unit);

...which is also shorter.

+  size.set_width(size.width() - (size.width() % unit));
+  return size;
+}
+
 // Returns the bounding box required to contain |size1| and |size2|, placed one
 // atop the other.

[Peter Kasting, 2017/03/21 19:17:03]

I wonder if this function is the wrong place to implement this snapping.

Basically, I read the name and comment as simply saying this returns the minimum
bounding box to contain these sizes, without regard to how caller code uses
them.  It's the caller who is using this as "the complete size of the dialog". 
Accordingly, I would compose this functionality: leave this function alone, and
have a separate function which takes a size and snaps it to the correct dialog
size.

This will help maintenance in the future if e.g. Bret changes icon/title/close
buttons to be rendered by a "horizontal row" much like the buttons at the bottom
already are.  Then we'll want to get the bounding box of three different stacked
sizes, and round up the whole thing, which could plausibly be done with two
calls to this existing method, plus one to the new method.

 gfx::Size GetBoundingSizeForVerticalStack(const gfx::Size& size1,

[tapted, 2017/03/15 22:26:11]

rename -> GetSnappedBoundingSize?

                                           const gfx::Size& size2) {
-  return gfx::Size(std::max(size1.width(), size2.width()),
-                   size1.height() + size2.height());
+  return SnapWidthToMultipleOf(
+      gfx::Size(std::max(size1.width(), size2.width()),
+                size1.height() + size2.height()),
+      ViewsDelegate::GetInstance()->GetDistanceMetric(
+          views::DistanceMetric::DIALOG_WIDTH_SNAPPING_UNIT));

[tapted, 2017/03/15 22:26:11]

This is the ClientView - I think we need to take into account the
NonClientView's margins as well. (i.e. the parameter passed as |content_margins|
to BubbleFrameView's constructor at
BubbleDialogDelegateView::CreateNonClientFrameView

 }
 
 }  // namespace