[rlz,cros] RLZ glue for ChromeOS.

[rlz,cros] RLZ glue for ChromeOS.


BUG=157348, 160059
TBR=nirnimesh,sky

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

[Ivan Korotkov, 2012-11-19 11:47:03]

http://codereview.chromium.org/11412067/diff/2001/build/common.gypi
File build/common.gypi (left):

http://codereview.chromium.org/11412067/diff/2001/build/common.gypi#oldcode1431
build/common.gypi:1431: ['branding=="Chrome" and (OS=="win" or OS=="mac")', {
This is for trybots

[Ivan Korotkov, 2012-11-19 11:47:44]

PTAL:
pkasting: chrome/browser/google
nkostylev: chromeos
rogerta: overall

[Nikita (slow), 2012-11-19 12:27:35]

http://codereview.chromium.org/11412067/diff/6001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

http://codereview.chromium.org/11412067/diff/6001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:218: Profile* profile =
ProfileManager::GetDefaultProfile();
This should be moved to LoginUtils::OnProfileCreated()

[Nikita (slow), 2012-11-19 12:32:52]

Please check whether you could as well add test to LoginUtilsTest

[Ivan Korotkov, 2012-11-19 14:00:40]

http://codereview.chromium.org/11412067/diff/6001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

http://codereview.chromium.org/11412067/diff/6001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:218: Profile* profile =
ProfileManager::GetDefaultProfile();
On 2012/11/19 12:27:35, Nikita Kostylev wrote:
> This should be moved to LoginUtils::OnProfileCreated()

Done.

[Nikita (slow), 2012-11-19 15:44:41]

lgtm but please add a test (separate CL - ok) that checks that RLZ is properly
initialized on Chrome OS with some test prefs file.

[Peter Kasting, 2012-11-19 20:59:09]

https://codereview.chromium.org/11412067/diff/9002/chrome/browser/google/goog...
File chrome/browser/google/google_util.cc (right):

https://codereview.chromium.org/11412067/diff/9002/chrome/browser/google/goog...
chrome/browser/google/google_util.cc:63: #if defined(OS_CHROMEOS)
Nit: Maybe this whole block should go in its own .cc/.h file instead so we could
reduce the amount if #ifdefed stuff in cross-platform files.

https://codereview.chromium.org/11412067/diff/9002/chrome/browser/google/goog...
chrome/browser/google/google_util.cc:68: // Simple class for caching brand code,
meant to be used as a LazyInstance.
Do we really need thread-safety?  I thought the google_util functions were
UI-thread only.

If we don't need thread safety, then LazyInstance isn't needed.  Do something
like this instead:

const std::string& GetBrand() {
  CR_DEFINE_STATIC_LOCAL(std::string, brand, ());
  ...
  file_util::ReadFileToString(brand_file_path, &brand);
  ...
  return brand;
}

This is simpler (the class disappears entirely) and avoids running a destructor
at program exit as well.

https://codereview.chromium.org/11412067/diff/9002/chrome/browser/google/goog...
chrome/browser/google/google_util.cc:71: BrandCode() {
Nit: Don't inline non-accessor methods, even for classes declared in a .cc file

https://codereview.chromium.org/11412067/diff/9002/chrome/browser/google/goog...
chrome/browser/google/google_util.cc:73: // TODO(ivankr): as discussed, this
will be moved to OOBE->EULA stage.
Can this be done now?  I'm leery of adding ScopedAllowIOs unless there is no
other choice.

https://codereview.chromium.org/11412067/diff/9002/chrome/browser/google/goog...
chrome/browser/google/google_util.cc:86: };
Nit: DISALLOW_COPY_AND_ASSIGN

[Ivan Korotkov, 2012-11-28 14:24:21]

*) I've added code that loads brand code for ChromeOS from file and stores it to
Local State after EULA has been accepted.

*) Added separate google_util_chromeos file pair.

*) Added OOBE UI for accepting RLZ tracking in EULA

