Notify ThemeService via callback when supervised user is ready.

Notify ThemeService via callback when supervised user is ready.

We didn't show the supervised user theme when a supervised user
was created. This CL fixes this by running a callback when the
supervised user profile is fully initialized. This also fixes
the bug that the label string was not drawn on Windows.

BUG=265387, 269718
TEST=Created a supervised user, added a unit test.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216917

[Adrian Kuegel, 2013-08-08 11:15:20]

Bernhard, can you please review this CL?
Peter, can you please look at my changes in chrome/browser/themes ?

[Bernhard Bauer, 2013-08-08 15:12:45]

https://codereview.chromium.org/22677002/diff/1/chrome/browser/managed_mode/m...
File chrome/browser/managed_mode/managed_user_service.h (right):

https://codereview.chromium.org/22677002/diff/1/chrome/browser/managed_mode/m...
chrome/browser/managed_mode/managed_user_service.h:144: void
AddManagedUserInitCallback(const base::Closure& callback);
Nit: Talking about "managed user" probably isn't necessary here.

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

https://codereview.chromium.org/22677002/diff/1/chrome/browser/themes/theme_s...
chrome/browser/themes/theme_service.cc:35: #include
"chrome/browser/managed_mode/managed_user_theme.h"
I'm pretty sure this is never built on a platform where ENABLE_MANAGED_USERS is
not defined (as evidenced by the fact that we have an unguarded reference to
ManagedUserTheme below). I don't think #ifdef'ing the includes out here gains us
anything if it's not enforced.

[Adrian Kuegel, 2013-08-08 15:26:34]

https://codereview.chromium.org/22677002/diff/1/chrome/browser/managed_mode/m...
File chrome/browser/managed_mode/managed_user_service.h (right):

https://codereview.chromium.org/22677002/diff/1/chrome/browser/managed_mode/m...
chrome/browser/managed_mode/managed_user_service.h:144: void
AddManagedUserInitCallback(const base::Closure& callback);
On 2013/08/08 15:12:45, Bernhard Bauer wrote:
> Nit: Talking about "managed user" probably isn't necessary here.

Done.

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

https://codereview.chromium.org/22677002/diff/1/chrome/browser/themes/theme_s...
chrome/browser/themes/theme_service.cc:35: #include
"chrome/browser/managed_mode/managed_user_theme.h"
On 2013/08/08 15:12:45, Bernhard Bauer wrote:
> I'm pretty sure this is never built on a platform where ENABLE_MANAGED_USERS
is
> not defined (as evidenced by the fact that we have an unguarded reference to
> ManagedUserTheme below). I don't think #ifdef'ing the includes out here gains
us
> anything if it's not enforced.

Right, I didn't notice that the ManagedUserTheme is not guarded.

[pkotwicz, 2013-08-08 17:02:28]

I will take a look at this tonight

[pkotwicz, 2013-08-09 05:47:08]

Let's put in a hacky fix for M30 and focus on getting a proper fix for M31.

The proper fix would be to move back the initialization of the ThemeService to
after the ManagedUserService is inited.
In order to do this we would need to make
ExtensionService::NotifyExtensionLoaded() not depend on ThemeService anymore via
ThemeSource. I suppose, the way to do that would be by creating the ThemeSource
only after the ThemeService has finished initing.

https://codereview.chromium.org/22677002/diff/15001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/22677002/diff/15001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:95: &ThemeService::SetManagedUserTheme,
weak_ptr_factory_.GetWeakPtr()));
If the managed user picked a custom theme, this would revert that?

[Adrian Kuegel, 2013-08-09 08:56:12]

Peter, you are right, this is a temporary fix for M30. I talked with Bernhard,
and it seems it is not so easy to get the initializations done in the right
order. We will look into that for M31.
I also noticed that we now need a SetCustomDefaultTheme function in
gtk_theme_service.cc which makes sure that the default values are loaded. But I
guess this should be there, anyway, although it only makes a difference after we
use that function outside of LoadThemePrefs.

