RLZ on ChromeOS-related polishing.

RLZ on ChromeOS-related polishing.

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


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


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

[Roger Tawa OOO till Jul 10th, 2012-12-20 18:37:23]

lgtm, one comment below

https://codereview.chromium.org/11645040/diff/1/chrome/browser/rlz/rlz.h
File chrome/browser/rlz/rlz.h (right):

https://codereview.chromium.org/11645040/diff/1/chrome/browser/rlz/rlz.h#newc...
chrome/browser/rlz/rlz.h:47: // restart of chrome. However, we only want this
behaviour on first run.
We don't wait for "the next restart of chrome".  If |send_ping_immediately| is
false, the ping will always be sent out |delay| seconds later.  If
|send_ping_immediately| is true, the ping will be sent out at the earliest of:

- the first search is recorded
- |delay| seconds

Please keep the part of the comment that states that |delay| is specified in
seconds.

[Roger Tawa OOO till Jul 10th, 2012-12-20 18:42:02]

On 2012/12/20 18:37:23, Roger Tawa wrote:
> lgtm, one comment below
> 
> https://codereview.chromium.org/11645040/diff/1/chrome/browser/rlz/rlz.h
> File chrome/browser/rlz/rlz.h (right):
> 
>
https://codereview.chromium.org/11645040/diff/1/chrome/browser/rlz/rlz.h#newc...
> chrome/browser/rlz/rlz.h:47: // restart of chrome. However, we only want this
> behaviour on first run.
> We don't wait for "the next restart of chrome".  If |send_ping_immediately| is
> false, the ping will always be sent out |delay| seconds later.  If
> |send_ping_immediately| is true, the ping will be sent out at the earliest of:
> 
> - the first search is recorded
> - |delay| seconds
> 
> Please keep the part of the comment that states that |delay| is specified in
> seconds.

oops, forget part about |delay| in seconds, its a TimeDelta.

[Ivan Korotkov, 2012-12-21 10:42:02]

Per Peter's proposal, I've moved EnableZeroDelayForTesting from startup code
path directly to RLZTracker.

https://codereview.chromium.org/11645040/diff/1/chrome/browser/rlz/rlz.h
File chrome/browser/rlz/rlz.h (right):

https://codereview.chromium.org/11645040/diff/1/chrome/browser/rlz/rlz.h#newc...
chrome/browser/rlz/rlz.h:47: // restart of chrome. However, we only want this
behaviour on first run.
On 2012/12/20 18:37:23, Roger Tawa wrote:
> We don't wait for "the next restart of chrome".  If |send_ping_immediately| is
> false, the ping will always be sent out |delay| seconds later.  If
> |send_ping_immediately| is true, the ping will be sent out at the earliest of:
> 
> - the first search is recorded
> - |delay| seconds
> 
> Please keep the part of the comment that states that |delay| is specified in
> seconds.

Done.

[Peter Kasting, 2012-12-21 18:24:11]

LGTM

https://codereview.chromium.org/11645040/diff/8001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/11645040/diff/8001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils_browsertest.cc:275:
RLZTracker::EnableZeroDelayForTesting();
Is this call still needed?  If not, EnableZeroDelayForTesting() can go away
entirely and rlz.cc can just do it inline internally where necessary.

https://codereview.chromium.org/11645040/diff/8001/chrome/browser/rlz/rlz.cc
File chrome/browser/rlz/rlz.cc (right):

https://codereview.chromium.org/11645040/diff/8001/chrome/browser/rlz/rlz.cc#...
chrome/browser/rlz/rlz.cc:270: delay = std::min(kMinInitDelay,
std::max(min_init_delay_, delay));
Nit: Can't this just be std::max(min_init_delay_, delay) ?  Since it seems like
|min_init_delay_| can only ever be kMinInitDelay or 0.

https://codereview.chromium.org/11645040/diff/8001/chrome/browser/rlz/rlz.h
File chrome/browser/rlz/rlz.h (right):

https://codereview.chromium.org/11645040/diff/8001/chrome/browser/rlz/rlz.h#n...
chrome/browser/rlz/rlz.h:45: // When |send_ping_immediately| is true, financial
ping should be sent
Nit: financial ping -> financial pings, or a financial ping.

[Ivan Korotkov, 2012-12-22 12:43:04]

https://codereview.chromium.org/11645040/diff/8001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/11645040/diff/8001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils_browsertest.cc:275:
RLZTracker::EnableZeroDelayForTesting();
On 2012/12/21 18:24:11, Peter Kasting wrote:
> Is this call still needed?  If not, EnableZeroDelayForTesting() can go away
> entirely and rlz.cc can just do it inline internally where necessary.

