Harmony: Don't snap the width of the chrome:// page info bubbles.

Harmony: Don't snap the width of the chrome:// page info bubbles.

Adds views::DialogDelegate::ShouldSnapFrameWidth() -> bool to do this.

BUG=718696, 719274
Review-Url: https://codereview.chromium.org/2863923002
Cr-Commit-Position: refs/heads/master@{#469914}
Committed: https://chromium.googlesource.com/chromium/src/+/0657b42372a48280879bb16061dcbd258a360814

[tapted, 2017-05-05 05:05:47]

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

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

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-05 05:06:54]

Description was changed from

==========
Harmony: Don't snap the width of the chrome:// page info bubbles.

Adds DialogDelegate::ShouldSnapFrameWidth() -> bool to do this.

BUG=718696
==========

to

==========
Harmony: Don't snap the width of the chrome:// page info bubbles.

Adds views::DialogDelegate::ShouldSnapFrameWidth() -> bool to do this.

BUG=718696
==========

[tapted, 2017-05-05 05:06:54]

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

[tapted, 2017-05-05 05:15:57]

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

[commit-bot: I haz the power, 2017-05-05 05:16:13]

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-05 06:08:08]

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

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

Dry run: This issue passed the CQ dry run.

[tapted, 2017-05-05 06:08:31]

Hi Peter, please take a look

[Peter Kasting, 2017-05-05 22:49:01]

LGTM, but with some thoughts.

We may need this oracle function regardless, but I'm wondering if there's a
better heuristic we can use to avoid needing to specify it in so many places --
as I think this won't be the only case we need this.

In my head, the distinction was "is this a dialog with buttons, or a "panel"
with no buttons?".  I think pretty much all the former are snapped and pretty
much none of the latter are.  Maybe we can just check whether we have a set of
bottom buttons specified, and not snap if there are none?  Can you think of
obvious examples this would get wrong (either direction)?

[tapted, 2017-05-07 12:30:18]

On 2017/05/05 22:49:01, Peter Kasting (catching up) wrote:
> LGTM, but with some thoughts.
> 
> We may need this oracle function regardless, but I'm wondering if there's a
> better heuristic we can use to avoid needing to specify it in so many places
--
> as I think this won't be the only case we need this.
> 
> In my head, the distinction was "is this a dialog with buttons, or a "panel"
> with no buttons?".  I think pretty much all the former are snapped and pretty
> much none of the latter are.  Maybe we can just check whether we have a set of
> bottom buttons specified, and not snap if there are none?  Can you think of
> obvious examples this would get wrong (either direction)?

I think "does it have buttons" could get us closer.. There are currently a few
dialogs that are rolling their own buttons rather than returning something
meaningful from DialogDelegate::GetDialogButtons(), but I'd consider that a bug,
since it will mean the dialog misses out on things like auto-button-widths and
other things we want to do at the framework level (TouchBar integration on Mac
is one..).

For example, the Add Bookmark bubble returns NONE from GetDialogButtons. The new
signin flow dialogs are also doing this, since they have a few "sheets" that
slide in with different buttons. WebUI dialogs are also in this boat
(chromecast, print preview). The TranslateBubble also had "TODO: This should be
using GetDialogButtons." last time I checked too :/

[tapted, 2017-05-07 12:30:47]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2017-05-07 12:30:53]

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-07 13:39:43]

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

[commit-bot: I haz the power, 2017-05-07 13:39:44]

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

[Peter Kasting, 2017-05-07 14:30:52]

On 2017/05/07 12:30:18, tapted wrote:
> I think "does it have buttons" could get us closer.. There are currently a few
> dialogs that are rolling their own buttons rather than returning something
> meaningful from DialogDelegate::GetDialogButtons(), but I'd consider that a
bug,
> since it will mean the dialog misses out on things like auto-button-widths and
> other things we want to do at the framework level (TouchBar integration on Mac
> is one..).
> 
> For example, the Add Bookmark bubble returns NONE from GetDialogButtons. The
new
> signin flow dialogs are also doing this, since they have a few "sheets" that
> slide in with different buttons. WebUI dialogs are also in this boat
> (chromecast, print preview). The TranslateBubble also had "TODO: This should
be
> using GetDialogButtons." last time I checked too :/

