+bsep this might be a regression from r482840 -> https://codereview.chromium.org/2907983002 "Allow dialogs to use a custom View as their title."

The problem seems to be that views::StyleLabel has an "over-efficient" CalculatePreferredSize():

gfx::Size StyledLabel::CalculatePreferredSize() const {
  return calculated_size_;
}


When the signin promo comes in, it changes the dialog title, triggering 

void StyledLabel::PreferredSizeChanged() {
  calculated_size_ = gfx::Size();
  width_at_last_size_calculation_ = 0;
  width_at_last_layout_ = 0;
  View::PreferredSizeChanged();
}


So when BubbleFrameView next tries to ask the title() view what its preferred size is, if it's a StyledLabel, it just says 0x0.


StyledLabel::GetHeightForWidth() will actually do the calculation:


int StyledLabel::GetHeightForWidth(int w) const {
  // TODO(erg): Munge the const-ness of the style label. CalculateAndDoLayout
  // doesn't actually make any changes to member variables when |dry_run| is
  // set to true. In general, the mutating and non-mutating parts shouldn't
  // be in the same codepath.
  return const_cast<StyledLabel*>(this)->CalculateAndDoLayout(w, true).height();
}


But BubbleFrameView doesn't use that.


We can munge something similar into StyledLabel::CalculatePreferedSize.. -> https://chromium-review.googlesource.com/601692 .. might be some subtleties with this though.

On windows, the non-MD dialog is just missing buttons completely, so it almost looks intentional.

I think this would have regressed in m61 and will need a merge.

But I haven't figured out an easy way to pop this dialog without the dialog browser test I'm adding.

Deleted: non_md.png 3.1 KB

	non_md.png 3.1 KB View Download

Deleted: md.png 4.5 KB

	md.png 4.5 KB View Download

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

commit 1453be1214185aed2b13fdec79c73f842fd2eb83
Author: Trent Apted <tapted@chromium.org>
Date: Sat Aug 12 05:54:58 2017

Fix SizeToContents() for bubbles that change their StyledLabel title.

This regressed in r482840. Causing, e.g., the save password signin
promo dialog to lose its buttons.

Some dialogs switch out their client view and change dialog
title when transitioning between states. r482840 meant that titles in
the BubbleFrameView could be views::StyledLabel as well as views::Label.

Unfortunately, views::StyledLabel has a complex way of calculating
its preferred size. Previously, asking for its preferred size
immediately after calling PreferredSizeChanged() would always return
0x0, unless there was an intervening call to GetHeightForWidth().
And changing StyledLabel text triggers PreferredSizeChanged().

To fix properly, we need to address a chicken-and-egg problem that
exists in BubbleFrameView regarding the the dialog title. The
height of the title depends on the width of the dialog, since the title can
wrap. But insets used for GetSizeForClientSize() determine the bubble size,
and that must account for the dialog title.

So split out the GetInset() calculation into versions that can be
done with the current client size and a forecast client size.

Bug:  751375 
Change-Id: I3e7b26989008285c625cd56ac553d4418f493906
Reviewed-on: https://chromium-review.googlesource.com/601692
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493962}
[modify] https://crrev.com/1453be1214185aed2b13fdec79c73f842fd2eb83/chrome/browser/password_manager/password_manager_browsertest.cc
[modify] https://crrev.com/1453be1214185aed2b13fdec79c73f842fd2eb83/ui/views/bubble/bubble_dialog_delegate_unittest.cc
[modify] https://crrev.com/1453be1214185aed2b13fdec79c73f842fd2eb83/ui/views/bubble/bubble_frame_view.cc
[modify] https://crrev.com/1453be1214185aed2b13fdec79c73f842fd2eb83/ui/views/bubble/bubble_frame_view.h
[modify] https://crrev.com/1453be1214185aed2b13fdec79c73f842fd2eb83/ui/views/bubble/bubble_frame_view_unittest.cc

Verified fixed in Version 62.0.3186.0 (Official Build) canary (32-bit)

This is broken in m61 Beta. Requesting merge of r493962 to m61.

Note other dialogs may be affected by the regression from r482840 as well (r493962 fixes the root cause).

Verification steps:
 1. In a new, signed-out, Chrome profile, login to a website (e.g. twitter.com)
 2. "Do you want to save this password" bubble should appear. Say yes.
 3. Dialog should now say "Password saved. To get your passwords on all your devices sign in to Chrome."

Expected: There should be buttons on the dialog ("Sign in", "No, thank you").
Without Fix: There are no buttons.

Deleted: m60-stable-EXPECTED.png 25.1 KB

	m60-stable-EXPECTED.png 25.1 KB View Download

Deleted: m61-beta-BROKEN.png 18.5 KB

	m61-beta-BROKEN.png 18.5 KB View Download

