MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be used

MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be used

The toolkit-views dialog will be used when --secondary-ui-md is enabled.

(With that flag) The dialog also becomes window-modal on Mac, rather
than tab-modal. This is consistent with other platforms. That's the easy
bit -- ProfileSigninConfirmationDialogViews::ShowDialog() "Just Works".

Adds a browser_test for showing the views dialog - it had no prior
coverage. This uses the TestBrowserDialog framework, which was using
GetAllChildWidgets() to detect a dialog being added. This worked on Mac,
but on Aura the dialog is added as a "transient" child. Update
TestBrowserDialog to use GetAllOwnedWidgets(), which includes transient
children as well as other child windows.

Also, there was a memory leak.
OneClickSigninSyncStarter::OnRegisteredForPolicy() allocated a
SigninDialogDelegate with `new` which was never released. Cocoa tests in
profile_signin_confirmation_view_controller_browsertest.mm didn't pick
this up because the tests use the test harness as the delegate (passing
`this`). The right fix is to pass a std::unique_ptr - do that.

BUG=681049
Review-Url: https://codereview.chromium.org/2625813003
Cr-Commit-Position: refs/heads/master@{#443751}
Committed: https://chromium.googlesource.com/chromium/src/+/0dd1562593d3cb4ebe882801d76d747641d7b7f0

[tapted, 2017-01-11 01:43:41]

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

[commit-bot: I haz the power, 2017-01-11 01:44:03]

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

[tapted, 2017-01-11 01:59:41]

Description was changed from

==========
Sync Confirmation Dialog now Harmony on Mac

BUG=677012
==========

to

==========
Sync Confirmation Dialog now Harmony on Mac

depends on http://crrev.com/2534743002

BUG=677012
==========

[tapted, 2017-01-11 02:01:12]

Description was changed from

==========
Sync Confirmation Dialog now Harmony on Mac

depends on http://crrev.com/2534743002

BUG=677012
==========

to

==========
MacViews: Allow the toolkit-views Sync Confirmation Dialog to be used

The toolkit-views dialog will be used when --secondary-ui-md is enabled.

This also makes it a window-modal sheet, rather than tab-modal, which is
consistent with other platforms.

depends on http://crrev.com/2534743002

BUG=677012
==========

[commit-bot: I haz the power, 2017-01-11 02:38:07]

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

[commit-bot: I haz the power, 2017-01-11 02:38:08]

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-01-12 18:00:23]

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

[tapted, 2017-01-12 18:00:53]

Description was changed from

==========
MacViews: Allow the toolkit-views Sync Confirmation Dialog to be used

The toolkit-views dialog will be used when --secondary-ui-md is enabled.

This also makes it a window-modal sheet, rather than tab-modal, which is
consistent with other platforms.

depends on http://crrev.com/2534743002

BUG=677012
==========

to

==========
MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be
used

The toolkit-views dialog will be used when --secondary-ui-md is enabled.

This also makes it a window-modal sheet, rather than tab-modal, which is
consistent with other platforms.

BUG=677012
==========

[commit-bot: I haz the power, 2017-01-12 18:00:57]

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

[tapted, 2017-01-12 18:05:00]

The CQ bit was unchecked by tapted@chromium.org

[tapted, 2017-01-12 18:10:46]

Description was changed from

==========
MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be
used

The toolkit-views dialog will be used when --secondary-ui-md is enabled.

This also makes it a window-modal sheet, rather than tab-modal, which is
consistent with other platforms.

BUG=677012
==========

to

==========
MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be
used

The toolkit-views dialog will be used when --secondary-ui-md is enabled.

(With that flag) The dialog also becomes window-modal on Mac, rather
than tab-modal. This is consistent with other platforms. Note on Mac,
window-modal dialogs appear as "sheets".

Adds a browser_test for showing this dialog - it had no prior coverage.

BUG=677012
==========

[tapted, 2017-01-12 18:10:47]

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

[tapted, 2017-01-12 20:00:30]

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

[commit-bot: I haz the power, 2017-01-12 20:01:28]

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

