Pre-defined widths:
Small - 332
Medium - 448
Large - 512

Min-height: 48px
Max-height: 256px

https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(MD)/Secondary%20UI%20Previews%20and%20specs%20(exports)#%2FSPEC-secondary-UI-04a-dialog-metrics.png

Deleted: SPEC-secondary-UI-04a-dialog-metrics.png 150 KB

	SPEC-secondary-UI-04a-dialog-metrics.png 150 KB View Download

are you sure we want a max height? what dialogs would trigger that? The page info bubble comes to mind, but do we really want to stuff it in a scroll view?

Collected cookies dialog before and after https://codereview.chromium.org/2375843003/ attached.

Deleted: collected cookies harmony medium.png 56.5 KB

	collected cookies harmony medium.png 56.5 KB View Download

Deleted: collected cookies default.png 60.2 KB

	collected cookies default.png 60.2 KB View Download

 Issue 652024  discusses the problem I mentioned this morning about dialogs resizing horizontally instead of vertically. bettes@ agrees that dialogs should prefer vertical resizing, so we need the dialog code to wrap the title and resize area above its contentview as needed. He attached details on line spacing for the wrapped title to that issue.

Is this something that you want to tackle, and should it be part of this dialog sizing bug? Just need to know if I need to file a separate bug on it and assign to someone else.

Dialog titles already do wrap. There may be some dialogs still remaining that implement their own title instead of using the shared title code. I tried to eliminate all of these, but our dialogs are countless.

It looks like that protocol handler bubble is one where we indeed still add a one-off title: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/content_setting_bubble_contents.cc?rcl=0&l=217

That should instead be overriding GetWindowTitle().

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/964f7ce4df1d586722cf3819eac9cc814cd98e1d

commit 964f7ce4df1d586722cf3819eac9cc814cd98e1d
Author: ellyjones <ellyjones@chromium.org>
Date: Wed Nov 16 20:30:01 2016

views: add layout delegates

This change:
1) Adds the concept of a "layout delegate" for a View, which is responsible for
   computing padding amounts and other minutiae of control positioning;
2) Adds a HarmonyLayoutDelegate which returns the Harmony specified values for
   some of the existing padding constants;
3) Changes GridLayout and DialogClientView to honor LayoutDelegate;
4) Makes chrome's LoginView use HarmonyLayoutDelegate when --secondary-ui-md.

BUG= 602392 , 635173 

