RLZ on ChromeOS-related polishing.

Follow-up to https://chromiumcodereview.appspot.com/11506006/.


BUG=None
TEST=No changes
TBR=sky@chromium.org


Review URL: https://chromiumcodereview.appspot.com/11645040

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@174607 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 65 insertions, 56 deletions

diff --git a/chrome/browser/rlz/rlz.cc b/chrome/browser/rlz/rlz.cc
index 4ed0f05..bad1e29 100644
--- a/chrome/browser/rlz/rlz.cc
+++ b/chrome/browser/rlz/rlz.cc
@@ -25,6 +25,7 @@
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/ui/startup/startup_browser_creator.h"
 #include "chrome/common/chrome_notification_types.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_entry.h"
@@ -59,8 +60,8 @@ namespace {
 
 // Maximum and minimum delay for financial ping we would allow to be set through
 // master preferences. Somewhat arbitrary, may need to be adjusted in future.
-const int kMaxDelay = 200 * 1000;
-const int kMinDelay = 20 * 1000;
+const base::TimeDelta kMaxInitDelay = base::TimeDelta::FromSeconds(200);
+const base::TimeDelta kMinInitDelay = base::TimeDelta::FromSeconds(20);
 
 bool IsGoogleUrl(const GURL& url) {
   return google_util::IsGoogleHomePageUrl(url.possibly_invalid_spec());
@@ -189,7 +190,7 @@ RLZTracker::RLZTracker()
       already_ran_(false),
       omnibox_used_(false),
       homepage_used_(false),
-      min_delay_(kMinDelay) {
+      min_init_delay_(kMinInitDelay) {
 }
 
 RLZTracker::~RLZTracker() {
@@ -197,18 +198,21 @@ RLZTracker::~RLZTracker() {
 
 // static
 bool RLZTracker::InitRlzDelayed(bool first_run,
-                                int delay,
+                                bool send_ping_immediately,
+                                base::TimeDelta delay,
                                 bool is_google_default_search,
                                 bool is_google_homepage,
                                 bool is_google_in_startpages) {
-  return GetInstance()->Init(first_run, delay, is_google_default_search,
-                             is_google_homepage, is_google_in_startpages);
+  return GetInstance()->Init(first_run, send_ping_immediately, delay,
+                             is_google_default_search, is_google_homepage,
+                             is_google_in_startpages);
 }
 
 // static
 bool RLZTracker::InitRlzFromProfileDelayed(Profile* profile,
                                            bool first_run,
-                                           int delay) {
+                                           bool send_ping_immediately,
+                                           base::TimeDelta delay) {
   bool is_google_default_search = false;
   TemplateURLService* template_url_service =
       TemplateURLServiceFactory::GetForProfile(profile);
@@ -233,7 +237,7 @@ bool RLZTracker::InitRlzFromProfileDelayed(Profile* profile,
                                             IsGoogleUrl) > 0;
   }
 
-  if (!InitRlzDelayed(first_run, delay,
+  if (!InitRlzDelayed(first_run, send_ping_immediately, delay,
                       is_google_default_search, is_google_homepage,
                       is_google_in_startpages)) {
     return false;
@@ -248,7 +252,8 @@ bool RLZTracker::InitRlzFromProfileDelayed(Profile* profile,
 }
 
 bool RLZTracker::Init(bool first_run,
-                      int delay,
+                      bool send_ping_immediately,
+                      base::TimeDelta delay,
                       bool is_google_default_search,
                       bool is_google_homepage,
                       bool is_google_in_startpages) {
@@ -256,19 +261,13 @@ bool RLZTracker::Init(bool first_run,
   is_google_default_search_ = is_google_default_search;
   is_google_homepage_ = is_google_homepage;
   is_google_in_startpages_ = is_google_in_startpages;
+  send_ping_immediately_ = send_ping_immediately;
 
-  // A negative delay means that a financial ping should be sent immediately
-  // after a first search is recorded, without waiting for the next restart
-  // of chrome. However, we only want this behaviour on first run.
-  send_ping_immediately_ = false;
-  if (delay < 0) {
-    send_ping_immediately_ = true;
-    delay = -delay;
-  }
+  // Enable zero delays for testing.
+  if (CommandLine::ForCurrentProcess()->HasSwitch(::switches::kTestType))
+    EnableZeroDelayForTesting();
 
-  delay *= 1000;
-  delay = (delay < min_delay_) ? min_delay_ : delay;
-  delay = (delay > kMaxDelay) ? kMaxDelay : delay;
+  delay = std::min(kMaxInitDelay, std::max(min_init_delay_, delay));
 
   if (google_util::GetBrand(&brand_) && !IsBrandOrganic(brand_)) {
     // Register for notifications from the omnibox so that we can record when
@@ -293,14 +292,14 @@ bool RLZTracker::Init(bool first_run,
   return true;
 }
 
-void RLZTracker::ScheduleDelayedInit(int delay) {
+void RLZTracker::ScheduleDelayedInit(base::TimeDelta delay) {
   // The RLZTracker is a singleton object that outlives any runnable tasks
   // that will be queued up.
   BrowserThread::GetBlockingPool()->PostDelayedSequencedWorkerTask(
       worker_pool_token_,
       FROM_HERE,
       base::Bind(&RLZTracker::DelayedInit, base::Unretained(this)),
-      base::TimeDelta::FromMilliseconds(delay));
+      delay);
 }
 
 void RLZTracker::DelayedInit() {
@@ -457,7 +456,7 @@ void RLZTracker::RecordFirstSearch(rlz_lib::AccessPoint point) {
   if (!RecordProductEvent(rlz_lib::CHROME, point, rlz_lib::FIRST_SEARCH))
     *record_used = true;
   else if (send_ping_immediately_ && point == CHROME_OMNIBOX)
-    ScheduleDelayedInit(0);
+    ScheduleDelayedInit(base::TimeDelta());
 }
 
 bool RLZTracker::ScheduleRecordFirstSearch(rlz_lib::AccessPoint point) {
@@ -573,5 +572,5 @@ void RLZTracker::CleanupRlz() {
 
 // static
 void RLZTracker::EnableZeroDelayForTesting() {
-  GetInstance()->min_delay_ = 0;
+  GetInstance()->min_init_delay_ = base::TimeDelta();
 }
diff --git a/chrome/browser/rlz/rlz.h b/chrome/browser/rlz/rlz.h
index 6d719c3..23075e1 100644
--- a/chrome/browser/rlz/rlz.h
+++ b/chrome/browser/rlz/rlz.h
@@ -16,6 +16,7 @@
 #include "base/memory/singleton.h"
 #include "base/string16.h"
 #include "base/threading/sequenced_worker_pool.h"
+#include "base/time.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "rlz/lib/rlz_lib.h"
@@ -37,14 +38,19 @@ class URLRequestContextGetter;
 
 class RLZTracker : public content::NotificationObserver {
  public:
-  // Initializes the RLZ library services for use in chrome. Schedules a
-  // delayed task (delayed by |delay| seconds) that performs the ping and
-  // registers some events when 'first-run' is true.
+  // Initializes the RLZ library services for use in chrome. Schedules a delayed
+  // task that performs the ping and registers some events when 'first-run' is
+  // true.
+  //
+  // When |send_ping_immediately| is true, a financial ping should be sent
+  // immediately after a first search is recorded, without waiting for |delay|.
+  // However, we only want this behaviour on first run.
   //
   // If the chrome brand is organic (no partners) then the pings don't occur.
   static bool InitRlzFromProfileDelayed(Profile* profile,
                                         bool first_run,
-                                        int delay);
+                                        bool send_ping_immediately,
+                                        base::TimeDelta delay);
 
   // Records an RLZ event. Some events can be access point independent.
   // Returns false it the event could not be recorded. Requires write access
@@ -92,7 +98,8 @@ class RLZTracker : public content::NotificationObserver {
 
   // Called by InitRlzFromProfileDelayed with values taken from |profile|.
   static bool InitRlzDelayed(bool first_run,
-                             int delay,
+                             bool send_ping_immediately,
+                             base::TimeDelta delay,
                              bool is_google_default_search,
                              bool is_google_homepage,
                              bool is_google_in_startpages);
@@ -123,7 +130,8 @@ class RLZTracker : public content::NotificationObserver {
 
   // Implementation called from InitRlzDelayed() static method.
   bool Init(bool first_run,
-            int delay,
+            bool send_ping_immediately,
+            base::TimeDelta delay,
             bool google_default_search,
             bool google_default_homepage,
             bool is_google_in_startpages);
@@ -141,7 +149,7 @@ class RLZTracker : public content::NotificationObserver {
 
   // Schedules the delayed initialization. This method is virtual to allow
   // tests to override how the scheduling is done.
-  virtual void ScheduleDelayedInit(int delay);
+  virtual void ScheduleDelayedInit(base::TimeDelta delay);
 
   // Schedules a call to rlz_lib::RecordProductEvent(). This method is virtual
   // to allow tests to override how the scheduling is done.
@@ -217,7 +225,7 @@ class RLZTracker : public content::NotificationObserver {
   content::NotificationRegistrar registrar_;
 
   // Minimum delay before sending financial ping after initialization.
-  int min_delay_;
+  base::TimeDelta min_init_delay_;
 
   DISALLOW_COPY_AND_ASSIGN(RLZTracker);
 };
diff --git a/chrome/browser/rlz/rlz_unittest.cc b/chrome/browser/rlz/rlz_unittest.cc
index 164d8b2..dc5152b 100644
--- a/chrome/browser/rlz/rlz_unittest.cc
+++ b/chrome/browser/rlz/rlz_unittest.cc
@@ -117,10 +117,10 @@ class TestRLZTracker : public RLZTracker {
   }
 
  private:
-  virtual void ScheduleDelayedInit(int delay) OVERRIDE {
+  virtual void ScheduleDelayedInit(base::TimeDelta delay) OVERRIDE {
     // If the delay is 0, invoke the delayed init now. Otherwise,
     // don't schedule anything, it will be manually called during tests.
-    if (delay == 0)
+    if (delay == base::TimeDelta())
       DelayedInit();
   }
 
@@ -388,6 +388,8 @@ const char kHomepageSetToGoogle[] = "CBS";
 const char kHomepageFirstSeach[] = "CBF";
 #endif
 
+const base::TimeDelta kDelay = base::TimeDelta::FromMilliseconds(20);
+
 TEST_F(RlzLibTest, RecordProductEvent) {
   RLZTracker::RecordProductEvent(rlz_lib::CHROME, RLZTracker::CHROME_OMNIBOX,
                                  rlz_lib::FIRST_SEARCH);
@@ -396,7 +398,7 @@ TEST_F(RlzLibTest, RecordProductEvent) {
 }
 
 TEST_F(RlzLibTest, QuickStopAfterStart) {
-  TestRLZTracker::InitRlzDelayed(true, 20, true, true, true);
+  TestRLZTracker::InitRlzDelayed(true, false, kDelay, true, true, true);
 
   // Omnibox events.
   ExpectEventRecorded(kOmniboxInstall, false);
@@ -412,7 +414,7 @@ TEST_F(RlzLibTest, QuickStopAfterStart) {
 }
 
 TEST_F(RlzLibTest, DelayedInitOnly) {
-  TestRLZTracker::InitRlzDelayed(true, 20, true, true, false);
+  TestRLZTracker::InitRlzDelayed(true, false, kDelay, true, true, false);
   InvokeDelayedInit();
 
   // Omnibox events.
@@ -429,7 +431,7 @@ TEST_F(RlzLibTest, DelayedInitOnly) {
 }
 
 TEST_F(RlzLibTest, DelayedInitOnlyGoogleAsStartup) {
-  TestRLZTracker::InitRlzDelayed(true, 20, false, false, true);
+  TestRLZTracker::InitRlzDelayed(true, false, kDelay, false, false, true);
   InvokeDelayedInit();
 
   // Omnibox events.
@@ -446,7 +448,7 @@ TEST_F(RlzLibTest, DelayedInitOnlyGoogleAsStartup) {
 }
 
 TEST_F(RlzLibTest, DelayedInitOnlyNoFirstRunNoRlzStrings) {
-  TestRLZTracker::InitRlzDelayed(false, 20, true, true, false);
+  TestRLZTracker::InitRlzDelayed(false, false, kDelay, true, true, false);
   InvokeDelayedInit();
 
   // Omnibox events.
@@ -463,7 +465,7 @@ TEST_F(RlzLibTest, DelayedInitOnlyNoFirstRunNoRlzStrings) {
 }
 
 TEST_F(RlzLibTest, DelayedInitOnlyNoFirstRunNoRlzStringsGoogleAsStartup) {
-  TestRLZTracker::InitRlzDelayed(false, 20, false, false, true);
+  TestRLZTracker::InitRlzDelayed(false, false, kDelay, false, false, true);
   InvokeDelayedInit();
 
   // Omnibox events.
@@ -485,7 +487,7 @@ TEST_F(RlzLibTest, DelayedInitOnlyNoFirstRun) {
   rlz_lib::SetAccessPointRlz(RLZTracker::CHROME_OMNIBOX, kOmniboxRlzString);
   rlz_lib::SetAccessPointRlz(RLZTracker::CHROME_HOME_PAGE, kHomepageRlzString);
 
-  TestRLZTracker::InitRlzDelayed(false, 20, true, true, true);
+  TestRLZTracker::InitRlzDelayed(false, false, kDelay, true, true, true);
   InvokeDelayedInit();
 
   // Omnibox events.
@@ -502,7 +504,7 @@ TEST_F(RlzLibTest, DelayedInitOnlyNoFirstRun) {
 }
 
 TEST_F(RlzLibTest, DelayedInitOnlyNoGoogleDefaultSearchOrHomepageOrStartup) {
-  TestRLZTracker::InitRlzDelayed(true, 20, false, false, false);
+  TestRLZTracker::InitRlzDelayed(true, false, kDelay, false, false, false);
   InvokeDelayedInit();
 
   // Omnibox events.
@@ -519,7 +521,7 @@ TEST_F(RlzLibTest, DelayedInitOnlyNoGoogleDefaultSearchOrHomepageOrStartup) {
 }
 
 TEST_F(RlzLibTest, OmniboxUsageOnly) {
-  TestRLZTracker::InitRlzDelayed(true, 20, true, true, false);
+  TestRLZTracker::InitRlzDelayed(true, false, kDelay, true, true, false);
   SimulateOmniboxUsage();
 
   // Omnibox events.
@@ -536,7 +538,7 @@ TEST_F(RlzLibTest, OmniboxUsageOnly) {
 }
 
 TEST_F(RlzLibTest, HomepageUsageOnly) {
-  TestRLZTracker::InitRlzDelayed(true, 20, true, true, false);
+  TestRLZTracker::InitRlzDelayed(true, false, kDelay, true, true, false);
   SimulateHomepageUsage();
 
   // Omnibox events.
@@ -553,7 +555,7 @@ TEST_F(RlzLibTest, HomepageUsageOnly) {
 }
 
 TEST_F(RlzLibTest, UsageBeforeDelayedInit) {
-  TestRLZTracker::InitRlzDelayed(true, 20, true, true, false);
+  TestRLZTracker::InitRlzDelayed(true, false, kDelay, true, true, false);
   SimulateOmniboxUsage();
   SimulateHomepageUsage();
   InvokeDelayedInit();
@@ -572,7 +574,7 @@ TEST_F(RlzLibTest, UsageBeforeDelayedInit) {
 }
 
 TEST_F(RlzLibTest, OmniboxUsageAfterDelayedInit) {
-  TestRLZTracker::InitRlzDelayed(true, 20, true, true, false);
+  TestRLZTracker::InitRlzDelayed(true, false, kDelay, true, true, false);
   InvokeDelayedInit();
   SimulateOmniboxUsage();
   SimulateHomepageUsage();
@@ -590,8 +592,8 @@ TEST_F(RlzLibTest, OmniboxUsageAfterDelayedInit) {
   ExpectRlzPingSent(true);
 }
 
-TEST_F(RlzLibTest, OmniboxUsageSendsPingWhenDelayNegative) {
-  TestRLZTracker::InitRlzDelayed(true, -20, true, true, false);
+TEST_F(RlzLibTest, OmniboxUsageSendsPingWhenSendPingImmediately) {
+  TestRLZTracker::InitRlzDelayed(true, true, kDelay, true, true, false);
   SimulateOmniboxUsage();
 
   // Omnibox events.
@@ -607,8 +609,8 @@ TEST_F(RlzLibTest, OmniboxUsageSendsPingWhenDelayNegative) {
   ExpectRlzPingSent(true);
 }
 
-TEST_F(RlzLibTest, HomepageUsageDoesNotSendPingWhenDelayNegative) {
-  TestRLZTracker::InitRlzDelayed(true, -20, true, true, false);
+TEST_F(RlzLibTest, HomepageUsageDoesNotSendPingWhenSendPingImmediately) {
+  TestRLZTracker::InitRlzDelayed(true, true, kDelay, true, true, false);
   SimulateHomepageUsage();
 
   // Omnibox events.
@@ -624,8 +626,8 @@ TEST_F(RlzLibTest, HomepageUsageDoesNotSendPingWhenDelayNegative) {
   ExpectRlzPingSent(false);
 }
 
-TEST_F(RlzLibTest, StartupUsageDoesNotSendPingWhenDelayNegative) {
-  TestRLZTracker::InitRlzDelayed(true, -20, true, false, true);
+TEST_F(RlzLibTest, StartupUsageDoesNotSendPingWhenSendPingImmediately) {
+  TestRLZTracker::InitRlzDelayed(true, true, kDelay, true, false, true);
   SimulateHomepageUsage();
 
   // Omnibox events.
@@ -707,7 +709,7 @@ TEST_F(RlzLibTest, PingUpdatesRlzCache) {
 
   // Perform ping.
   tracker_.set_assume_not_ui_thread(true);
-  TestRLZTracker::InitRlzDelayed(true, 20, true, true, false);
+  TestRLZTracker::InitRlzDelayed(true, false, kDelay, true, true, false);
   InvokeDelayedInit();
   ExpectRlzPingSent(true);
 
@@ -738,7 +740,7 @@ TEST_F(RlzLibTest, ObserveHandlesBadArgs) {
 TEST_F(RlzLibTest, ReactivationNonOrganicNonOrganic) {
   SetReactivationBrand("REAC");
 
-  TestRLZTracker::InitRlzDelayed(true, 20, true, true, false);
+  TestRLZTracker::InitRlzDelayed(true, false, kDelay, true, true, false);
   InvokeDelayedInit();
 
   ExpectRlzPingSent(true);
@@ -749,7 +751,7 @@ TEST_F(RlzLibTest, ReactivationOrganicNonOrganic) {
   SetMainBrand("GGLS");
   SetReactivationBrand("REAC");
 
-  TestRLZTracker::InitRlzDelayed(true, 20, true, true, false);
+  TestRLZTracker::InitRlzDelayed(true, false, kDelay, true, true, false);
   InvokeDelayedInit();
 
   ExpectRlzPingSent(false);
@@ -760,7 +762,7 @@ TEST_F(RlzLibTest, ReactivationNonOrganicOrganic) {
   SetMainBrand("TEST");
   SetReactivationBrand("GGLS");
 
-  TestRLZTracker::InitRlzDelayed(true, 20, true, true, false);
+  TestRLZTracker::InitRlzDelayed(true, false, kDelay, true, true, false);
   InvokeDelayedInit();
 
   ExpectRlzPingSent(true);
@@ -771,7 +773,7 @@ TEST_F(RlzLibTest, ReactivationOrganicOrganic) {
   SetMainBrand("GGLS");
   SetReactivationBrand("GGRS");
 
-  TestRLZTracker::InitRlzDelayed(true, 20, true, true, false);
+  TestRLZTracker::InitRlzDelayed(true, false, kDelay, true, true, false);
   InvokeDelayedInit();
 
   ExpectRlzPingSent(false);