[tapted, 2017-01-12 20:11:09]

Description was changed from

==========
MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be
used

The toolkit-views dialog will be used when --secondary-ui-md is enabled.

(With that flag) The dialog also becomes window-modal on Mac, rather
than tab-modal. This is consistent with other platforms. Note on Mac,
window-modal dialogs appear as "sheets".

Adds a browser_test for showing this dialog - it had no prior coverage.

BUG=677012
==========

to

==========
MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be
used

The toolkit-views dialog will be used when --secondary-ui-md is enabled.

(With that flag) The dialog also becomes window-modal on Mac, rather
than tab-modal. This is consistent with other platforms. Note on Mac,
window-modal dialogs appear as "sheets". That's the easy bit --
ProfileSigninConfirmationDialogViews::ShowDialog() "Just Works"

Adds a browser_test for showing this dialog - it had no prior coverage.
This bit is harder, since there hasn't been much using sheets before.
NativeWidgetMac once added sheets to -[NSWindow childWindows], but that
causes glitches. So ensure both -[NSWindow childWindows] and -[NSWindow
sheets] (introduced 10.9) are treated as child windows and test.

Also implement+test NativeWidgetMac::GetAllOwnedWindows(). On Mac, this
is almost identical to GetAllChildWindows(). It's needed now so that the
dialog testing framework can properly detect dialogs added in aura,
which are sometimes "transient" children rather than "owned" children.

BUG=677012
==========

[commit-bot: I haz the power, 2017-01-12 22:29:36]

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

[commit-bot: I haz the power, 2017-01-12 22:29:38]

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

[tapted, 2017-01-12 23:45:31]

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

[commit-bot: I haz the power, 2017-01-12 23:46:33]

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

[tapted, 2017-01-12 23:53:59]

Description was changed from

==========
MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be
used

The toolkit-views dialog will be used when --secondary-ui-md is enabled.

(With that flag) The dialog also becomes window-modal on Mac, rather
than tab-modal. This is consistent with other platforms. Note on Mac,
window-modal dialogs appear as "sheets". That's the easy bit --
ProfileSigninConfirmationDialogViews::ShowDialog() "Just Works"

Adds a browser_test for showing this dialog - it had no prior coverage.
This bit is harder, since there hasn't been much using sheets before.
NativeWidgetMac once added sheets to -[NSWindow childWindows], but that
causes glitches. So ensure both -[NSWindow childWindows] and -[NSWindow
sheets] (introduced 10.9) are treated as child windows and test.

Also implement+test NativeWidgetMac::GetAllOwnedWindows(). On Mac, this
is almost identical to GetAllChildWindows(). It's needed now so that the
dialog testing framework can properly detect dialogs added in aura,
which are sometimes "transient" children rather than "owned" children.

BUG=677012
==========

to

==========
MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be
used

The toolkit-views dialog will be used when --secondary-ui-md is enabled.

(With that flag) The dialog also becomes window-modal on Mac, rather
than tab-modal. This is consistent with other platforms. Note on Mac,
window-modal dialogs appear as "sheets". That's the easy bit --
ProfileSigninConfirmationDialogViews::ShowDialog() "Just Works"

Adds a browser_test for showing this dialog - it had no prior coverage.
This bit is harder, since there hasn't been much using sheets before.
NativeWidgetMac once added sheets to -[NSWindow childWindows], but that
causes glitches. So ensure both -[NSWindow childWindows] and -[NSWindow
sheets] (introduced 10.9) are treated as child windows and test.

Also, there was a memory leak.
OneClickSigninSyncStarter::OnRegisteredForPolicy() allocated a
SigninDialogDelegate with `new` which is never released. Cocoa tests in
profile_signin_confirmation_view_controller_browsertest.mm didn't pick
this up because the tests use the test harness as the delegate (passing
`this`). The right fix is to pass a std::unique_ptr - do that.

Also implement+test NativeWidgetMac::GetAllOwnedWindows(). On Mac, this
is almost identical to GetAllChildWindows(). It's needed now so that the
dialog testing framework can properly detect dialogs added in aura,
which are sometimes "transient" children rather than "owned" children.

