Activate the VariationsService for ChromeOS and ensure that it does not ping the server until the E…

chrome/browser/chromeos/login/base_login_display_host.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/login/base_login_display_host.h"
 
 #include "ash/shell.h"
 #include "ash/shell_window_ids.h"
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 11 matching lines @@
 #include "chrome/browser/chromeos/login/existing_user_controller.h"
 #include "chrome/browser/chromeos/login/helper.h"
 #include "chrome/browser/chromeos/login/language_switch_menu.h"
 #include "chrome/browser/chromeos/login/login_utils.h"
 #include "chrome/browser/chromeos/login/login_wizard.h"
 #include "chrome/browser/chromeos/login/user_manager.h"
 #include "chrome/browser/chromeos/login/webui_login_display_host.h"
 #include "chrome/browser/chromeos/login/wizard_controller.h"
 #include "chrome/browser/chromeos/mobile_config.h"
 #include "chrome/browser/chromeos/system/timezone_settings.h"
+#include "chrome/browser/metrics/variations/variations_service.h"
 #include "chrome/browser/policy/auto_enrollment_client.h"
 #include "chrome/browser/policy/browser_policy_connector.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/session_manager_client.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
@@ skipping 147 matching lines @@
     const std::string& first_screen_name,
     DictionaryValue* screen_parameters) {
   DVLOG(1) << "Starting wizard, first_screen_name: " << first_screen_name;
   // Create and show the wizard.
   // Note, dtor of the old WizardController should be called before ctor of the
   // new one, because "default_controller()" is updated there. So pure "reset()"
   // is done before new controller creation.
   wizard_controller_.reset();
   wizard_controller_.reset(CreateWizardController());
 
+  g_browser_process->variations_service()->
+      RegisterForEulaAcceptedNotification(wizard_controller_.get());

[Alexei Svitkine (slow), 2012/09/14 20:50:16]

I don't like this at all. I understand you're trying to solve the problem of
using "all sources" for notifications, but this now introduces a lot of extra
dependencies between things that shouldn't have dependencies. Wizard controller
now has to know about variations service and variations service has to know
about wizard controller.

Additionally, any class using ResourceRequestAllowedNotifier would now have to
do this sort of thing too.

I understand that the problem is that it's not clear how to get at an instance
of WizardController - and perhaps it may not even exist. However, I have another
proposal of how to solve this problem.

For the notification source, you just need a pointer to something. Let's add a
static member variable to WizardController, let's say
notification_source_token_. Then, we can have a static
WizardController::GetNotificationSource() function that returns
&notification_source_token_.

ResourceRequestAllowedNotifier could then register for the notification using
WizardController::GetNotificationSource() while VariationsService could be
completely oblivious of the wizard controller and vice versa. 

[SteveT, 2012/09/14 20:52:45]

Hmm, this seems less hacky in the sense that the deps are cleaned up, but still
hacky in the sense that we're abusing NotificationSource.

Ilya, what do you think?

[Ilya Sherman, 2012/09/14 20:55:43]

Ick.  It looks like the WizardController class provides a default_controller()
method.  Can we use that instead?  Or, perhaps someone more knowledgeable with
the ChromeOS structure can suggest a better option?

[Alexei Svitkine (slow), 2012/09/14 20:59:21]

It looks like that is set to NULL until the wizard actually gets created.

[SteveT, 2012/09/14 21:01:02]

That was my original approach WAAAAYY back. The issue is that
default_controller() is NULL (the Wizard has not been created) at the time
VariationsService is created. So either way, we'd have to make a call within the
Wizard code.

Nikita and I discussed possibly listening to this other Notification that fires
when WizardController has been created, but that involves using AllSources ;)

[Ilya Sherman, 2012/09/14 21:05:17]

