[cros] RlzValueStore implementation for ChromeOS.

[cros] RlzValueStore implementation for ChromeOS.


BUG=157348
TEST=rlz_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166891

[Ivan Korotkov, 2012-11-06 16:30:49]

Hi Roger, 

This adds ChromeOS-specific impl of RlzValueStore. It's deliberately not
thread-safe because the underlying JsonPrefStore is supposed to be used on a
single thread only, but that should be OK given the way chrome/browser/rlz works
with it.

[Roger Tawa OOO till Jul 10th, 2012-11-06 20:55:11]

Hi Ivan,

some questions and comments below.  Thanks.

http://codereview.chromium.org/11365107/diff/8010/rlz/chromeos/lib/rlz_value_...
File rlz/chromeos/lib/rlz_value_store_chromeos.cc (right):

http://codereview.chromium.org/11365107/diff/8010/rlz/chromeos/lib/rlz_value_...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:38: return FilePath("/tmp/1.rlz"); 
// TODO(ivankr)
for my own info, why is this a TODO for later?

http://codereview.chromium.org/11365107/diff/8010/rlz/chromeos/lib/rlz_value_...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:65: }
Why is RlzValueStoreChromeOS a singleton?  Why is not like the mac
implementation in this regard?  If it were not a singleton, you would not need
the changes in rlz_value_store.h

http://codereview.chromium.org/11365107/diff/8010/rlz/chromeos/lib/rlz_value_...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:78: rlz_store_->ReadPrefs();
Is ReadPrefs() synchronous?

http://codereview.chromium.org/11365107/diff/8010/rlz/chromeos/lib/rlz_value_...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:166: }
Instead of writing a for loop, use ListValue::AppendStrings().

http://codereview.chromium.org/11365107/diff/8010/rlz/chromeos/lib/rlz_value_...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:222:
rlz_store_->ReportValueChanged(list_name);
why report value changed?

http://codereview.chromium.org/11365107/diff/8010/rlz/chromeos/lib/rlz_value_...
File rlz/chromeos/lib/rlz_value_store_chromeos.h (right):

http://codereview.chromium.org/11365107/diff/8010/rlz/chromeos/lib/rlz_value_...
rlz/chromeos/lib/rlz_value_store_chromeos.h:24: // Thread instance that has a
MessageLoop.
Why is that a valid assumption?  I don't believe it is.  The rlz api is called
from the ui thread and the blocking poll thread.

http://codereview.chromium.org/11365107/diff/8010/rlz/lib/rlz_lib.cc
File rlz/lib/rlz_lib.cc (right):

http://codereview.chromium.org/11365107/diff/8010/rlz/lib/rlz_lib.cc#newcode21
rlz/lib/rlz_lib.cc:21: #endif  //  defined(OS_CHROMEOS)
one space after //

http://codereview.chromium.org/11365107/diff/8010/rlz/lib/rlz_lib.h
File rlz/lib/rlz_lib.h (right):

http://codereview.chromium.org/11365107/diff/8010/rlz/lib/rlz_lib.h#newcode50
rlz/lib/rlz_lib.h:50: }  // namespace base
ifdef this for chromeos?

http://codereview.chromium.org/11365107/diff/8010/rlz/lib/rlz_lib.h#newcode78
rlz/lib/rlz_lib.h:78: // Set that MessageLoopProxy used by RLZ store to run I/O
tasks on.
that --> the ?

http://codereview.chromium.org/11365107/diff/8010/rlz/lib/rlz_lib.h#newcode79
rlz/lib/rlz_lib.h:79: void RLZ_LIB_API SetIOTaskRunner(base::MessageLoopProxy*
io_task_runner);
Add a comment that this should be called before any other api call?

[Ivan Korotkov, 2012-11-07 10:49:12]

http://codereview.chromium.org/11365107/diff/8010/rlz/chromeos/lib/rlz_value_...
File rlz/chromeos/lib/rlz_value_store_chromeos.cc (right):

http://codereview.chromium.org/11365107/diff/8010/rlz/chromeos/lib/rlz_value_...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:38: return FilePath("/tmp/1.rlz"); 
// TODO(ivankr)
On 2012/11/06 20:55:11, Roger Tawa wrote:
> for my own info, why is this a TODO for later?

Oops. I wrote it long ago and forgot to fix. Done now.

http://codereview.chromium.org/11365107/diff/8010/rlz/chromeos/lib/rlz_value_...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:65: }
On 2012/11/06 20:55:11, Roger Tawa wrote:
> Why is RlzValueStoreChromeOS a singleton?  Why is not like the mac
> implementation in this regard?  If it were not a singleton, you would not need
> the changes in rlz_value_store.h

On Mac/Win, multiple processes can write to RLZ store. On ChromeOS, it's only
Chrome.
So there is no need to write RLZ data to disk each time before releasing
ScopedRlzValueStoreLock and reading it from disk after acquiring it; instead
it's a pref store like user prefs/local state prefs, existing for the life of
Chrome process and writing to disk in lazy manner.

http://codereview.chromium.org/11365107/diff/8010/rlz/chromeos/lib/rlz_value_...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:78: rlz_store_->ReadPrefs();
On 2012/11/06 20:55:11, Roger Tawa wrote:
> Is ReadPrefs() synchronous?