BUG=677012
==========

[tapted, 2017-01-13 00:24:13]

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

[commit-bot: I haz the power, 2017-01-13 00:26:29]

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

[tapted, 2017-01-13 02:40:41]

Description was changed from

==========
MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be
used

The toolkit-views dialog will be used when --secondary-ui-md is enabled.

(With that flag) The dialog also becomes window-modal on Mac, rather
than tab-modal. This is consistent with other platforms. Note on Mac,
window-modal dialogs appear as "sheets". That's the easy bit --
ProfileSigninConfirmationDialogViews::ShowDialog() "Just Works"

Adds a browser_test for showing this dialog - it had no prior coverage.
This bit is harder, since there hasn't been much using sheets before.
NativeWidgetMac once added sheets to -[NSWindow childWindows], but that
causes glitches. So ensure both -[NSWindow childWindows] and -[NSWindow
sheets] (introduced 10.9) are treated as child windows and test.

Also, there was a memory leak.
OneClickSigninSyncStarter::OnRegisteredForPolicy() allocated a
SigninDialogDelegate with `new` which is never released. Cocoa tests in
profile_signin_confirmation_view_controller_browsertest.mm didn't pick
this up because the tests use the test harness as the delegate (passing
`this`). The right fix is to pass a std::unique_ptr - do that.

Also implement+test NativeWidgetMac::GetAllOwnedWindows(). On Mac, this
is almost identical to GetAllChildWindows(). It's needed now so that the
dialog testing framework can properly detect dialogs added in aura,
which are sometimes "transient" children rather than "owned" children.

BUG=677012
==========

to

==========
MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be
used

The toolkit-views dialog will be used when --secondary-ui-md is enabled.

(With that flag) The dialog also becomes window-modal on Mac, rather
than tab-modal. This is consistent with other platforms. That's the easy
bit -- ProfileSigninConfirmationDialogViews::ShowDialog() "Just Works".

Adds a browser_test for showing the views dialog - it had no prior
coverage. This bit is harder, since there hasn't been much using sheets
before. NativeWidgetMac once added sheets to -[NSWindow childWindows],
but that causes glitches. So ensure both -[NSWindow childWindows] and
-[NSWindow sheets] (API introduced 10.9) are treated as child windows
and test.

Also, there was a memory leak.
OneClickSigninSyncStarter::OnRegisteredForPolicy() allocated a
SigninDialogDelegate with `new` which was never released. Cocoa tests in
profile_signin_confirmation_view_controller_browsertest.mm didn't pick
this up because the tests use the test harness as the delegate (passing
`this`). The right fix is to pass a std::unique_ptr - do that.

Also implement+test NativeWidgetMac::GetAllOwnedWindows(). On Mac, this
is almost identical to GetAllChildWindows(). It's needed now so that the
dialog testing framework can properly detect dialogs added in aura,
which are sometimes "transient" children rather than "owned" children.

BUG=677012
==========

[tapted, 2017-01-13 02:47:39]

Patchset #4 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-01-13 04:10:24]

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

[commit-bot: I haz the power, 2017-01-13 04:10:25]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-01-13 16:58:57]

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

[commit-bot: I haz the power, 2017-01-13 16:59:25]

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

[tapted, 2017-01-13 19:42:20]

Description was changed from

==========
MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be
used

The toolkit-views dialog will be used when --secondary-ui-md is enabled.

(With that flag) The dialog also becomes window-modal on Mac, rather
than tab-modal. This is consistent with other platforms. That's the easy
bit -- ProfileSigninConfirmationDialogViews::ShowDialog() "Just Works".

Adds a browser_test for showing the views dialog - it had no prior
coverage. This bit is harder, since there hasn't been much using sheets
before. NativeWidgetMac once added sheets to -[NSWindow childWindows],
but that causes glitches. So ensure both -[NSWindow childWindows] and
-[NSWindow sheets] (API introduced 10.9) are treated as child windows
and test.

