Fix Harmony popover buttons being flush with their content.

Fix Harmony popover buttons being flush with their content.

When I wrote crrev.com/2888563004 I didn't realize that Harmony popover
bubbles were relying on the special case in DialogClientView to space
out their buttons and their content. This patch completely removes that
special case and makes the spacing explicit. It also includes some minor
opportunistic cleanup.

BUG=727520
Review-Url: https://codereview.chromium.org/2932523003
Cr-Commit-Position: refs/heads/master@{#479251}
Committed: https://chromium.googlesource.com/chromium/src/+/02f8ec0767ac92101a2135f0f681528a16bda1ce

[Bret, 2017-06-07 22:52:17]

bsep@chromium.org changed reviewers:
+ pkasting@chromium.org, sky@chromium.org

[Bret, 2017-06-07 22:52:18]

pkasting@: Harmony considerations
sky@: OWNERS
PTAL!

https://codereview.chromium.org/2932523003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/2932523003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:340: if
(provider->UseExtraDialogPadding()) {
This is redundant post-Harmony with the new padding in BubbleDialogDelegateView.

https://codereview.chromium.org/2932523003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/chooser_dialog_view.h (left):

https://codereview.chromium.org/2932523003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view.h:35: views::ClientView*
CreateClientView(views::Widget* widget) override;
Removing these changes the layout very slightly pre-Harmony, but it's hard to
see.

https://codereview.chromium.org/2932523003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/harmony/chrome_layout_provider.cc (left):

https://codereview.chromium.org/2932523003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/chrome_layout_provider.cc:33: case
DISTANCE_DIALOG_BUTTON_TOP:
This was unused.

https://codereview.chromium.org/2932523003/diff/40001/ui/views/window/dialog_...
File ui/views/window/dialog_client_view.cc (left):

https://codereview.chromium.org/2932523003/diff/40001/ui/views/window/dialog_...
ui/views/window/dialog_client_view.cc:354: if (insets.top() == 0 &&
Removing this block reduces the pre-Harmony content/button spacing from 21 to 13
pixels on all dialogs.

If we really want to keep it I can add DISTANCE_BUBBLE_BUTTON_TOP_MARGIN to
INSETS_DIALOG_CONTENTS bottom in LayoutProvider, and move the existing
INSETS_DIALOG_CONTENTS definition to HarmonyLayoutProvider. But doing that
refactor was the point at which I questioned whether we actually want that
behavior.

[sky, 2017-06-07 23:20:05]

LGTM

[Peter Kasting, 2017-06-10 01:51:11]

https://codereview.chromium.org/2932523003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/chooser_dialog_view.h (left):

https://codereview.chromium.org/2932523003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view.h:35: views::ClientView*
CreateClientView(views::Widget* widget) override;
On 2017/06/07 22:52:18, Bret wrote:
> Removing these changes the layout very slightly pre-Harmony, but it's hard to
> see.

Mostly because the net button spacing went from "unrelated" (in the .cc file
here) to "related" (in your new layout provider constant), right?

https://codereview.chromium.org/2932523003/diff/40001/ui/views/layout/layout_...
File ui/views/layout/layout_provider.h (right):

https://codereview.chromium.org/2932523003/diff/40001/ui/views/layout/layout_...
ui/views/layout/layout_provider.h:54: DISTANCE_BUBBLE_BUTTON_TOP_MARGIN =
VIEWS_DISTANCE_START,
I'm not sure whether adding this is right.

We seem to have two sets of constants today: one for "dialogs" and one for
"bubbles".  AFAICT the distinction between those two is that dialogs have a
button row and bubbles do not.  That's certainly how I've been operating.

But the premise of this patch is that we have "bubbles" with buttons.  In which
case, my question is, why aren't these just dialogs, made to use the
DIALOG_BUTTON_ROW insets (and dialog content insets, etc.)?

[Bret, 2017-06-10 23:42:23]

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

[commit-bot: I haz the power, 2017-06-10 23:42:28]

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-06-10 23:44:20]

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

[commit-bot: I haz the power, 2017-06-10 23:44:21]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Bret, 2017-06-11 00:14:10]

https://codereview.chromium.org/2932523003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/chooser_dialog_view.h (left):

https://codereview.chromium.org/2932523003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view.h:35: views::ClientView*
CreateClientView(views::Widget* widget) override;
On 2017/06/10 01:51:11, Peter Kasting wrote:
> On 2017/06/07 22:52:18, Bret wrote:
> > Removing these changes the layout very slightly pre-Harmony, but it's hard
to
> > see.
> 
> Mostly because the net button spacing went from "unrelated" (in the .cc file
> here) to "related" (in your new layout provider constant), right?

Oops, I went to double-check this and it turns out I was invoking the wrong
dialog (ChooserBubbleUiViewDelegate 😬). I split this off into
crrev.com/2932243002 since it's a bit more involved.

https://codereview.chromium.org/2932523003/diff/40001/ui/views/layout/layout_...
File ui/views/layout/layout_provider.h (right):

https://codereview.chromium.org/2932523003/diff/40001/ui/views/layout/layout_...
ui/views/layout/layout_provider.h:54: DISTANCE_BUBBLE_BUTTON_TOP_MARGIN =
VIEWS_DISTANCE_START,
On 2017/06/10 01:51:11, Peter Kasting wrote:
> I'm not sure whether adding this is right.
> 
> We seem to have two sets of constants today: one for "dialogs" and one for
> "bubbles".  AFAICT the distinction between those two is that dialogs have a
> button row and bubbles do not.  That's certainly how I've been operating.
> 
> But the premise of this patch is that we have "bubbles" with buttons.  In
which
> case, my question is, why aren't these just dialogs, made to use the
> DIALOG_BUTTON_ROW insets (and dialog content insets, etc.)?

No, lots of bubbles have buttons (e.g. the bookmark bubble, crashed session
bubble, translate bubble, and others). You might be right that they SHOULD be
dialogs, but that's not really how the code works.

Post-Harmony the distinction I've been using is that dialogs are modal and
bubbles aren't, with the attendant consequences: bubbles have a close-X and
close when you click away. Pre-Harmony there are a few extra differences:
* Bubbles have arrows and dialogs don't.
* Bubbles are anchored and not draggable, and dialogs are floating and
draggable.
* Dialogs have slightly larger margins.

All that is baked into DialogDelegateView vs BubbleDialogDelegateView. This is
why I mentioned in an earlier CL I thought we'd eventually merge the
distinction, because conceptually these ought to be dictated by the same
delegate class (with a switch for modality). But I think we should hold off on
doing that refactor till Harmony is done.

[Bret, 2017-06-11 00:31:09]

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

[commit-bot: I haz the power, 2017-06-11 00:31:14]

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-06-11 02:05:25]

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