Yes, unfortunately :( login_utils_browsertest is not really an
InProcessBrowserTest and lots of things break if kTestType is added to
command-line in it.

https://codereview.chromium.org/11645040/diff/8001/chrome/browser/rlz/rlz.cc
File chrome/browser/rlz/rlz.cc (right):

https://codereview.chromium.org/11645040/diff/8001/chrome/browser/rlz/rlz.cc#...
chrome/browser/rlz/rlz.cc:270: delay = std::min(kMinInitDelay,
std::max(min_init_delay_, delay));
On 2012/12/21 18:24:11, Peter Kasting wrote:
> Nit: Can't this just be std::max(min_init_delay_, delay) ?  Since it seems
like
> |min_init_delay_| can only ever be kMinInitDelay or 0.

Ouch, that should be std::min(kMaxInitDelay, ...). Fixed.

https://codereview.chromium.org/11645040/diff/8001/chrome/browser/rlz/rlz.h
File chrome/browser/rlz/rlz.h (right):

https://codereview.chromium.org/11645040/diff/8001/chrome/browser/rlz/rlz.h#n...
chrome/browser/rlz/rlz.h:45: // When |send_ping_immediately| is true, financial
ping should be sent
On 2012/12/21 18:24:11, Peter Kasting wrote:
> Nit: financial ping -> financial pings, or a financial ping.

Done.

[commit-bot: I haz the power, 2012-12-24 15:44:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ivankr@chromium.org/11645040/16001

[commit-bot: I haz the power, 2012-12-24 15:44:56]

Failed to apply patch for chrome/browser/chromeos/login/login_utils.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/login/login_utils.cc
  Hunk #3 FAILED at 656.
  1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/login/login_utils.cc.rej

Patch:       chrome/browser/chromeos/login/login_utils.cc
Index: chrome/browser/chromeos/login/login_utils.cc
diff --git a/chrome/browser/chromeos/login/login_utils.cc
b/chrome/browser/chromeos/login/login_utils.cc
index
0ba105660a1c6f594e23d73dc1aa9c2c125860b2..5e7ae372aff5fb999dcc1f3947e2072ffd0fc863
100644
--- a/chrome/browser/chromeos/login/login_utils.cc
+++ b/chrome/browser/chromeos/login/login_utils.cc
@@ -312,7 +312,7 @@ class LoginUtilsImpl
   // Restores GAIA auth cookies for the created profile.
   void RestoreAuthCookies(Profile* user_profile);
 
-  // Initializes RLZ. If |disabled| is true, financial pings are turned off.
+  // Initializes RLZ. If |disabled| is true, RLZ pings are disabled.
   void InitRlz(Profile* user_profile, bool disabled);
 
   std::string password_;
@@ -646,7 +646,7 @@ void LoginUtilsImpl::InitRlzDelayed(Profile* user_profile) {
     return;
   }
   base::PostTaskAndReplyWithResult(
-      base::WorkerPool::GetTaskRunner(false /* task_is_slow */),
+      base::WorkerPool::GetTaskRunner(false),
       FROM_HERE,
       base::Bind(&file_util::PathExists, GetRlzDisabledFlagPath()),
       base::Bind(&LoginUtilsImpl::InitRlz, AsWeakPtr(), user_profile));
@@ -656,14 +656,17 @@ void LoginUtilsImpl::InitRlzDelayed(Profile* user_profile)
{
 void LoginUtilsImpl::InitRlz(Profile* user_profile, bool disabled) {
 #if defined(ENABLE_RLZ)
   if (disabled) {
-    // Empty brand code turns financial pings off.
+    // Empty brand code means an organic install (no RLZ pings are sent).
     google_util::chromeos::ClearBrandForCurrentSession();
   }
   // Init the RLZ library.
   int ping_delay = user_profile->GetPrefs()->GetInteger(
       first_run::GetPingDelayPrefName().c_str());
+  // Negative ping delay means to send ping immediately after a first search is
+  // recorded.
   RLZTracker::InitRlzFromProfileDelayed(
-      user_profile, UserManager::Get()->IsCurrentUserNew(), ping_delay);
+      user_profile, UserManager::Get()->IsCurrentUserNew(),
+      ping_delay < 0, base::TimeDelta::FromMilliseconds(abs(ping_delay)));
   if (delegate_)
     delegate_->OnRlzInitialized(user_profile);
 #endif

[commit-bot: I haz the power, 2012-12-25 10:38:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ivankr@chromium.org/11645040/9010

[commit-bot: I haz the power, 2012-12-25 10:38:42]

Presubmit check for 11645040-9010 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  chrome/browser/rlz/rlz.cc:267
    EnableZeroDelayForTesting();

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/browser/chromeos

Presubmit checks took 1.7s to calculate.

[Ivan Korotkov, 2012-12-25 10:45:28]

TBR=sky for chrome_browser_main.cc

[commit-bot: I haz the power, 2012-12-25 10:45:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ivankr@chromium.org/11645040/9010

[commit-bot: I haz the power, 2012-12-25 13:08:43]

Change committed as 174607