Also, there was a memory leak.
OneClickSigninSyncStarter::OnRegisteredForPolicy() allocated a
SigninDialogDelegate with `new` which was never released. Cocoa tests in
profile_signin_confirmation_view_controller_browsertest.mm didn't pick
this up because the tests use the test harness as the delegate (passing
`this`). The right fix is to pass a std::unique_ptr - do that.

Also implement+test NativeWidgetMac::GetAllOwnedWindows(). On Mac, this
is almost identical to GetAllChildWindows(). It's needed now so that the
dialog testing framework can properly detect dialogs added in aura,
which are sometimes "transient" children rather than "owned" children.

BUG=677012
==========

to

==========
MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be
used

The toolkit-views dialog will be used when --secondary-ui-md is enabled.

(With that flag) The dialog also becomes window-modal on Mac, rather
than tab-modal. This is consistent with other platforms. That's the easy
bit -- ProfileSigninConfirmationDialogViews::ShowDialog() "Just Works".

Adds a browser_test for showing the views dialog - it had no prior
coverage. 

Also, there was a memory leak.
OneClickSigninSyncStarter::OnRegisteredForPolicy() allocated a
SigninDialogDelegate with `new` which was never released. Cocoa tests in
profile_signin_confirmation_view_controller_browsertest.mm didn't pick
this up because the tests use the test harness as the delegate (passing
`this`). The right fix is to pass a std::unique_ptr - do that.

BUG=681049
==========

[tapted, 2017-01-13 19:42:20]

tapted@chromium.org changed reviewers:
- rsesek@chromium.org

[tapted, 2017-01-13 22:53:17]

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

[tapted, 2017-01-13 22:53:18]

Hi Mike - please take a look. I really just wanted to use
profile_signin_confirmation_dialog_views.cc on Mac (i.e. from
tab_dialogs_views_mac.mm ), but the `InvokeDialog` test I added with it exposed
a memory leak -- the fix for that leak ended up touching a lot of files.

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/sync...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (left):

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/sync...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:250: new
SigninDialogDelegate(weak_pointer_factory_.GetWeakPtr()));
This is the leak - nothing would ever free the delegate allocated with `new`
here.

Plumbing through a unique_ptr seems right, although it's a lot of churn.

Another possibility: declare a function-local `static SigninDialogDelegate
delegate;` here and do `delegate.SetClient(weak_pointer_factory.GetWeakPtr());`
then pass the address of the local. I think that would work fine - there'd be
less codegen in the text segment, but of course the `leak` would be permanent
for everyone since the static needs space in the data segment.

I could also just restructure the test so that it doesn't expose the leak....

[msw, 2017-01-13 23:23:51]

lgtm with minor comments.

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/sync...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (left):

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/sync...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:250: new
SigninDialogDelegate(weak_pointer_factory_.GetWeakPtr()));
On 2017/01/13 22:53:18, tapted wrote:
> This is the leak - nothing would ever free the delegate allocated with `new`
> here.
> 
> Plumbing through a unique_ptr seems right, although it's a lot of churn.
> 
> Another possibility: declare a function-local `static SigninDialogDelegate
> delegate;` here and do
`delegate.SetClient(weak_pointer_factory.GetWeakPtr());`
> then pass the address of the local. I think that would work fine - there'd be
> less codegen in the text segment, but of course the `leak` would be permanent
> for everyone since the static needs space in the data segment.
> 
> I could also just restructure the test so that it doesn't expose the leak....

