Change the views sign-in dialog to be tab modal.

Change the views sign-in dialog to be tab modal.

This CL changes the views sign-in dialod to be tab modal and keeps the
views sync confirmation dialog and the sign-in erorr dialogs as window modal.

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

[msarda, 2017-01-10 11:58:26]

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

[msarda, 2017-01-10 11:58:26]

Please take a look.

[jlebel, 2017-01-10 13:20:18]

lgtm

LGTM

https://codereview.chromium.org/2619963003/diff/1/chrome/browser/ui/views/pro...
File chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.h
(right):

https://codereview.chromium.org/2619963003/diff/1/chrome/browser/ui/views/pro...
chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.h:83: 
 I would remove the empty line.

https://codereview.chromium.org/2619963003/diff/1/chrome/browser/ui/views/pro...
chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.h:86: 
Same here.

[msarda, 2017-01-10 17:32:27]

https://codereview.chromium.org/2619963003/diff/1/chrome/browser/ui/views/pro...
File chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.h
(right):

https://codereview.chromium.org/2619963003/diff/1/chrome/browser/ui/views/pro...
chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.h:83: 
On 2017/01/10 13:20:18, jlebel wrote:
>  I would remove the empty line.

We prefer having a blank line if the comment starts at the same indentation as
the previous line (this is for code review tool).

https://codereview.chromium.org/2619963003/diff/1/chrome/browser/ui/views/pro...
chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.h:86: 
On 2017/01/10 13:20:18, jlebel wrote:
> Same here.

Ditto.

[msarda, 2017-01-10 17:32:46]

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

[msarda, 2017-01-10 17:32:46]

pkasting@chromium.org: Please do an owners review of this CL.

[Peter Kasting, 2017-01-11 01:24:45]

LGTM

https://codereview.chromium.org/2619963003/diff/1/chrome/browser/ui/signin_vi...
File chrome/browser/ui/signin_view_controller_delegate.h (right):

https://codereview.chromium.org/2619963003/diff/1/chrome/browser/ui/signin_vi...
chrome/browser/ui/signin_view_controller_delegate.h:76: // with the parent
window an
Nit: Comment cut off

https://codereview.chromium.org/2619963003/diff/1/chrome/browser/ui/views/pro...
File chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc
(right):

https://codereview.chromium.org/2619963003/diff/1/chrome/browser/ui/views/pro...
chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc:87:
return ui::MODAL_TYPE_WINDOW;
Why not just use these values directly everywhere, instead of introducing a new
enum for this class?  MODAL_TYPE_CHILD/WINDOW still seem pretty clear to me.

That would make this function simple, eliminate the enum declaration in the
header, and not add any meaningful complication elsewhere.

https://codereview.chromium.org/2619963003/diff/1/chrome/browser/ui/views/pro...
File chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.h
(right):

https://codereview.chromium.org/2619963003/diff/1/chrome/browser/ui/views/pro...
chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.h:84: //
The dialog modal presentation type.
Nit: I agree with your rationale on adding blank lines due to comments -- but I
think this comment adds little to the code, so I'd just remove it (and then
remove the blank lines above and below.

[msarda, 2017-01-11 09:54:25]

https://codereview.chromium.org/2619963003/diff/1/chrome/browser/ui/signin_vi...
File chrome/browser/ui/signin_view_controller_delegate.h (right):

https://codereview.chromium.org/2619963003/diff/1/chrome/browser/ui/signin_vi...
chrome/browser/ui/signin_view_controller_delegate.h:76: // with the parent
window an
On 2017/01/11 01:24:45, Peter Kasting wrote:
> Nit: Comment cut off

Removed this enum.

https://codereview.chromium.org/2619963003/diff/1/chrome/browser/ui/views/pro...
File chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc
(right):

https://codereview.chromium.org/2619963003/diff/1/chrome/browser/ui/views/pro...
chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc:87:
return ui::MODAL_TYPE_WINDOW;
On 2017/01/11 01:24:45, Peter Kasting wrote:
> Why not just use these values directly everywhere, instead of introducing a
new
> enum for this class?  MODAL_TYPE_CHILD/WINDOW still seem pretty clear to me.
> 
> That would make this function simple, eliminate the enum declaration in the
> header, and not add any meaningful complication elsewhere.

I hesitated to do that because we only support 2 values of this enum. But I
agree it does simplify the code without any risks as the constructor is only
used in from the methods internal to this class.

Done.

https://codereview.chromium.org/2619963003/diff/1/chrome/browser/ui/views/pro...
File chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.h
(right):

https://codereview.chromium.org/2619963003/diff/1/chrome/browser/ui/views/pro...
chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.h:84: //
The dialog modal presentation type.
On 2017/01/11 01:24:45, Peter Kasting wrote:
> Nit: I agree with your rationale on adding blank lines due to comments -- but
I
> think this comment adds little to the code, so I'd just remove it (and then
> remove the blank lines above and below.

Done.

[msarda, 2017-01-11 10:07:21]

The CQ bit was checked by msarda@chromium.org

[msarda, 2017-01-11 10:07:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, jlebel@chromium.org
Link to the patchset: https://codereview.chromium.org/2619963003/#ps20001
(title: "Remove enum")

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

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-11 10:20:48]

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

[commit-bot: I haz the power, 2017-01-11 10:20:49]

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

[msarda, 2017-01-11 10:33:49]

The CQ bit was checked by msarda@chromium.org

[msarda, 2017-01-11 10:33:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, jlebel@chromium.org
Link to the patchset: https://codereview.chromium.org/2619963003/#ps40001
(title: "Fix field order")

[commit-bot: I haz the power, 2017-01-11 10:34:01]

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

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484130829558450,
"parent_rev": "8b090304788061c3bb4b2d4ca58d0461c01c646c", "commit_rev":
"c713c645abfea6684e7ed83d48ee7c334b3979dd"}

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

Description was changed from

==========
Change the views sign-in dialog to be tab modal.

This CL changes the views sign-in dialod to be tab modal and keeps the
views sync confirmation dialog and the sign-in erorr dialogs as window modal.

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

to

==========
Change the views sign-in dialog to be tab modal.

This CL changes the views sign-in dialod to be tab modal and keeps the
views sync confirmation dialog and the sign-in erorr dialogs as window modal.

BUG=677012

Review-Url: https://codereview.chromium.org/2619963003
Cr-Commit-Position: refs/heads/master@{#442875}
Committed:
https://chromium.googlesource.com/chromium/src/+/c713c645abfea6684e7ed83d48ee...
==========

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

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/c713c645abfea6684e7ed83d48ee...