Makes 'Disconnect Account' overlay in chrome://settings accessible thru a URL

Makes 'Disconnect Account' overlay in chrome://settings accessible thru a URL

BUG=615893
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/37bd798e002dd4a0830927024695c953f86bea76
Cr-Commit-Position: refs/heads/master@{#405570}

[Moe, 2016-07-12 20:35:14]

Description was changed from

==========
Makes 'Disconnect Account' overlay in chrome://settings accessible thru a URL

BUG=615893
==========

to

==========
Makes 'Disconnect Account' overlay in chrome://settings accessible thru a URL

BUG=615893
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Moe, 2016-07-12 20:37:49]

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

[Moe, 2016-07-12 20:37:50]

Hi Dan, Please take a look at this.

I think @tommycli will be doing the MD settings work after he's done revamping
the routing there.

[Dan Beam, 2016-07-14 00:28:36]

so wait, what's going on here?  can you send a screenshot or further
explanation?

https://codereview.chromium.org/2136103008/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/2136103008/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/manage_profile_overlay.js:623: 
/**
 * @extends {ManageProfileOverlay}
 * @constructor
 */

https://codereview.chromium.org/2136103008/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/manage_profile_overlay.js:627:
'manage-profile-overlay');
why are you using this name?  shouldn't it be disconnect-account-overlay?

https://codereview.chromium.org/2136103008/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/manage_profile_overlay.js:628: };
}; -> }

https://codereview.chromium.org/2136103008/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/manage_profile_overlay.js:633: // Inherit from
ManageProfileOverlay.
remove this comment

[Moe, 2016-07-14 15:08:47]

My bad. Should've explained more. Here's what's going on:

http://imgur.com/LiGgd7S

we would like signed-in managed accounts to be taken to the 'disconnect account'
overlay in the settings page upon signout from the user menu. currently this
overlay isn't exposed via a URL.

This CL registers a new overlay (DisconnectAccountOverlay). upon showing,
DisconnectAccountOverlay makes a browser call that later calls
ManageProfileOverlay#showDisconnectManagedProfileDialog with the correct
arguments. This solves the case where we want to navigate to
chrome://settings/signOut

I also modified ManageProfileOverlay#showDisconnectManagedProfileDialog to open
DisconnectAccountOverlay instead so we get a update hash, history state, and
page title.

https://codereview.chromium.org/2136103008/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/2136103008/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/manage_profile_overlay.js:623: 
On 2016/07/14 00:28:36, Dan Beam wrote:
> /**
>  * @extends {ManageProfileOverlay}
>  * @constructor
>  */

Done.

https://codereview.chromium.org/2136103008/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/manage_profile_overlay.js:627:
'manage-profile-overlay');
On 2016/07/14 00:28:36, Dan Beam wrote:
> why are you using this name?  shouldn't it be disconnect-account-overlay?

the third argument is the ID of the div which is the overlay. I'm not adding any
new html. '#manage-profile-overlay' contains different sections for managing,
creating, deleting, and disconnecting enterprise profiles with only one being
visible at a time.

https://codereview.chromium.org/2136103008/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/manage_profile_overlay.js:628: };
On 2016/07/14 00:28:36, Dan Beam wrote:
> }; -> }

Done.

https://codereview.chromium.org/2136103008/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/manage_profile_overlay.js:633: // Inherit from
ManageProfileOverlay.
On 2016/07/14 00:28:36, Dan Beam wrote:
> remove this comment

Done.

[Dan Beam, 2016-07-14 18:46:44]

lgtm i guess (cuz this is deprecated code and all), but this is a pretty lame
user experience.  we probably close the user manager and show the settings page
and pop open a dialog just to add a confirmation... that seems fairly jarring
and bad for a11y, IMO

[Moe, 2016-07-14 19:21:44]

On 2016/07/14 18:46:44, Dan Beam wrote:
> lgtm i guess (cuz this is deprecated code and all), but this is a pretty lame
> user experience.  we probably close the user manager and show the settings
page
> and pop open a dialog just to add a confirmation... that seems fairly jarring
> and bad for a11y, IMO

Thanks for reviewing!

for what it's worth, I agree with you from a11y POV. the confirmation could also
be shown on the "user menu". There isn't much real estate three though. also the
action is pretty heavy-weight (wipes all user data). So I guess opening the
confirmation dialog in the settings page implies a bolder warning. here's the
design doc if you're interested: http://shortn/_2bHCdZ56mB

[Moe, 2016-07-14 19:23:17]

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

[commit-bot: I haz the power, 2016-07-14 19:23: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-07-14 19:23:57]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-14 20:37:38]

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

[commit-bot: I haz the power, 2016-07-14 20:37:40]

Dry run: This issue passed the CQ dry run.

[Moe, 2016-07-14 21:07:10]

The CQ bit was checked by mahmadi@chromium.org

[commit-bot: I haz the power, 2016-07-14 21:07:48]

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-07-14 21:07:49]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-14 21:13:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-14 21:13:15]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-14 21:15:00]

Description was changed from

==========
Makes 'Disconnect Account' overlay in chrome://settings accessible thru a URL

BUG=615893
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Makes 'Disconnect Account' overlay in chrome://settings accessible thru a URL

BUG=615893
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/37bd798e002dd4a0830927024695c953f86bea76
Cr-Commit-Position: refs/heads/master@{#405570}
==========

[commit-bot: I haz the power, 2016-07-14 21:15:01]

Patchset 2 (id:??) landed as
https://crrev.com/37bd798e002dd4a0830927024695c953f86bea76
Cr-Commit-Position: refs/heads/master@{#405570}