Use buttons from DialogClientView in BookmakBubbleView.

Use buttons from DialogClientView in BookmakBubbleView.

Currently the bookmark bubble builds its own button row, but that's
unnecessary and causes it to miss out on shared framework code for
layout and button style.

BUG=726187
Review-Url: https://codereview.chromium.org/2908963002
Cr-Commit-Position: refs/heads/master@{#475833}
Committed: https://chromium.googlesource.com/chromium/src/+/f9f292034311121b7777645eb48522ddaa6a5272

[tapted, 2017-05-29 01:54:52]

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

[commit-bot: I haz the power, 2017-05-29 01:55:08]

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-05-29 02:07:08]

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

[commit-bot: I haz the power, 2017-05-29 02:07:09]

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

[tapted, 2017-05-29 02:57:59]

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

[commit-bot: I haz the power, 2017-05-29 02:58:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-05-29 02:59:56]

Description was changed from

==========
cl format


fix brtest


cl format


Use DCV in BBV

BUG=
==========

to

==========
Use buttons from DialogClientView in BookmakBubbleView.

Currently the bookmark bubble builds its own button row, but that's
unnecessary and causes it to miss out on shared framework code for
layout and button style.

BUG=726187
==========

[tapted, 2017-05-29 03:10:00]

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

[commit-bot: I haz the power, 2017-05-29 03:10:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-05-29 03:38:00]

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

[commit-bot: I haz the power, 2017-05-29 03:38: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-05-29 04:41:20]

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

[commit-bot: I haz the power, 2017-05-29 04:41:21]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-05-29 06:40:29]

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

[commit-bot: I haz the power, 2017-05-29 06:40:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-05-29 06:49:24]

Patchset #2 (id:20001) has been deleted

[tapted, 2017-05-29 06:49:33]

Patchset #2 (id:40001) has been deleted

[tapted, 2017-05-29 06:57:12]

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

[commit-bot: I haz the power, 2017-05-29 06:57:26]

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-05-29 07:38:44]

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

[commit-bot: I haz the power, 2017-05-29 07:38:45]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-05-29 07:55:44]

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

[tapted, 2017-05-29 07:55:45]

Hi Peter, please take a look

https://codereview.chromium.org/2908963002/diff/120001/chrome/browser/ui/view...
File
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc
(right):

https://codereview.chromium.org/2908963002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc:27:
IDS_DESKTOP_IOS_PROMOTION_SAVE_PASSWORDS_BUBBLE_TEXT_WIDTH_CHARS);
Just updating the height was never sufficient for adapting to the bookmark
bubble... the label is given a fixed width here so if the bookmark bubble wasn't
wide enough to begin with, the label gets clipped.

[Peter Kasting, 2017-05-31 02:43:31]

https://codereview.chromium.org/2908963002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/2908963002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:147: return
base::string16();
Nit: Simpler:

  return l10n_util::GetStringUTF16((button == ui::DIALOG_BUTTON_OK)
                                       ? IDS_DONE
                                       : IDS_BOOKMARK_BUBBLE_REMOVE_BOOKMARK);

Since DIALOG_BUTTON_NONE never reaches here anyway (and callers would not expect
it to), I think it's fine not to have extra code just to (effectively) verify
that and return string16().

https://codereview.chromium.org/2908963002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:240:
desktop_ios_promotion::PromotionEntryPoint::BOOKMARKS_BUBBLE)) {
Nit: Maybe "using desktop_ios_promotion::PromotionEntryPoint;" above this block
to reduce the qualifiers below, allowing you to avoid so much line-wrapping?

https://codereview.chromium.org/2908963002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:324:
layout->StartRow(kFixed, kColumnId);
This block is basically a placeholder until we switch to using the correct title
machinery, right?

https://codereview.chromium.org/2908963002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:328: 
Don't we need to start a new row here or something?  And shouldn't there be a
padding row for the dialog content top inset?

https://codereview.chromium.org/2908963002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:350:
provider->GetInsetsMetric(views::INSETS_DIALOG_CONTENTS).bottom());
Can we just set the dialog contents insets as insets for the whole gridlayout
when we create it?  Seems like this would eliminate the need for this, address
my question above about whether we need a top inset padding row, and answer the
question I didn't ask about the dialog side insets I don't see anywhere.

https://codereview.chromium.org/2908963002/diff/120001/chrome/browser/ui/view...
File
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc
(right):

https://codereview.chromium.org/2908963002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc:105:
gfx::Rect old_bounds = GetWidget()->GetWindowBoundsInScreen();
Nit: Maybe pull GetWidget() out into a temp

[tapted, 2017-05-31 04:18:31]

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

[commit-bot: I haz the power, 2017-05-31 04:18:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-05-31 04:27:49]

