Fix layout of BubbleFrameView when there's only a close button in the

Fix layout of BubbleFrameView when there's only a close button in the
title row.

Push the client area down slightly so it doesn't overlap close. This
is a bandaid until bug 702196 is fixed.

BUG=709380, 702196

[Evan Stade, 2017-04-11 17:10:45]

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

[Evan Stade, 2017-04-11 17:10:46]

One alternative would be to allow the frame view and client view to overlap but
give event targeting precedence to the close button. BubbleFrameView would have
to override DoesIntersectRect like BrowserNonClientFrameView does. If that's the
best route, I'll let Trent or someone else take the bug.

[Peter Kasting, 2017-04-11 20:12:59]

Is it possible to test this?

https://codereview.chromium.org/2807243004/diff/1/ui/views/bubble/bubble_fram...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/2807243004/diff/1/ui/views/bubble/bubble_fram...
ui/views/bubble/bubble_frame_view.cc:262: DistanceMetric::CLOSE_BUTTON_MARGIN)
I'm wondering whether this is correct, or we only want to add 1x instead of 2x. 
The margin is technically only on the top, not the top and bottom.  If we only
add 1x, then in theory the content below can come up to the bottom of the (24 px
including padding) close button.  I think maybe that's actually correct, and we
should put padding elsewhere if we don't like it.

[Evan Stade, 2017-04-11 20:20:16]

On 2017/04/11 20:12:59, Peter Kasting wrote:
> Is it possible to test this?

Good idea. I'll look at adding tests after we settle on a fix.

https://codereview.chromium.org/2807243004/diff/1/ui/views/bubble/bubble_fram...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/2807243004/diff/1/ui/views/bubble/bubble_fram...
ui/views/bubble/bubble_frame_view.cc:262: DistanceMetric::CLOSE_BUTTON_MARGIN)
On 2017/04/11 20:12:58, Peter Kasting wrote:
> I'm wondering whether this is correct, or we only want to add 1x instead of
2x. 
> The margin is technically only on the top, not the top and bottom.  If we only
> add 1x, then in theory the content below can come up to the bottom of the (24
px
> including padding) close button.  I think maybe that's actually correct, and
we
> should put padding elsewhere if we don't like it.

I think whether it's 1x or 2x here defines whether the margin is technically on
top or both top and bottom. I don't have an opinion about which is more correct.
I actually think reserving no space in this case would be nicest (not even
close_->height()), and making the hit test update instead. But perhaps that
breaks some other random dialog.

[Peter Kasting, 2017-04-11 20:23:25]

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

[Peter Kasting, 2017-04-11 20:23:25]

+bsep -- seems like this might overlap with
https://bugs.chromium.org/p/chromium/issues/detail?id=702196 .  Bret, can you
read through our discussion above and opine what might work best with that bug? 
(Or do you want to just take this?)

Personally I'd say, for now let's add 1x margin, which should at least fix the
overlap problem, and then maybe if the above bug gets fixed this will all change
again.

[Bret, 2017-04-11 20:38:02]

On 2017/04/11 20:23:25, Peter Kasting wrote:
> +bsep -- seems like this might overlap with
> https://bugs.chromium.org/p/chromium/issues/detail?id=702196 .  Bret, can you
> read through our discussion above and opine what might work best with that
bug? 
> (Or do you want to just take this?)

This whole issue should go away once I'm done with that bug. Not only am I
moving all the controls into the client area, the dialog is also getting a total
redesign. However, all that is going to take a while, so since you're fixing a
regression then just go ahead and I'll work around it.

[Peter Kasting, 2017-04-11 20:58:09]

Cool.  Let's land the 1x change as a quick fix then, and whether there should be
a test depends on whether the test would still be valuable/have meaning if there
was no nonclient area (which will be the world Bret moves to).

[Evan Stade, 2017-04-12 15:40:19]

Description was changed from

==========
Fix layout of BubbleFrameView when there's only a close button in the
title row.

BUG=709380
==========

to

==========
Fix layout of BubbleFrameView when there's only a close button in the
title row.

Push the client area down slightly so it doesn't overlap close. This
is a bandaid until bug 702196 is fixed.

BUG=709380,702196
==========

[Evan Stade, 2017-04-12 15:40:29]

On 2017/04/11 20:58:09, Peter Kasting wrote:
> Cool.  Let's land the 1x change as a quick fix then, and whether there should
be
> a test depends on whether the test would still be valuable/have meaning if
there
> was no nonclient area (which will be the world Bret moves to).

done + updated CL desc.

[Evan Stade, 2017-04-12 15:42:13]

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

[commit-bot: I haz the power, 2017-04-12 15:42:47]

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

[Peter Kasting, 2017-04-12 15:43:44]

LGTM