It sounds like we will likely need this function regardless, so it seems OK to
land this.  For the heuristic tweak described above, should I file a bug?  Just
don't want this to get lost.

[tapted, 2017-05-08 00:13:36]

Description was changed from

==========
Harmony: Don't snap the width of the chrome:// page info bubbles.

Adds views::DialogDelegate::ShouldSnapFrameWidth() -> bool to do this.

BUG=718696
==========

to

==========
Harmony: Don't snap the width of the chrome:// page info bubbles.

Adds views::DialogDelegate::ShouldSnapFrameWidth() -> bool to do this.

BUG=718696, 719274
==========

[tapted, 2017-05-08 00:14:02]

On 2017/05/07 14:30:52, Peter Kasting wrote:
> On 2017/05/07 12:30:18, tapted wrote:
> > I think "does it have buttons" could get us closer.. There are currently a
few
> > dialogs that are rolling their own buttons rather than returning something
> > meaningful from DialogDelegate::GetDialogButtons(), but I'd consider that a
> bug,
> > since it will mean the dialog misses out on things like auto-button-widths
and
> > other things we want to do at the framework level (TouchBar integration on
Mac
> > is one..).
> > 
> > For example, the Add Bookmark bubble returns NONE from GetDialogButtons. The
> new
> > signin flow dialogs are also doing this, since they have a few "sheets" that
> > slide in with different buttons. WebUI dialogs are also in this boat
> > (chromecast, print preview). The TranslateBubble also had "TODO: This should
> be
> > using GetDialogButtons." last time I checked too :/
> 
> It sounds like we will likely need this function regardless, so it seems OK to
> land this.  For the heuristic tweak described above, should I file a bug? 
Just
> don't want this to get lost.

Filed http://crbug.com/719274 with a brain dump

[tapted, 2017-05-08 00:14:18]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2017-05-08 00:14:32]

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-08 01:31:11]

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

[commit-bot: I haz the power, 2017-05-08 01:31:12]

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-08 05:49:00]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2017-05-08 05:49: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-08 07:39:13]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494222540670500,
"parent_rev": "ab30c21b503253f8a9bb9e92f04db009f1fc4fa1", "commit_rev":
"0657b42372a48280879bb16061dcbd258a360814"}

[commit-bot: I haz the power, 2017-05-08 07:39:57]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494222540670500,
"parent_rev": "ab30c21b503253f8a9bb9e92f04db009f1fc4fa1", "commit_rev":
"0657b42372a48280879bb16061dcbd258a360814"}

[commit-bot: I haz the power, 2017-05-08 07:40:24]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494222540670500,
"parent_rev": "ab30c21b503253f8a9bb9e92f04db009f1fc4fa1", "commit_rev":
"0657b42372a48280879bb16061dcbd258a360814"}

[commit-bot: I haz the power, 2017-05-08 07:40:26]

Description was changed from

==========
Harmony: Don't snap the width of the chrome:// page info bubbles.

Adds views::DialogDelegate::ShouldSnapFrameWidth() -> bool to do this.

BUG=718696, 719274
==========

to

==========
Harmony: Don't snap the width of the chrome:// page info bubbles.

Adds views::DialogDelegate::ShouldSnapFrameWidth() -> bool to do this.

BUG=718696, 719274

Review-Url: https://codereview.chromium.org/2863923002
Cr-Commit-Position: refs/heads/master@{#469914}
Committed:
https://chromium.googlesource.com/chromium/src/+/0657b42372a48280879bb16061dc...
==========

[commit-bot: I haz the power, 2017-05-08 07:40:28]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/0657b42372a48280879bb16061dc...