PTAL. I decided to bite the bullet. Plan would be to sweep through a bunch of
dialogs that have an unnecessary padding row in follow-ups.

https://codereview.chromium.org/2908963002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/2908963002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:147: return
base::string16();
On 2017/05/31 02:43:31, Peter Kasting wrote:
> Nit: Simpler:
> 
>   return l10n_util::GetStringUTF16((button == ui::DIALOG_BUTTON_OK)
>                                        ? IDS_DONE
>                                        : IDS_BOOKMARK_BUBBLE_REMOVE_BOOKMARK);
> 
> Since DIALOG_BUTTON_NONE never reaches here anyway (and callers would not
expect
> it to), I think it's fine not to have extra code just to (effectively) verify
> that and return string16().

Done.

https://codereview.chromium.org/2908963002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:240:
desktop_ios_promotion::PromotionEntryPoint::BOOKMARKS_BUBBLE)) {
On 2017/05/31 02:43:31, Peter Kasting wrote:
> Nit: Maybe "using desktop_ios_promotion::PromotionEntryPoint;" above this
block
> to reduce the qualifiers below, allowing you to avoid so much line-wrapping?

Done.

https://codereview.chromium.org/2908963002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:324:
layout->StartRow(kFixed, kColumnId);
On 2017/05/31 02:43:31, Peter Kasting wrote:
> This block is basically a placeholder until we switch to using the correct
title
> machinery, right?

Nah - the title machinery is being used correctly already. This layout block now
holds the textfield and combobox only (with their corresponding labels).

https://codereview.chromium.org/2908963002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:326:
l10n_util::GetStringUTF16(IDS_BOOKMARK_BUBBLE_TITLE_TEXT));
I think this is the bit that makes it confusing IDS_BOOKMARK_BUBBLE_TITLE_TEXT
is actually "Name:". I'll fix it. (I renamed the textfield data member from
title -> name in the precursor since it tripped me up too.., this should be
consistent).

https://codereview.chromium.org/2908963002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:328: 
On 2017/05/31 02:43:31, Peter Kasting wrote:
> Don't we need to start a new row here or something?  And shouldn't there be a
> padding row for the dialog content top inset?

The top and side padding is handled by |margins_|, at:

NonClientFrameView* BubbleDialogDelegateView::CreateNonClientFrameView(
    Widget* widget) {
  BubbleFrameView* frame = new BubbleFrameView(title_margins_, margins_);
...

  margins_ = provider->GetInsetsMetric(INSETS_BUBBLE_CONTENTS);

(which for harmony is kHarmonyLayoutUnit on all sides and
kPanel{Horiz,Vert}Margin otherwise)


those margins encompass contents AND buttons.  So, the annoying bit is the
padding between the contents and the button row. BubbleDialogDelegateView sets
it to zero with a hardcoded constant here:
ClientView* BubbleDialogDelegateView::CreateClientView(Widget* widget) {
  DialogClientView* client = new DialogClientView(widget, GetContentsView());
  client->SetButtonRowInsets(gfx::Insets());
..



... see later comments.

https://codereview.chromium.org/2908963002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:350:
provider->GetInsetsMetric(views::INSETS_DIALOG_CONTENTS).bottom());
On 2017/05/31 02:43:31, Peter Kasting wrote:
> Can we just set the dialog contents insets as insets for the whole gridlayout
> when we create it?  Seems like this would eliminate the need for this, address
> my question above about whether we need a top inset padding row, and answer
the
> question I didn't ask about the dialog side insets I don't see anywhere.

Yeah - this still confounds me :/

Here, we only need the bottom margin; to set the distance between the bottom of
the combobox and the top of the button row. The old code used
views::DISTANCE_RELATED_CONTROL_VERTICAL for this. I don't think that's correct.
I think DISTANCE_UNRELATED_CONTROL_VERTICAL makes the most sense for now.

BubbleDialogDelegateView::margins_ handles the rest, but that only provides the
padding below the buttons, not above.

I think what we really want is for BubbleDialogDelegateView to still provide a
top() inset (only) for DialogClientView::button_row_insets_, and all the dialogs
(like this one) that put in their own padding row to remove the handcrafted one.
DialogClientView already has the logic to remove all button_row_insets_ when
there are no buttons, but it only works for webmodal dialogs since, for bubbles,
those insets are always zero anyway.

So, I've started the path towards this. It's awkward since the DialogClientView
is created after Init(), and we don't want the dialog to relayout after
Widget::Init determines the initial size.


(There was a views::INSETS_DIALOG_CONTENTS.top() previously, but that was being
used for the padding *between* the textfield and combo controls, so definitely
not an inset at all).

https://codereview.chromium.org/2908963002/diff/120001/chrome/browser/ui/view...
File
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc
(right):

https://codereview.chromium.org/2908963002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc:105:
gfx::Rect old_bounds = GetWidget()->GetWindowBoundsInScreen();
On 2017/05/31 02:43:31, Peter Kasting wrote:
> Nit: Maybe pull GetWidget() out into a temp

Done.

[commit-bot: I haz the power, 2017-05-31 05:21:47]

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

[commit-bot: I haz the power, 2017-05-31 05:21:48]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2017-05-31 05:45:46]