[commit-bot: I haz the power, 2017-04-12 16:22:35]

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

[commit-bot: I haz the power, 2017-04-12 16:22:36]

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

[tapted, 2017-04-13 01:19:37]

lgtm

[Evan Stade, 2017-04-13 16:47:48]

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

[commit-bot: I haz the power, 2017-04-13 16:48: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-04-13 17:47:01]

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

[commit-bot: I haz the power, 2017-04-13 17:47:02]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2017-04-13 21:57:03]

On 2017/04/13 01:19:37, tapted wrote:
> lgtm

had to update tests. ptal

[Peter Kasting, 2017-04-13 22:36:29]

LGTM

https://codereview.chromium.org/2807243004/diff/40001/ui/views/window/dialog_...
File ui/views/window/dialog_delegate_unittest.cc (right):

https://codereview.chromium.org/2807243004/diff/40001/ui/views/window/dialog_...
ui/views/window/dialog_delegate_unittest.cc:47: bool ShouldShowCloseButton()
const override { return show_close_button_; }
Nit: Maybe no blank line above this

https://codereview.chromium.org/2807243004/diff/40001/ui/views/window/dialog_...
ui/views/window/dialog_delegate_unittest.cc:205: int point;
Nit: I suppose these could be const like the old ones were, but I don't care
much (2 places)

https://codereview.chromium.org/2807243004/diff/40001/ui/views/window/dialog_...
ui/views/window/dialog_delegate_unittest.cc:226:
dialog->set_show_close_button(false);
I don't suppose there's an easy way to toggle this off after showing, so we
don't need to recreate DialogTest::SetUp()/TearDown() in this test?

If not, I suggest making DialogTest have a one-arg constructor that causes it to
set_show_close_button(...) on the dialog it creates, then creating a DialogTest
subclass whose null constructor calls that constructor.  Then this test case can
use that test class instead of this one.

This is more lines of code on net, but it's easier to read the test itself, and
it's more lines of boilerplate instead of lines of actual copied code that's
sort of magic (e.g. I just spent a few minutes looking at the memory management
of TestDialog and determining that what's happening here is correct, but kinda
scary).

[Evan Stade, 2017-04-13 23:39:18]

https://codereview.chromium.org/2807243004/diff/40001/ui/views/window/dialog_...
File ui/views/window/dialog_delegate_unittest.cc (right):

https://codereview.chromium.org/2807243004/diff/40001/ui/views/window/dialog_...
ui/views/window/dialog_delegate_unittest.cc:47: bool ShouldShowCloseButton()
const override { return show_close_button_; }
On 2017/04/13 22:36:29, Peter Kasting wrote:
> Nit: Maybe no blank line above this

Done.

https://codereview.chromium.org/2807243004/diff/40001/ui/views/window/dialog_...
ui/views/window/dialog_delegate_unittest.cc:205: int point;
On 2017/04/13 22:36:29, Peter Kasting wrote:
> Nit: I suppose these could be const like the old ones were, but I don't care
> much (2 places)

Done.

https://codereview.chromium.org/2807243004/diff/40001/ui/views/window/dialog_...
ui/views/window/dialog_delegate_unittest.cc:226:
dialog->set_show_close_button(false);
On 2017/04/13 22:36:28, Peter Kasting wrote:
> I don't suppose there's an easy way to toggle this off after showing, so we
> don't need to recreate DialogTest::SetUp()/TearDown() in this test?

I tried a few ways of doing that but to no avail. I decided to leave that can of
worms alone.

> 
> If not, I suggest making DialogTest have a one-arg constructor that causes it
to
> set_show_close_button(...) on the dialog it creates, then creating a
DialogTest
> subclass whose null constructor calls that constructor.  Then this test case
can
> use that test class instead of this one.
> 
> This is more lines of code on net, but it's easier to read the test itself,
and
> it's more lines of boilerplate instead of lines of actual copied code that's
> sort of magic (e.g. I just spent a few minutes looking at the memory
management
> of TestDialog and determining that what's happening here is correct, but kinda
> scary).

we already do this (create a dialog inside a test) for BoundsAccommodateTitle. I
looked at following your suggestion and my biggest gripe is we get a different
test class name (DialogTest vs DialogTestWithNoCloseButton, which doesn't seem
attractive or scalable to more parameters). So I exposed a way to recreate the
dialog and tweak it as appropriate.

[Evan Stade, 2017-04-13 23:39:33]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-04-13 23:39:33]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/2807243004/#ps40001
(title: "fix tests")

[commit-bot: I haz the power, 2017-04-13 23:39:55]

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

[Peter Kasting, 2017-04-13 23:41:29]

https://codereview.chromium.org/2807243004/diff/40001/ui/views/window/dialog_...
File ui/views/window/dialog_delegate_unittest.cc (right):