Plumbing unique_ptr seems right to me; thanks for the fix!

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/test...
File chrome/browser/ui/test/test_browser_dialog.cc (right):

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:91:
views::Widget::GetAllOwnedWidgets(parent, &widgets_before);
What's the reason behind these changes? (worth mentioning in cl desc)

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc
(right):

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:61: //
Hides the new avatar bubble if it is currently shown. The new avatar bubble
aside: It'd be nice to just remove this workaround, but I bet it's still needed.

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/view...
File
chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views_browsertest.cc
(right):

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views_browsertest.cc:19:
class TestSigninDialogDelegate : public ui::ProfileSigninConfirmationDelegate {
optional nit: comment

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views_browsertest.cc:43:
// TestDialogInterface:
nit: use the appropriate base class name.
(optionally also fix collected_cookies_browsertest.cc)

[tapted, 2017-01-13 23:31:03]

Description was changed from

==========
MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be
used

The toolkit-views dialog will be used when --secondary-ui-md is enabled.

(With that flag) The dialog also becomes window-modal on Mac, rather
than tab-modal. This is consistent with other platforms. That's the easy
bit -- ProfileSigninConfirmationDialogViews::ShowDialog() "Just Works".

Adds a browser_test for showing the views dialog - it had no prior
coverage. 

Also, there was a memory leak.
OneClickSigninSyncStarter::OnRegisteredForPolicy() allocated a
SigninDialogDelegate with `new` which was never released. Cocoa tests in
profile_signin_confirmation_view_controller_browsertest.mm didn't pick
this up because the tests use the test harness as the delegate (passing
`this`). The right fix is to pass a std::unique_ptr - do that.

BUG=681049
==========

to

==========
MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be
used

The toolkit-views dialog will be used when --secondary-ui-md is enabled.

(With that flag) The dialog also becomes window-modal on Mac, rather
than tab-modal. This is consistent with other platforms. That's the easy
bit -- ProfileSigninConfirmationDialogViews::ShowDialog() "Just Works".

Adds a browser_test for showing the views dialog - it had no prior
coverage. This uses the TestBrowserDialog framework, which was using
GetAllChildWidgets() to detect a dialog being added. This worked on Mac,
but on Aura the dialog is added as a "transient" child. Update
TestBrowserDialog to use GetAllOwnedWidgets(), which includes transient
children as well as other child windows.

Also, there was a memory leak.
OneClickSigninSyncStarter::OnRegisteredForPolicy() allocated a
SigninDialogDelegate with `new` which was never released. Cocoa tests in
profile_signin_confirmation_view_controller_browsertest.mm didn't pick
this up because the tests use the test harness as the delegate (passing
`this`). The right fix is to pass a std::unique_ptr - do that.

BUG=681049
==========

[tapted, 2017-01-13 23:48:45]

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/test...
File chrome/browser/ui/test/test_browser_dialog.cc (right):

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:91:
views::Widget::GetAllOwnedWidgets(parent, &widgets_before);
On 2017/01/13 23:23:51, msw wrote:
> What's the reason behind these changes? (worth mentioning in cl desc)

whoops - I deleted that bit in an edit. Added back:

This uses the TestBrowserDialog framework, which was using
GetAllChildWidgets() to detect a dialog being added. This worked on Mac,
but on Aura the dialog is added as a "transient" child. Update
TestBrowserDialog to use GetAllOwnedWidgets(), which includes transient
children as well as other child windows.

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc
(right):

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:61: //
Hides the new avatar bubble if it is currently shown. The new avatar bubble
On 2017/01/13 23:23:51, msw wrote:
> aside: It'd be nice to just remove this workaround, but I bet it's still
needed.

With the recent changes in http://crbug.com/677012 it might not be needed any
longer. But the particular bubble sequence this talks about is tricky to invoke
in Chromium. I'll poke it some more on Mac+Windows & follow-up.

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/view...
File
chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views_browsertest.cc
(right):

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views_browsertest.cc:19:
class TestSigninDialogDelegate : public ui::ProfileSigninConfirmationDelegate {
On 2017/01/13 23:23:51, msw wrote:
> optional nit: comment

Done.

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views_browsertest.cc:43:
// TestDialogInterface:
On 2017/01/13 23:23:51, msw wrote:
> nit: use the appropriate base class name.
> (optionally also fix collected_cookies_browsertest.cc)

Done & Done. Although I guess I should start a chromium-dev thread about this
;).

I see the appeal of picking something from the list of direct base classes, and
that's the more consistent approach for now. However, another interpretation is
that this comment appears in lieu of duplicating the "real" function comments,
so it should point to the class declaration that has those comments (which may
not be a direct base class).

[tapted, 2017-01-13 23:49:10]

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

[commit-bot: I haz the power, 2017-01-13 23:49:53]

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

[msw, 2017-01-13 23:53:52]

lgtm

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc
(right):

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:61: //
Hides the new avatar bubble if it is currently shown. The new avatar bubble
On 2017/01/13 23:48:45, tapted wrote:
> On 2017/01/13 23:23:51, msw wrote:
> > aside: It'd be nice to just remove this workaround, but I bet it's still
> needed.
> 
> With the recent changes in http://crbug.com/677012 it might not be needed any
> longer. But the particular bubble sequence this talks about is tricky to
invoke
> in Chromium. I'll poke it some more on Mac+Windows & follow-up.

sgtm, it's not a blocker for this CL.

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/view...
File
chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views_browsertest.cc
(right):

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views_browsertest.cc:43:
// TestDialogInterface:
On 2017/01/13 23:48:45, tapted wrote:
> On 2017/01/13 23:23:51, msw wrote:
> > nit: use the appropriate base class name.
> > (optionally also fix collected_cookies_browsertest.cc)
> 
> Done & Done. Although I guess I should start a chromium-dev thread about this
> ;).
> 
> I see the appeal of picking something from the list of direct base classes,
and
> that's the more consistent approach for now. However, another interpretation
is
> that this comment appears in lieu of duplicating the "real" function comments,
> so it should point to the class declaration that has those comments (which may
> not be a direct base class).

