Show Goodies page to new Chromebook users

chrome/browser/chromeos/first_run/goodies_displayer.h

+// 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.
+
+#ifndef CHROME_BROWSER_CHROMEOS_FIRST_RUN_GOODIES_DISPLAYER_H_
+#define CHROME_BROWSER_CHROMEOS_FIRST_RUN_GOODIES_DISPLAYER_H_
+
+#include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/browser_list_observer.h"
+
+namespace chromeos {
+namespace first_run {
+
+// Handles display of OOBE Goodies page on first display of browser window on
+// new Chromebooks.
+class GoodiesDisplayer : public chrome::BrowserListObserver {
+ public:
+  // Primarily used to create, host, and destroy singleton GoodiesDisplayer.

[achuithb, 2015/09/18 16:39:11]

Drop Primarily, and singleton.

[Greg Levin, 2015/09/18 18:49:27]

Done.

+  class Delegate {
+   public:
+    Delegate();
+    virtual ~Delegate();

[Greg Levin, 2015/09/17 23:21:40]

Is it possible to in-line these?  When I tried I got

error: [chromium-style] Complex con/destructor has an inlined body.

[achuithb, 2015/09/18 16:39:11]

No, you cannot/should not since this class owns a scoped_ptr.

You should make the dtor protected.

THe scoped_ptr should be owned by UserSessionManager. This class is supposed to
just be an interface. A TestDelegate would implement this interface, right?

[Greg Levin, 2015/09/18 18:49:27]

Acknowledged.
Didn't make dtor protected since class contains implementation and is
instantiated.

+
+    // Create singleton object if it doesn't already exist.
+    void CreateGoodiesDisplayer();
+
+    // Destroy singleton object.
+    void DestroyGoodiesDisplayer();
+
+    // Get age of device; overridable for testing.
+    virtual base::TimeDelta GetTimeSinceOobe();

[Greg Levin, 2015/09/17 23:21:40]

It appears to be atypical to put implementation in the base delegate class, but
that seems to make more sense here.  The UserSessionManager and BrowserTest
implementations should be identical for everything except GetTimeSinceOobe() (at
least, based on my current thoughts).  And the base implementation I have for
this function is the one "correct" one, so making this function pure virtual and
writing this implementation over in UserSessionManager didn't seem right either.
 I just need to override the "correct" version for testing.

That's my intuition here, but my intuition on these things is often wrong :-P

[achuithb, 2015/09/18 16:39:11]

No, you're not allowed to do this. You can only inherit from pure interface
classes.

You copy paste the common methods for the TestDelegate, and that's totally fine.

[Greg Levin, 2015/09/18 18:49:27]

Acknowledged.

+
+   private:
+    // Holds singleton GoodiesDisplayer object.
+    scoped_ptr<GoodiesDisplayer> goodies_displayer_;
+  };

[achuithb, 2015/09/18 16:39:11]

DISALLOW_COPY_AND_ASSIGN

[Greg Levin, 2015/09/18 18:49:27]

Done.

+
+  explicit GoodiesDisplayer(Delegate* delegate);
+  ~GoodiesDisplayer() override;
+
+  static void Init(Delegate* delegate);
+
+ private:
+  // Overridden from chrome::BrowserListObserver.
+  void OnBrowserSetLastActive(Browser* browser) override;
+
+  Delegate* delegate_;

[Greg Levin, 2015/09/17 23:21:40]

Is there a smart pointer type I should use here instead?  The delegate owns this
object, and should destroy it before the delegate is itself destroyed, so this
shouldn't be a problem...?

[achuithb, 2015/09/18 16:39:11]

It shouldn't be a problem, but you should add a comment like:
// Pointer to owner.
or
// Not owned.

[Greg Levin, 2015/09/18 18:49:27]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(GoodiesDisplayer);
+};
+
+}  // namespace first_run
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_FIRST_RUN_GOODIES_DISPLAYER_H_