[commit-bot: I haz the power, 2017-06-11 02:05:26]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2017-06-12 22:47:15]

https://codereview.chromium.org/2932523003/diff/40001/ui/views/layout/layout_...
File ui/views/layout/layout_provider.h (right):

https://codereview.chromium.org/2932523003/diff/40001/ui/views/layout/layout_...
ui/views/layout/layout_provider.h:54: DISTANCE_BUBBLE_BUTTON_TOP_MARGIN =
VIEWS_DISTANCE_START,
On 2017/06/11 00:14:09, Bret wrote:
> On 2017/06/10 01:51:11, Peter Kasting wrote:
> > I'm not sure whether adding this is right.
> > 
> > We seem to have two sets of constants today: one for "dialogs" and one for
> > "bubbles".  AFAICT the distinction between those two is that dialogs have a
> > button row and bubbles do not.  That's certainly how I've been operating.
> > 
> > But the premise of this patch is that we have "bubbles" with buttons.  In
> which
> > case, my question is, why aren't these just dialogs, made to use the
> > DIALOG_BUTTON_ROW insets (and dialog content insets, etc.)?
> 
> No, lots of bubbles have buttons (e.g. the bookmark bubble, crashed session
> bubble, translate bubble, and others). You might be right that they SHOULD be
> dialogs, but that's not really how the code works.
> 
> Post-Harmony the distinction I've been using is that dialogs are modal and
> bubbles aren't, with the attendant consequences: bubbles have a close-X and
> close when you click away. Pre-Harmony there are a few extra differences:
> * Bubbles have arrows and dialogs don't.
> * Bubbles are anchored and not draggable, and dialogs are floating and
> draggable.
> * Dialogs have slightly larger margins.
> 
> All that is baked into DialogDelegateView vs BubbleDialogDelegateView. This is
> why I mentioned in an earlier CL I thought we'd eventually merge the
> distinction, because conceptually these ought to be dictated by the same
> delegate class (with a switch for modality). But I think we should hold off on
> doing that refactor till Harmony is done.

OK, that all makes sense.  But in that case it seems like we should make the
bubble and dialog insets all just match (as the dialog insets) for now.  The
dialog inset pre-Harmony is kButtonHorizontalMargin; if some bubbles can have
buttons too, then they should also be using those margins.  The only exception
would be bubbles that lack any buttons, which I suppose could stay the old way,
but it doesn't seem like a win to make them do so.

If we were going to do this, then basically all the BUBBLE_ insets/distances
would disappear, and we'd use the dialog ones everywhere.  And that, I think,
would still mean we could avoid adding this distance metric, and you could just
use the .top() value of INSETS_DIALOG_BUTTON_ROW (or something like that) in the
callers instead of this.

The actual change to nuke the BUBBLE_ constants would probably happen
separately, maybe as a precursor to this change.

WDYT?

[Bret, 2017-06-13 21:00:42]

https://codereview.chromium.org/2932523003/diff/40001/ui/views/layout/layout_...
File ui/views/layout/layout_provider.h (right):