I'm not picky, just don't use the non-existent 'TestDialogInterface' :)

[tapted, 2017-01-14 00:06:23]

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/view...
File
chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views_browsertest.cc
(right):

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views_browsertest.cc:43:
// TestDialogInterface:
On 2017/01/13 23:53:52, msw wrote:
> On 2017/01/13 23:48:45, tapted wrote:
> > On 2017/01/13 23:23:51, msw wrote:
> > > nit: use the appropriate base class name.
> > > (optionally also fix collected_cookies_browsertest.cc)
> > 
> > Done & Done. Although I guess I should start a chromium-dev thread about
this
> > ;).
> > 
> > I see the appeal of picking something from the list of direct base classes,
> and
> > that's the more consistent approach for now. However, another interpretation
> is
> > that this comment appears in lieu of duplicating the "real" function
comments,
> > so it should point to the class declaration that has those comments (which
may
> > not be a direct base class).
> 
> I'm not picky, just don't use the non-existent 'TestDialogInterface' :)

Ohhhhh. Doh - you're right. I forgot I renamed it to TestBrowserDialog :/.

I may actually circle back on that -- I picked TestBrowserDialog to match the
filename. And although DialogBrowserTest feels nice (e.g. as a replacement for
InProcessBrowserTest), I find I'm mixing up the order of things in
TestBrowserDialog :/ (probably because BrowserDialogTest is also taken by the
actual test harness in browser_dialog_browsertest.cc). Maybe
DialogBrowserTestInterface and dialog_browser_test.h... (and
dialog_browser_test_browsertest.cc..)

Anyway that's orthogonal, and I guess it means "DialogBrowserTest" is best for
the comment here, because that's the name I'm not thinking about changing :)

[tapted, 2017-01-14 00:06:23]

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/view...
File
chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views_browsertest.cc
(right):

https://codereview.chromium.org/2625813003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views_browsertest.cc:43:
// TestDialogInterface:
On 2017/01/13 23:53:52, msw wrote:
> On 2017/01/13 23:48:45, tapted wrote:
> > On 2017/01/13 23:23:51, msw wrote:
> > > nit: use the appropriate base class name.
> > > (optionally also fix collected_cookies_browsertest.cc)
> > 
> > Done & Done. Although I guess I should start a chromium-dev thread about
this
> > ;).
> > 
> > I see the appeal of picking something from the list of direct base classes,
> and
> > that's the more consistent approach for now. However, another interpretation
> is
> > that this comment appears in lieu of duplicating the "real" function
comments,
> > so it should point to the class declaration that has those comments (which
may
> > not be a direct base class).
> 
> I'm not picky, just don't use the non-existent 'TestDialogInterface' :)