Review-Url: https://codereview.chromium.org/2485083003
Cr-Commit-Position: refs/heads/master@{#432611}

[modify] https://crrev.com/964f7ce4df1d586722cf3819eac9cc814cd98e1d/chrome/browser/ui/BUILD.gn
[add] https://crrev.com/964f7ce4df1d586722cf3819eac9cc814cd98e1d/chrome/browser/ui/views/harmony/harmony_layout_delegate.cc
[add] https://crrev.com/964f7ce4df1d586722cf3819eac9cc814cd98e1d/chrome/browser/ui/views/harmony/harmony_layout_delegate.h
[add] https://crrev.com/964f7ce4df1d586722cf3819eac9cc814cd98e1d/chrome/browser/ui/views/harmony/layout_delegate.cc
[add] https://crrev.com/964f7ce4df1d586722cf3819eac9cc814cd98e1d/chrome/browser/ui/views/harmony/layout_delegate.h
[add] https://crrev.com/964f7ce4df1d586722cf3819eac9cc814cd98e1d/chrome/browser/ui/views/layout_utils.cc
[add] https://crrev.com/964f7ce4df1d586722cf3819eac9cc814cd98e1d/chrome/browser/ui/views/layout_utils.h
[modify] https://crrev.com/964f7ce4df1d586722cf3819eac9cc814cd98e1d/chrome/browser/ui/views/login_view.cc
[modify] https://crrev.com/964f7ce4df1d586722cf3819eac9cc814cd98e1d/ui/views/views_delegate.cc
[modify] https://crrev.com/964f7ce4df1d586722cf3819eac9cc814cd98e1d/ui/views/views_delegate.h
[modify] https://crrev.com/964f7ce4df1d586722cf3819eac9cc814cd98e1d/ui/views/window/dialog_client_view.cc

@1: The 256px max height is for things like JS alerts, but not for dialogs whose content we control (e.g. collected cookies), right?  I'm assuming it will apply in very limited cases.

@7: Filed  bug 685444  for this.

The layout delegate now has a GetDialogPreferredWidth() function that returns these values.  Is there more to do on this bug, or is the remaining work basically covered under Harmonization bugs for individual dialogs?  Specifically, I'm wondering whether we should have people constructing a DialogClientView (or some other common class) have to provide a dialog size, and that view then enforces that this function's return value will be used to size the dialog.  As-is, individual dialogs have to remember to call this method and abide by it in their own GetPreferredSize() methods.

Attempting to summarize today's meeting:

Dialogs should auto-compute size as the preferred size of the contents, rounded up to the next larger Harmony dialog width; if this is larger than the largest dialog, round up to nearest kHarmonyLayoutUnit.

This may require changing the preferred size calculations for basic control classes that are used in a lot of places (e.g. ChromeOS), introducing the possibility of bugs and behavior changes.

Let's try for this anyway, as ChromeOS is wanting to fully Harmonize eventually as well, and if there are issues I can try and get some of the CrOS-specific folks to help test and fix.

Ideally, this would all be implemented in one common location.  In practice, I don't know whether that's immediately feasible for all dialogs.  I guess it depends on if they all inherit from the same dialog classes.

(Finally, regarding dialog height, Alan confirmed over email that the 256 max height should be for web developer-type cases; for other dialogs there's no clear rule at the moment.)

This should be straightforward; with https://codereview.chromium.org/2706423002/ (at least for dialogs using DialogClientView)

Simply rounding up the width reported by DialogClientView::GetPreferredSize() should cover most cases.

Where something unexpected may happen is when the DialogClientView's contents_view() doesn't properly Layout() when it is sized to something larger that its previously reported contents_view()::PreferredSize(). But I think we just need to consider this a "bug" of the individual dialog and chase them down.

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

attaching example of a views::TooltipIcon, which is a BubbleDialogDelegateView and probably shouldn't be subject to dialog snapping.

(or should it? this example does

  views::TooltipIcon* icon = new views::TooltipIcon(l10n_util::GetStringUTF16(
      IDS_AUTOFILL_CARD_UNMASK_PROMPT_STORAGE_TOOLTIP));
  const int kTooltipWidth = 233;
  icon->set_bubble_width(kTooltipWidth);
  storage_row_->AddChildView(icon);


should that hardcoded `233` be something else?)

Deleted: Screen Shot 2017-03-30 at 5.02.51 pm.png 38.4 KB

	Screen Shot 2017-03-30 at 5.02.51 pm.png 38.4 KB View Download

(to get that dialog, run

./out/gn_Debug/browser_tests --gtest_filter=BrowserDialogTest.Invoke --interactive --dialog=CardUnmaskPromptViewBrowserTest.InvokeDialog_expired

and hover the mouse over the (i) in a circle.

The hardcoded 233 is definitely wrong -- we'd want to specify the width in characters, possibly as a localizable value.  As for snapping, I dunno.  It would be easier codewise if we could say these things get snapped too.  If we can't, the best way to handle this might be to break these cases of "small thing that's not really a dialog" into their own class that doesn't fit into the whole bubble/dialog class hierarchy.  This would make sense if the only cases for this are restricted to e.g. "label and optional icon".  Then we could maybe use composition or similar to draw the "bubble border". 
 If we have more varied cases, we'd need to do something more like you suggest on the CL, where we have a virtual ShouldSnapWidth kind of function that defaults to true (I'd be more aggressive than defaulting to "has buttons").

hardcoded 233 seems not terrible to me given that it's only the width --- the height grows as needed.

Hardcoding any pixel width is problematic because it doesn't deal well with e.g. users with dramatically larger fonts (for a11y).  Hence the suggestion to hardcode a character-based width instead of a pixel-based width.

is the dialog itself not hard-coded to one of several pixel widths?

This bug is, in fact, about how we'll be doing that -- the goal is not to hardcode specific snap widths for any dialogs, but rather to snap to whatever the next larger width is above what the dialog actually needs (which will be computed based on things like actual label widths).  Of course, with a "tooltip-like" thing like in this specific screenshot, we may not do the snapping portion, but the underlying preferred width would still ideally be computed in character-widths instead of raw pixels.

Here's another weirdo dialog :/

https://cs.chromium.org/chromium/src/chrome/browser/ui/views/toolbar/home_button.cc?q=HomePageUndoBubble&l=26 "HomePageUndoBubble"

Deleted: Screen Shot 2017-03-31 at 4.51.10 pm.png 12.2 KB

	Screen Shot 2017-03-31 at 4.51.10 pm.png 12.2 KB View Download

I wonder how that fits in with the UX guidelines on closability.  Is it a third category of thing that isn't a popover or modal dialog?  Maybe this third category of thing is never width-snapped and is the type of thing I suggested pulling out to its own class?  It's beginning to sound like we have to find all the cases of this and characterize what they do and what we want them to do in order to know how to implement them.

Maybe the productive way forward is to try to snap everything for now, but file a bug about how we're going to handle these cases.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ce9d455e92cf41f171d31093cee055e41d634ea9

commit ce9d455e92cf41f171d31093cee055e41d634ea9
Author: ellyjones <ellyjones@chromium.org>
Date: Tue Apr 25 17:20:20 2017

views: support dialog width snapping once and for all

This change:
1) Adds LayoutProvider::GetSnappedDialogWidth() to implement
   the snapping;
2) Adds a unit test BubbleFrameViewTest.WidthSnaps to
   ensure snapping actually works.