https://codereview.chromium.org/22677002/diff/15001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/22677002/diff/15001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:95: &ThemeService::SetManagedUserTheme,
weak_ptr_factory_.GetWeakPtr()));
On 2013/08/09 05:47:08, pkotwicz wrote:
> If the managed user picked a custom theme, this would revert that?

Right, I forgot about this. Thanks for noticing.

[pkotwicz, 2013-08-09 14:23:02]

Looks good, can you add a new test which tests that we end up with the managed
user theme for a new managed user profile?

https://codereview.chromium.org/22677002/diff/33001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/22677002/diff/33001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:470: if (theme_supplier_.get() ||
!IsManagedUser())
This code is temporary. However, please add a comment discussing what you are
doing.
For instance, it does matter that OnManagedUserInitialized() will always occur
after NOTIFICATION_EXTENSIONS_READY notification.
The initialization of the ManagedUserService is not really interesting per se,
we really care that ProfileManager::InitProfileUserPrefs() has run.
The point is, this code is pretty subtle and worth a comment.

https://codereview.chromium.org/22677002/diff/33001/chrome/browser/ui/gtk/gtk...
File chrome/browser/ui/gtk/gtk_theme_service.cc (right):

https://codereview.chromium.org/22677002/diff/33001/chrome/browser/ui/gtk/gtk...
chrome/browser/ui/gtk/gtk_theme_service.cc:357:
profile()->GetPrefs()->SetBoolean(prefs::kUsesSystemTheme, false);
For the sake of consistency, do you need to also set |use_gtk_|? I do not think
it is needed but GtkThemeService::UseDefaultTheme() does this.

[Adrian Kuegel, 2013-08-09 15:43:01]

Thanks for the early review :)
I added a unit test which simulates the current initialization of a new profile.

https://codereview.chromium.org/22677002/diff/33001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/22677002/diff/33001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:470: if (theme_supplier_.get() ||
!IsManagedUser())
On 2013/08/09 14:23:02, pkotwicz wrote:
> This code is temporary. However, please add a comment discussing what you are
> doing.
> For instance, it does matter that OnManagedUserInitialized() will always occur
> after NOTIFICATION_EXTENSIONS_READY notification.
> The initialization of the ManagedUserService is not really interesting per se,
> we really care that ProfileManager::InitProfileUserPrefs() has run.
> The point is, this code is pretty subtle and worth a comment.

Done.

https://codereview.chromium.org/22677002/diff/33001/chrome/browser/ui/gtk/gtk...
File chrome/browser/ui/gtk/gtk_theme_service.cc (right):

https://codereview.chromium.org/22677002/diff/33001/chrome/browser/ui/gtk/gtk...
chrome/browser/ui/gtk/gtk_theme_service.cc:357:
profile()->GetPrefs()->SetBoolean(prefs::kUsesSystemTheme, false);
On 2013/08/09 14:23:02, pkotwicz wrote:
> For the sake of consistency, do you need to also set |use_gtk_|? I do not
think
> it is needed but GtkThemeService::UseDefaultTheme() does this.

Right. I also think it should be in the SetTheme() function, I added it there as
well.

[Adrian Kuegel, 2013-08-09 15:44:23]

Elliot, can you please approve my changes to gtk_theme_service.*?

[pkotwicz, 2013-08-09 16:49:36]

LGTM

https://codereview.chromium.org/22677002/diff/45001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/22677002/diff/45001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:471: // before the boolean flag
indicating it is a supervised user profile gets
Nit: "indicating the profile belongs to a supervised user gets set"

https://codereview.chromium.org/22677002/diff/45001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:472: // set. So in order to get the
custom managed user theme, we get a callback
Nit: "In order to get the custom managed user theme,"

https://codereview.chromium.org/22677002/diff/45001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:474: // boolean flag has been set in
ProfileManager::InitProfileUserPrefs().
Also mention that it occurs after the NOTIFICATION_EXTENSIONS_READY notification
is sent.

https://codereview.chromium.org/22677002/diff/45001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service_unittest.cc (right):

https://codereview.chromium.org/22677002/diff/45001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:51: EXPECT_TRUE(success);
Nit: ASSERT_TRUE() is probably more appropriate.

