Create a PrefStore in support of Blimp metrics collection.

Create a PrefStore in support of Blimp metrics collection.

A light-weight memory-backed perfstore implementation. Needed for MetricService support. Used to hold logs until upload.

Moving the existing store for android webview into a shared space under components/pref and adding tests.

BUG=592757, 604955
Committed: https://crrev.com/b891cd667f263782a6f97466e264de3e729d3bc0
Cr-Commit-Position: refs/heads/master@{#389291}

[Jess, 2016-04-13 00:04:36]

Description was changed from

==========
Create a BlimpPrefStore in support of metrics collection.

A light-weight memory backed perfstore implementation. Needed for
MetricService support.


BUG=592757
==========

to

==========
Create a BlimpPrefStore in support of metrics collection.

A light-weight memory-backed perfstore implementation. Needed for MetricService
support. Used to hold logs until upload.


BUG=592757
==========

[Jess, 2016-04-13 00:04:36]

jessicag@chromium.org changed reviewers:
+ kmarshall@chromium.org

[Jess, 2016-04-13 00:04:36]

jessicag@chromium.org changed required reviewers:
+ kmarshall@chromium.org

[Jess, 2016-04-14 18:04:18]

I went with a minimal lightweight implementation since that should be all that
is required for metrics collection support.

[Kevin M, 2016-04-14 21:49:50]

https://codereview.chromium.org/1881253003/diff/20001/blimp/engine/common/bli...
File blimp/engine/common/blimp_pref_store.cc (right):

https://codereview.chromium.org/1881253003/diff/20001/blimp/engine/common/bli...
blimp/engine/common/blimp_pref_store.cc:14: BlimpPrefStore::BlimpPrefStore() {}
Add newline above

https://codereview.chromium.org/1881253003/diff/20001/blimp/engine/common/bli...
File blimp/engine/common/blimp_pref_store.h (right):

https://codereview.chromium.org/1881253003/diff/20001/blimp/engine/common/bli...
blimp/engine/common/blimp_pref_store.h:20: // A light-weight prefstore
implementation that keeps preferences
Add newline above

https://codereview.chromium.org/1881253003/diff/20001/blimp/engine/common/bli...
blimp/engine/common/blimp_pref_store.h:23: // PrefService, which in turn is
needed by the metrics component.
Could you reference AwPrefStore as the basis for this class?

Have you considered making this a common class, e.g. InMemoryPrefStore, which
android_webview and Blimp can share?

https://codereview.chromium.org/1881253003/diff/20001/blimp/engine/common/bli...
blimp/engine/common/blimp_pref_store.h:24: class BlimpPrefStore : public
PersistentPrefStore {
Unit tests?

https://codereview.chromium.org/1881253003/diff/20001/blimp/engine/common/bli...
blimp/engine/common/blimp_pref_store.h:28: // Overriden from PrefStore.
PrefStore implementation.

https://codereview.chromium.org/1881253003/diff/20001/blimp/engine/common/bli...
blimp/engine/common/blimp_pref_store.h:36: // PersistentPrefStore overrides:
"PersistentPrefStore implementation."

[Jess, 2016-04-14 22:59:49]

Please share your thoughts on the refactor.

Unfortunately there are some implementation differences between AwPrefStore and
BlimpPerfStore already so we can't just blindly use the same class without at
least a little refactoring.

https://codereview.chromium.org/1881253003/diff/20001/blimp/engine/common/bli...
File blimp/engine/common/blimp_pref_store.cc (right):

https://codereview.chromium.org/1881253003/diff/20001/blimp/engine/common/bli...
blimp/engine/common/blimp_pref_store.cc:14: BlimpPrefStore::BlimpPrefStore() {}
On 2016/04/14 21:49:50, Kevin M wrote:
> Add newline above

Done.

https://codereview.chromium.org/1881253003/diff/20001/blimp/engine/common/bli...
File blimp/engine/common/blimp_pref_store.h (right):

https://codereview.chromium.org/1881253003/diff/20001/blimp/engine/common/bli...
blimp/engine/common/blimp_pref_store.h:20: // A light-weight prefstore
implementation that keeps preferences
On 2016/04/14 21:49:50, Kevin M wrote:
> Add newline above

Done.

https://codereview.chromium.org/1881253003/diff/20001/blimp/engine/common/bli...
blimp/engine/common/blimp_pref_store.h:23: // PrefService, which in turn is
needed by the metrics component.
On 2016/04/14 21:49:50, Kevin M wrote:
> Could you reference AwPrefStore as the basis for this class?
> 
> Have you considered making this a common class, e.g. InMemoryPrefStore, which
> android_webview and Blimp can share?

Yes.

I'd been assuming the engine need something more in terms of PrefStore
relatively soon, which is why I copied instead of refactoring.

If that isn't the case, I'm happy to refactor things out. I'd appreciate some
guidance on Chromium best practices for this.  I'm not sure where the code would
live or how best to bring in the android webview folks onboard for the refactor.

https://codereview.chromium.org/1881253003/diff/20001/blimp/engine/common/bli...
blimp/engine/common/blimp_pref_store.h:24: class BlimpPrefStore : public
PersistentPrefStore {
On 2016/04/14 21:49:50, Kevin M wrote:
> Unit tests?

Do you have a specific function or set of function in mind? The parent classes
are doing the heavy lifting.

https://codereview.chromium.org/1881253003/diff/20001/blimp/engine/common/bli...
blimp/engine/common/blimp_pref_store.h:28: // Overriden from PrefStore.
On 2016/04/14 21:49:50, Kevin M wrote:
> PrefStore implementation.

Done.

https://codereview.chromium.org/1881253003/diff/20001/blimp/engine/common/bli...
blimp/engine/common/blimp_pref_store.h:36: // PersistentPrefStore overrides:
On 2016/04/14 21:49:50, Kevin M wrote:
> "PersistentPrefStore implementation."

Done.

[Kevin M, 2016-04-15 17:45:38]

https://codereview.chromium.org/1881253003/diff/20001/blimp/engine/common/bli...
File blimp/engine/common/blimp_pref_store.h (right):

https://codereview.chromium.org/1881253003/diff/20001/blimp/engine/common/bli...
blimp/engine/common/blimp_pref_store.h:24: class BlimpPrefStore : public
PersistentPrefStore {
On 2016/04/14 22:59:49, Jess wrote:
> On 2016/04/14 21:49:50, Kevin M wrote:
> > Unit tests?
> 
> Do you have a specific function or set of function in mind? The parent classes
> are doing the heavy lifting.

Can you inject a test-owned prefs_ and verify that the setters and getters are
passing through the data correctly, and invoking the observers when they should?

[Jess, 2016-04-18 21:40:45]

Sounds reasonable. Will structure tests as soon as the decision on the refactor
seems firm.

What are your thoughts there? I don't have the insight into the engine plans.
Will the basic shared structure live long enough to be worth the refactoring
effort?

[Kevin M, 2016-04-19 19:57:37]

lgtm

Can you create a bug to track for the refactoring task? I don't think we need to
hold up the metrics collection effort for it. This code is fairly trivial, so
some duplication isn't a huge issue.

[Jess, 2016-04-20 00:00:54]

PTAL. Tests added.  Please confirm I covered all cases you wanted to see.

[Kevin M, 2016-04-20 23:03:35]

lgtm

Yeah, that's what I had in mind. Thanks for writing the tests.

https://codereview.chromium.org/1881253003/diff/80001/blimp/engine/common/bli...
File blimp/engine/common/blimp_pref_store_unittest.cc (right):

https://codereview.chromium.org/1881253003/diff/80001/blimp/engine/common/bli...
blimp/engine/common/blimp_pref_store_unittest.cc:48: // Add one.
Add newlines before commented blocks, so...

EXPECT_FALSE(...)

// Add one
store_->AddObserver(...)
EXPECT_TRUE(...)

// Remove only observer.


Ditto for other comments in this file.

[Jess, 2016-04-20 23:31:24]

The CQ bit was checked by jessicag@chromium.org

[Jess, 2016-04-20 23:31:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1881253003/#ps100001
(title: "Requested comment style clean up.")

[Jess, 2016-04-20 23:32:07]

https://codereview.chromium.org/1881253003/diff/80001/blimp/engine/common/bli...
File blimp/engine/common/blimp_pref_store_unittest.cc (right):

https://codereview.chromium.org/1881253003/diff/80001/blimp/engine/common/bli...
blimp/engine/common/blimp_pref_store_unittest.cc:48: // Add one.
On 2016/04/20 23:03:35, Kevin M wrote:
> Add newlines before commented blocks, so...
> 
> EXPECT_FALSE(...)
> 
> // Add one
> store_->AddObserver(...)
> EXPECT_TRUE(...)
> 
> // Remove only observer.
> 
> 
> Ditto for other comments in this file.

Done.

[Jess, 2016-04-20 23:32:47]

The CQ bit was unchecked by jessicag@chromium.org

[Jess, 2016-04-20 23:42:56]

The CQ bit was checked by jessicag@chromium.org

[Jess, 2016-04-20 23:42:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1881253003/#ps120001
(title: "Missed comment style clean up.")

[commit-bot: I haz the power, 2016-04-20 23:44:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1881253003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1881253003/120001

[commit-bot: I haz the power, 2016-04-21 00:04:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-21 00:04:27]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Jess, 2016-04-21 00:09:30]

jessicag@chromium.org changed reviewers:
+ battre@chromium.org

[Jess, 2016-04-21 00:09:31]

jessicag@chromium.org changed required reviewers:
+ battre@chromium.org

[Jess, 2016-04-21 00:09:32]

Could you please review for the addition of //components/pref to Blimps DEPS?

[battre, 2016-04-21 08:15:38]

battre@chromium.org changed reviewers:
+ bauerb@chromium.org

[battre, 2016-04-21 08:15:39]

Isn't this a complete copy of AwPrefStore? Wouldn't it be better to add an
InMemoryPrefStore to components/prefs?

+bauerb for second/third opinion.

[Bernhard Bauer, 2016-04-21 09:00:57]

On 2016/04/21 08:15:39, battre wrote:
> Isn't this a complete copy of AwPrefStore? Wouldn't it be better to add an
> InMemoryPrefStore to components/prefs?
> 
> +bauerb for second/third opinion.

+1! All that seems to require would be to rename the class and move it into
components/prefs/ (of which there are now two OWNERS already on the review 😃).

[Jess, 2016-04-21 21:26:55]

Description was changed from

==========
Create a BlimpPrefStore in support of metrics collection.

A light-weight memory-backed perfstore implementation. Needed for MetricService
support. Used to hold logs until upload.


BUG=592757
==========

to

==========
Create a PrefStore in support of Blimp metrics collection.

A light-weight memory-backed perfstore implementation. Needed for MetricService
support. Used to hold logs until upload.

Moving the existing store for android webview into a shared space under
components/pref and adding tests.

BUG=592757,604955
==========

[Jess, 2016-04-21 21:26:55]

jessicag@chromium.org changed reviewers:
+ michaelbai@chromium.org, torne@chromium.org

[Jess, 2016-04-21 21:26:55]

jessicag@chromium.org changed required reviewers:
+ torne@chromium.org

[Jess, 2016-04-21 21:32:59]

+ Webview folks

Was hoping to get something working in quickly with minimal changes outside the
Blimp code base to unblock follow up work and a handle refactor involving other
groups in parallel.

Still getting a feel for how these trade offs work in Chromium. =)

[Torne, 2016-04-22 13:00:22]

This seems like a brilliant idea to me. Even adding unit tests! The luxury ;P

One comment nit, but LGTM in general :)

https://codereview.chromium.org/1881253003/diff/180001/components/prefs/in_me...
File components/prefs/in_memory_pref_store.h (right):

https://codereview.chromium.org/1881253003/diff/180001/components/prefs/in_me...
components/prefs/in_memory_pref_store.h:21: // PrefService, which in turn is
needed by the metrics component.
Maybe just say "various components"? There's several things that need prefs that
will end up being used by top-level targets that pull this in.

[Jess, 2016-04-22 17:23:24]

https://codereview.chromium.org/1881253003/diff/180001/components/prefs/in_me...
File components/prefs/in_memory_pref_store.h (right):

https://codereview.chromium.org/1881253003/diff/180001/components/prefs/in_me...
components/prefs/in_memory_pref_store.h:21: // PrefService, which in turn is
needed by the metrics component.
On 2016/04/22 13:00:22, Torne wrote:
> Maybe just say "various components"? There's several things that need prefs
that
> will end up being used by top-level targets that pull this in.

Done.

[Bernhard Bauer, 2016-04-22 18:10:57]

LGTM, thanks!

[Jess, 2016-04-22 19:24:02]

The CQ bit was checked by jessicag@chromium.org

[Jess, 2016-04-22 19:24:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
kmarshall@chromium.org, torne@chromium.org
Link to the patchset: https://codereview.chromium.org/1881253003/#ps220001
(title: "Remove extraneous the in comment.")

[commit-bot: I haz the power, 2016-04-22 19:24:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1881253003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1881253003/220001

[commit-bot: I haz the power, 2016-04-22 19:24:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-22 19:24:26]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Jess, 2016-04-22 19:49:12]

jessicag@chromium.org changed required reviewers:
+ bauerb@chromium.org
- battre@chromium.org

[Jess, 2016-04-22 19:49:25]

The CQ bit was checked by jessicag@chromium.org

[commit-bot: I haz the power, 2016-04-22 19:50:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1881253003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1881253003/220001

[commit-bot: I haz the power, 2016-04-22 20:46:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-22 20:46:31]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jess, 2016-04-22 21:18:59]

The CQ bit was checked by jessicag@chromium.org

[Jess, 2016-04-22 21:19:00]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
kmarshall@chromium.org, torne@chromium.org
Link to the patchset: https://codereview.chromium.org/1881253003/#ps240001
(title: "Fix conflict with ptr migration work.")

[commit-bot: I haz the power, 2016-04-22 21:19:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1881253003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1881253003/240001

[commit-bot: I haz the power, 2016-04-22 23:16:28]

Description was changed from

==========
Create a PrefStore in support of Blimp metrics collection.

A light-weight memory-backed perfstore implementation. Needed for MetricService
support. Used to hold logs until upload.

Moving the existing store for android webview into a shared space under
components/pref and adding tests.

BUG=592757,604955
==========

to

==========
Create a PrefStore in support of Blimp metrics collection.

A light-weight memory-backed perfstore implementation. Needed for MetricService
support. Used to hold logs until upload.

Moving the existing store for android webview into a shared space under
components/pref and adding tests.

BUG=592757,604955
==========

[commit-bot: I haz the power, 2016-04-22 23:16:30]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-04-22 23:17:24]

Description was changed from

==========
Create a PrefStore in support of Blimp metrics collection.

A light-weight memory-backed perfstore implementation. Needed for MetricService
support. Used to hold logs until upload.

Moving the existing store for android webview into a shared space under
components/pref and adding tests.

BUG=592757,604955
==========

to

==========
Create a PrefStore in support of Blimp metrics collection.

A light-weight memory-backed perfstore implementation. Needed for MetricService
support. Used to hold logs until upload.

Moving the existing store for android webview into a shared space under
components/pref and adding tests.

BUG=592757,604955

Committed: https://crrev.com/b891cd667f263782a6f97466e264de3e729d3bc0
Cr-Commit-Position: refs/heads/master@{#389291}
==========

[commit-bot: I haz the power, 2016-04-22 23:17:26]

Patchset 13 (id:??) landed as
https://crrev.com/b891cd667f263782a6f97466e264de3e729d3bc0
Cr-Commit-Position: refs/heads/master@{#389291}