https://codereview.chromium.org/2908963002/diff/150001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/2908963002/diff/150001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:342:
provider->GetDistanceMetric(DISTANCE_UNRELATED_CONTROL_VERTICAL), 0, 0,
I don't think we want to do this, as it makes bubbles and dialogs do opposite
things with their padding.  Bret is making dialogs use INSETS_DIALOG_BUTON_ROW,
which has a 0 top inset, with the expectation that dialog content area (above
the button row) uses INSETS_DIALOG_CONTENTS, and the nonzero bottom inset there
spaces things out correctly in total.  This would basically put the 0 on the
other side of the dividing line between contents and button row (which is more
how I wanted to do it, but whatever).  I think we should be consistent.

If BubbleDialogDelegateView::margins_ is being drawn around the bubble content
and buttons (but not title), then it seems like what we really want is for it to
be around content (but not buttons or title), and then we can set the title,
content, and button row insets with the three INSETS_DIALOG_XXX values like we
already do for other dialogs.  In an ideal world this would mean all the
BubbleDialogDelegateView stuff would just go away and parent class
implementations would do all this.

If you not up to speed on what Bret is doing, you might want to coordinate with
him here.  Otherwise, if you want to get something landed for now so larger
changes can be done separately, I'd basically go back to what you were doing in
your previous patch set.

[tapted, 2017-05-31 05:52:50]

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

[commit-bot: I haz the power, 2017-05-31 05:53:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-05-31 05:54:27]

https://codereview.chromium.org/2908963002/diff/150001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/2908963002/diff/150001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:342:
provider->GetDistanceMetric(DISTANCE_UNRELATED_CONTROL_VERTICAL), 0, 0,
On 2017/05/31 05:45:46, Peter Kasting wrote:
> I don't think we want to do this, as it makes bubbles and dialogs do opposite
> things with their padding.  Bret is making dialogs use
INSETS_DIALOG_BUTON_ROW,
> which has a 0 top inset, with the expectation that dialog content area (above
> the button row) uses INSETS_DIALOG_CONTENTS, and the nonzero bottom inset
there
> spaces things out correctly in total.  This would basically put the 0 on the
> other side of the dividing line between contents and button row (which is more
> how I wanted to do it, but whatever).  I think we should be consistent.
> 
> If BubbleDialogDelegateView::margins_ is being drawn around the bubble content
> and buttons (but not title), then it seems like what we really want is for it
to
> be around content (but not buttons or title), and then we can set the title,
> content, and button row insets with the three INSETS_DIALOG_XXX values like we
> already do for other dialogs.  In an ideal world this would mean all the
> BubbleDialogDelegateView stuff would just go away and parent class
> implementations would do all this.
> 
> If you not up to speed on what Bret is doing, you might want to coordinate
with
> him here.  Otherwise, if you want to get something landed for now so larger
> changes can be done separately, I'd basically go back to what you were doing
in
> your previous patch set.

Done.

[Peter Kasting, 2017-05-31 06:16:53]

LGTM

[commit-bot: I haz the power, 2017-05-31 06:51:53]

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

[commit-bot: I haz the power, 2017-05-31 06:51:53]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-05-31 06:55:18]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2017-05-31 06:55:31]

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-05-31 06:59:32]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1496213718925750,
"parent_rev": "09bcca49766516c2b8a685420b7b075f39350500", "commit_rev":
"f9f292034311121b7777645eb48522ddaa6a5272"}

[commit-bot: I haz the power, 2017-05-31 06:59:42]

Description was changed from

==========
Use buttons from DialogClientView in BookmakBubbleView.

Currently the bookmark bubble builds its own button row, but that's
unnecessary and causes it to miss out on shared framework code for
layout and button style.

BUG=726187
==========

to

==========
Use buttons from DialogClientView in BookmakBubbleView.

Currently the bookmark bubble builds its own button row, but that's
unnecessary and causes it to miss out on shared framework code for
layout and button style.

BUG=726187

Review-Url: https://codereview.chromium.org/2908963002
Cr-Commit-Position: refs/heads/master@{#475833}
Committed:
https://chromium.googlesource.com/chromium/src/+/f9f292034311121b7777645eb485...
==========

[commit-bot: I haz the power, 2017-05-31 06:59:43]

Committed patchset #8 (id:170001) as
https://chromium.googlesource.com/chromium/src/+/f9f292034311121b7777645eb485...