https://codereview.chromium.org/22677002/diff/45001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:122: TEST_F(ThemeServiceTest,
ManagedUserThemeNewUser) {
I was hoping we could get a browsertest with a real profile. However, I am not
sure if that is possible and I will not block this CL on a test.
The lofty goal is that we would never have to change the test and the test would
stay valid regardless of how we change the order in which ThemeService gets
initialized relative to ExtensionService and ManagedUserService

https://codereview.chromium.org/22677002/diff/45001/chrome/browser/ui/gtk/gtk...
File chrome/browser/ui/gtk/gtk_theme_service.h (right):

https://codereview.chromium.org/22677002/diff/45001/chrome/browser/ui/gtk/gtk...
chrome/browser/ui/gtk/gtk_theme_service.h:80: scoped_refptr<CustomThemeSupplier>
theme_supplier) OVERRIDE;
Nit: Order SetCustomDefaultTheme() to match order in theme_service.h

[Elliot Glaysher, 2013-08-09 17:33:29]

lgtm

[Bernhard Bauer, 2013-08-09 19:56:26]

lgtm

[Adrian Kuegel, 2013-08-09 21:21:59]

https://codereview.chromium.org/22677002/diff/45001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/22677002/diff/45001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:471: // before the boolean flag
indicating it is a supervised user profile gets
On 2013/08/09 16:49:37, pkotwicz wrote:
> Nit: "indicating the profile belongs to a supervised user gets set"

Done.

https://codereview.chromium.org/22677002/diff/45001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:472: // set. So in order to get the
custom managed user theme, we get a callback
On 2013/08/09 16:49:37, pkotwicz wrote:
> Nit: "In order to get the custom managed user theme,"

Done.

https://codereview.chromium.org/22677002/diff/45001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:474: // boolean flag has been set in
ProfileManager::InitProfileUserPrefs().
On 2013/08/09 16:49:37, pkotwicz wrote:
> Also mention that it occurs after the NOTIFICATION_EXTENSIONS_READY
notification
> is sent.

Done.

https://codereview.chromium.org/22677002/diff/45001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service_unittest.cc (right):

https://codereview.chromium.org/22677002/diff/45001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:51: EXPECT_TRUE(success);
On 2013/08/09 16:49:37, pkotwicz wrote:
> Nit: ASSERT_TRUE() is probably more appropriate.

Done.

https://codereview.chromium.org/22677002/diff/45001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:122: TEST_F(ThemeServiceTest,
ManagedUserThemeNewUser) {
On 2013/08/09 16:49:37, pkotwicz wrote:
> I was hoping we could get a browsertest with a real profile. However, I am not
> sure if that is possible and I will not block this CL on a test.
> The lofty goal is that we would never have to change the test and the test
would
> stay valid regardless of how we change the order in which ThemeService gets
> initialized relative to ExtensionService and ManagedUserService

Ok, I will try to write a browser test for this, but I guess it will be too late
for M30. Anyway, I agree it would be better to have a test that does not need to
be rewritten if we fix the initialization order.

https://codereview.chromium.org/22677002/diff/45001/chrome/browser/ui/gtk/gtk...
File chrome/browser/ui/gtk/gtk_theme_service.h (right):

https://codereview.chromium.org/22677002/diff/45001/chrome/browser/ui/gtk/gtk...
chrome/browser/ui/gtk/gtk_theme_service.h:80: scoped_refptr<CustomThemeSupplier>
theme_supplier) OVERRIDE;
On 2013/08/09 16:49:37, pkotwicz wrote:
> Nit: Order SetCustomDefaultTheme() to match order in theme_service.h

Done.

[commit-bot: I haz the power, 2013-08-09 21:24:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akuegel@chromium.org/22677002/40002

[commit-bot: I haz the power, 2013-08-09 22:05:36]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-10 06:24:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akuegel@chromium.org/22677002/40002

[commit-bot: I haz the power, 2013-08-10 15:40:40]

Failed to request the patch to try. Please note that binary files are still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 500: 
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>

[commit-bot: I haz the power, 2013-08-10 21:14:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akuegel@chromium.org/22677002/40002

[commit-bot: I haz the power, 2013-08-12 04:51:17]

Change committed as 216917