Fix build when ENABLE_MANAGED_USERS is not defined

Fix build when ENABLE_MANAGED_USERS is not defined

This commit resolves build errors that occur when ENABLE_MANAGED_USERS is not
defined:

1. Managed/supervised code for various components are now conditionally
   excluded in gyp files.
2. Added #ifdef guards in source files that include files in the
   chrome/browser/supervised_user folder.
3. Add missing include to supervised_users.h in files in the
   supervised_user folder to force compiler errors when not guarded properly.
4. Added an include in create_profile_handler.cc to web_ui.h, since it is
   indirectly included by the supervised_user* .h files.

BUG=389611
Committed: https://crrev.com/a1a18dc36d89030a8fb6c514fcdf3c7a0153ee07
Cr-Commit-Position: refs/heads/master@{#297219}

[mckev, 2014-06-27 17:35:16]

PTAL, thanks!

[James Hawkins, 2014-06-27 19:32:08]

Which files are you asking me to review?

[mckev, 2014-06-27 20:36:50]

On 2014/06/27 19:32:08, James Hawkins wrote:
> Which files are you asking me to review?

Hi James, thanks for taking a look.  I put you as a reviewer because you were
listed as owner for the following files, it seemed relevant:

jhawkins owns 10 file(s):
  chrome/browser/search/search_unittest.cc [12]
  chrome/browser/supervised_user/supervised_user_theme.h [10]
  chrome/browser/themes/theme_service.cc [9]
  chrome/browser/themes/theme_service_unittest.cc [9]
  chrome/browser/ui/startup/startup_browser_creator_browsertest.cc [10]
  chrome/browser/ui/webui/downloads_ui_browsertest.cc [18]
  chrome/browser/ui/webui/extensions/extension_settings_handler.cc [21]
  chrome/browser/ui/webui/options/create_profile_handler.cc [19]
  chrome/browser/ui/webui/options/create_profile_handler.h [19]
  chrome/browser/ui/webui/options/options_ui.cc [19]

Feel free to suggest other reviewers as necessary.  Please let me know if you
have any questions! :-)

[James Hawkins, 2014-06-27 20:48:14]

On 2014/06/27 20:36:50, mckev wrote:
> On 2014/06/27 19:32:08, James Hawkins wrote:
> > Which files are you asking me to review?
> 
> Hi James, thanks for taking a look.  I put you as a reviewer because you were
> listed as owner for the following files, it seemed relevant:
> 
> jhawkins owns 10 file(s):
>   chrome/browser/search/search_unittest.cc [12]
>   chrome/browser/supervised_user/supervised_user_theme.h [10]
>   chrome/browser/themes/theme_service.cc [9]
>   chrome/browser/themes/theme_service_unittest.cc [9]
>   chrome/browser/ui/startup/startup_browser_creator_browsertest.cc [10]
>   chrome/browser/ui/webui/downloads_ui_browsertest.cc [18]
>   chrome/browser/ui/webui/extensions/extension_settings_handler.cc [21]
>   chrome/browser/ui/webui/options/create_profile_handler.cc [19]
>   chrome/browser/ui/webui/options/create_profile_handler.h [19]
>   chrome/browser/ui/webui/options/options_ui.cc [19]
> 
> Feel free to suggest other reviewers as necessary.  Please let me know if you
> have any questions! :-)

Thanks; I recommend finding more-specific OWNERS where possible, e.g.,
chrome/browser/search/OWNERS, etc.  I'm happy to do webui.

[James Hawkins, 2014-06-27 20:49:08]

When is the ENABLE_MANAGED_USERS flag expected to be removed?

[mckev, 2014-06-27 20:56:12]

On 2014/06/27 20:49:08, James Hawkins wrote:
> When is the ENABLE_MANAGED_USERS flag expected to be removed?

To my knowledge there is no plan, you may have better context on your end.

Thanks for the tip re: OWNERs and for reviewing the webui portion.  I'll make
sure I have sufficient coverage and update the reviewers for this CL.

[James Hawkins, 2014-06-27 21:09:57]

https://codereview.chromium.org/355233002/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/create_profile_handler.cc (right):

https://codereview.chromium.org/355233002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/create_profile_handler.cc:96: #if
defined(ENABLE_MANAGED_USERS)
OK, you're not gonna like this.  (At least) the webui code that deals with
managed users needs to be refactored such that we don't have this ifdef
splattered across the code.

[mckev, 2014-06-27 21:10:18]

I updated the reviewers to be more granular.  Please advise if that's
appropriate.

Notes to reviewers:  Please review the changes as such:

samarth@chromium.org - chrome/browser/search
bauerb@chromium.org - chrome/browser/supervised_user
pkotwicz@chromium.org - chrome/browser/themes
jhawkins@chromium.org - chrome/browser/ui/webui
ben@chromium.org - chrome/browser/ui/startup

If all of the reviewers could review the *.gyp*  files I would appreciate that
as well.  Thanks!

[mckev, 2014-06-27 21:14:10]

https://codereview.chromium.org/355233002/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/create_profile_handler.cc (right):

https://codereview.chromium.org/355233002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/create_profile_handler.cc:96: #if
defined(ENABLE_MANAGED_USERS)
On 2014/06/27 21:09:57, James Hawkins wrote:
> OK, you're not gonna like this.  (At least) the webui code that deals with
> managed users needs to be refactored such that we don't have this ifdef
> splattered across the code.

Haha, no worries at all.  I'd be happy to assist if that makes sense.  There is
a similar issue with signin that required a wrapper.  Does that approach make
sense here?

[James Hawkins, 2014-06-27 21:25:58]

On 2014/06/27 21:14:10, mckev wrote:
>
https://codereview.chromium.org/355233002/diff/1/chrome/browser/ui/webui/opti...
> File chrome/browser/ui/webui/options/create_profile_handler.cc (right):
> 
>
https://codereview.chromium.org/355233002/diff/1/chrome/browser/ui/webui/opti...
> chrome/browser/ui/webui/options/create_profile_handler.cc:96: #if
> defined(ENABLE_MANAGED_USERS)
> On 2014/06/27 21:09:57, James Hawkins wrote:
> > OK, you're not gonna like this.  (At least) the webui code that deals with
> > managed users needs to be refactored such that we don't have this ifdef
> > splattered across the code.
> 
> Haha, no worries at all.  I'd be happy to assist if that makes sense.  There
is
> a similar issue with signin that required a wrapper.  Does that approach make
> sense here?

I'm not familiar with it.  Do you have a link to the code review or codesearch?

[mckev, 2014-06-27 21:33:21]

