Using native sheet to display modal dialogs for sign in

Using native sheet to display modal dialogs for sign in

BUG=677012
Review-Url: https://codereview.chromium.org/2617583006
Cr-Commit-Position: refs/heads/master@{#443241}
Committed: https://chromium.googlesource.com/chromium/src/+/972a4d1ae26576f2a0cee1066a6ded6616538641

[jlebel, 2017-01-06 12:30:01]

Patchset #2 (id:20001) has been deleted

[jlebel, 2017-01-06 12:30:08]

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

[commit-bot: I haz the power, 2017-01-06 12:30:19]

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

[jlebel, 2017-01-06 23:44:20]

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

[commit-bot: I haz the power, 2017-01-06 23:44:38]

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

[jlebel, 2017-01-06 23:55:42]

Hello Mihai,

Can you review my patch?

Thanks,

[jlebel, 2017-01-07 00:01:44]

jlebel@chromium.org changed reviewers:
+ msarda@chromium.org

[jlebel, 2017-01-07 00:01:45]

Hello msarda,

Can you review my patch?

Thanks,

[commit-bot: I haz the power, 2017-01-07 00:39:50]

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

[commit-bot: I haz the power, 2017-01-07 00:39:50]

Dry run: This issue passed the CQ dry run.

[msarda, 2017-01-10 14:16:13]

https://codereview.chromium.org/2617583006/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/signin_view_controller_delegate_mac.mm
(right):

https://codereview.chromium.org/2617583006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/signin_view_controller_delegate_mac.mm:145:
ResetSigninViewControllerDelegate();
I think it would be better to use a DeleteSoon here to avoid crashes if the code
calling PerformClose uses the object after this is called.

See:
https://cs.chromium.org/chromium/src/base/sequenced_task_runner.h?rcl=0&l=123
and
https://cs.chromium.org/chromium/src/ash/common/system/tray/system_tray_bubbl...

[jlebel, 2017-01-10 14:42:01]

Patchset #3 (id:60001) has been deleted

[jlebel, 2017-01-10 14:47:25]

Patchset #3 (id:80001) has been deleted

[jlebel, 2017-01-10 15:13:00]

Patchset #3 (id:100001) has been deleted

[jlebel, 2017-01-10 15:43:35]

https://codereview.chromium.org/2617583006/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/signin_view_controller_delegate_mac.mm
(right):

https://codereview.chromium.org/2617583006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/signin_view_controller_delegate_mac.mm:145:
ResetSigninViewControllerDelegate();
On 2017/01/10 14:16:13, msarda wrote:
> I think it would be better to use a DeleteSoon here to avoid crashes if the
code
> calling PerformClose uses the object after this is called.
> 
> See:
> https://cs.chromium.org/chromium/src/base/sequenced_task_runner.h?rcl=0&l=123
> and
>
https://cs.chromium.org/chromium/src/ash/common/system/tray/system_tray_bubbl...

As we talked offline, this would require to move the destructor method from
private to public. Plus it is better to be more restrictive by adding a comment
saying that as soon as PerformClose() is called, the current instance should be
considered as destroyed.

I've added some comments in signin_view_controller_delegate.h.

[msarda, 2017-01-10 15:58:44]

LGTM with a comment nit.

https://codereview.chromium.org/2617583006/diff/120001/chrome/browser/ui/sign...
File chrome/browser/ui/signin_view_controller_delegate.h (right):

https://codereview.chromium.org/2617583006/diff/120001/chrome/browser/ui/sign...
chrome/browser/ui/signin_view_controller_delegate.h:51: void CloseModalSignin();
There is a typo in this comment: "the instance should be considered has being
destroyed."

I would change this comment and the ones below as the following:
"Closes the sign-in dialog. Note that this method may destroy this object,
so the caller should no longer use this object after calling this method."

[jlebel, 2017-01-11 13:00:12]

Patchset #4 (id:140001) has been deleted

[jlebel, 2017-01-11 14:25:24]

Thanks

https://codereview.chromium.org/2617583006/diff/120001/chrome/browser/ui/sign...
File chrome/browser/ui/signin_view_controller_delegate.h (right):

https://codereview.chromium.org/2617583006/diff/120001/chrome/browser/ui/sign...
chrome/browser/ui/signin_view_controller_delegate.h:51: void CloseModalSignin();
On 2017/01/10 15:58:43, msarda wrote:
> There is a typo in this comment: "the instance should be considered has being
> destroyed."
> 
> I would change this comment and the ones below as the following:
> "Closes the sign-in dialog. Note that this method may destroy this object,
> so the caller should no longer use this object after calling this method."

Done.

[msarda, 2017-01-11 14:29:21]

lgtm

https://codereview.chromium.org/2617583006/diff/160001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/profiles/signin_view_controller_delegate_mac.h
(right):

https://codereview.chromium.org/2617583006/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/profiles/signin_view_controller_delegate_mac.h:47:
ui::ModalType dialog_modal_type,
For consistency with the views implementation, please make the constructor a
private method and make SigninViewControllerDelegate a friend - see
https://codereview.chromium.org/2619963003/diff/40001/chrome/browser/ui/views...

[jlebel, 2017-01-11 14:39:52]

Thanks

https://codereview.chromium.org/2617583006/diff/160001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/profiles/signin_view_controller_delegate_mac.h
(right):

https://codereview.chromium.org/2617583006/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/profiles/signin_view_controller_delegate_mac.h:47:
ui::ModalType dialog_modal_type,
On 2017/01/11 14:29:21, msarda wrote:
> For consistency with the views implementation, please make the constructor a
> private method and make SigninViewControllerDelegate a friend - see
>
https://codereview.chromium.org/2619963003/diff/40001/chrome/browser/ui/views...

Done.

[jlebel, 2017-01-11 14:41:21]

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

[jlebel, 2017-01-11 14:41:21]

Hello Peter,

Can you review my patch? That's the macOS version of crrev.com/2619963003

Thanks

[Peter Kasting, 2017-01-11 19:38:29]

LGTM

https://codereview.chromium.org/2617583006/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/profiles/signin_view_controller_delegate_mac.h
(right):

https://codereview.chromium.org/2617583006/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/profiles/signin_view_controller_delegate_mac.h:81: void
DeleteThis();
Nit: I wonder if this should have a different name/description since it does
more cleanup than just deleting the object.

[jlebel, 2017-01-12 09:51:26]

Thanks

https://codereview.chromium.org/2617583006/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/profiles/signin_view_controller_delegate_mac.h
(right):

https://codereview.chromium.org/2617583006/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/profiles/signin_view_controller_delegate_mac.h:81: void
DeleteThis();
On 2017/01/11 19:38:29, Peter Kasting wrote:
> Nit: I wonder if this should have a different name/description since it does
> more cleanup than just deleting the object.

Done.

[jlebel, 2017-01-12 09:51:34]

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

[commit-bot: I haz the power, 2017-01-12 09:51:41]

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-01-12 09:53:41]

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