PTAL:
Nikita: chromeos
Roger: rlz and chrome/browser/chromeos/login/login_utils.cc
Peter: google_util*

One change remaining in RLZTracker for this CL is to call GetBrand() on UI
thread in Init() instead of worker pool, I'll upload it later.

https://codereview.chromium.org/11412067/diff/9002/chrome/browser/google/goog...
File chrome/browser/google/google_util.cc (right):

https://codereview.chromium.org/11412067/diff/9002/chrome/browser/google/goog...
chrome/browser/google/google_util.cc:63: #if defined(OS_CHROMEOS)
On 2012/11/19 20:59:10, Peter Kasting wrote:
> Nit: Maybe this whole block should go in its own .cc/.h file instead so we
could
> reduce the amount if #ifdefed stuff in cross-platform files.

Done.

https://codereview.chromium.org/11412067/diff/9002/chrome/browser/google/goog...
chrome/browser/google/google_util.cc:73: // TODO(ivankr): as discussed, this
will be moved to OOBE->EULA stage.
On 2012/11/19 20:59:10, Peter Kasting wrote:
> Can this be done now?  I'm leery of adding ScopedAllowIOs unless there is no
> other choice.

Done.

[Ivan Korotkov, 2012-11-28 14:35:40]

Updated RLZTracker as described in previous message as well.

[Roger Tawa OOO till Jul 10th, 2012-11-28 15:23:18]

lgtm

Just one comment about making a method non-public.

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

https://codereview.chromium.org/11412067/diff/3010/chrome/browser/rlz/rlz.cc#...
chrome/browser/rlz/rlz.cc:241: }
I think its great to pull this code into rlz.cc!

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

https://codereview.chromium.org/11412067/diff/3010/chrome/browser/rlz/rlz.h#n...
chrome/browser/rlz/rlz.h:49: bool is_google_in_startpages);
This function is no longer called externally except in tests.  I think it should
be made protected and access through TestRLZTracker in the tests.

[Nikita (slow), 2012-11-29 11:17:48]

chromeos/* lgtm with nits

https://codereview.chromium.org/11412067/diff/3010/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/11412067/diff/3010/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wizard_controller.cc:503: LOG(ERROR) <<
"WizardController::OnEulaBlockingTasksDone";
nit: Cleanup logging.

https://codereview.chromium.org/11412067/diff/3010/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/oobe.js (right):

https://codereview.chromium.org/11412067/diff/3010/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe.js:82: // TODO: Cleanup with old
OOBE style removal.
nit: This TODO could be removed.

https://codereview.chromium.org/11412067/diff/3010/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/oobe_screen_eula.html (right):

https://codereview.chromium.org/11412067/diff/3010/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe_screen_eula.html:23: <div
id="installation-settings" class="step-links">
Please merge with my CL that adds a11y menu.

https://codereview.chromium.org/11412067/diff/3010/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/oobe_screen_eula.js (right):

https://codereview.chromium.org/11412067/diff/3010/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe_screen_eula.js:48: // TODO(ivankr):
focus cycling.
We should really fix this in M25. a11y bugs should be fixed right away.

[commit-bot: I haz the power, 2012-11-29 12:08:37]

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

[commit-bot: I haz the power, 2012-11-29 12:08:49]

Failed to apply patch for chrome/browser/resources/chromeos/login/oobe.css:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/resources/chromeos/login/oobe.css
  Hunk #1 FAILED at 139.
  Hunk #2 succeeded at 780 (offset 3 lines).
  Hunk #3 succeeded at 789 (offset 3 lines).
  Hunk #4 succeeded at 858 (offset 3 lines).
  Hunk #5 FAILED at 879.
  Hunk #6 succeeded at 1060 (offset 21 lines).
  Hunk #7 succeeded at 1071 (offset 21 lines).
  Hunk #8 succeeded at 1087 (offset 21 lines).
  Hunk #9 succeeded at 1102 (offset 21 lines).
  2 out of 9 hunks FAILED -- saving rejects to file
chrome/browser/resources/chromeos/login/oobe.css.rej

Patch:       chrome/browser/resources/chromeos/login/oobe.css
Index: chrome/browser/resources/chromeos/login/oobe.css
diff --git a/chrome/browser/resources/chromeos/login/oobe.css
b/chrome/browser/resources/chromeos/login/oobe.css
index
3804a19eaacc14d338c694c6924d9b8f80209328..3ace415f4fcbad2cd7aec65a20acb4b302da6a59
100644
--- a/chrome/browser/resources/chromeos/login/oobe.css
+++ b/chrome/browser/resources/chromeos/login/oobe.css
@@ -139,7 +139,7 @@ html[oobe=new] #inner-container.down {
 }
 
 #security-info a,
-#security-tpm a,
+#installation-settings a,
 #eula a {
   color: rgb(0, 102, 104);
   text-decoration: none;
@@ -777,7 +777,8 @@ html[oobe=old] .step-controls {
 }
 
 .step.loading .step-contents,
-.step.loading .step-controls {
+.step.loading .step-controls,
+.step.loading .step-links {
   visibility: hidden;
 }
 
@@ -785,6 +786,10 @@ html[oobe=old] .step-controls {
   visibility: hidden;
 }
 
+.step:not(.loading) #eula-busy {
+  visibility: hidden;
+}
+
 /* Center spinner wrapper vertically and horizontally.
  * Vertical centering uses top: 50% and negative margin
  * instead of top: 0; bottom: 0 due to http://crbug.com/155320.
@@ -850,7 +855,7 @@ html[oobe=old] .step-controls {
 }
 
 /* TODO(nkostylev): Cleanup, security-info - old design,
-   security-tpm - new design. */
+   installation-settings - new design. */
 #security-info {
   bottom: 10px;
   left: 0;
