Settings Rewrite: Fix Opening Sync User login

Settings Rewrite: Fix Opening Sync User login

Hijacked from https://codereview.chromium.org/1607833004/.

dschuyler reported the problem and suggested a patch.

BUG=NONE

Committed: https://crrev.com/bbeb329a82b8b7e50b1078752d6fd429e8f622e7
Cr-Commit-Position: refs/heads/master@{#374718}

[tommycli, 2016-01-20 01:57:19]

tommycli@chromium.org changed reviewers:
+ dschuyler@chromium.org

[tommycli, 2016-01-20 01:57:20]

dschuyler: PTAL. Sorry to hijack your patch.

During the course of investigating your patch, I ended up figuring out how to
fix it... so... yeah sorry.

Tommy

[dschuyler, 2016-01-20 02:16:21]

This lgtm.

I made a suggestion in two places.

Be sure to get an owner review for these files :)

https://codereview.chromium.org/1608103002/diff/1/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/sync_setup_handler.cc (right):

https://codereview.chromium.org/1608103002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/sync_setup_handler.cc:671:
creating_supervised_user = access_point == "access-point-supervised-user";
(optional) What do you think of:

std::string access_point;
bool creating_supervised_user = args->GetString(0, &access_point) &&
    access_point == "access-point-supervised-user";

https://codereview.chromium.org/1608103002/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/people_handler.cc (right):

https://codereview.chromium.org/1608103002/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/people_handler.cc:623: creating_supervised_user
= access_point == "access-point-supervised-user";
(optional) What do you think of:

std::string access_point;
bool creating_supervised_user = args->GetString(0, &access_point) &&
    access_point == "access-point-supervised-user";

[tommycli, 2016-01-20 19:17:12]

tommycli@chromium.org changed reviewers:
+ gogerald@chromium.org

[tommycli, 2016-01-20 19:17:12]

gogerald: PTAL, this fixes a crash in debug mode introduced by
https://chromium.googlesource.com/chromium/src/+/71bf6c90880518262efc373771eb...

I just wanted to make sure this didn't break your metrics collection.

Thanks!

https://codereview.chromium.org/1608103002/diff/1/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/sync_setup_handler.cc (right):

https://codereview.chromium.org/1608103002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/sync_setup_handler.cc:671:
creating_supervised_user = access_point == "access-point-supervised-user";
On 2016/01/20 02:16:21, dschuyler wrote:
> (optional) What do you think of:
> 
> std::string access_point;
> bool creating_supervised_user = args->GetString(0, &access_point) &&
>     access_point == "access-point-supervised-user";

Done. Yes that def better.

https://codereview.chromium.org/1608103002/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/people_handler.cc (right):

https://codereview.chromium.org/1608103002/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/people_handler.cc:623: creating_supervised_user
= access_point == "access-point-supervised-user";
On 2016/01/20 02:16:21, dschuyler wrote:
> (optional) What do you think of:
> 
> std::string access_point;
> bool creating_supervised_user = args->GetString(0, &access_point) &&
>     access_point == "access-point-supervised-user";
> 

Done.

[gogerald1, 2016-01-20 20:30:01]

On 2016/01/20 19:17:12, tommycli wrote:
> gogerald: PTAL, this fixes a crash in debug mode introduced by
>
https://chromium.googlesource.com/chromium/src/+/71bf6c90880518262efc373771eb...
> 
> I just wanted to make sure this didn't break your metrics collection.
> 
> Thanks!
> 
>
https://codereview.chromium.org/1608103002/diff/1/chrome/browser/ui/webui/opt...
> File chrome/browser/ui/webui/options/sync_setup_handler.cc (right):
> 
>
https://codereview.chromium.org/1608103002/diff/1/chrome/browser/ui/webui/opt...
> chrome/browser/ui/webui/options/sync_setup_handler.cc:671:
> creating_supervised_user = access_point == "access-point-supervised-user";
> On 2016/01/20 02:16:21, dschuyler wrote:
> > (optional) What do you think of:
> > 
> > std::string access_point;
> > bool creating_supervised_user = args->GetString(0, &access_point) &&
> >     access_point == "access-point-supervised-user";
> 
> Done. Yes that def better.
> 
>
https://codereview.chromium.org/1608103002/diff/1/chrome/browser/ui/webui/set...
> File chrome/browser/ui/webui/settings/people_handler.cc (right):
> 
>
https://codereview.chromium.org/1608103002/diff/1/chrome/browser/ui/webui/set...
> chrome/browser/ui/webui/settings/people_handler.cc:623:
creating_supervised_user
> = access_point == "access-point-supervised-user";
> On 2016/01/20 02:16:21, dschuyler wrote:
> > (optional) What do you think of:
> > 
> > std::string access_point;
> > bool creating_supervised_user = args->GetString(0, &access_point) &&
> >     access_point == "access-point-supervised-user";
> > 
> 
> Done.