https://codereview.chromium.org/2932523003/diff/40001/ui/views/layout/layout_...
ui/views/layout/layout_provider.h:54: DISTANCE_BUBBLE_BUTTON_TOP_MARGIN =
VIEWS_DISTANCE_START,
On 2017/06/12 22:47:14, Peter Kasting wrote:
> On 2017/06/11 00:14:09, Bret wrote:
> > On 2017/06/10 01:51:11, Peter Kasting wrote:
> > > I'm not sure whether adding this is right.
> > > 
> > > We seem to have two sets of constants today: one for "dialogs" and one for
> > > "bubbles".  AFAICT the distinction between those two is that dialogs have
a
> > > button row and bubbles do not.  That's certainly how I've been operating.
> > > 
> > > But the premise of this patch is that we have "bubbles" with buttons.  In
> > which
> > > case, my question is, why aren't these just dialogs, made to use the
> > > DIALOG_BUTTON_ROW insets (and dialog content insets, etc.)?
> > 
> > No, lots of bubbles have buttons (e.g. the bookmark bubble, crashed session
> > bubble, translate bubble, and others). You might be right that they SHOULD
be
> > dialogs, but that's not really how the code works.
> > 
> > Post-Harmony the distinction I've been using is that dialogs are modal and
> > bubbles aren't, with the attendant consequences: bubbles have a close-X and
> > close when you click away. Pre-Harmony there are a few extra differences:
> > * Bubbles have arrows and dialogs don't.
> > * Bubbles are anchored and not draggable, and dialogs are floating and
> > draggable.
> > * Dialogs have slightly larger margins.
> > 
> > All that is baked into DialogDelegateView vs BubbleDialogDelegateView. This
is
> > why I mentioned in an earlier CL I thought we'd eventually merge the
> > distinction, because conceptually these ought to be dictated by the same
> > delegate class (with a switch for modality). But I think we should hold off
on
> > doing that refactor till Harmony is done.
> 
> OK, that all makes sense.  But in that case it seems like we should make the
> bubble and dialog insets all just match (as the dialog insets) for now.  The
> dialog inset pre-Harmony is kButtonHorizontalMargin; if some bubbles can have
> buttons too, then they should also be using those margins.  The only exception
> would be bubbles that lack any buttons, which I suppose could stay the old
way,
> but it doesn't seem like a win to make them do so.
> 
> If we were going to do this, then basically all the BUBBLE_ insets/distances
> would disappear, and we'd use the dialog ones everywhere.  And that, I think,
> would still mean we could avoid adding this distance metric, and you could
just
> use the .top() value of INSETS_DIALOG_BUTTON_ROW (or something like that) in
the
> callers instead of this.
> 
> The actual change to nuke the BUBBLE_ constants would probably happen
> separately, maybe as a precursor to this change.
> 
> WDYT?

I'd really like to get everything into a working state before I do
refactor-style changes. Plus removing the special case in
DialogClientView::SetupLayout will make refactors easier. I think getting those
two things is worth adding this constant, even if I have to remove it later.

I think it will stick around, though. My goal is to collapse all the constants
into a metric for the edges of the dialog, and then have this and a "between
title and contents" metric.

I do like having a mandate to make the bubble and dialog margins the same
though, it will make my life a lot easier.

[Peter Kasting, 2017-06-14 00:45:16]

LGTM

Feel free to file bugs on collapsing bubble and dialog constants into just
dialog ones.

I think your idea of "insets for the whole dialog, separate constants for the
spacing above and below content" makes sense.  Although another option might be
to do those as "additional content insets" (which would have top and bottom
values, and zero for the sides).  I don't have much instinct for what would work
better.

[Bret, 2017-06-14 00:50:59]

The CQ bit was checked by bsep@chromium.org

[Bret, 2017-06-14 00:50:59]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2932523003/#ps80001
(title: "fix merge")

[commit-bot: I haz the power, 2017-06-14 00:51:29]

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-06-14 02:09:33]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497401459022750,
"parent_rev": "6bf9424938dd25151dd5d9e828594a5bf1d2cc2b", "commit_rev":
"02f8ec0767ac92101a2135f0f681528a16bda1ce"}

[commit-bot: I haz the power, 2017-06-14 02:09:45]

Description was changed from

==========
Fix Harmony popover buttons being flush with their content.

When I wrote crrev.com/2888563004 I didn't realize that Harmony popover
bubbles were relying on the special case in DialogClientView to space
out their buttons and their content. This patch completely removes that
special case and makes the spacing explicit. It also includes some minor
opportunistic cleanup.

BUG=727520
==========

to

==========
Fix Harmony popover buttons being flush with their content.

When I wrote crrev.com/2888563004 I didn't realize that Harmony popover
bubbles were relying on the special case in DialogClientView to space
out their buttons and their content. This patch completely removes that
special case and makes the spacing explicit. It also includes some minor
opportunistic cleanup.

BUG=727520

Review-Url: https://codereview.chromium.org/2932523003
Cr-Commit-Position: refs/heads/master@{#479251}
Committed:
https://chromium.googlesource.com/chromium/src/+/02f8ec0767ac92101a2135f0f681...
==========

[commit-bot: I haz the power, 2017-06-14 02:09:46]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/02f8ec0767ac92101a2135f0f681...