@@ -874,26 +879,26 @@ html[oobe=new] #security-info {
   display: none;
 }
 
-/* TODO(nkostylev): Remove #security-tpm element and rules. */
-html[oobe=old] #security-tpm {
+/* TODO(nkostylev): Remove #security-info element and rules. */
+html[oobe=old] #installation-settings {
   display: none;
 }
 
-#security-tpm a {
+#installation-settings a {
   color: #8c8c8c;
   font-size: 12px;
 }
 
-#security-tpm {
+#installation-settings {
   bottom: 21px;
   position: absolute;
 }
 
-html[dir=ltr] #security-tpm {
+html[dir=ltr] #installation-settings {
   left: 49px;
 }
 
-html[dir=rtl] #security-tpm {
+html[dir=rtl] #installation-settings {
   right: 49px;
 }
 
@@ -1034,7 +1039,7 @@ html[oobe=new] #oem-eula {
   display: none;
 }
 
-#security-info-popup {
+#installation-settings-popup {
   background: white;
   border: 1px solid rgb(188, 193, 208);
   border-radius: 2px;
@@ -1045,7 +1050,7 @@ html[oobe=new] #oem-eula {
   z-index: 10;
 }
 
-#security-info-popup h1 {
+#installation-settings-popup h1 {
   -webkit-padding-end: 24px;
   -webkit-user-select: none;
   background: -webkit-linear-gradient(white, #F8F8F8);
@@ -1061,7 +1066,7 @@ html[oobe=new] #oem-eula {
   padding: 0 20px 60px 20px;
 }
 
-#security-info-popup button {
+#installation-settings-popup button {
   bottom: 20px;
   position: absolute;
   right: 20px;
@@ -1076,6 +1081,15 @@ html[oobe=new] #oem-eula {
   text-align: center;
 }
 
+html[rlz=disabled] #rlz-settings {
+  display: none;
+}
+
+#rlz-enable {
+  -webkit-margin-end: 10px;
+  -webkit-margin-start: 20px;
+}
+
 [hidden] {
   display: none !important;
 }