LGTM, thanks,

[tommycli, 2016-01-21 19:15:04]

tommycli@chromium.org changed reviewers:
+ dbeam@chromium.org

[tommycli, 2016-01-21 19:15:04]

dbeam: PTAL thanks! Need owner stamp on options handlers.

[tommycli, 2016-01-22 19:25:10]

dbeam: ping thanks!

[tommycli, 2016-02-01 17:03:17]

dbeam: ping! thanks

[tommycli, 2016-02-02 18:23:21]

dbeam: ping, thanks!

[Dan Beam, 2016-02-03 00:45:35]

why can't you just make SyncSetupOverlay.startSignIn() accept a boolean instead
of a string?

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

https://codereview.chromium.org/1608103002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/sync_setup_handler.cc (right):

https://codereview.chromium.org/1608103002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/sync_setup_handler.cc:671: access_point ==
"access-point-supervised-user";
if we stick with a string, make this a constant and share it with other places

https://codereview.chromium.org/1608103002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/sync_setup_handler.cc:773: :
signin_metrics::AccessPoint::ACCESS_POINT_SETTINGS);
i think ternaries should be like this:

  creating_supervised_user ?
      signin_metrics::AccessPoint::ACCESS_POINT_SUPERVISED_USER :
      signin_metrics::AccessPoint::ACCESS_POINT_SETTINGS);

[tommycli, 2016-02-03 01:56:22]

Patchset #4 (id:60001) has been deleted

[tommycli, 2016-02-03 01:56:28]

Patchset #4 (id:80001) has been deleted

[tommycli, 2016-02-03 02:04:21]

dbeam: I took your suggestion to make SyncSetupOverlay.startSignIn just take a
boolean. Thanks.

Tommy

https://codereview.chromium.org/1608103002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/sync_setup_handler.cc (right):

https://codereview.chromium.org/1608103002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/sync_setup_handler.cc:671: access_point ==
"access-point-supervised-user";
On 2016/02/03 00:45:35, Dan Beam wrote:
> if we stick with a string, make this a constant and share it with other places

Done. I just changed it to a bool.

https://codereview.chromium.org/1608103002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/sync_setup_handler.cc:773: :
signin_metrics::AccessPoint::ACCESS_POINT_SETTINGS);
On 2016/02/03 00:45:35, Dan Beam wrote:
> i think ternaries should be like this:
> 
>   creating_supervised_user ?
>       signin_metrics::AccessPoint::ACCESS_POINT_SUPERVISED_USER :
>       signin_metrics::AccessPoint::ACCESS_POINT_SETTINGS);

Done.

[Dan Beam, 2016-02-03 21:54:44]

lgtm

[tommycli, 2016-02-03 22:58:20]

dbeam: thank you sir

[tommycli, 2016-02-03 22:58:23]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-02-03 22:58:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org, gogerald@chromium.org
Link to the patchset: https://codereview.chromium.org/1608103002/#ps140001
(title: " ")

[commit-bot: I haz the power, 2016-02-03 22:59:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1608103002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1608103002/140001

[commit-bot: I haz the power, 2016-02-03 23:51:29]

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

[commit-bot: I haz the power, 2016-02-03 23:51:30]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tommycli, 2016-02-10 20:19:38]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-02-10 20:19:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org, gogerald@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1608103002/#ps200001
(title: " ")

[commit-bot: I haz the power, 2016-02-10 20:20:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1608103002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1608103002/200001

[commit-bot: I haz the power, 2016-02-10 21:09:28]

Committed patchset #9 (id:200001)

[commit-bot: I haz the power, 2016-02-16 22:30:55]

Description was changed from

==========
Settings Rewrite: Fix Opening Sync User login

Hijacked from https://codereview.chromium.org/1607833004/.

dschuyler reported the problem and suggested a patch.

BUG=NONE
==========

to

==========
Settings Rewrite: Fix Opening Sync User login

Hijacked from https://codereview.chromium.org/1607833004/.

dschuyler reported the problem and suggested a patch.

BUG=NONE

Committed: https://crrev.com/bbeb329a82b8b7e50b1078752d6fd429e8f622e7
Cr-Commit-Position: refs/heads/master@{#374718}
==========

[commit-bot: I haz the power, 2016-02-16 22:30:56]

Patchset 9 (id:??) landed as
https://crrev.com/bbeb329a82b8b7e50b1078752d6fd429e8f622e7
Cr-Commit-Position: refs/heads/master@{#374718}