BUG= 635173 

Review-Url: https://codereview.chromium.org/2821413002
Cr-Commit-Position: refs/heads/master@{#467031}

[modify] https://crrev.com/ce9d455e92cf41f171d31093cee055e41d634ea9/chrome/browser/ui/views/collected_cookies_views.cc
[modify] https://crrev.com/ce9d455e92cf41f171d31093cee055e41d634ea9/chrome/browser/ui/views/collected_cookies_views.h
[modify] https://crrev.com/ce9d455e92cf41f171d31093cee055e41d634ea9/chrome/browser/ui/views/content_setting_bubble_contents.cc
[modify] https://crrev.com/ce9d455e92cf41f171d31093cee055e41d634ea9/chrome/browser/ui/views/device_chooser_content_view.cc
[modify] https://crrev.com/ce9d455e92cf41f171d31093cee055e41d634ea9/chrome/browser/ui/views/harmony/chrome_layout_provider.cc
[modify] https://crrev.com/ce9d455e92cf41f171d31093cee055e41d634ea9/chrome/browser/ui/views/harmony/chrome_layout_provider.h
[modify] https://crrev.com/ce9d455e92cf41f171d31093cee055e41d634ea9/chrome/browser/ui/views/harmony/harmony_layout_provider.cc
[modify] https://crrev.com/ce9d455e92cf41f171d31093cee055e41d634ea9/chrome/browser/ui/views/harmony/harmony_layout_provider.h
[modify] https://crrev.com/ce9d455e92cf41f171d31093cee055e41d634ea9/ui/views/bubble/bubble_frame_view.cc
[modify] https://crrev.com/ce9d455e92cf41f171d31093cee055e41d634ea9/ui/views/bubble/bubble_frame_view_unittest.cc
[modify] https://crrev.com/ce9d455e92cf41f171d31093cee055e41d634ea9/ui/views/layout/layout_provider.cc
[modify] https://crrev.com/ce9d455e92cf41f171d31093cee055e41d634ea9/ui/views/layout/layout_provider.h

:)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8a857186d00beb74ffb1ebb6870d2ebdd351882e

commit 8a857186d00beb74ffb1ebb6870d2ebdd351882e
Author: varkha <varkha@chromium.org>
Date: Thu Jun 29 02:57:05 2017

[ui] Adds a browser_tests hook to show ZoomBubbleView

To show the dialog use:
browser_tests --gtest_filter=BrowserDialogTest.Invoke --interactive
 --dialog=ZoomBubbleDialogTest.InvokeDialog_default

BUG= 635173 

Review-Url: https://codereview.chromium.org/2963893002
Cr-Commit-Position: refs/heads/master@{#483270}

[modify] https://crrev.com/8a857186d00beb74ffb1ebb6870d2ebdd351882e/chrome/browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc