Settings People Revamp: Add special Disconnect logic for domain profiles

Settings People Revamp: Add special Disconnect logic for domain profiles

Disconnecting a profile with a domain, i.e. "managed by google.com"
should force the user to delete the local profile.

BUG=621223, 563721
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/3d05058f45ac24c192143f721572339af69f0acd
Cr-Commit-Position: refs/heads/master@{#403001}

[tommycli, 2016-06-27 21:19:27]

Description was changed from

==========
Settings People Revamp: Add special Disconnect logic for domain profiles

Disconnecting a profile with a domain, i.e. "managed by google.com"
should force the user to delete the local profile.

BUG=621223,563721
==========

to

==========
Settings People Revamp: Add special Disconnect logic for domain profiles

Disconnecting a profile with a domain, i.e. "managed by google.com"
should force the user to delete the local profile.

BUG=621223,563721
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[tommycli, 2016-06-27 21:22:04]

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

[commit-bot: I haz the power, 2016-06-27 21:22:49]

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

[tommycli, 2016-06-27 22:06:54]

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

[tommycli, 2016-06-27 22:06:54]

dpapad: PTAL, this implements the separate sync disconnect dialogs for
domain-managed and non-managed profiles.

The only difference is that domain-managed profiles are forced to clear the
local profile on sync disconnect.

[dpapad, 2016-06-28 22:17:22]

Apologies for the delayed review.

https://codereview.chromium.org/2102803002/diff/40001/chrome/app/settings_chr...
File chrome/app/settings_chromium_strings.grdp (right):

https://codereview.chromium.org/2102803002/diff/40001/chrome/app/settings_chr...
chrome/app/settings_chromium_strings.grdp:62: <message
name="IDS_SETTINGS_SYNC_DISCONNECT_TITLE" desc="The title of the dialog for
stopping sync and signing out.">
Nit (optional): s/stopping sync and signing out/stop syncing and sign out

https://codereview.chromium.org/2102803002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/people_page.html (right):

https://codereview.chromium.org/2102803002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.html:268:
<paper-dialog modal id="disconnectDialog">
Unrelated to this CL, but wondering if this should be cr-dialog instead.

https://codereview.chromium.org/2102803002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/sync_browser_proxy.js
(right):

https://codereview.chromium.org/2102803002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/sync_browser_proxy.js:103: *
@param {boolean} domainManagedProfile
It is a bit odd that we send information from JS to C++ about whether the
currently signed-in user is domain-managed. Doesn't C++ already have that
information?

https://codereview.chromium.org/2102803002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/people_handler.cc (right):

https://codereview.chromium.org/2102803002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/people_handler.cc:520: if
(!domain_managed_profile) {
Same question here. Why are we trusting JS for this boolean? Is it safe?

[tommycli, 2016-06-28 23:26:01]

dpapad: thanks! I tested the new changes manually. I also merged origin/master
in.

https://codereview.chromium.org/2102803002/diff/40001/chrome/app/settings_chr...
File chrome/app/settings_chromium_strings.grdp (right):

https://codereview.chromium.org/2102803002/diff/40001/chrome/app/settings_chr...
chrome/app/settings_chromium_strings.grdp:62: <message
name="IDS_SETTINGS_SYNC_DISCONNECT_TITLE" desc="The title of the dialog for
stopping sync and signing out.">
On 2016/06/28 22:17:22, dpapad wrote:
> Nit (optional): s/stopping sync and signing out/stop syncing and sign out

Done. I changed it to "The title of the dialog to stop syncing and sign out."

https://codereview.chromium.org/2102803002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/people_page.html (right):

https://codereview.chromium.org/2102803002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.html:268:
<paper-dialog modal id="disconnectDialog">
On 2016/06/28 22:17:22, dpapad wrote:
> Unrelated to this CL, but wondering if this should be cr-dialog instead.

Done. Yes, it's just old code. I changed it to use cr-dialog in this patch. If
you'd prefer it split up into a separate CL, let me know and I can revert these
changes.

https://codereview.chromium.org/2102803002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/sync_browser_proxy.js
(right):

https://codereview.chromium.org/2102803002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/sync_browser_proxy.js:103: *
@param {boolean} domainManagedProfile
On 2016/06/28 22:17:22, dpapad wrote:
> It is a bit odd that we send information from JS to C++ about whether the
> currently signed-in user is domain-managed. Doesn't C++ already have that
> information?

Done. Good point. This does subtly change the behavior from the Options, but I
think this is for the better.

https://codereview.chromium.org/2102803002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/people_handler.cc (right):

https://codereview.chromium.org/2102803002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/people_handler.cc:520: if
(!domain_managed_profile) {
On 2016/06/28 22:17:22, dpapad wrote:
> Same question here. Why are we trusting JS for this boolean? Is it safe?

Done. It was the way before just because I was copying Options.

But I agree - I think just retrieving this value on the C++ side is cleaner.

[dpapad, 2016-06-28 23:57:29]

LGTM.
Perhaps it would be valuable to add a simple test that checks that the new
checkbox is visible if all pre-conditions are met.

[tommycli, 2016-06-29 00:04:41]

On 2016/06/28 23:57:29, dpapad wrote:
> LGTM.
> Perhaps it would be valuable to add a simple test that checks that the new
> checkbox is visible if all pre-conditions are met.

Sounds good. I will add a test tomorrow and send this CL back to you then.

[tommycli, 2016-06-29 19:20:31]

dpapad: I added a test, though it's not that small, it's about 100 lines.
Hopefully that's alright. It improves the test coverage of the signout flow on
the people page a lot though.

https://codereview.chromium.org/2102803002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_test.js (right):

https://codereview.chromium.org/2102803002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:106: var
TestSyncBrowserProxy = function() {
I realize there is also a TestSyncBrowserProxy within the
people_page_sync_page_test.js, but they mock completely separate methods.

Arguably, they should probably be split into to separate browser proxys and two
separate C++-side handlers also.

[dpapad, 2016-06-29 22:09:34]

Thanks for adding tests!

https://codereview.chromium.org/2102803002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_test.js (right):

https://codereview.chromium.org/2102803002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:23: fakeProfileInfo: {
You probably want this declared in the constructor instead of the prototype.

https://codereview.chromium.org/2102803002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:101: /**
Nit(optional): Is a bit odd to define a whole class within a function (other
than functions used to wrap the entire file in a scope). Perhaps consider just
moving all the class definitions (the two new test proxies) at the top of this
file?

[tommycli, 2016-06-29 22:30:10]

dpapad: thanks! i improved this in accordance with your suggestions.

I'm sending this to CQ dry run now. let me know if you have further suggestions.
Tommy

https://codereview.chromium.org/2102803002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_test.js (right):

https://codereview.chromium.org/2102803002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:23: fakeProfileInfo: {
On 2016/06/29 22:09:34, dpapad wrote:
> You probably want this declared in the constructor instead of the prototype.

Done.

https://codereview.chromium.org/2102803002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:101: /**
On 2016/06/29 22:09:34, dpapad wrote:
> Nit(optional): Is a bit odd to define a whole class within a function (other
> than functions used to wrap the entire file in a scope). Perhaps consider just
> moving all the class definitions (the two new test proxies) at the top of this
> file?

Done.

[tommycli, 2016-06-29 22:31:00]

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

[commit-bot: I haz the power, 2016-06-29 22:32:56]

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, 2016-06-29 23:47:19]

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

[commit-bot: I haz the power, 2016-06-29 23:47:20]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-06-29 23:48:06]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-06-29 23:48:06]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2102803002/#ps100001
(title: "address test feedback")

[commit-bot: I haz the power, 2016-06-29 23:49:09]

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, 2016-06-30 00:04:25]

Description was changed from

==========
Settings People Revamp: Add special Disconnect logic for domain profiles

Disconnecting a profile with a domain, i.e. "managed by google.com"
should force the user to delete the local profile.

BUG=621223,563721
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Settings People Revamp: Add special Disconnect logic for domain profiles

Disconnecting a profile with a domain, i.e. "managed by google.com"
should force the user to delete the local profile.

BUG=621223,563721
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/3d05058f45ac24c192143f721572339af69f0acd
Cr-Commit-Position: refs/heads/master@{#403001}
==========

[commit-bot: I haz the power, 2016-06-30 00:04:27]

Patchset 6 (id:??) landed as
https://crrev.com/3d05058f45ac24c192143f721572339af69f0acd
Cr-Commit-Position: refs/heads/master@{#403001}

[commit-bot: I haz the power, 2016-06-30 00:04:30]

CQ bit was unchecked.