On 2014/06/27 21:25:58, James Hawkins wrote:
> On 2014/06/27 21:14:10, mckev wrote:
> >
>
https://codereview.chromium.org/355233002/diff/1/chrome/browser/ui/webui/opti...
> > File chrome/browser/ui/webui/options/create_profile_handler.cc (right):
> > 
> >
>
https://codereview.chromium.org/355233002/diff/1/chrome/browser/ui/webui/opti...
> > chrome/browser/ui/webui/options/create_profile_handler.cc:96: #if
> > defined(ENABLE_MANAGED_USERS)
> > On 2014/06/27 21:09:57, James Hawkins wrote:
> > > OK, you're not gonna like this.  (At least) the webui code that deals with
> > > managed users needs to be refactored such that we don't have this ifdef
> > > splattered across the code.
> > 
> > Haha, no worries at all.  I'd be happy to assist if that makes sense.  There
> is
> > a similar issue with signin that required a wrapper.  Does that approach
make
> > sense here?
> 
> I'm not familiar with it.  Do you have a link to the code review or
codesearch?

Of course - take a look at:  https://codereview.chromium.org/137753012  This
effectively separates the supervised and non-supervised code, reducing the need
for #ifdefs, if I understand it correctly.

[James Hawkins, 2014-06-27 21:34:47]

On 2014/06/27 21:33:21, mckev wrote:
> On 2014/06/27 21:25:58, James Hawkins wrote:
> > On 2014/06/27 21:14:10, mckev wrote:
> > >
> >
>
https://codereview.chromium.org/355233002/diff/1/chrome/browser/ui/webui/opti...
> > > File chrome/browser/ui/webui/options/create_profile_handler.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/355233002/diff/1/chrome/browser/ui/webui/opti...
> > > chrome/browser/ui/webui/options/create_profile_handler.cc:96: #if
> > > defined(ENABLE_MANAGED_USERS)
> > > On 2014/06/27 21:09:57, James Hawkins wrote:
> > > > OK, you're not gonna like this.  (At least) the webui code that deals
with
> > > > managed users needs to be refactored such that we don't have this ifdef
> > > > splattered across the code.
> > > 
> > > Haha, no worries at all.  I'd be happy to assist if that makes sense. 
There
> > is
> > > a similar issue with signin that required a wrapper.  Does that approach
> make
> > > sense here?
> > 
> > I'm not familiar with it.  Do you have a link to the code review or
> codesearch?
> 
> Of course - take a look at:  https://codereview.chromium.org/137753012  This
> effectively separates the supervised and non-supervised code, reducing the
need
> for #ifdefs, if I understand it correctly.

That's fine.  Ideally we can use that directly.

[Bernhard Bauer, 2014-06-30 16:04:19]

+treib

