Clean up LoginUIService. Remove Singleton behavior from PeopleHandler

Clean up LoginUIService. Remove Singleton behavior from PeopleHandler

The old SyncSetupHandler used in Old Options should retain its Singleton behavior.

BUG=563721
Committed: https://crrev.com/86fed6eaf1ec5922b03282b0e99d91c5b87ea1da
Cr-Commit-Position: refs/heads/master@{#401357}

[tommycli, 2016-05-16 22:00:30]

Description was changed from

==========
Settings People Revamp: Remove Singleton-dialog logic from PeopleHandler

BUG=563721
==========

to

==========
LoginUIService: Remove unused methods. Allow multiple SetLoginUI calls

BUG=563721
==========

[tommycli, 2016-05-16 22:08:22]

Description was changed from

==========
LoginUIService: Remove unused methods. Allow multiple SetLoginUI calls

BUG=563721
==========

to

==========
Clean up LoginUIService. Remove Singleton behavior from PeopleHandler

The old SyncSetupHandler used in Old Options should retain its Singleton
behavior.

BUG=563721
==========

[tommycli, 2016-05-16 22:24:16]

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

[commit-bot: I haz the power, 2016-05-16 22:24:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984863002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984863002/40001

[commit-bot: I haz the power, 2016-05-16 23:06:29]

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

[commit-bot: I haz the power, 2016-05-16 23:06:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[tommycli, 2016-05-17 00:39:17]

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

[commit-bot: I haz the power, 2016-05-17 00:39:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984863002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984863002/60001

[commit-bot: I haz the power, 2016-05-17 02:09:00]

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

[commit-bot: I haz the power, 2016-05-17 02:09:01]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-05-17 16:11:41]

tommycli@chromium.org changed reviewers:
+ rogerta@chromium.org, zea@chromium.org

[tommycli, 2016-05-17 16:11:41]

zea, rogerta: PTAL, thanks!

This removes the One True Sync Dialog behavior we discussed.

It leaves the old Options one alone, but affects the new MD Settings subpage.

[tommycli, 2016-05-17 18:24:14]

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

[commit-bot: I haz the power, 2016-05-17 18:24:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984863002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984863002/80001

[Nicolas Zea, 2016-05-17 20:00:55]

Just reviewing sync/

https://codereview.chromium.org/1984863002/diff/80001/components/browser_sync...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/1984863002/diff/80001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.cc:1708: NotifyObservers();
Is this necessary? Configuration should notify the observer anyways.

[commit-bot: I haz the power, 2016-05-17 20:01:10]

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

[commit-bot: I haz the power, 2016-05-17 20:01:11]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-05-17 23:23:22]

zea: thanks for the fast review. My response below. :)

https://codereview.chromium.org/1984863002/diff/80001/components/browser_sync...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/1984863002/diff/80001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.cc:1708: NotifyObservers();
On 2016/05/17 20:00:55, Nicolas Zea wrote:
> Is this necessary? Configuration should notify the observer anyways.

Hi, I added this because OnUserChoseDatatypes wasn't notifying the observers
previously.

I looked through the code for the ReconfigureDatatypeManager and it also didn't
look like there was any path that notified the observers. (Unless there was an
unrecoverable error)

[Nicolas Zea, 2016-05-18 00:29:17]

On 2016/05/17 23:23:22, tommycli wrote:
> zea: thanks for the fast review. My response below. :)
> 
>
https://codereview.chromium.org/1984863002/diff/80001/components/browser_sync...
> File components/browser_sync/browser/profile_sync_service.cc (right):
> 
>
https://codereview.chromium.org/1984863002/diff/80001/components/browser_sync...
> components/browser_sync/browser/profile_sync_service.cc:1708:
NotifyObservers();
> On 2016/05/17 20:00:55, Nicolas Zea wrote:
> > Is this necessary? Configuration should notify the observer anyways.
> 
> Hi, I added this because OnUserChoseDatatypes wasn't notifying the observers
> previously.
> 
> I looked through the code for the ReconfigureDatatypeManager and it also
didn't
> look like there was any path that notified the observers. (Unless there was an
> unrecoverable error)

It should actually happen in the OnConfigureDone step (which is triggered by the
DataTypeManager). It's possible that there are some cases where we short circuit
and skip the DTM.

[tommycli, 2016-05-18 20:21:30]

On 2016/05/18 00:29:17, Nicolas Zea wrote:
> On 2016/05/17 23:23:22, tommycli wrote:
> > zea: thanks for the fast review. My response below. :)
> > 
> >
>
https://codereview.chromium.org/1984863002/diff/80001/components/browser_sync...
> > File components/browser_sync/browser/profile_sync_service.cc (right):
> > 
> >
>
https://codereview.chromium.org/1984863002/diff/80001/components/browser_sync...
> > components/browser_sync/browser/profile_sync_service.cc:1708:
> NotifyObservers();
> > On 2016/05/17 20:00:55, Nicolas Zea wrote:
> > > Is this necessary? Configuration should notify the observer anyways.
> > 
> > Hi, I added this because OnUserChoseDatatypes wasn't notifying the observers
> > previously.
> > 
> > I looked through the code for the ReconfigureDatatypeManager and it also
> didn't
> > look like there was any path that notified the observers. (Unless there was
an
> > unrecoverable error)
> 
> It should actually happen in the OnConfigureDone step (which is triggered by
the
> DataTypeManager). It's possible that there are some cases where we short
circuit
> and skip the DTM.