Yes. But that shouldn't be a problem since ScopedRlzValueStoreLock is only
acquired on thread allowing I/O (blocking pool currently), right?

http://codereview.chromium.org/11365107/diff/8010/rlz/chromeos/lib/rlz_value_...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:166: }
On 2012/11/06 20:55:11, Roger Tawa wrote:
> Instead of writing a for loop, use ListValue::AppendStrings().

AppendString is for appending string to ListValue. Here I'm reading string from
ListValue into vector<string>

http://codereview.chromium.org/11365107/diff/8010/rlz/chromeos/lib/rlz_value_...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:222:
rlz_store_->ReportValueChanged(list_name);
On 2012/11/06 20:55:11, Roger Tawa wrote:
> why report value changed?

Because the underlying JsonPrefStore doesn't know that list has changed (unless
it wasn't there at all and we created it). ReportValueChanged schedules a write
to disk.

In chrome/browser/prefs, there is ListPrefUpdate/DictionaryPrefUpdate for that,
but they depend on PrefService.

http://codereview.chromium.org/11365107/diff/8010/rlz/chromeos/lib/rlz_value_...
File rlz/chromeos/lib/rlz_value_store_chromeos.h (right):

http://codereview.chromium.org/11365107/diff/8010/rlz/chromeos/lib/rlz_value_...
rlz/chromeos/lib/rlz_value_store_chromeos.h:24: // Thread instance that has a
MessageLoop.
On 2012/11/06 20:55:11, Roger Tawa wrote:
> Why is that a valid assumption?  I don't believe it is.  The rlz api is called
> from the ui thread and the blocking poll thread.

Yes, currently RlzValueStore is accessed on a blocking SequencedWorkerPool
(which uses SimpleThreads w/o a MessageLoop).

The tricky part is, JsonPrefStore havily relies on (1) having a valid
ThreadTaskRunnerHandle, which requires a MessageLoop and (2) being always called
on the same thread.

So on ChromeOS, I'm going to run RLZ code on a dedicated Thread instead of
blocking pool. This shouldn't change anything in terms of performace, since it's
a SequencedWorkerPool anyway, right?

Actually, if that's OK with you, it'd be simpler to not create different
versions of code and introduce this dedicated thread on Win/Mac as well.

http://codereview.chromium.org/11365107/diff/8010/rlz/lib/rlz_lib.cc
File rlz/lib/rlz_lib.cc (right):

http://codereview.chromium.org/11365107/diff/8010/rlz/lib/rlz_lib.cc#newcode21
rlz/lib/rlz_lib.cc:21: #endif  //  defined(OS_CHROMEOS)
On 2012/11/06 20:55:11, Roger Tawa wrote:
> one space after //

Done.

http://codereview.chromium.org/11365107/diff/8010/rlz/lib/rlz_lib.h
File rlz/lib/rlz_lib.h (right):

http://codereview.chromium.org/11365107/diff/8010/rlz/lib/rlz_lib.h#newcode50
rlz/lib/rlz_lib.h:50: }  // namespace base
On 2012/11/06 20:55:11, Roger Tawa wrote:
> ifdef this for chromeos?

Oops, right.

http://codereview.chromium.org/11365107/diff/8010/rlz/lib/rlz_lib.h#newcode78
rlz/lib/rlz_lib.h:78: // Set that MessageLoopProxy used by RLZ store to run I/O
tasks on.
On 2012/11/06 20:55:11, Roger Tawa wrote:
> that --> the ?

Done.

http://codereview.chromium.org/11365107/diff/8010/rlz/lib/rlz_lib.h#newcode79
rlz/lib/rlz_lib.h:79: void RLZ_LIB_API SetIOTaskRunner(base::MessageLoopProxy*
io_task_runner);
On 2012/11/06 20:55:11, Roger Tawa wrote:
> Add a comment that this should be called before any other api call?

Done.

[Ivan Korotkov, 2012-11-08 15:28:27]

Hey Roger, I've updated tests as well so that they pass with chromeos=1
enable_rlz=1

[Roger Tawa OOO till Jul 10th, 2012-11-08 19:26:58]

lgtm, but you need add the branding back in.

http://codereview.chromium.org/11365107/diff/4007/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/11365107/diff/4007/build/common.gypi#newcode1411
build/common.gypi:1411: ['OS=="win" or OS=="mac"', {
I just noticed that you removed the branding.  We don't want rlz enabled for
chromium builds.  Is this just for testing?

[Ivan Korotkov, 2012-11-08 20:02:49]

http://codereview.chromium.org/11365107/diff/4007/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/11365107/diff/4007/build/common.gypi#newcode1411
build/common.gypi:1411: ['OS=="win" or OS=="mac"', {
On 2012/11/08 19:26:58, Roger Tawa wrote:
> I just noticed that you removed the branding.  We don't want rlz enabled for
> chromium builds.  Is this just for testing?

Yeah, it's for testing. I modified some code in financial_ping that runs on all
platforms and wanted to check it on trybots.

[commit-bot: I haz the power, 2012-11-09 08:35:12]

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

[commit-bot: I haz the power, 2012-11-09 11:05:00]

Change committed as 166891