MD user manager (C++ for create profile flow)

MD user manager (C++ for create profile flow)

Part 1 of https://codereview.chromium.org/1630903002/

BUG=563722
Committed: https://crrev.com/c2029d459ec656ad49799d2e47c75795a42c185a
Cr-Commit-Position: refs/heads/master@{#379058}

[Moe, 2016-02-22 23:57:53]

Patchset #1 (id:1) has been deleted

[Moe, 2016-02-23 00:00:20]

Description was changed from

==========
MD user manager (C++ for create profile flow)

Part 1 of https://codereview.chromium.org/1630903002/

BUG=563722
==========

to

==========
MD user manager (C++ for create profile flow)

Part 1 of https://codereview.chromium.org/1630903002/

BUG=563722
==========

[Moe, 2016-02-23 00:00:20]

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

[Moe, 2016-02-23 00:03:26]

dbeam@ I broke https://codereview.chromium.org/1630903002/ into three parts.
Here's part 1

[Dan Beam, 2016-02-25 01:04:40]

looks pretty good to me

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/md_user_manager_ui.cc (right):

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/md_user_manager_ui.cc:25: // The web_ui object
takes ownership of the handler, and will
these handlers

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/md_user_manager_ui.cc:26: // destroy it when it
(the WebUI) is destroyed.
destroy them

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/md_user_manager_ui.cc:28:
signin_create_profile_handler_ = new SigninCreateProfileHandler();
nit: put these in initializer list

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/md_user_manager_ui.h (right):

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/md_user_manager_ui.h:12: class
SigninCreateProfileHandler;
alphabetize

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/signin_create_profile_handler.cc (right):

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:73:
l10n_util::GetStringUTF16(IDS_LEGACY_SUPERVISED_USER_LEARN_MORE_TITLE));
why are you adding another use of a _LEGACY_ string?

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:83:
chrome::kLegacySupervisedUserManagementDisplayURL)));
what does legacy mean here as well?

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:138:
base::DictionaryValue* user_info = new base::DictionaryValue();
nit: can you use scoped_ptr<base::DictionaryValue> here instead and .release()
later?  if this line was above the continue; statement it'd be a leak

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:311:
options::helper::OpenNewWindowForProfile(
can you move this code out of options?  seems odd to depend on it from signin
and it doesn't really seem options specific.

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:404: success =
args->GetString(4, supervised_user_id);
should this GetString() for supervised_user_id be inside of if
(supervised_user)?

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:413: return
supervised_user && !supervisor_profile_path->empty();
bool supervised_user = false;
bool success = args->GetBoolean(3, &supervised_user);
DCHECK(success);

if (!supervised_user)
  return false;

...

return supervised_profile_path->empty();

https://codereview.chromium.org/1716163005/diff/20001/chrome/common/url_const...
File chrome/common/url_constants.h (right):

https://codereview.chromium.org/1716163005/diff/20001/chrome/common/url_const...
chrome/common/url_constants.h:71: extern const char kChromeUIMDUserManagerURL[];
nit: arguably UIMd or UiMd because it's clearer that the abbreviations are "UI"
and "MD" rather than "UIMD"

fwiw: if you're wondering, "but all of these constants end with URL!" -> they
probably should end with "Url" instead and a few (more recently ones) do

[Moe, 2016-02-25 16:03:03]

Addressed your comments Dan. Thank you.

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/md_user_manager_ui.cc (right):

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/md_user_manager_ui.cc:25: // The web_ui object
takes ownership of the handler, and will
On 2016/02/25 01:04:40, Dan Beam wrote:
> these handlers

Done.

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/md_user_manager_ui.cc:26: // destroy it when it
(the WebUI) is destroyed.
On 2016/02/25 01:04:40, Dan Beam wrote:
> destroy them

Done.

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/md_user_manager_ui.cc:28:
signin_create_profile_handler_ = new SigninCreateProfileHandler();
On 2016/02/25 01:04:40, Dan Beam wrote:
> nit: put these in initializer list

Done.

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/md_user_manager_ui.h (right):

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/md_user_manager_ui.h:12: class
SigninCreateProfileHandler;
On 2016/02/25 01:04:40, Dan Beam wrote:
> alphabetize

Done.

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/signin_create_profile_handler.cc (right):

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:73:
l10n_util::GetStringUTF16(IDS_LEGACY_SUPERVISED_USER_LEARN_MORE_TITLE));
On 2016/02/25 01:04:40, Dan Beam wrote:
> why are you adding another use of a _LEGACY_ string?

I'm reusing the strings that haven't changed in the new design. I haven't been
using "LEGACY" in the new ones I've added.

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:83:
chrome::kLegacySupervisedUserManagementDisplayURL)));
On 2016/02/25 01:04:40, Dan Beam wrote:
> what does legacy mean here as well?

This is another existing string. I don't know why some are referred to as
LEGACY.

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:138:
base::DictionaryValue* user_info = new base::DictionaryValue();
On 2016/02/25 01:04:40, Dan Beam wrote:
> nit: can you use scoped_ptr<base::DictionaryValue> here instead and .release()
> later?  if this line was above the continue; statement it'd be a leak