Ah that makes sense. It wasn't getting to OnConfigureDone because opening the
Sync subpage puts the Service into SetSetupInProgress(true).

I moved the NotfiyObservers to a better location.

I assume opening the subpage should still put the service into setup-mode right?

Tommy

[Nicolas Zea, 2016-05-19 22:23:12]

On 2016/05/18 20:21:30, tommycli wrote:
> On 2016/05/18 00:29:17, Nicolas Zea wrote:
> > On 2016/05/17 23:23:22, tommycli wrote:
> > > zea: thanks for the fast review. My response below. :)
> > > 
> > >
> >
>
https://codereview.chromium.org/1984863002/diff/80001/components/browser_sync...
> > > File components/browser_sync/browser/profile_sync_service.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1984863002/diff/80001/components/browser_sync...
> > > components/browser_sync/browser/profile_sync_service.cc:1708:
> > NotifyObservers();
> > > On 2016/05/17 20:00:55, Nicolas Zea wrote:
> > > > Is this necessary? Configuration should notify the observer anyways.
> > > 
> > > Hi, I added this because OnUserChoseDatatypes wasn't notifying the
observers
> > > previously.
> > > 
> > > I looked through the code for the ReconfigureDatatypeManager and it also
> > didn't
> > > look like there was any path that notified the observers. (Unless there
was
> an
> > > unrecoverable error)
> > 
> > It should actually happen in the OnConfigureDone step (which is triggered by
> the
> > DataTypeManager). It's possible that there are some cases where we short
> circuit
> > and skip the DTM.
> 
> Ah that makes sense. It wasn't getting to OnConfigureDone because opening the
> Sync subpage puts the Service into SetSetupInProgress(true).
> 
> I moved the NotfiyObservers to a better location.
> 
> I assume opening the subpage should still put the service into setup-mode
right?
> 
> Tommy

Right, it should, although that raises an interesting point about behavior. If
you have two pages open, and trigger configuration in one, will that toggle the
setup in progress big? Will that then confuse the logic on the first page if you
try to configure differently there?

[tommycli, 2016-05-20 00:02:31]