Deleted: m62-canary-FIXED.png 41.6 KB

	m62-canary-FIXED.png 41.6 KB View Download

This bug requires manual review: M61 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Thank you  tapted@ for verifying the fix on Canary at #4 and confirming this regressed in M61.

Before we approve merge to M61 for cl listed at #3, please answer followings:
* Is the change having enough automation tests coverage and safe to merge to M61?
* Any other important details to justify the merge.

Please note M61 is already in Beta, so merge bar is very high. Thank you.

Tested the issue on Windows-7,Ubuntu 14.04,Mac OS 10.12.6 & Chrome OS(peppy) using chrome latest Canary M62-62.0.3187.0 and chrome OS Canary #62.0.3186.0 by following steps mentioned in the original comment. Observed that Buttons(Sign in & No Thank You)are displaying as expected. Hence adding TE-Verified label.

Please find the screen cast(Win) for reference.
Note: In Linux and Chrome OS "Password saved" alert message is not coming even after enabling the "secondary-ui-md" flag.

Thank you!

Deleted: 751375.mp4 2.6 MB

	751375.mp4 2.6 MB View Download

Before we approve merge to M61 for cl listed at #3, please answer followings:
* Is the change having enough automation tests coverage and safe to merge to M61?

r493962 has automated tests at 3 layers. The fix is not simple, but I think it is safe to merge.


* Any other important details to justify the merge.

The regression doesn't cause a crash or data loss, but the dialog becomes unusable and can only be dismissed.


(removing ChromeOS. After looking around, I think it's just the passwords dialog that's affected. And it's impossible to be signed out on ChromeOS, so the dialog offering to sign in is never shown).

Approving merge to M61 branch 3163 based on comment #4, #7 #8. Please merge ASAP. Thank you.

Actually putting it back to "Merge-Review-61" and want to take +abdulsyed@ input here for approval as Cl listed at #3 is large and not simple. 

Thanks for the fix tapted@. Looks like a clear regression. Is this dialogue commonly triggered?

If the fix is overall safe and if this is a commonly triggered dialogue, I'm fine with taking this for M61. 

UMA suggests 200,000 - 300,000 impressions per day on Windows Stable for for UMA-enabled users - go/qyjsc . With about 45% interacting with the buttons (which will drop to zero without the fix). This drop can be seen in m61 beta already - go/omsnf

Issue 751345 has been merged into this issue.

Approving merge to M61 branch 3163 based on comments #7, #8, #11 and #12. Please merge ASAP. Thank you.

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

commit e8e97e2f94c0316f90c7dca85abc281b52b3af6e
Author: Trent Apted <tapted@chromium.org>
Date: Thu Aug 17 07:23:07 2017

[merge-m61] Fix SizeToContents() for bubbles that change their StyledLabel title.

This regressed in r482840. Causing, e.g., the save password signin
promo dialog to lose its buttons.

Some dialogs switch out their client view and change dialog
title when transitioning between states. r482840 meant that titles in
the BubbleFrameView could be views::StyledLabel as well as views::Label.

Unfortunately, views::StyledLabel has a complex way of calculating
its preferred size. Previously, asking for its preferred size
immediately after calling PreferredSizeChanged() would always return
0x0, unless there was an intervening call to GetHeightForWidth().
And changing StyledLabel text triggers PreferredSizeChanged().

To fix properly, we need to address a chicken-and-egg problem that
exists in BubbleFrameView regarding the the dialog title. The
height of the title depends on the width of the dialog, since the title can
wrap. But insets used for GetSizeForClientSize() determine the bubble size,
and that must account for the dialog title.

So split out the GetInset() calculation into versions that can be
done with the current client size and a forecast client size.

TBR=tapted@chromium.org

(cherry picked from commit 1453be1214185aed2b13fdec79c73f842fd2eb83)

Bug:  751375 
Change-Id: I3e7b26989008285c625cd56ac553d4418f493906
Reviewed-on: https://chromium-review.googlesource.com/601692
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#493962}
Reviewed-on: https://chromium-review.googlesource.com/618531
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#626}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/e8e97e2f94c0316f90c7dca85abc281b52b3af6e/chrome/browser/password_manager/password_manager_browsertest.cc
[modify] https://crrev.com/e8e97e2f94c0316f90c7dca85abc281b52b3af6e/ui/views/bubble/bubble_dialog_delegate_unittest.cc
[modify] https://crrev.com/e8e97e2f94c0316f90c7dca85abc281b52b3af6e/ui/views/bubble/bubble_frame_view.cc
[modify] https://crrev.com/e8e97e2f94c0316f90c7dca85abc281b52b3af6e/ui/views/bubble/bubble_frame_view.h
[modify] https://crrev.com/e8e97e2f94c0316f90c7dca85abc281b52b3af6e/ui/views/bubble/bubble_frame_view_unittest.cc

 Issue 739681  has been merged into this issue.