Ohhhhh. Doh - you're right. I forgot I renamed it to TestBrowserDialog :/.

I may actually circle back on that -- I picked TestBrowserDialog to match the
filename. And although DialogBrowserTest feels nice (e.g. as a replacement for
InProcessBrowserTest), I find I'm mixing up the order of things in
TestBrowserDialog :/ (probably because BrowserDialogTest is also taken by the
actual test harness in browser_dialog_browsertest.cc). Maybe
DialogBrowserTestInterface and dialog_browser_test.h... (and
dialog_browser_test_browsertest.cc..)

Anyway that's orthogonal, and I guess it means "DialogBrowserTest" is best for
the comment here, because that's the name I'm not thinking about changing :)

[tapted, 2017-01-14 00:06:50]

The CQ bit was unchecked by tapted@chromium.org

[tapted, 2017-01-14 00:06:57]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2017-01-14 00:07:36]

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-01-14 00:58:43]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484352417271680,
"parent_rev": "0e4e655d4e8de0b4ec34964b3d41179198e459f1", "commit_rev":
"0dd1562593d3cb4ebe882801d76d747641d7b7f0"}

[commit-bot: I haz the power, 2017-01-14 00:59:14]

Description was changed from

==========
MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be
used

The toolkit-views dialog will be used when --secondary-ui-md is enabled.

(With that flag) The dialog also becomes window-modal on Mac, rather
than tab-modal. This is consistent with other platforms. That's the easy
bit -- ProfileSigninConfirmationDialogViews::ShowDialog() "Just Works".

Adds a browser_test for showing the views dialog - it had no prior
coverage. This uses the TestBrowserDialog framework, which was using
GetAllChildWidgets() to detect a dialog being added. This worked on Mac,
but on Aura the dialog is added as a "transient" child. Update
TestBrowserDialog to use GetAllOwnedWidgets(), which includes transient
children as well as other child windows.

Also, there was a memory leak.
OneClickSigninSyncStarter::OnRegisteredForPolicy() allocated a
SigninDialogDelegate with `new` which was never released. Cocoa tests in
profile_signin_confirmation_view_controller_browsertest.mm didn't pick
this up because the tests use the test harness as the delegate (passing
`this`). The right fix is to pass a std::unique_ptr - do that.

BUG=681049
==========

to

==========
MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be
used

The toolkit-views dialog will be used when --secondary-ui-md is enabled.

(With that flag) The dialog also becomes window-modal on Mac, rather
than tab-modal. This is consistent with other platforms. That's the easy
bit -- ProfileSigninConfirmationDialogViews::ShowDialog() "Just Works".

Adds a browser_test for showing the views dialog - it had no prior
coverage. This uses the TestBrowserDialog framework, which was using
GetAllChildWidgets() to detect a dialog being added. This worked on Mac,
but on Aura the dialog is added as a "transient" child. Update
TestBrowserDialog to use GetAllOwnedWidgets(), which includes transient
children as well as other child windows.

Also, there was a memory leak.
OneClickSigninSyncStarter::OnRegisteredForPolicy() allocated a
SigninDialogDelegate with `new` which was never released. Cocoa tests in
profile_signin_confirmation_view_controller_browsertest.mm didn't pick
this up because the tests use the test harness as the delegate (passing
`this`). The right fix is to pass a std::unique_ptr - do that.

BUG=681049

Review-Url: https://codereview.chromium.org/2625813003
Cr-Commit-Position: refs/heads/master@{#443751}
Committed:
https://chromium.googlesource.com/chromium/src/+/0dd1562593d3cb4ebe882801d76d...
==========

[commit-bot: I haz the power, 2017-01-14 00:59:15]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/0dd1562593d3cb4ebe882801d76d...

[pkalinnikov, 2017-01-16 13:45:12]

A revert of this CL (patchset #7 id:140001) has been created in
https://codereview.chromium.org/2632123003/ by pkalinnikov@chromium.org.

The reason for reverting is: The
"ProfileSigninConfirmationDialogTest.InvokeDialog_default" test is failing on
"Win10 Tests x64" builder..