Show Goodies page to new Chromebook users

chrome/browser/chromeos/first_run/goodies_displayer.cc

+// Copyright 2015 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/first_run/goodies_displayer.h"
+
+#include "base/lazy_instance.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/chromeos/login/startup_utils.h"
+#include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
+#include "chrome/browser/prefs/pref_service_syncable.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_tabstrip.h"
+#include "chrome/common/pref_names.h"
+#include "content/public/browser/browser_thread.h"
+
+namespace chromeos {
+namespace first_run {
+
+#if defined(OS_CHROMEOS)
+
+namespace {
+
+// ChromeOS Goodies page for device's first New Window.
+const char kGoodiesURL[] = "https://www.google.com/chrome/devices/goodies.html";
+
+// Max days after initial login that we're willing to show Goodies.
+static const int kMaxDaysAfterOobeForGoodies = 14;
+
+base::LazyInstance<GoodiesDisplayer> g_goodies_displayer =

[achuithb, 2015/09/15 18:24:32]

I don't see any reason why this should be a singleton.

You can create this object in UpdateGoodiesPrefCantShow, and DeleteSoon it
OnBrowserSetLastActive

[Greg Levin, 2015/09/15 21:44:48]

My concern was the case when User A logs in and out w/o showing browser, then
User B logs in.  This would create a second instance, and unless I cleaned up
the first one on logout (and user switch, if User A logs back in via Multiuser
from B's session), they'd both be observing.  It seemed easier to have a
singleton, since it will use whichever profile is attached to the browser
window.

[achuithb, 2015/09/16 03:40:29]

Hmm, are you sure that's possible? What about deleting OnBrowserRemoved?

This isn't really a singleton, it just feels like we're trying to punt the
lifetime management problem.

[Greg Levin, 2015/09/16 16:55:13]

|> Hmm, are you sure that's possible?

Yes, I've tested it.  Since I'm calling GoodiesDisplayer::Init() from
UserSessionManager::FinalizePrepareProfile(), it does get called for each user
login, and if a second user logs in before a browser has been opened, this will
try to Init() a second GoodiesDisplayer while the first one is still running. 
Since we only need one, a singleton seemed appropriate.

|> What about deleting OnBrowserRemoved?

GoodiesDisplayer::OnBrowserRemoved() would never be called.  It's called when a
browser window is closed, but the observer would have been removed before that,
when that browser window was opened.

|> This isn't really a singleton, it just feels like we're trying to punt the
|> lifetime management problem.

I don't have much experience with singletons in our code, so I'm not sure why
not.  It doesn't really belong to any other class, and we don't need (and in
fact, don't want) more than one.  And we need to keep it around, possibly across
multiple  simultaneous sessions, until it gets used.

[achuithb, 2015/09/16 17:17:30]

I view singletons as being some kind of service that gets launched and is used
from multiple places multiple times.

Here you seem to have a one-time task that we haven't found a good destruction
point for.

Singletons are considered an anti-pattern and are supposed to be used sparingly.


I'm sure we have signals for when the profile switches, which seems to be a good
place to delete this object?

If one of your other reviewers is fine with the use of a singleton here, I can
lg this change, but personally I think we can do better.

[Greg Levin, 2015/09/16 20:48:23]

Done-  Moved instance to be member of UserSessionManager.  Thanks for the
insights on this!

+    LAZY_INSTANCE_INITIALIZER;
+
+// Checks timestamp on OOBE Complete flag file.  kCanShowOobeGoodiesPage
+// defaults to |true|; we set it to |false| (return |false|) for any device over
+// kMaxDaysAfterOobeForGoodies days old, to avoid showing it after update on
+// older devices.
+static bool CheckGoodiesPrefAgainstOobeTimestamp() {

[achuithb, 2015/09/15 18:20:54]

Don't need static. anon namespace functions do not have external linkage

[Greg Levin, 2015/09/15 21:44:48]

Done.

+  DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);

[achuithb, 2015/09/15 18:20:54]

BrowserThread::FILE is deprecated. You should use BlockingPool

[Greg Levin, 2015/09/15 21:44:48]

Done.

+  const base::FilePath oobe_timestamp_file =
+      StartupUtils::GetOobeCompleteFlagPath();
+  base::File::Info fileInfo;
+  if (base::GetFileInfo(oobe_timestamp_file, &fileInfo)) {
+    base::TimeDelta time_since_oobe =

[achuithb, 2015/09/15 18:20:54]

const

[Greg Levin, 2015/09/15 21:44:48]

Done.

+        base::Time::Now() - fileInfo.creation_time;
+    if (time_since_oobe >
+        base::TimeDelta::FromDays(kMaxDaysAfterOobeForGoodies))
+      return false;
+  }
+  return true;
+}
+
+// Callback into main thread to set pref to |false| if too long since oobe, or
+// to create GoodiesDisplayer otherwise.
+static void UpdateGoodiesPrefCantShow(bool can_show_goodies) {

[achuithb, 2015/09/15 18:20:54]

No static

[Greg Levin, 2015/09/15 21:44:48]

Done.

+  if (can_show_goodies) {
+    g_goodies_displayer.Get();
+  } else {
+    PrefService* prefs = g_browser_process->local_state();

[achuithb, 2015/09/15 18:20:54]

No reason to have this temporary variable prefs

[Greg Levin, 2015/09/15 21:44:48]

Done.

+    prefs->SetBoolean(prefs::kCanShowOobeGoodiesPage, false);
+  }
+}
+
+}  // namespace
+
+GoodiesDisplayer::GoodiesDisplayer() {
+  BrowserList::AddObserver(this);
+}
+
+// If conditions enumerated below are met, this loads the Oobe Goodies page for
+// new Chromebooks; when appropriate, it uses pref to mark page as shown,
+// removes itself from BrowserListObservers, and deletes itself.
+void GoodiesDisplayer::OnBrowserSetLastActive(Browser* browser) {
+  // 1. Not guest or incognito session (keep observing).
+  if (browser->profile()->IsOffTheRecord())
+    return;
+
+  PrefService* local_state = g_browser_process->local_state();
+  DCHECK(local_state);

[achuithb, 2015/09/15 18:20:53]

No point in having this DCHECK.

[Greg Levin, 2015/09/15 21:44:48]

Done.

+  // 2. Not previously shown, or otherwise marked as unavailable.
+  if (local_state->GetBoolean(prefs::kCanShowOobeGoodiesPage)) {
+    // 3. Device not enterprise enrolled.
+    if (!g_browser_process->platform_part()
+             ->browser_policy_connector_chromeos()
+             ->IsEnterpriseManaged())
+      chrome::AddTabAt(browser, GURL(kGoodiesURL), -1, false);
+
+    // Set to |false| whether enterprise enrolled or Goodies shown.
+    local_state->SetBoolean(prefs::kCanShowOobeGoodiesPage, false);
+  }
+
+  // Regardless of how we got here, we don't henceforth need to show Goodies.
+  BrowserList::RemoveObserver(this);
+}
+
+#endif  // defined(OS_CHROMEOS)
+
+// Check time since Oobe completion on FILE thread.
+void InitializeGoodiesDisplayer() {
+#if defined(OS_CHROMEOS)
+  PrefService* prefs = g_browser_process->local_state();
+  if (prefs->GetBoolean(prefs::kCanShowOobeGoodiesPage))
+    content::BrowserThread::PostTaskAndReplyWithResult(

[achuithb, 2015/09/15 18:20:53]

Use BlockingPool

[Greg Levin, 2015/09/15 21:44:48]

Done.

+        content::BrowserThread::FILE, FROM_HERE,
+        base::Bind(&CheckGoodiesPrefAgainstOobeTimestamp),
+        base::Bind(&UpdateGoodiesPrefCantShow));
+#endif  // defined(OS_CHROMEOS)
+}
+
+}  // namespace first_run
+}  // namespace chromeos