https://codereview.chromium.org/2807243004/diff/40001/ui/views/window/dialog_...
ui/views/window/dialog_delegate_unittest.cc:226:
dialog->set_show_close_button(false);
On 2017/04/13 23:39:18, Evan Stade wrote:
> On 2017/04/13 22:36:28, Peter Kasting wrote:
> > I don't suppose there's an easy way to toggle this off after showing, so we
> > don't need to recreate DialogTest::SetUp()/TearDown() in this test?
> 
> I tried a few ways of doing that but to no avail. I decided to leave that can
of
> worms alone.
> 
> > 
> > If not, I suggest making DialogTest have a one-arg constructor that causes
it
> to
> > set_show_close_button(...) on the dialog it creates, then creating a
> DialogTest
> > subclass whose null constructor calls that constructor.  Then this test case
> can
> > use that test class instead of this one.
> > 
> > This is more lines of code on net, but it's easier to read the test itself,
> and
> > it's more lines of boilerplate instead of lines of actual copied code that's
> > sort of magic (e.g. I just spent a few minutes looking at the memory
> management
> > of TestDialog and determining that what's happening here is correct, but
kinda
> > scary).
> 
> we already do this (create a dialog inside a test) for BoundsAccommodateTitle.
I
> looked at following your suggestion and my biggest gripe is we get a different
> test class name (DialogTest vs DialogTestWithNoCloseButton, which doesn't seem
> attractive or scalable to more parameters). So I exposed a way to recreate the
> dialog and tweak it as appropriate.

I went to look, but I don't see the patch doing this uploaded?

The idea you describe sounds reasonable.

[Evan Stade, 2017-04-13 23:42:43]

On 2017/04/13 23:41:29, Peter Kasting wrote:
>
https://codereview.chromium.org/2807243004/diff/40001/ui/views/window/dialog_...
> File ui/views/window/dialog_delegate_unittest.cc (right):
> 
>
https://codereview.chromium.org/2807243004/diff/40001/ui/views/window/dialog_...
> ui/views/window/dialog_delegate_unittest.cc:226:
> dialog->set_show_close_button(false);
> On 2017/04/13 23:39:18, Evan Stade wrote:
> > On 2017/04/13 22:36:28, Peter Kasting wrote:
> > > I don't suppose there's an easy way to toggle this off after showing, so
we
> > > don't need to recreate DialogTest::SetUp()/TearDown() in this test?
> > 
> > I tried a few ways of doing that but to no avail. I decided to leave that
can
> of
> > worms alone.
> > 
> > > 
> > > If not, I suggest making DialogTest have a one-arg constructor that causes
> it
> > to
> > > set_show_close_button(...) on the dialog it creates, then creating a
> > DialogTest
> > > subclass whose null constructor calls that constructor.  Then this test
case
> > can
> > > use that test class instead of this one.
> > > 
> > > This is more lines of code on net, but it's easier to read the test
itself,
> > and
> > > it's more lines of boilerplate instead of lines of actual copied code
that's
> > > sort of magic (e.g. I just spent a few minutes looking at the memory
> > management
> > > of TestDialog and determining that what's happening here is correct, but
> kinda
> > > scary).
> > 
> > we already do this (create a dialog inside a test) for
BoundsAccommodateTitle.
> I
> > looked at following your suggestion and my biggest gripe is we get a
different
> > test class name (DialogTest vs DialogTestWithNoCloseButton, which doesn't
seem
> > attractive or scalable to more parameters). So I exposed a way to recreate
the
> > dialog and tweak it as appropriate.
> 
> I went to look, but I don't see the patch doing this uploaded?
> 
> The idea you describe sounds reasonable.

oops, thanks. Uploaded for real now.

[Evan Stade, 2017-04-13 23:43:31]

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

[commit-bot: I haz the power, 2017-04-13 23:44:03]

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

[Peter Kasting, 2017-04-13 23:47:35]

https://codereview.chromium.org/2807243004/diff/40001/ui/views/window/dialog_...
File ui/views/window/dialog_delegate_unittest.cc (right):

https://codereview.chromium.org/2807243004/diff/40001/ui/views/window/dialog_...
ui/views/window/dialog_delegate_unittest.cc:47: bool ShouldShowCloseButton()
const override { return show_close_button_; }
On 2017/04/13 23:39:17, Evan Stade wrote:
> On 2017/04/13 22:36:29, Peter Kasting wrote:
> > Nit: Maybe no blank line above this
> 
> Done.

You didn't, but it's not important :)

Same for the other nit below.

https://codereview.chromium.org/2807243004/diff/60001/ui/views/window/dialog_...
File ui/views/window/dialog_delegate_unittest.cc (right):