On 2016/05/19 at 22:23:12, zea wrote:
> On 2016/05/18 20:21:30, tommycli wrote:
> > On 2016/05/18 00:29:17, Nicolas Zea wrote:
> > > On 2016/05/17 23:23:22, tommycli wrote:
> > > > zea: thanks for the fast review. My response below. :)
> > > > 
> > > >
> > >
> >
https://codereview.chromium.org/1984863002/diff/80001/components/browser_sync...
> > > > File components/browser_sync/browser/profile_sync_service.cc (right):
> > > > 
> > > >
> > >
> >
https://codereview.chromium.org/1984863002/diff/80001/components/browser_sync...
> > > > components/browser_sync/browser/profile_sync_service.cc:1708:
> > > NotifyObservers();
> > > > On 2016/05/17 20:00:55, Nicolas Zea wrote:
> > > > > Is this necessary? Configuration should notify the observer anyways.
> > > > 
> > > > Hi, I added this because OnUserChoseDatatypes wasn't notifying the
observers
> > > > previously.
> > > > 
> > > > I looked through the code for the ReconfigureDatatypeManager and it also
> > > didn't
> > > > look like there was any path that notified the observers. (Unless there
was
> > an
> > > > unrecoverable error)
> > > 
> > > It should actually happen in the OnConfigureDone step (which is triggered
by
> > the
> > > DataTypeManager). It's possible that there are some cases where we short
> > circuit
> > > and skip the DTM.
> > 
> > Ah that makes sense. It wasn't getting to OnConfigureDone because opening
the
> > Sync subpage puts the Service into SetSetupInProgress(true).
> > 
> > I moved the NotfiyObservers to a better location.
> > 
> > I assume opening the subpage should still put the service into setup-mode
right?
> > 
> > Tommy
> 
> Right, it should, although that raises an interesting point about behavior. If
you have two pages open, and trigger configuration in one, will that toggle the
setup in progress big? Will that then confuse the logic on the first page if you
try to configure differently there?

Arg, well it seems like the SetupInProgress business assumes that there's only
one possible subpage configuring it - which is why it's a simple boolean.

Would it make sense for SetupInProgress to instead be a counter to allow for
multiple subpages to be open?

[Nicolas Zea, 2016-05-20 00:13:42]

On 2016/05/20 00:02:31, tommycli wrote:
> On 2016/05/19 at 22:23:12, zea wrote:
> > On 2016/05/18 20:21:30, tommycli wrote:
> > > On 2016/05/18 00:29:17, Nicolas Zea wrote:
> > > > On 2016/05/17 23:23:22, tommycli wrote:
> > > > > zea: thanks for the fast review. My response below. :)
> > > > > 
> > > > >
> > > >
> > >
>
https://codereview.chromium.org/1984863002/diff/80001/components/browser_sync...
> > > > > File components/browser_sync/browser/profile_sync_service.cc (right):
> > > > > 
> > > > >
> > > >
> > >
>
https://codereview.chromium.org/1984863002/diff/80001/components/browser_sync...
> > > > > components/browser_sync/browser/profile_sync_service.cc:1708:
> > > > NotifyObservers();
> > > > > On 2016/05/17 20:00:55, Nicolas Zea wrote:
> > > > > > Is this necessary? Configuration should notify the observer anyways.
> > > > > 
> > > > > Hi, I added this because OnUserChoseDatatypes wasn't notifying the
> observers
> > > > > previously.
> > > > > 
> > > > > I looked through the code for the ReconfigureDatatypeManager and it
also
> > > > didn't
> > > > > look like there was any path that notified the observers. (Unless
there
> was
> > > an
> > > > > unrecoverable error)
> > > > 
> > > > It should actually happen in the OnConfigureDone step (which is
triggered
> by
> > > the
> > > > DataTypeManager). It's possible that there are some cases where we short
> > > circuit
> > > > and skip the DTM.
> > > 
> > > Ah that makes sense. It wasn't getting to OnConfigureDone because opening
> the
> > > Sync subpage puts the Service into SetSetupInProgress(true).
> > > 
> > > I moved the NotfiyObservers to a better location.
> > > 
> > > I assume opening the subpage should still put the service into setup-mode
> right?
> > > 
> > > Tommy
> > 
> > Right, it should, although that raises an interesting point about behavior.
If
> you have two pages open, and trigger configuration in one, will that toggle
the
> setup in progress big? Will that then confuse the logic on the first page if
you
> try to configure differently there?
> 
> Arg, well it seems like the SetupInProgress business assumes that there's only
> one possible subpage configuring it - which is why it's a simple boolean.
> 
> Would it make sense for SetupInProgress to instead be a counter to allow for
> multiple subpages to be open?