[Ivan Korotkov, 2012-11-29 17:42:27]

https://codereview.chromium.org/11412067/diff/3010/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/11412067/diff/3010/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wizard_controller.cc:503: LOG(ERROR) <<
"WizardController::OnEulaBlockingTasksDone";
On 2012/11/29 11:17:48, Nikita Kostylev wrote:
> nit: Cleanup logging.

Done.

https://codereview.chromium.org/11412067/diff/3010/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/oobe.js (right):

https://codereview.chromium.org/11412067/diff/3010/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe.js:82: // TODO: Cleanup with old
OOBE style removal.
On 2012/11/29 11:17:48, Nikita Kostylev wrote:
> nit: This TODO could be removed.

It's related to #security-link, which is still there :(

https://codereview.chromium.org/11412067/diff/3010/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/oobe_screen_eula.html (right):

https://codereview.chromium.org/11412067/diff/3010/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe_screen_eula.html:23: <div
id="installation-settings" class="step-links">
On 2012/11/29 11:17:48, Nikita Kostylev wrote:
> Please merge with my CL that adds a11y menu.

Done.

https://codereview.chromium.org/11412067/diff/3010/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/oobe_screen_eula.js (right):

https://codereview.chromium.org/11412067/diff/3010/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe_screen_eula.js:48: // TODO(ivankr):
focus cycling.
On 2012/11/29 11:17:48, Nikita Kostylev wrote:
> We should really fix this in M25. a11y bugs should be fixed right away.

I'll do this in a follow-up CL

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

https://codereview.chromium.org/11412067/diff/3010/chrome/browser/rlz/rlz.h#n...
chrome/browser/rlz/rlz.h:49: bool is_google_in_startpages);
On 2012/11/28 15:23:18, Roger Tawa wrote:
> This function is no longer called externally except in tests.  I think it
should
> be made protected and access through TestRLZTracker in the tests.

Done.

[Peter Kasting, 2012-12-01 03:06:22]

LGTM with comments

https://codereview.chromium.org/11412067/diff/16047/chrome/browser/google/goo...
File chrome/browser/google/google_util_chromeos.cc (right):

https://codereview.chromium.org/11412067/diff/16047/chrome/browser/google/goo...
chrome/browser/google/google_util_chromeos.cc:32: LOG(WARNING) << "Brand code
file missing: " << brand_file_path.value();
Nit: Is this log something you'll be able to capture and read somewhere?  If not
you may want to eliminate this or replace with VLOG(1) or something.