Done.

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:311:
options::helper::OpenNewWindowForProfile(
On 2016/02/25 01:04:40, Dan Beam wrote:
> can you move this code out of options?  seems odd to depend on it from signin
> and it doesn't really seem options specific.

Done.

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:404: success =
args->GetString(4, supervised_user_id);
On 2016/02/25 01:04:40, Dan Beam wrote:
> should this GetString() for supervised_user_id be inside of if
> (supervised_user)?

Acknowledged.

https://codereview.chromium.org/1716163005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:413: return
supervised_user && !supervisor_profile_path->empty();
On 2016/02/25 01:04:40, Dan Beam wrote:
> bool supervised_user = false;
> bool success = args->GetBoolean(3, &supervised_user);
> DCHECK(success);
> 
> if (!supervised_user)
>   return false;
> 
> ...
> 
> return supervised_profile_path->empty();

Done.

https://codereview.chromium.org/1716163005/diff/20001/chrome/common/url_const...
File chrome/common/url_constants.h (right):

https://codereview.chromium.org/1716163005/diff/20001/chrome/common/url_const...
chrome/common/url_constants.h:71: extern const char kChromeUIMDUserManagerURL[];
On 2016/02/25 01:04:40, Dan Beam wrote:
> nit: arguably UIMd or UiMd because it's clearer that the abbreviations are
"UI"
> and "MD" rather than "UIMD"
> 
> fwiw: if you're wondering, "but all of these constants end with URL!" -> they
> probably should end with "Url" instead and a few (more recently ones) do

Done.

[Dan Beam, 2016-02-29 20:25:33]

lgtm but I still don't understand what the _LEGACY_ is about in supervised user
strings.  the CL that added this[1] is pretty quiet, as is the bug[2].

pam@/treib@: what's up with that?

[1] https://crrev.com/1285333004
[2] https://crbug.com/520307

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

https://codereview.chromium.org/1716163005/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/create_profile_handler.cc:221:
Profile::CREATE_STATUS_INITIALIZED);
nit: fix indent

https://codereview.chromium.org/1716163005/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/profile_helper.h (right):

https://codereview.chromium.org/1716163005/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/profile_helper.h:16: namespace profiles {
nit: i don't really get the "helper" namespace (in its previous form), and
profiles:: probably should be used from a /profiles/ directory (i.e.
chrome/browser/profiles)

could these just be ::webui::{OpenNewWindowForProfile,DeleteProfileAtPath}
instead?

https://codereview.chromium.org/1716163005/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/profile_helper.h:25: }  // namespace signin
nit: signin -> profiles

[Marc Treib, 2016-03-01 09:13:46]

Much of the supervised users code in Chrome is used for both supervised users
and for child accounts. To make clear which parts only apply to the old
supervised users, we've started calling them "legacy supervised users".
"Legacy" isn't quite "deprecated", so it's perfectly fine to keep using those
strings.

[Moe, 2016-03-01 18:29:53]

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

https://codereview.chromium.org/1716163005/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/create_profile_handler.cc:221:
Profile::CREATE_STATUS_INITIALIZED);
On 2016/02/29 20:25:32, Dan Beam wrote:
> nit: fix indent

Done.

https://codereview.chromium.org/1716163005/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/profile_helper.h (right):

https://codereview.chromium.org/1716163005/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/profile_helper.h:16: namespace profiles {
On 2016/02/29 20:25:32, Dan Beam wrote:
> nit: i don't really get the "helper" namespace (in its previous form), and
> profiles:: probably should be used from a /profiles/ directory (i.e.
> chrome/browser/profiles)
> 
> could these just be ::webui::{OpenNewWindowForProfile,DeleteProfileAtPath}
> instead?

Done.

https://codereview.chromium.org/1716163005/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/profile_helper.h:25: }  // namespace signin
On 2016/02/29 20:25:33, Dan Beam wrote:
> nit: signin -> profiles

Done.

[Moe, 2016-03-03 18:23:53]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2016-03-03 18:23:53]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1716163005/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-03-03 18:24:21]

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

[commit-bot: I haz the power, 2016-03-03 19:38:16]

Description was changed from

==========
MD user manager (C++ for create profile flow)

Part 1 of https://codereview.chromium.org/1630903002/

BUG=563722
==========

to

==========
MD user manager (C++ for create profile flow)

Part 1 of https://codereview.chromium.org/1630903002/

BUG=563722
==========

[commit-bot: I haz the power, 2016-03-03 19:38:17]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-03-03 19:45:11]

Description was changed from

==========
MD user manager (C++ for create profile flow)

Part 1 of https://codereview.chromium.org/1630903002/

BUG=563722
==========

to

==========
MD user manager (C++ for create profile flow)

Part 1 of https://codereview.chromium.org/1630903002/

BUG=563722

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

[commit-bot: I haz the power, 2016-03-03 19:45:12]

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