That sounds reasonable. I'm slightly concerned that there may be places in the
code though that call SetSetupInProgress more than once (unnecessarily). We
should probably make sure with DCHECKs and comments that the number of times
it's set to true and false match exactly at shutdown time.

[tommycli, 2016-06-13 22:02:58]

zea: This is the followup to the other Sync refactor. PTAL. Thanks!

[Nicolas Zea, 2016-06-20 07:14:54]

sync LGTM (sorry for the slow response!)

[Roger Tawa OOO till Jul 10th, 2016-06-20 13:50:44]

rogerta@chromium.org changed reviewers:
+ anthonyvd@chromium.org

[Roger Tawa OOO till Jul 10th, 2016-06-20 13:50:45]

Clean up looks good.  With then new sign in flow that always uses native UI to
confirm sign in, I don't think we we need the untrusted sign code path.  Anthony
can confirm/deny.

I'm not sure about removal of the loginui singleton behaviour.  Having multiple
concurrent loginuis is confusing, although the underlying sign in code will
handle it gracefully.  Under what circumstances will a user have multiple
concurrent loginuis?

[anthonyvd, 2016-06-20 13:58:57]

On 2016/06/20 at 13:50:45, rogerta wrote:
> Clean up looks good.  With then new sign in flow that always uses native UI to
confirm sign in, I don't think we we need the untrusted sign code path.  Anthony
can confirm/deny.

That's correct and we'll be at 100% everywhere for the new flow by the time this
change reaches users.

> 
> I'm not sure about removal of the loginui singleton behaviour.  Having
multiple concurrent loginuis is confusing, although the underlying sign in code
will handle it gracefully.  Under what circumstances will a user have multiple
concurrent loginuis?

[tommycli, 2016-06-21 19:10:04]

On 2016/06/20 13:58:57, anthonyvd wrote:
> On 2016/06/20 at 13:50:45, rogerta wrote:
> > Clean up looks good.  With then new sign in flow that always uses native UI
to
> confirm sign in, I don't think we we need the untrusted sign code path. 
Anthony
> can confirm/deny.
> 
> That's correct and we'll be at 100% everywhere for the new flow by the time
this
> change reaches users.
> 
> > 
> > I'm not sure about removal of the loginui singleton behaviour.  Having
> multiple concurrent loginuis is confusing, although the underlying sign in
code
> will handle it gracefully.  Under what circumstances will a user have multiple
> concurrent loginuis?

rogerta:

The LoginUIService naming is kind of misleading. Looking at the implementing
subclasses of LoginUI[1], the only implementors are the SyncSetupHandler and
PeopleHandler under Settings.

These two UI surfaces are for customizing Advanced Sync Settings, i.e. what
datatypes to sync.

There probably is some mechanism to prevent multiple Logins from displaying at
once, but LoginUIService::SetLoginUI isn't it. It just prevents multiple
Advanced Sync Settings from displaying at once.

Tommy

[1]
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/signin/login_ui_...

[tommycli, 2016-06-21 19:14:37]