I didn't see Alexei's suggestion until after I had mailed my previous comment. 
This proposal sounds like it is entirely equivalent to using AllSources(), but
just adds extra code to do so.  (Admittedly, that's more or less true of my
suggestion to use default_controller() as well :()  It does have the advantage
that anyone working on the WizardController class would be more likely to notice
this and update the dependencies if we ever need to listen to more fine-grained
notifications...

I think it would be helpful for someone who is more familiar with the
WizardController code to chime in.  Right now, the class is structured to
essentially be a singleton.  Since we don't use singletons much in Chrome code,
it's harder to get a feel for what the best way to integrate with notifications
is.

Here's another way to think about this issue: Who owns the WizardController, and
the WizardController's owner, and the owner of that?  At any point in this
ownership chain, do we reach a concrete object that the VariationsService can
also grasp?  If so, perhaps that object should be treated as the source of the
notification?

[SteveT, 2012/09/14 21:09:24]

That's a possibility, assuming that common object exists. The WizardController
lives in the scary pre-first-Chrome-window land of CrOS.

Nikita - any thoughts on this?

[Nikita (slow), 2012/09/17 07:57:47]

Instance of LoginDisplayHost/BaseLoginDisplayHost owns controller:
wizard_controller_.reset(CreateWizardController());

There's a BaseLoginDisplayHost::default_host() but keep in mind that
LoginDisplayHost instance that it returns only exists during login.

I would actually step back a bit and list all possible entry points that
VariationService should handle.

1. Normal boot on Chrome OS - boot into login screen.
BaseLoginDisplayHost::default_host() does exist on boot but we're not going
through OOBE so WizardController is not created too.

Solution: Rely on WizardController::IsEulaAccepted static method that returns
value of Local State pref.

2. OOBE boot. 
BaseLoginDisplayHost::default_host() does exist on boot, then instance of
WizardController is created - these lines.
That's the only case when EULA is not accepted on boot so you have to subscribe
to notification / delegate call etc.

3. Restart after crash (inside user session). This goes through special code
path because SessionManager passes --login-user=<...> switch meaning that
session has already started.
LoginDisplayHost instance is not created, same for WizardController. See code
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chromeos/chr...

VariationService should use IsEulaAccepted() as in #1 case.

Btw, this case is also used for guest mode which essentially is done as Chrome
restart on login screen.

As was discussed here in the comments I don't think that we ever plan to
introduce multiple EULAs on ChromeOS meaning that each user would have to accept
it. You should not target this case and assume there's only single global EULA
accepted during out ouf box flow (OOBE).

[SteveT, 2012/09/17 14:32:28]

From offline discussion with Nikita:

It looks like the first call to (possibly) show the wizard is in PostProfileInit
(http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/ch...),
which happens before VariationsService attempts its initial ping.

This means that when the VS attempts its first ping, it (or rather, the
RRANotifier) can check to see if WizardController::default_controller() is not
NULL, and register itself for the notification* at that point. This removes the
coupling of WizCtrl with VS.

We should also be able to DCHECK that default_controller is not NULL if
IsEulaAccepted had returned false at that time, give the discussion of how
IsEulaAccepted should always return true after the first accept... otherwise if
it was not accepted it will initiate the OOBE login wizard.

Note that all this stuff (including VS' first ping attempt) happens before the
main msg loop, so the Wizard UI won't be shown to users yet, so we avoid any
races where the user accepts the Eula around the time that the VS checks
it/registers for notifications.

+
   oobe_progress_bar_visible_ = !WizardController::IsDeviceRegistered();
   SetOobeProgressBarVisible(oobe_progress_bar_visible_);
   wizard_controller_->Init(first_screen_name, screen_parameters);
 }
 
 void BaseLoginDisplayHost::StartSignInScreen() {
   DVLOG(1) << "Starting sign in screen";
   const chromeos::UserList& users = chromeos::UserManager::Get()->GetUsers();
 
   // Fix for users who updated device and thus never passed register screen.
@@ skipping 358 matching lines @@
   VLOG(1) << "Initial time zone: " << timezone_name;
   // Apply locale customizations only once to preserve whatever locale
   // user has changed to during OOBE.
   if (!timezone_name.empty()) {
     chromeos::system::TimezoneSettings::GetInstance()->SetTimezoneFromID(
         UTF8ToUTF16(timezone_name));
   }
 }
 
 }  // namespace chromeos