https://codereview.chromium.org/11412067/diff/16047/chrome/browser/google/goo...
chrome/browser/google/google_util_chromeos.cc:46:
DCHECK(!BrowserThread::IsWellKnownThread(BrowserThread::UI) ||
Nit: If you change to this:

  DCHECK(
      !content::BrowserThread::IsWellKnownThread(content::BrowserThread::UI) ||
      content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

...you can eliminate the using statement at top (we try to only use these when
they save a significant number of lines), and also fix the fact that currently
you indent even but probably should indent 4 instead since your second line
continues the first rather than starts a new argument.

https://codereview.chromium.org/11412067/diff/16047/chrome/browser/google/goo...
chrome/browser/google/google_util_chromeos.cc:49: return local_state ?
local_state->GetString(prefs::kRLZBrand) : std::string();
Can this ever be NULL?  We shouldn't NULL-check if it can't.

https://codereview.chromium.org/11412067/diff/16047/chrome/browser/google/goo...
chrome/browser/google/google_util_chromeos.cc:53: std::string* brand = new
std::string;
Instead of allocating this, can we just pass the string directly to SetBrand()
via PostTaskAndReplyWithResult()?

https://codereview.chromium.org/11412067/diff/16047/chrome/browser/google/goo...
File chrome/browser/google/google_util_chromeos.h (right):

https://codereview.chromium.org/11412067/diff/16047/chrome/browser/google/goo...
chrome/browser/google/google_util_chromeos.h:19: // Reads the brand code from a
board-specific file and stores it to Local State.
"board" is slightly unclear here... does the brand code really depend on the
mainboard in the CrOS device?  Maybe this should say "device-specific"?  I don't
know, honestly.

[Ivan Korotkov, 2012-12-02 12:01:15]

https://codereview.chromium.org/11412067/diff/16047/chrome/browser/google/goo...
File chrome/browser/google/google_util_chromeos.cc (right):

https://codereview.chromium.org/11412067/diff/16047/chrome/browser/google/goo...
chrome/browser/google/google_util_chromeos.cc:32: LOG(WARNING) << "Brand code
file missing: " << brand_file_path.value();
On 2012/12/01 03:06:22, Peter Kasting wrote:
> Nit: Is this log something you'll be able to capture and read somewhere?  If
not
> you may want to eliminate this or replace with VLOG(1) or something.

Yes, I'd like to have this in log reports.

https://codereview.chromium.org/11412067/diff/16047/chrome/browser/google/goo...
chrome/browser/google/google_util_chromeos.cc:46:
DCHECK(!BrowserThread::IsWellKnownThread(BrowserThread::UI) ||
On 2012/12/01 03:06:22, Peter Kasting wrote:
> Nit: If you change to this:
> 
>   DCHECK(
>       !content::BrowserThread::IsWellKnownThread(content::BrowserThread::UI)
||
>       content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
> 
> ...you can eliminate the using statement at top (we try to only use these when
> they save a significant number of lines), and also fix the fact that currently
> you indent even but probably should indent 4 instead since your second line
> continues the first rather than starts a new argument.

Done :)

https://codereview.chromium.org/11412067/diff/16047/chrome/browser/google/goo...
chrome/browser/google/google_util_chromeos.cc:49: return local_state ?
local_state->GetString(prefs::kRLZBrand) : std::string();
On 2012/12/01 03:06:22, Peter Kasting wrote:
> Can this ever be NULL?  We shouldn't NULL-check if it can't. 

I don't think it should ever be, only if GetBrand is called from a startup path
too early. Currently the earliest call comes from SetupMetricsAndFieldTrials
where local state is already created. I'll replace |if| with a DCHECK.

https://codereview.chromium.org/11412067/diff/16047/chrome/browser/google/goo...
chrome/browser/google/google_util_chromeos.cc:53: std::string* brand = new
std::string;
On 2012/12/01 03:06:22, Peter Kasting wrote:
> Instead of allocating this, can we just pass the string directly to SetBrand()
> via PostTaskAndReplyWithResult()?

Wow, I've missed this method completely. Thanks!

https://codereview.chromium.org/11412067/diff/16047/chrome/browser/google/goo...
File chrome/browser/google/google_util_chromeos.h (right):

https://codereview.chromium.org/11412067/diff/16047/chrome/browser/google/goo...
chrome/browser/google/google_util_chromeos.h:19: // Reads the brand code from a
board-specific file and stores it to Local State.
On 2012/12/01 03:06:22, Peter Kasting wrote:
> "board" is slightly unclear here... does the brand code really depend on the
> mainboard in the CrOS device?  Maybe this should say "device-specific"?  I
don't
> know, honestly.

Yes, right now the brand code depends on the mainboard and the file is in
/opt/oem/etc, so I called it board-specific.
I'm not sure which wording is better, either.

[commit-bot: I haz the power, 2012-12-05 10:28:40]

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

[commit-bot: I haz the power, 2012-12-05 10:28:58]

Presubmit check for 11412067-17001 failed and returned exit status 1.


Running presubmit commit checks ...

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

Presubmit checks took 8.1s to calculate.

[Ivan Korotkov, 2012-12-05 10:32:59]

Added TBR= for trivial changes in pyauto/, test/ and common/

[commit-bot: I haz the power, 2012-12-05 10:33:17]

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

[commit-bot: I haz the power, 2012-12-05 16:02:26]

Change committed as 171233