[commit-bot: I haz the power, 2017-01-12 09:53:42]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[jlebel, 2017-01-12 11:49:30]

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

[commit-bot: I haz the power, 2017-01-12 11:49:43]

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-01-12 11:51:37]

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

[commit-bot: I haz the power, 2017-01-12 11:51:38]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[jlebel, 2017-01-12 12:48:14]

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

[commit-bot: I haz the power, 2017-01-12 12:48:27]

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-01-12 12:50:10]

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

[commit-bot: I haz the power, 2017-01-12 12:50:11]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jlebel, 2017-01-12 13:52:11]

Patchset #7 (id:220001) has been deleted

[jlebel, 2017-01-12 14:43:18]

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

[commit-bot: I haz the power, 2017-01-12 14:43:44]

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-01-12 15:40:41]

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

[commit-bot: I haz the power, 2017-01-12 15:40:42]

Dry run: This issue passed the CQ dry run.

[jlebel, 2017-01-12 15:45:50]

The CQ bit was checked by jlebel@chromium.org

[jlebel, 2017-01-12 15:45:51]

The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2617583006/#ps240001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-12 15:46:08]

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-12 15:51:44]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1484235950947000,
"parent_rev": "b103d6c66bbd7bebfe22affab2ea4930685a55bc", "commit_rev":
"972a4d1ae26576f2a0cee1066a6ded6616538641"}

[commit-bot: I haz the power, 2017-01-12 15:52:19]

Description was changed from

==========
Using native sheet to display modal dialogs for sign in

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

to

==========
Using native sheet to display modal dialogs for sign in

BUG=677012

Review-Url: https://codereview.chromium.org/2617583006
Cr-Commit-Position: refs/heads/master@{#443241}
Committed:
https://chromium.googlesource.com/chromium/src/+/972a4d1ae26576f2a0cee1066a6d...
==========

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

Committed patchset #7 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/972a4d1ae26576f2a0cee1066a6d...