https://codereview.chromium.org/2807243004/diff/60001/ui/views/window/dialog_...
ui/views/window/dialog_delegate_unittest.cc:238: ShowDialog();
Cool, this route is more readable than the old code.  Can we rewrite
BoundsAccommodateTitle to make use of this too?  I'm thinking:

* Capture the relevant metrics (with and without title) from the dialog, check
that without title is shorter than with
* Then use init/show to make a new dialog and set its title before showing, and
check that it's the same as the previous with-title dialog

[Evan Stade, 2017-04-13 23:51:24]

https://codereview.chromium.org/2807243004/diff/40001/ui/views/window/dialog_...
File ui/views/window/dialog_delegate_unittest.cc (right):

https://codereview.chromium.org/2807243004/diff/40001/ui/views/window/dialog_...
ui/views/window/dialog_delegate_unittest.cc:47: bool ShouldShowCloseButton()
const override { return show_close_button_; }
On 2017/04/13 23:47:34, Peter Kasting wrote:
> On 2017/04/13 23:39:17, Evan Stade wrote:
> > On 2017/04/13 22:36:29, Peter Kasting wrote:
> > > Nit: Maybe no blank line above this
> > 
> > Done.
> 
> You didn't, but it's not important :)
> 
> Same for the other nit below.

oi, I seem to be in too much of a hurry to do anything right today. Uploaded
correct patch.

https://codereview.chromium.org/2807243004/diff/60001/ui/views/window/dialog_...
File ui/views/window/dialog_delegate_unittest.cc (right):

https://codereview.chromium.org/2807243004/diff/60001/ui/views/window/dialog_...
ui/views/window/dialog_delegate_unittest.cc:238: ShowDialog();
On 2017/04/13 23:47:35, Peter Kasting wrote:
> Cool, this route is more readable than the old code.  Can we rewrite
> BoundsAccommodateTitle to make use of this too?  I'm thinking:
> 
> * Capture the relevant metrics (with and without title) from the dialog, check
> that without title is shorter than with
> * Then use init/show to make a new dialog and set its title before showing,
and
> check that it's the same as the previous with-title dialog

honestly, would rather not let this change snowball any further.

[Evan Stade, 2017-04-13 23:51:29]

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

[commit-bot: I haz the power, 2017-04-13 23:51:57]

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

[Peter Kasting, 2017-04-13 23:53:35]

https://codereview.chromium.org/2807243004/diff/60001/ui/views/window/dialog_...
File ui/views/window/dialog_delegate_unittest.cc (right):

https://codereview.chromium.org/2807243004/diff/60001/ui/views/window/dialog_...
ui/views/window/dialog_delegate_unittest.cc:238: ShowDialog();
On 2017/04/13 23:51:24, Evan Stade wrote:
> On 2017/04/13 23:47:35, Peter Kasting wrote:
> > Cool, this route is more readable than the old code.  Can we rewrite
> > BoundsAccommodateTitle to make use of this too?  I'm thinking:
> > 
> > * Capture the relevant metrics (with and without title) from the dialog,
check
> > that without title is shorter than with
> > * Then use init/show to make a new dialog and set its title before showing,
> and
> > check that it's the same as the previous with-title dialog
> 
> honestly, would rather not let this change snowball any further.

OK.  I just figured you introduced this in part because of the needs of that
function, so it made sense to make use of it there.  If it would add too much
hassle, that's cool too.

[commit-bot: I haz the power, 2017-04-14 00:48:22]

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

[commit-bot: I haz the power, 2017-04-14 00:48:23]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2017-04-14 15:55:05]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-04-14 15:55:05]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2807243004/#ps40002
(title: "actually done")

[commit-bot: I haz the power, 2017-04-14 15:55:17]

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-04-14 16:01:33]

CQ is committing da patch.

Bot data: {"patchset_id": 40002, "attempt_start_ts": 1492185305259410,
"parent_rev": "9d47da2334d77bd084aef8e0d5765a47967d449a", "commit_rev":
"4ced9f3c7e9534b6b730d3b265e39a0fe5b17dae"}

[commit-bot: I haz the power, 2017-04-14 16:02:28]

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

[commit-bot: I haz the power, 2017-04-14 16:02:30]

Prior attempt to commit was detected, but we were not able to check whether the
issue was successfully committed. Please check Git history manually and re-check
CQ or close this issue as needed.

[findit-for-me, 2017-04-14 17:10:34]

Findit(https://goo.gl/kROfz5) identified this CL at revision 464731 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[alexmos, 2017-04-14 18:21:40]

A revert of this CL (patchset #5 id:40002) has been created in
https://codereview.chromium.org/2821463004/ by alexmos@chromium.org.

The reason for reverting is: Breaks DialogTest.AcceptAndCancel on a couple of
bots
(https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2...
and
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%....