As a heads-up, Marc (cc'd) is also working on a refactoring here, which will
rename things refering to "managed users" (the old name) to "supervised users"
(the current name). That shouldn't materially impact you, but it might create
merge conflicts for either side.

Also: AFAIK there are currently no buildbots that build with this configuration,
so this might break at any time. If you're okay with that, go ahead :)

https://codereview.chromium.org/355233002/diff/1/chrome/browser/themes/theme_...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/355233002/diff/1/chrome/browser/themes/theme_...
chrome/browser/themes/theme_service.cc:591: void
ThemeService::SetSupervisedUserTheme() {
Could we move the whole method into the #ifdef?

https://codereview.chromium.org/355233002/diff/1/chrome/browser/themes/theme_...
chrome/browser/themes/theme_service.cc:595: DCHECK(false) << "Supervised users
aren't enabled.";
This is more commonly written as NOTREACHED() (see also below).

https://codereview.chromium.org/355233002/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/create_profile_handler.cc (right):

https://codereview.chromium.org/355233002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/create_profile_handler.cc:200: #if
defined(ENABLE_MANAGED_USERS)
We might also want to move this whole method (and the one below) into the
#ifdef. Basically, anything that has "supervised user" in its name shouldn't
exist if the #define is not present.

[mckev, 2014-07-01 23:08:05]

On 2014/06/30 16:04:19, Bernhard Bauer wrote:
> +treib
> 
> As a heads-up, Marc (cc'd) is also working on a refactoring here, which will
> rename things refering to "managed users" (the old name) to "supervised users"
> (the current name). That shouldn't materially impact you, but it might create
> merge conflicts for either side.

Thanks for the heads up!

> Also: AFAIK there are currently no buildbots that build with this
configuration,
> so this might break at any time. If you're okay with that, go ahead :)

No problem and thanks for the flexibility.  We're happy to submit patches in
case it breaks again.

[samarth, 2014-07-02 21:53:31]

lgtm for search

[James Hawkins, 2014-07-07 18:27:22]

mckev: Ping when a new patch set is ready.  If you don't hear back from me, you
should send me a direct email, because these codereview emails are hitting my
spam bucket.

[mckev, 2014-07-07 18:42:03]

On 2014/07/07 18:27:22, James Hawkins wrote:
> mckev: Ping when a new patch set is ready.  If you don't hear back from me,
you
> should send me a direct email, because these codereview emails are hitting my
> spam bucket.

Thanks James, sorry for the delay.  I'm almost through a refactored CL.  I
should have something ready in the next day or two.

[mckev, 2014-07-11 18:27:54]

OK all, PTAL.  I responded to the nits and attempted to do a better job of
separating out all references to supervised/managed code.  I found a sneaky use
of it in AvatarLabel which I renamed to make it more apparent. All targets still
successfully build with both build configurations.

I took a handful of small liberties (extracted a few methods, consolidated some
managed code into single #ifdef blocks) to reduce the noise, but I suspect this
isn't the ideal case in the long run.  I did take a stab at a more aggressive
refactoring as suggested, but I'm afraid I don't have the context nor would it
be feasible to accomplish that in a single CL.

I'll defer completely to your judgement: whether you'd collectively like to
accept this patch, if there are any other improvements I can make to it, or if
there is an alternate way forward. At the end of the day for this patch, it's
certainly not mission critical - we'd simply like the build for all targets to
succeed when this flag is enabled (only the /chrome* targets fail to build
currently).

Thanks to everyone for their input and help! :-)

https://chromiumcodereview.appspot.com/355233002/diff/1/chrome/browser/themes...
File chrome/browser/themes/theme_service.cc (right):

https://chromiumcodereview.appspot.com/355233002/diff/1/chrome/browser/themes...
chrome/browser/themes/theme_service.cc:591: void
ThemeService::SetSupervisedUserTheme() {
On 2014/06/30 16:04:18, Bernhard Bauer wrote:
> Could we move the whole method into the #ifdef?

Done.

https://chromiumcodereview.appspot.com/355233002/diff/1/chrome/browser/themes...
chrome/browser/themes/theme_service.cc:595: DCHECK(false) << "Supervised users
aren't enabled.";
On 2014/06/30 16:04:18, Bernhard Bauer wrote:
> This is more commonly written as NOTREACHED() (see also below).

Awesome, thanks for the tip!

[Bernhard Bauer, 2014-07-14 10:17:29]

I think you forgot to upload a new patch set...

[mckev, 2014-07-14 15:36:58]

On 2014/07/14 10:17:29, Bernhard Bauer wrote:
> I think you forgot to upload a new patch set...

Sorry about that.  It's there now.

[Bernhard Bauer, 2014-07-14 16:39:06]

https://codereview.chromium.org/355233002/diff/20001/chrome/browser/themes/th...
File chrome/browser/themes/theme_service_unittest.cc (right):

https://codereview.chromium.org/355233002/diff/20001/chrome/browser/themes/th...
chrome/browser/themes/theme_service_unittest.cc:275: #endif
It might be useful to add // defined(...) here.

https://codereview.chromium.org/355233002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/355233002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:208: if
(IsWithinAvatarMenuButton(hit_test, point) ||
You could probably pull this check out of the #ifdef.

https://codereview.chromium.org/355233002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/355233002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:214:
(avatar_button()->GetMirroredBounds().Contains(point))) ||
Consider also splitting this up into two checks.

https://codereview.chromium.org/355233002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:226: if
(IsWithinAvatarMenuButtons(point) ||
Same here.

https://codereview.chromium.org/355233002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.h (right):

https://codereview.chromium.org/355233002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.h:15: #if
defined(ENABLE_MANAGED_USERS)
Nit: empty line before the #ifdef block?

https://codereview.chromium.org/355233002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/downloads_ui_browsertest.cc (left):

https://codereview.chromium.org/355233002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/downloads_ui_browsertest.cc:28:
command_line->AppendSwitchASCII(switches::kSupervisedUserId, "asdf");
Hm... you might want to un-#ifdef this test too, just for completeness' sake.

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

https://codereview.chromium.org/355233002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/create_profile_handler.cc:25: #if
defined(ENABLE_MANAGED_USERS)
This file is a bit pushing it with regards to the number of #ifdefs I think are
palatable. Another approach would be not to #ifdef out any and all code that
refers to supervised users, but e.g. only code that refers to classes from
c/b/supervised_user/, replace it with NOTREACHED(), and just disable entry
points for the feature (i.e. callbacks from WebUI in this case).

https://codereview.chromium.org/355233002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/create_profile_handler.cc:196: default:
Is this actually necessary? If ENABLE_MANAGED_USERS is not defined, there are no
SUPERVISED_PROFILE_* values in the enum.

https://codereview.chromium.org/355233002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/create_profile_handler.cc:235: if
(profile_creation_type_ == NON_SUPERVISED_PROFILE_CREATION) {
This doesn't quite match the old behavior for importing an existing supervised
user (see the comment).

https://codereview.chromium.org/355233002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/create_profile_handler.cc:275: if
(!new_profile->IsSupervised())
This is also a slight change from the old behavior. If ENABLE_MANAGED_USERS is
not defined, no profile is supervised, so we should always return.

https://codereview.chromium.org/355233002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/create_profile_handler.cc:308: base::string16
CreateProfileHandler::GetProfileCreationErrorMessage(
This whole method is only called for supervised user creation. I would restore
it to the old version and put it into the #ifdef as a whole.

https://codereview.chromium.org/355233002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/create_profile_handler.h (right):

https://codereview.chromium.org/355233002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/create_profile_handler.h:140:
base::WeakPtrFactory<CreateProfileHandler> weak_ptr_factory_;
This should be the last member of the object, to ensure it's destroyed first
(thereby invalidating all weak pointers handed out by it).

[mckev, 2014-07-14 18:57:47]

bauerb, thanks for the thorough review!  I'll have some responses and an updated
diff as soon as possible. :-)

[mckev, 2014-07-14 23:50:03]

bauerb: Resolved as much as I could in the latest patch set. Thanks in advance
for your help!

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/th...
File chrome/browser/themes/theme_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/th...
chrome/browser/themes/theme_service_unittest.cc:275: #endif
On 2014/07/14 16:39:05, Bernhard Bauer wrote:
> It might be useful to add // defined(...) here.

Good call - added.

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:208: if
(IsWithinAvatarMenuButton(hit_test, point) ||
On 2014/07/14 16:39:05, Bernhard Bauer wrote:
> You could probably pull this check out of the #ifdef.

Yep, certainly.  Would you prefer it to still be extracted to a separate method
as well? (It's only used here).

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:214:
(avatar_button()->GetMirroredBounds().Contains(point))) ||
On 2014/07/14 16:39:05, Bernhard Bauer wrote:
> Consider also splitting this up into two checks.

OK - I took a stab at it.  Let me know what you think of the approach I took.

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:226: if
(IsWithinAvatarMenuButtons(point) ||
On 2014/07/14 16:39:05, Bernhard Bauer wrote:
> Same here.

Sure thing, makes sense since I made a similar change in
browser_non-client_frame_view_ash.cc.

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.h (right):

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.h:15: #if
defined(ENABLE_MANAGED_USERS)
On 2014/07/14 16:39:05, Bernhard Bauer wrote:
> Nit: empty line before the #ifdef block?

Done.

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
File chrome/browser/ui/webui/downloads_ui_browsertest.cc (left):

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
chrome/browser/ui/webui/downloads_ui_browsertest.cc:28:
command_line->AppendSwitchASCII(switches::kSupervisedUserId, "asdf");
On 2014/07/14 16:39:05, Bernhard Bauer wrote:
> Hm... you might want to un-#ifdef this test too, just for completeness' sake.

Good point.  I dove a bit deeper and figured out to compile out the generated
test that depended on this.

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
File chrome/browser/ui/webui/options/create_profile_handler.cc (right):

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
chrome/browser/ui/webui/options/create_profile_handler.cc:25: #if
defined(ENABLE_MANAGED_USERS)
On 2014/07/14 16:39:05, Bernhard Bauer wrote:
> This file is a bit pushing it with regards to the number of #ifdefs I think
are
> palatable. Another approach would be not to #ifdef out any and all code that
> refers to supervised users, but e.g. only code that refers to classes from
> c/b/supervised_user/, replace it with NOTREACHED(), and just disable entry
> points for the feature (i.e. callbacks from WebUI in this case).

I tend to agree - this is a huge number of #ifdefs that I'd like to avoid as
well.  The only issue I foresee with this approach would be that this class
wouldn't be compilable, since it would permanently include these files and all
of their dependencies.

For this class at least, would it be feasible to put the supervised user logic
in a separate .h/.cc files, a la create_profile_handler_supervised?  There might
be some #ifdefs in the main files, but at least we'd be one step closer to
factoring it out entirely.

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
chrome/browser/ui/webui/options/create_profile_handler.cc:196: default:
On 2014/07/14 16:39:05, Bernhard Bauer wrote:
> Is this actually necessary? If ENABLE_MANAGED_USERS is not defined, there are
no
> SUPERVISED_PROFILE_* values in the enum.

Yep, you're right.  I added that opportunistically to guard against unused
future values.  I just re-read the style guide and I see this isn't correct for
enumerations (the compiler will already complain about unused values).

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
chrome/browser/ui/webui/options/create_profile_handler.cc:235: if
(profile_creation_type_ == NON_SUPERVISED_PROFILE_CREATION) {
On 2014/07/14 16:39:06, Bernhard Bauer wrote:
> This doesn't quite match the old behavior for importing an existing supervised
> user (see the comment).

Indeed.  My apologies for missing that.

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
chrome/browser/ui/webui/options/create_profile_handler.cc:275: if
(!new_profile->IsSupervised())
On 2014/07/14 16:39:06, Bernhard Bauer wrote:
> This is also a slight change from the old behavior. If ENABLE_MANAGED_USERS is
> not defined, no profile is supervised, so we should always return.

Agreed.  I added an early return at the beginning of the function in that case.

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
chrome/browser/ui/webui/options/create_profile_handler.cc:308: base::string16
CreateProfileHandler::GetProfileCreationErrorMessage(
On 2014/07/14 16:39:06, Bernhard Bauer wrote:
> This whole method is only called for supervised user creation. I would restore
> it to the old version and put it into the #ifdef as a whole.

It looks there is one exception - it's also called from OnProfileCreated -
GetProfileCreationErrorMessage(LOCAL_ERROR).  I think nesting switch statements
is less than ideal, but I can't think of a better way to consolidate this logic
completely or without duplicating a few more lines of code.  If you can suggest
something, I'll be happy to comply.

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
File chrome/browser/ui/webui/options/create_profile_handler.h (right):

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
chrome/browser/ui/webui/options/create_profile_handler.h:140:
base::WeakPtrFactory<CreateProfileHandler> weak_ptr_factory_;
On 2014/07/14 16:39:06, Bernhard Bauer wrote:
> This should be the last member of the object, to ensure it's destroyed first
> (thereby invalidating all weak pointers handed out by it).

Good catch - thanks for pointing it out.

[Bernhard Bauer, 2014-07-16 09:54:00]

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:208: if
(IsWithinAvatarMenuButton(hit_test, point) ||
On 2014/07/14 23:50:03, mckev wrote:
> On 2014/07/14 16:39:05, Bernhard Bauer wrote:
> > You could probably pull this check out of the #ifdef.
> 
> Yep, certainly.  Would you prefer it to still be extracted to a separate
method
> as well? (It's only used here).

Yeah, it might make sense to inline this.

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:214:
(avatar_button()->GetMirroredBounds().Contains(point))) ||
On 2014/07/14 23:50:03, mckev wrote:
> On 2014/07/14 16:39:05, Bernhard Bauer wrote:
> > Consider also splitting this up into two checks.
> 
> OK - I took a stab at it.  Let me know what you think of the approach I took.

You can simplify this with multiple returns:

  if (avatar_button() &&
      avatar_button()->GetMirroredBounds().Contains(point)) {
    return true;
  }

  if (new_avatar_button() &&
      new_avatar_button()->GetMirroredBounds().Contains(point)) {
    return true;
  }

  return false;

That makes it a bit more readable IMO.

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
File chrome/browser/ui/webui/options/create_profile_handler.cc (right):

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
chrome/browser/ui/webui/options/create_profile_handler.cc:25: #if
defined(ENABLE_MANAGED_USERS)
On 2014/07/14 23:50:03, mckev wrote:
> On 2014/07/14 16:39:05, Bernhard Bauer wrote:
> > This file is a bit pushing it with regards to the number of #ifdefs I think
> are
> > palatable. Another approach would be not to #ifdef out any and all code that
> > refers to supervised users, but e.g. only code that refers to classes from
> > c/b/supervised_user/, replace it with NOTREACHED(), and just disable entry
> > points for the feature (i.e. callbacks from WebUI in this case).
> 
> I tend to agree - this is a huge number of #ifdefs that I'd like to avoid as
> well.  The only issue I foresee with this approach would be that this class
> wouldn't be compilable, since it would permanently include these files and all
> of their dependencies.
> 
> For this class at least, would it be feasible to put the supervised user logic
> in a separate .h/.cc files, a la create_profile_handler_supervised?  There
might
> be some #ifdefs in the main files, but at least we'd be one step closer to
> factoring it out entirely.

Maybe -- it's a bit tricky of course, because our goal here was reusing existing
profile creation code, not keeping supervised user code separate from other
code. That being said, if you want to try your hand at it, I'd be willing to
review patches :)

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
chrome/browser/ui/webui/options/create_profile_handler.cc:275: if
(!new_profile->IsSupervised())
On 2014/07/14 23:50:03, mckev wrote:
> On 2014/07/14 16:39:06, Bernhard Bauer wrote:
> > This is also a slight change from the old behavior. If ENABLE_MANAGED_USERS
is
> > not defined, no profile is supervised, so we should always return.
> 
> Agreed.  I added an early return at the beginning of the function in that
case.

Would it make sense to put the whole method body into one big #ifdef? Then we
don't need the second one here.

https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
chrome/browser/ui/webui/options/create_profile_handler.cc:308: base::string16
CreateProfileHandler::GetProfileCreationErrorMessage(
On 2014/07/14 23:50:03, mckev wrote:
> On 2014/07/14 16:39:06, Bernhard Bauer wrote:
> > This whole method is only called for supervised user creation. I would
restore
> > it to the old version and put it into the #ifdef as a whole.
> 
> It looks there is one exception - it's also called from OnProfileCreated -
> GetProfileCreationErrorMessage(LOCAL_ERROR).  I think nesting switch
statements
> is less than ideal, but I can't think of a better way to consolidate this
logic
> completely or without duplicating a few more lines of code.  If you can
suggest
> something, I'll be happy to comply.

Hmm, looking at it in more detail, each of the error types is used exactly once,
as a constant argument to this method. We could split this up into three
methods, two of which only make sense for supervised users (so they could be
completely wrapped in #ifdef's), so only the third one would have an #ifdef in
the method body. WDYT?

[mckev, 2014-07-18 17:45:09]

On 2014/07/16 09:54:00, Bernhard Bauer wrote:
>
https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
> File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc
(right):
> 
>
https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
> chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:208: if
> (IsWithinAvatarMenuButton(hit_test, point) ||
> On 2014/07/14 23:50:03, mckev wrote:
> > On 2014/07/14 16:39:05, Bernhard Bauer wrote:
> > > You could probably pull this check out of the #ifdef.
> > 
> > Yep, certainly.  Would you prefer it to still be extracted to a separate
> method
> > as well? (It's only used here).
> 
> Yeah, it might make sense to inline this.

Done.

> 
>
https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
> File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):
> 
>
https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
> chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:214:
> (avatar_button()->GetMirroredBounds().Contains(point))) ||
> On 2014/07/14 23:50:03, mckev wrote:
> > On 2014/07/14 16:39:05, Bernhard Bauer wrote:
> > > Consider also splitting this up into two checks.
> > 
> > OK - I took a stab at it.  Let me know what you think of the approach I
took.
> 
> You can simplify this with multiple returns:
> 
>   if (avatar_button() &&
>       avatar_button()->GetMirroredBounds().Contains(point)) {
>     return true;
>   }
> 
>   if (new_avatar_button() &&
>       new_avatar_button()->GetMirroredBounds().Contains(point)) {
>     return true;
>   }
> 
>   return false;
> 
> That makes it a bit more readable IMO.

Yeah, I figured I might be getting a little too clever there.  Changed.

> 
>
https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
> File chrome/browser/ui/webui/options/create_profile_handler.cc (right):
> 
>
https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
> chrome/browser/ui/webui/options/create_profile_handler.cc:25: #if
> defined(ENABLE_MANAGED_USERS)
> On 2014/07/14 23:50:03, mckev wrote:
> > On 2014/07/14 16:39:05, Bernhard Bauer wrote:
> > > This file is a bit pushing it with regards to the number of #ifdefs I
think
> > are
> > > palatable. Another approach would be not to #ifdef out any and all code
that
> > > refers to supervised users, but e.g. only code that refers to classes from
> > > c/b/supervised_user/, replace it with NOTREACHED(), and just disable entry
> > > points for the feature (i.e. callbacks from WebUI in this case).
> > 
> > I tend to agree - this is a huge number of #ifdefs that I'd like to avoid as
> > well.  The only issue I foresee with this approach would be that this class
> > wouldn't be compilable, since it would permanently include these files and
all
> > of their dependencies.
> > 
> > For this class at least, would it be feasible to put the supervised user
logic
> > in a separate .h/.cc files, a la create_profile_handler_supervised?  There
> might
> > be some #ifdefs in the main files, but at least we'd be one step closer to
> > factoring it out entirely.
> 
> Maybe -- it's a bit tricky of course, because our goal here was reusing
existing
> profile creation code, not keeping supervised user code separate from other
> code. That being said, if you want to try your hand at it, I'd be willing to
> review patches :)

Point taken.  I'll do the best I can, and I really appreciate your help and
feedback.  In the next patch set I'm about to post I think almost all of the
supervised logic is separated into a single ifdef, which might make a meaningful
refactor possible while still sharing the core profile creation logic.

> 
>
https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
> chrome/browser/ui/webui/options/create_profile_handler.cc:275: if
> (!new_profile->IsSupervised())
> On 2014/07/14 23:50:03, mckev wrote:
> > On 2014/07/14 16:39:06, Bernhard Bauer wrote:
> > > This is also a slight change from the old behavior. If
ENABLE_MANAGED_USERS
> is
> > > not defined, no profile is supervised, so we should always return.
> > 
> > Agreed.  I added an early return at the beginning of the function in that
> case.
> 
> Would it make sense to put the whole method body into one big #ifdef? Then we
> don't need the second one here.

Good point, much nicer.  One less #ifdef!

> 
>
https://chromiumcodereview.appspot.com/355233002/diff/20001/chrome/browser/ui...
> chrome/browser/ui/webui/options/create_profile_handler.cc:308: base::string16
> CreateProfileHandler::GetProfileCreationErrorMessage(
> On 2014/07/14 23:50:03, mckev wrote:
> > On 2014/07/14 16:39:06, Bernhard Bauer wrote:
> > > This whole method is only called for supervised user creation. I would
> restore
> > > it to the old version and put it into the #ifdef as a whole.
> > 
> > It looks there is one exception - it's also called from OnProfileCreated -
> > GetProfileCreationErrorMessage(LOCAL_ERROR).  I think nesting switch
> statements
> > is less than ideal, but I can't think of a better way to consolidate this
> logic
> > completely or without duplicating a few more lines of code.  If you can
> suggest
> > something, I'll be happy to comply.
> 
> Hmm, looking at it in more detail, each of the error types is used exactly
once,
> as a constant argument to this method. We could split this up into three
> methods, two of which only make sense for supervised users (so they could be
> completely wrapped in #ifdef's), so only the third one would have an #ifdef in
> the method body. WDYT?

I like it.  Let me know what you think of the final product.

[Bernhard Bauer, 2014-07-21 08:55:16]

This is coming along nicely! :)

https://codereview.chromium.org/355233002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/355233002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:208: return
HTCLIENT;
This should be only two spaces indented; sorry I missed that previously. You
could also wrap the return statement in braces; I find that to be a bit more
readable if the if-condition is more than one line.

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

https://codereview.chromium.org/355233002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/create_profile_handler.cc:462: bool
CreateProfileHandler::GetSupervisedUserIdFromArgs(
This is meant as a starting point for future refactorings, right? In the long
term I would rather have a single definition for each method (it makes it easier
to grep, for example).

https://codereview.chromium.org/355233002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/create_profile_handler.h (right):

https://codereview.chromium.org/355233002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/create_profile_handler.h:75: std::string&
supervised_user_id);
Outparameters are pointers. You should probably also mention that this will set
the |profile_creation_type_|.

https://codereview.chromium.org/355233002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/create_profile_handler.h:120: base::string16
GetProfileCreationErrorMessageLocal() const;
I think I would actually prefer to move the supervised user creation error
messages here, because that way we have all the different kinds of error
messages together, even if means an additional #ifdef.

[mckev, 2014-07-29 17:59:43]

On 2014/07/21 08:55:16, Bernhard Bauer wrote:
> This is coming along nicely! :)

Thanks!  Sorry for the long delay, I was out for a few days.  I'll have an
updated diff out pretty soon.  Thanks again for the review.

[mckev, 2014-08-05 01:09:14]

bauerb, new patch set uploaded, PTAL.  (Also, ping to jhawkins, pkotwics, ben,
and goodger.)

Thanks!

https://codereview.chromium.org/355233002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/355233002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:208: return
HTCLIENT;
On 2014/07/21 08:55:15, Bernhard Bauer wrote:
> This should be only two spaces indented; sorry I missed that previously. You
> could also wrap the return statement in braces; I find that to be a bit more
> readable if the if-condition is more than one line.

No problem.  I made the change above as well.

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

https://codereview.chromium.org/355233002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/create_profile_handler.cc:462: bool
CreateProfileHandler::GetSupervisedUserIdFromArgs(
On 2014/07/21 08:55:15, Bernhard Bauer wrote:
> This is meant as a starting point for future refactorings, right? In the long
> term I would rather have a single definition for each method (it makes it
easier
> to grep, for example).

Fair point.  As an alternative I've ifdef'd their only usage and removed the
stub implementations, since they're no longer needed.  Let me know your
thoughts.

https://codereview.chromium.org/355233002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/create_profile_handler.h (right):

https://codereview.chromium.org/355233002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/create_profile_handler.h:75: std::string&
supervised_user_id);
On 2014/07/21 08:55:15, Bernhard Bauer wrote:
> Outparameters are pointers. You should probably also mention that this will
set
> the |profile_creation_type_|.

Done.  Changed the method name to be more descriptive, too.

https://codereview.chromium.org/355233002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/create_profile_handler.h:120: base::string16
GetProfileCreationErrorMessageLocal() const;
On 2014/07/21 08:55:15, Bernhard Bauer wrote:
> I think I would actually prefer to move the supervised user creation error
> messages here, because that way we have all the different kinds of error
> messages together, even if means an additional #ifdef.

No problem, done.

[Bernhard Bauer, 2014-08-05 08:33:36]

LGTM! Thanks for your patience with my nitpicking :)

[James Hawkins, 2014-08-05 08:50:49]

https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/downloads_ui_browsertest.h (right):

https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/downloads_ui_browsertest.h:25: class
DownloadsWebUIForSupervisedUsersTest : public DownloadsUIBrowserTest {
I believe we can pull this out into a separate test file (both h/cc) and then we
can guard that the gypi level, obviating the need for source-level #ifdefs.

https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/create_profile_handler.h (right):

https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/create_profile_handler.h:124: #if
defined(ENABLE_MANAGED_USERS)
This is still a ton of code that is only for ENABLE_MANAGED_USERS.  Can we not
refactor it out into a managed users-specific place?

[mckev, 2014-08-05 23:52:37]

On 2014/08/05 08:50:49, James Hawkins wrote:
>
https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
> File chrome/browser/ui/webui/downloads_ui_browsertest.h (right):
> 
>
https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
> chrome/browser/ui/webui/downloads_ui_browsertest.h:25: class
> DownloadsWebUIForSupervisedUsersTest : public DownloadsUIBrowserTest {
> I believe we can pull this out into a separate test file (both h/cc) and then
we
> can guard that the gypi level, obviating the need for source-level #ifdefs.

Yep, no problem, will do.

>
https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
> File chrome/browser/ui/webui/options/create_profile_handler.h (right):
> 
>
https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
> chrome/browser/ui/webui/options/create_profile_handler.h:124: #if
> defined(ENABLE_MANAGED_USERS)
> This is still a ton of code that is only for ENABLE_MANAGED_USERS.  Can we not
> refactor it out into a managed users-specific place?

I think it's possible as I suggested a few weeks ago.  The first step was
extracting the majority of it into a single place (the huge #ifdef block in
these files).  There are still a few places where there is supervised logic is
intertwined that would need to be factored out, but it's pretty close.  As of
the latest diff in this CL, I haven't entirely extracted this class into
separate source code files or classes, but I'd be happy to go a bit further in a
follow-up CL after this one, if that is OK with you.  As an alternative, I can
remove the webui code from this CL and make it a separate CL immediately if
you'd prefer.  Just let me know either way.

Thanks for the review!

[James Hawkins, 2014-08-15 14:37:43]

On 2014/08/05 23:52:37, mckev wrote:
> On 2014/08/05 08:50:49, James Hawkins wrote:
> >
>
https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
> > File chrome/browser/ui/webui/downloads_ui_browsertest.h (right):
> > 
> >
>
https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
> > chrome/browser/ui/webui/downloads_ui_browsertest.h:25: class
> > DownloadsWebUIForSupervisedUsersTest : public DownloadsUIBrowserTest {
> > I believe we can pull this out into a separate test file (both h/cc) and
then
> we
> > can guard that the gypi level, obviating the need for source-level #ifdefs.
> 
> Yep, no problem, will do.
> 
> >
>
https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
> > File chrome/browser/ui/webui/options/create_profile_handler.h (right):
> > 
> >
>
https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
> > chrome/browser/ui/webui/options/create_profile_handler.h:124: #if
> > defined(ENABLE_MANAGED_USERS)
> > This is still a ton of code that is only for ENABLE_MANAGED_USERS.  Can we
not
> > refactor it out into a managed users-specific place?
> 
> I think it's possible as I suggested a few weeks ago.  The first step was
> extracting the majority of it into a single place (the huge #ifdef block in
> these files).  There are still a few places where there is supervised logic is
> intertwined that would need to be factored out, but it's pretty close.  As of
> the latest diff in this CL, I haven't entirely extracted this class into
> separate source code files or classes, but I'd be happy to go a bit further in
a
> follow-up CL after this one, if that is OK with you.  As an alternative, I can
> remove the webui code from this CL and make it a separate CL immediately if
> you'd prefer.  Just let me know either way.
> 
> Thanks for the review!

I'm assuming the remaining fixes are coming in a followup patch set.  Is that
correct?

[mckev, 2014-08-19 00:48:44]

On 2014/08/15 14:37:43, James Hawkins wrote:
> On 2014/08/05 23:52:37, mckev wrote:
> > On 2014/08/05 08:50:49, James Hawkins wrote:
> > >
> >
>
https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
> > > File chrome/browser/ui/webui/downloads_ui_browsertest.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
> > > chrome/browser/ui/webui/downloads_ui_browsertest.h:25: class
> > > DownloadsWebUIForSupervisedUsersTest : public DownloadsUIBrowserTest {
> > > I believe we can pull this out into a separate test file (both h/cc) and
> then
> > we
> > > can guard that the gypi level, obviating the need for source-level
#ifdefs.
> > 
> > Yep, no problem, will do.
> > 
> > >
> >
>
https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
> > > File chrome/browser/ui/webui/options/create_profile_handler.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
> > > chrome/browser/ui/webui/options/create_profile_handler.h:124: #if
> > > defined(ENABLE_MANAGED_USERS)
> > > This is still a ton of code that is only for ENABLE_MANAGED_USERS.  Can we
> not
> > > refactor it out into a managed users-specific place?
> > 
> > I think it's possible as I suggested a few weeks ago.  The first step was
> > extracting the majority of it into a single place (the huge #ifdef block in
> > these files).  There are still a few places where there is supervised logic
is
> > intertwined that would need to be factored out, but it's pretty close.  As
of
> > the latest diff in this CL, I haven't entirely extracted this class into
> > separate source code files or classes, but I'd be happy to go a bit further
in
> a
> > follow-up CL after this one, if that is OK with you.  As an alternative, I
can
> > remove the webui code from this CL and make it a separate CL immediately if
> > you'd prefer.  Just let me know either way.
> > 
> > Thanks for the review!
> 
> I'm assuming the remaining fixes are coming in a followup patch set.  Is that
> correct?

Hi James, sorry for the delay, again.  I'll have the fix for
downloads_ui_browsertest.h by tomorrow morning.

[mckev, 2014-08-25 20:28:02]

On 2014/08/19 00:48:44, mckev wrote:
> On 2014/08/15 14:37:43, James Hawkins wrote:
> > On 2014/08/05 23:52:37, mckev wrote:
> > > On 2014/08/05 08:50:49, James Hawkins wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
> > > > File chrome/browser/ui/webui/downloads_ui_browsertest.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
> > > > chrome/browser/ui/webui/downloads_ui_browsertest.h:25: class
> > > > DownloadsWebUIForSupervisedUsersTest : public DownloadsUIBrowserTest {
> > > > I believe we can pull this out into a separate test file (both h/cc) and
> > then
> > > we
> > > > can guard that the gypi level, obviating the need for source-level
> #ifdefs.
> > > 
> > > Yep, no problem, will do.
> > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
> > > > File chrome/browser/ui/webui/options/create_profile_handler.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
> > > > chrome/browser/ui/webui/options/create_profile_handler.h:124: #if
> > > > defined(ENABLE_MANAGED_USERS)
> > > > This is still a ton of code that is only for ENABLE_MANAGED_USERS.  Can
we
> > not
> > > > refactor it out into a managed users-specific place?
> > > 
> > > I think it's possible as I suggested a few weeks ago.  The first step was
> > > extracting the majority of it into a single place (the huge #ifdef block
in
> > > these files).  There are still a few places where there is supervised
logic
> is
> > > intertwined that would need to be factored out, but it's pretty close.  As
> of
> > > the latest diff in this CL, I haven't entirely extracted this class into
> > > separate source code files or classes, but I'd be happy to go a bit
further
> in
> > a
> > > follow-up CL after this one, if that is OK with you.  As an alternative, I
> can
> > > remove the webui code from this CL and make it a separate CL immediately
if
> > > you'd prefer.  Just let me know either way.
> > > 
> > > Thanks for the review!
> > 
> > I'm assuming the remaining fixes are coming in a followup patch set.  Is
that
> > correct?
> 
> Hi James, sorry for the delay, again.  I'll have the fix for
> downloads_ui_browsertest.h by tomorrow morning.

The new diff is posted, let me know your thoughts and thanks again!

[mckev, 2014-08-25 20:28:47]

https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/downloads_ui_browsertest.h (right):

https://codereview.chromium.org/355233002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/downloads_ui_browsertest.h:25: class
DownloadsWebUIForSupervisedUsersTest : public DownloadsUIBrowserTest {
On 2014/08/05 08:50:48, James Hawkins wrote:
> I believe we can pull this out into a separate test file (both h/cc) and then
we
> can guard that the gypi level, obviating the need for source-level #ifdefs.

Done.

[James Hawkins, 2014-09-02 22:11:01]

https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/downloads_ui_supervised_browsertest.h (right):

https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/downloads_ui_supervised_browsertest.h:12: //
InProcessBrowserTest overrides:
nit: 

// InProcessBrowserTest implementation.

https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:96: #include
"chrome/browser/supervised_user/supervised_user_service.h"
How are these includes used in this file?

https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/create_profile_handler.h (right):

https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/create_profile_handler.h:124: #if
defined(ENABLE_MANAGED_USERS)
Where are we with respect to removing #ifdef checks for ENABLE_MANAGED_USERS?

[mckev, 2014-09-03 20:20:18]

jhawkins, PTAL, thanks!

https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/downloads_ui_supervised_browsertest.h (right):

https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/downloads_ui_supervised_browsertest.h:12: //
InProcessBrowserTest overrides:
On 2014/09/02 22:11:01, James Hawkins wrote:
> nit: 
> 
> // InProcessBrowserTest implementation.

Done.  Thanks for pointing this one out!

https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:96: #include
"chrome/browser/supervised_user/supervised_user_service.h"
On 2014/09/02 22:11:01, James Hawkins wrote:
> How are these includes used in this file?

Good question.  It appears that they aren't used anymore (bauerb@ added this
include over a year ago).  I've nuked them.  One less #ifdef! :-)

https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/create_profile_handler.h (right):

https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/create_profile_handler.h:124: #if
defined(ENABLE_MANAGED_USERS)
On 2014/09/02 22:11:01, James Hawkins wrote:
> Where are we with respect to removing #ifdef checks for ENABLE_MANAGED_USERS?

Good question.

If it's OK with you, I'd like to attempt the refactor of the WebUI code as
suggested in a separate CL, since this CL as-is turned out to be quite a big one
just to fix the overall build breakage.  If that won't work, just let me know.

[James Hawkins, 2014-09-03 20:26:21]

On 2014/09/03 20:20:18, mckev wrote:
> jhawkins, PTAL, thanks!
> 
>
https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/downloads_ui_supervised_browsertest.h (right):
> 
>
https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/downloads_ui_supervised_browsertest.h:12: //
> InProcessBrowserTest overrides:
> On 2014/09/02 22:11:01, James Hawkins wrote:
> > nit: 
> > 
> > // InProcessBrowserTest implementation.
> 
> Done.  Thanks for pointing this one out!
> 
>
https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):
> 
>
https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/extensions/extension_settings_handler.cc:96: #include
> "chrome/browser/supervised_user/supervised_user_service.h"
> On 2014/09/02 22:11:01, James Hawkins wrote:
> > How are these includes used in this file?
> 
> Good question.  It appears that they aren't used anymore (bauerb@ added this
> include over a year ago).  I've nuked them.  One less #ifdef! :-)
> 
>
https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/options/create_profile_handler.h (right):
> 
>
https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/options/create_profile_handler.h:124: #if
> defined(ENABLE_MANAGED_USERS)
> On 2014/09/02 22:11:01, James Hawkins wrote:
> > Where are we with respect to removing #ifdef checks for
ENABLE_MANAGED_USERS?
> 
> Good question.
> 
> If it's OK with you, I'd like to attempt the refactor of the WebUI code as
> suggested in a separate CL, since this CL as-is turned out to be quite a big
one
> just to fix the overall build breakage.  If that won't work, just let me know.

I agree the CL is quite large as-is, so a follow-up CL is OK.  I'm trusting that
you will follow up on this task.

LGTM

[pkotwicz, 2014-09-03 20:33:53]

chrome/browser/themes LGTM

[Bernhard Bauer, 2014-09-03 20:35:38]

Yay!

[mckev, 2014-09-03 20:39:09]

On 2014/09/03 20:26:21, James Hawkins wrote:
> On 2014/09/03 20:20:18, mckev wrote:
> > jhawkins, PTAL, thanks!
> > 
> >
>
https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
> > File chrome/browser/ui/webui/downloads_ui_supervised_browsertest.h (right):
> > 
> >
>
https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
> > chrome/browser/ui/webui/downloads_ui_supervised_browsertest.h:12: //
> > InProcessBrowserTest overrides:
> > On 2014/09/02 22:11:01, James Hawkins wrote:
> > > nit: 
> > > 
> > > // InProcessBrowserTest implementation.
> > 
> > Done.  Thanks for pointing this one out!
> > 
> >
>
https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
> > File chrome/browser/ui/webui/extensions/extension_settings_handler.cc
(right):
> > 
> >
>
https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
> > chrome/browser/ui/webui/extensions/extension_settings_handler.cc:96:
#include
> > "chrome/browser/supervised_user/supervised_user_service.h"
> > On 2014/09/02 22:11:01, James Hawkins wrote:
> > > How are these includes used in this file?
> > 
> > Good question.  It appears that they aren't used anymore (bauerb@ added this
> > include over a year ago).  I've nuked them.  One less #ifdef! :-)
> > 
> >
>
https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
> > File chrome/browser/ui/webui/options/create_profile_handler.h (right):
> > 
> >
>
https://codereview.chromium.org/355233002/diff/100001/chrome/browser/ui/webui...
> > chrome/browser/ui/webui/options/create_profile_handler.h:124: #if
> > defined(ENABLE_MANAGED_USERS)
> > On 2014/09/02 22:11:01, James Hawkins wrote:
> > > Where are we with respect to removing #ifdef checks for
> ENABLE_MANAGED_USERS?
> > 
> > Good question.
> > 
> > If it's OK with you, I'd like to attempt the refactor of the WebUI code as
> > suggested in a separate CL, since this CL as-is turned out to be quite a big
> one
> > just to fix the overall build breakage.  If that won't work, just let me
know.
> 
> I agree the CL is quite large as-is, so a follow-up CL is OK.  I'm trusting
that
> you will follow up on this task.
> 
> LGTM

Thanks, James!  I'll follow-up after this CL lands.

I'll post one more diff after rebasing - I'm sure quite a bit has changed. 
After that, if everything looks good (I think just an lgtm from ben@ is still
pending) - can someone submit this to the CQ?

[Bernhard Bauer, 2014-09-04 18:57:53]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2014-09-04 19:00:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mckev@amazon.com/355233002/120001

[commit-bot: I haz the power, 2014-09-04 23:20:42]

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

[commit-bot: I haz the power, 2014-09-04 23:20:43]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/51806)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/56623)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mckev, 2014-09-16 01:02:23]

On 2014/09/04 23:20:43, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_gpu on tryserver.chromium.gpu
>
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
>   mac_gpu on tryserver.chromium.gpu
>
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/51806)
>   win_gpu on tryserver.chromium.gpu
>
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/56623)
>   android_aosp on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
>   android_arm64_dbg_recipe on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
>   android_chromium_gn_compile_rel on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
>   android_clang_dbg_recipe on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
>   android_dbg_tests_recipe on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
>   linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   linux_chromium_gn_rel on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   linux_chromium_rel_swarming on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   ios_dbg_simulator on tryserver.chromium.mac
>
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
>   ios_rel_device on tryserver.chromium.mac
>
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
>   ios_rel_device_ninja on tryserver.chromium.mac
>
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
>   mac_chromium_compile_dbg on tryserver.chromium.mac
>
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
>   mac_chromium_rel_swarming on tryserver.chromium.mac
>
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
>   win8_chromium_rel on tryserver.chromium.win
>
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
>   win_chromium_compile_dbg on tryserver.chromium.win
>
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
>   win_chromium_rel_swarming on tryserver.chromium.win
>
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
>   win_chromium_x64_rel_swarming on tryserver.chromium.win
>
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

Just a quick update, I'm in the process of rebasing this to ToT.  I'll have a
diff up as soon as it's ready.

[mckev, 2014-09-27 01:04:37]

> Just a quick update, I'm in the process of rebasing this to ToT.  I'll have a
> diff up as soon as it's ready.

Sorry for the long delay - this one took quite some time to rebase.  Can someone
resubmit to the CQ on my behalf?  Thanks!

[Bernhard Bauer, 2014-09-27 05:53:52]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2014-09-27 05:54:21]

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

[commit-bot: I haz the power, 2014-09-27 07:05:24]

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

[commit-bot: I haz the power, 2014-09-27 07:05:27]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mckev, 2014-09-27 19:53:22]

On 2014/09/27 07:05:27, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

OK, I think I identified the problem.  Interesting that the test didn't fail
locally.  I think the updated patch should resolve the problem, though I might
be more comfortable submitting a try job to linux_chromium_chromeos_rel_swarming
first.  I can't submit try jobs, unfortunately.

At your discretion, could you submit the latest patchset to either the CQ or the
try server?  Thanks again for your help!

[mckev, 2014-09-29 17:52:26]

On 2014/09/27 19:53:22, mckev wrote:
> On 2014/09/27 07:05:27, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
> OK, I think I identified the problem.  Interesting that the test didn't fail
> locally.  I think the updated patch should resolve the problem, though I might
> be more comfortable submitting a try job to
linux_chromium_chromeos_rel_swarming
> first.  I can't submit try jobs, unfortunately.
> 
> At your discretion, could you submit the latest patchset to either the CQ or
the
> try server?  Thanks again for your help!

Looks like the try jobs of interest succeeded!  The failures on android_rel and
android_rel_triggered_tests appear to be due to flakiness (sheriffs were
contacted).  To whomever submitted the jobs, thank you!

One last favor, can you somebody submit this patch to the CQ?  Thanks again.

[Bernhard Bauer, 2014-09-29 18:00:44]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2014-09-29 18:01:13]

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

[commit-bot: I haz the power, 2014-09-29 18:52:48]

Committed patchset #9 (id:160001) as 3a48aff912fb805b4af3300dab87fd4c92afbbd7

[commit-bot: I haz the power, 2014-09-29 18:53:30]

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

[mckev, 2015-02-24 21:20:48]

Patchset #10 (id:180001) has been deleted