On 2016/06/21 19:10:04, tommycli wrote:
> On 2016/06/20 13:58:57, anthonyvd wrote:
> > On 2016/06/20 at 13:50:45, rogerta wrote:
> > > Clean up looks good.  With then new sign in flow that always uses native
UI
> to
> > confirm sign in, I don't think we we need the untrusted sign code path. 
> Anthony
> > can confirm/deny.
> > 
> > That's correct and we'll be at 100% everywhere for the new flow by the time
> this
> > change reaches users.
> > 
> > > 
> > > I'm not sure about removal of the loginui singleton behaviour.  Having
> > multiple concurrent loginuis is confusing, although the underlying sign in
> code
> > will handle it gracefully.  Under what circumstances will a user have
multiple
> > concurrent loginuis?
> 
> rogerta:
> 
> The LoginUIService naming is kind of misleading. Looking at the implementing
> subclasses of LoginUI[1], the only implementors are the SyncSetupHandler and
> PeopleHandler under Settings.
> 
> These two UI surfaces are for customizing Advanced Sync Settings, i.e. what
> datatypes to sync.
> 
> There probably is some mechanism to prevent multiple Logins from displaying at
> once, but LoginUIService::SetLoginUI isn't it. It just prevents multiple
> Advanced Sync Settings from displaying at once.
> 
> Tommy
> 
> [1]
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/signin/login_ui_...

I just manually tested, and this patch doesn't break the one-login-dialog
behavior.

Tommy

[tommycli, 2016-06-22 17:47:16]

rogerta: ping, thanks!

[Roger Tawa OOO till Jul 10th, 2016-06-22 17:59:48]

On 2016/06/22 17:47:16, tommycli wrote:
> rogerta: ping, thanks!

Thanks for checking Tommy.

lgtm, but I think I'll talk to the chrome identity team, there are callers of
this class that no longer make sense if this is tied to sync settings only.

[tommycli, 2016-06-22 18:01:58]

On 2016/06/22 17:59:48, Roger Tawa wrote:
> On 2016/06/22 17:47:16, tommycli wrote:
> > rogerta: ping, thanks!
> 
> Thanks for checking Tommy.
> 
> lgtm, but I think I'll talk to the chrome identity team, there are callers of
> this class that no longer make sense if this is tied to sync settings only.

Thanks! Should I hold off on sending this patch to CQ until you are done talking
to them?

[Roger Tawa OOO till Jul 10th, 2016-06-22 18:18:31]

On 2016/06/22 18:01:58, tommycli wrote:
> On 2016/06/22 17:59:48, Roger Tawa wrote:
> > On 2016/06/22 17:47:16, tommycli wrote:
> > > rogerta: ping, thanks!
> > 
> > Thanks for checking Tommy.
> > 
> > lgtm, but I think I'll talk to the chrome identity team, there are callers
of
> > this class that no longer make sense if this is tied to sync settings only.
> 
> Thanks! Should I hold off on sending this patch to CQ until you are done
talking
> to them?

No, go ahead.  I opened a bug to track this, I cc'ed you.

[tommycli, 2016-06-22 18:18:49]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-06-22 18:18:49]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1984863002/#ps200001
(title: "fix merge issue")

[commit-bot: I haz the power, 2016-06-22 18:19:38]

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

[commit-bot: I haz the power, 2016-06-22 18:25:45]

Description was changed from

==========
Clean up LoginUIService. Remove Singleton behavior from PeopleHandler

The old SyncSetupHandler used in Old Options should retain its Singleton
behavior.

BUG=563721
==========

to

==========
Clean up LoginUIService. Remove Singleton behavior from PeopleHandler

The old SyncSetupHandler used in Old Options should retain its Singleton
behavior.

BUG=563721
==========

[commit-bot: I haz the power, 2016-06-22 18:25:47]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-06-22 18:29:39]

Description was changed from

==========
Clean up LoginUIService. Remove Singleton behavior from PeopleHandler

The old SyncSetupHandler used in Old Options should retain its Singleton
behavior.

BUG=563721
==========

to

==========
Clean up LoginUIService. Remove Singleton behavior from PeopleHandler

The old SyncSetupHandler used in Old Options should retain its Singleton
behavior.

BUG=563721

Committed: https://crrev.com/86fed6eaf1ec5922b03282b0e99d91c5b87ea1da
Cr-Commit-Position: refs/heads/master@{#401357}
==========

[commit-bot: I haz the power, 2016-06-22 18:29:40]

Patchset 11 (id:??) landed as
https://crrev.com/86fed6eaf1ec5922b03282b0e99d91c5b87ea1da
Cr-Commit-Position: refs/heads/master@{#401357}