Abstract pref storage from net::SdchOwner

Abstract pref storage from net::SdchOwner

This is in preparation for moving prefs out of base, so it can no longer be
used directly by net. This abstract interface gives allows enough of the
storage system to be implemented for SdchOwner's needs by the embedder.

Implementations in terms of the prefs system are provided for Chrome and
Cronet.

The SDCH unit tests changed the most. Since the new API takes ownership of
the storage object, the tests have to do some management of the storage
objects while the old tests just kept the current one in memory.

Committed: https://crrev.com/66fdcb70dd8379e558e004da476c3088480aac3d
Cr-Commit-Position: refs/heads/master@{#372261}

[brettw, 2016-01-26 21:23:19]

Description was changed from

==========
Abstract pref storage from net::SdchOwner

This is in preparation for moving prefs out of base, so it can no longer be
used directly by net. This abstract interface gives allows enough of the
storage system to be implemented for SdchOwner's needs by the embedder.

Implementations in terms of the prefs system are provided for Chrome and
Cronet.
==========

to

==========
Abstract pref storage from net::SdchOwner

This is in preparation for moving prefs out of base, so it can no longer be
used directly by net. This abstract interface gives allows enough of the
storage system to be implemented for SdchOwner's needs by the embedder.

Implementations in terms of the prefs system are provided for Chrome and
Cronet.

The SDCH unit tests changed the most. Since the new API takes ownership of
the storage object, the tests have to do some management of the storage
objects while the old tests just kept the current one in memory.
==========

[brettw, 2016-01-26 21:23:19]

brettw@chromium.org changed reviewers:
+ mef@chromium.org, rtenneti@chromium.org

[brettw, 2016-01-26 21:24:07]

Raman: net
mef: cronet

This is the second part of https://codereview.chromium.org/1626673002/
With this patch, there are no prefs usage in net.

[brettw, 2016-01-26 21:24:10]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-26 21:25:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634653003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634653003/40001

[commit-bot: I haz the power, 2016-01-26 21:35:15]

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

[commit-bot: I haz the power, 2016-01-26 21:35:16]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[brettw, 2016-01-26 23:15:14]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-26 23:18:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634653003/50001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634653003/50001

[commit-bot: I haz the power, 2016-01-26 23:37:11]

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

[commit-bot: I haz the power, 2016-01-26 23:37:11]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[ramant (doing other things), 2016-01-26 23:54:34]

rtenneti@chromium.org changed reviewers:
+ rdsmith@chromium.org

[brettw, 2016-01-27 00:02:23]

mef: I need an adapter for ios/crnet also. I assume this is like cronet for iOS.
It looks like ios/crnet can use components. Is there a good place that
ios/chrome and ios/crnet can share an adapter implementation, or do I need to
duplicate it?

[ramant (doing other things), 2016-01-27 00:12:34]

lgtm

+rdsmith (owner of sdch).

[mef, 2016-01-27 15:25:56]

On 2016/01/27 00:02:23, brettw wrote:
> mef: I need an adapter for ios/crnet also. I assume this is like cronet for
iOS.
> It looks like ios/crnet can use components. Is there a good place that
> ios/chrome and ios/crnet can share an adapter implementation, or do I need to
> duplicate it?

Hrm, good question, I'm not quite sure about sharing between ios/chrome and
ios/crnet.
I think stuartmorgan@, davidroger@ or ellyjones@ might know.

The ios/crnet is indeed like components/cronet/android, and we expected to use
components/cronet for common cross-platform code.
Maybe SdchOwnerPrefStorage could be extracted into separate file and shared that
way?

[mef, 2016-01-27 15:26:58]

mef@chromium.org changed reviewers:
+ ellyjones@chromium.org

[mef, 2016-01-27 15:26:58]

+ellyjones to comment on ios/crnet sharing with ios/chrome.

[mef, 2016-01-27 16:21:34]

mef@chromium.org changed reviewers:
+ xunjieli@chromium.org

[mef, 2016-01-27 16:21:34]

Assigning xunjieli for cronet/ review as she did the integration with sdch and
pref storage.

[Randy Smith (Not in Mondays), 2016-01-27 21:48:17]

Thanks for doing this!  I think this looks basically good, but I'd like to do
another round for the issues below.  

(I'm reviewing everything but the ios and android specific code.)

https://codereview.chromium.org/1634653003/diff/50001/chrome/browser/net/sdch...
File chrome/browser/net/sdch_owner_pref_storage.cc (right):

https://codereview.chromium.org/1634653003/diff/50001/chrome/browser/net/sdch...
chrome/browser/net/sdch_owner_pref_storage.cc:14: storage_key_("SDCH"),
I'd like this to be abstracted out into a constant.  In an ideal world, there
would be some way that multiple different clients using the same
PersistentPrefStore could automatically confirm that they weren't colliding on
the same key, but I assume for the moment that's not a concern so I won't worry
about it.

https://codereview.chromium.org/1634653003/diff/50001/chrome/browser/net/sdch...
chrome/browser/net/sdch_owner_pref_storage.cc:54: // "other" error value.
Could you say a bit more about your logic here?  I would imagine that we'd want
to map these to an "other" error value, so that there's the possibility of UMA
raising a flag if unanticipated things start happening.  If we map to NONE, I
don't think there's any way we'd ever notice.

https://codereview.chromium.org/1634653003/diff/50001/chrome/browser/net/sdch...
chrome/browser/net/sdch_owner_pref_storage.cc:105:
init_observer_->OnPrefStorageInitializationComplete(succeeded);
There's an implication in this line about the interface contract (i.e. you must
call StartObservingInit before the initialization could be complete, and you
can't call StopObservingInit() until after it has completed).  Would you be
willing to either document the requirements in the header file or relax them
here (presumably by testing init_observer_ for nullity)?   

(I'm torn about which of those is better, as testing init_observer_ for nullity
opens us up to silent races; I *think* I'd prefer documentation + some DCHECKs,
but I'm not sure, so I'll leaving it up to you.)

https://codereview.chromium.org/1634653003/diff/50001/net/sdch/sdch_owner.h
File net/sdch/sdch_owner.h (right):

https://codereview.chromium.org/1634653003/diff/50001/net/sdch/sdch_owner.h#n...
net/sdch/sdch_owner.h:65: virtual ReadError GetReadError() const = 0;
I think it's worth documenting that it's possible if unlikely for this to return
PERSISTENCE_FAILURE_NONE even if initialization failed (as I understand the
comments in the derived class) so that future code knows to just read this for
histograms and not for success/failure conditionals.

[brettw, 2016-01-27 22:22:38]

Description was changed from

==========
Abstract pref storage from net::SdchOwner

This is in preparation for moving prefs out of base, so it can no longer be
used directly by net. This abstract interface gives allows enough of the
storage system to be implemented for SdchOwner's needs by the embedder.

Implementations in terms of the prefs system are provided for Chrome and
Cronet.

The SDCH unit tests changed the most. Since the new API takes ownership of
the storage object, the tests have to do some management of the storage
objects while the old tests just kept the current one in memory.
==========

to

==========
Abstract pref storage from net::SdchOwner

This is in preparation for moving prefs out of base, so it can no longer be
used directly by net. This abstract interface gives allows enough of the
storage system to be implemented for SdchOwner's needs by the embedder.

Implementations in terms of the prefs system are provided for Chrome and
Cronet.

The SDCH unit tests changed the most. Since the new API takes ownership of
the storage object, the tests have to do some management of the storage
objects while the old tests just kept the current one in memory.

TBR=asvitkine for histograms.xml
==========

[brettw, 2016-01-27 22:22:38]

brettw@chromium.org changed reviewers:
+ asvitkine@google.com

[brettw, 2016-01-27 22:23:54]

+asvitkine TBR for histograms.xml

I added an adapter for iOS. It looks like the only way to share this is to make
a new component containing only this file. Given that this tries to be the
thinnest possible shim and this code doesn't really change, I think it's
actualyl better to duplicate in this case.

https://codereview.chromium.org/1634653003/diff/50001/chrome/browser/net/sdch...
File chrome/browser/net/sdch_owner_pref_storage.cc (right):

https://codereview.chromium.org/1634653003/diff/50001/chrome/browser/net/sdch...
chrome/browser/net/sdch_owner_pref_storage.cc:54: // "other" error value.
I was trying to avoid updating the histogram but now can't think of why it's
necessary to avoid doing that. I added OTHER here and to the histogram.

https://codereview.chromium.org/1634653003/diff/50001/chrome/browser/net/sdch...
chrome/browser/net/sdch_owner_pref_storage.cc:105:
init_observer_->OnPrefStorageInitializationComplete(succeeded);
On 2016/01/27 21:48:17, Randy Smith - Not in Fridays wrote:
> There's an implication in this line about the interface contract (i.e. you
must
> call StartObservingInit before the initialization could be complete, and you
> can't call StopObservingInit() until after it has completed). 

I changed the comments in the interface to make them a bit more precise and
added a DCHECK here for null. But I don't agree with what you wrote here. You
can register or unregister the observer whenever you want as long as there's
only one. This class will only be a pref store observer when the SdchOwner is
registered as an observer, so we'll only get here when the SdchOwner is
registered.

[brettw, 2016-01-27 22:24:00]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-27 22:25:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634653003/70001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634653003/70001

[commit-bot: I haz the power, 2016-01-27 22:37:10]

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

[commit-bot: I haz the power, 2016-01-27 22:37:11]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[Alexei Svitkine (slow), 2016-01-27 22:39:43]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-01-27 22:39:43]

histograms lgtm

[xunjieli, 2016-01-27 22:57:39]

components/cronet/android/cronet_url_request_context_adapter.cc lgtm.

https://codereview.chromium.org/1634653003/diff/70001/ios/crnet/sdch_owner_pr...
File ios/crnet/sdch_owner_pref_storage.h (right):

https://codereview.chromium.org/1634653003/diff/70001/ios/crnet/sdch_owner_pr...
ios/crnet/sdch_owner_pref_storage.h:14: // Provides and implementation of
SdchOwner::PrefStorage that maps to
s/and/an

https://codereview.chromium.org/1634653003/diff/70001/net/sdch/sdch_owner.h
File net/sdch/sdch_owner.h (right):

https://codereview.chromium.org/1634653003/diff/70001/net/sdch/sdch_owner.h#n...
net/sdch/sdch_owner.h:83: // Starts and stops observing preferences storage
changes. There will only
Can we either update this comment or remove the "Init" suffix from the method
names? I know SdchOwner only care about the init event at the moment.

[brettw, 2016-01-27 23:20:19]

Description was changed from

==========
Abstract pref storage from net::SdchOwner

This is in preparation for moving prefs out of base, so it can no longer be
used directly by net. This abstract interface gives allows enough of the
storage system to be implemented for SdchOwner's needs by the embedder.

Implementations in terms of the prefs system are provided for Chrome and
Cronet.

The SDCH unit tests changed the most. Since the new API takes ownership of
the storage object, the tests have to do some management of the storage
objects while the old tests just kept the current one in memory.

TBR=asvitkine for histograms.xml
==========

to

==========
Abstract pref storage from net::SdchOwner

This is in preparation for moving prefs out of base, so it can no longer be
used directly by net. This abstract interface gives allows enough of the
storage system to be implemented for SdchOwner's needs by the embedder.

Implementations in terms of the prefs system are provided for Chrome and
Cronet.

The SDCH unit tests changed the most. Since the new API takes ownership of
the storage object, the tests have to do some management of the storage
objects while the old tests just kept the current one in memory.
==========

[brettw, 2016-01-27 23:20:39]

https://codereview.chromium.org/1634653003/diff/70001/ios/crnet/sdch_owner_pr...
File ios/crnet/sdch_owner_pref_storage.h (right):

https://codereview.chromium.org/1634653003/diff/70001/ios/crnet/sdch_owner_pr...
ios/crnet/sdch_owner_pref_storage.h:14: // Provides and implementation of
SdchOwner::PrefStorage that maps to
On 2016/01/27 22:57:39, xunjieli wrote:
> s/and/an

Done.

https://codereview.chromium.org/1634653003/diff/70001/net/sdch/sdch_owner.h
File net/sdch/sdch_owner.h (right):

https://codereview.chromium.org/1634653003/diff/70001/net/sdch/sdch_owner.h#n...
net/sdch/sdch_owner.h:83: // Starts and stops observing preferences storage
changes. There will only
On 2016/01/27 22:57:39, xunjieli wrote:
> Can we either update this comment or remove the "Init" suffix from the method
> names? I know SdchOwner only care about the init event at the moment.

Done.

[brettw, 2016-01-27 23:20:45]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-27 23:21:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634653003/90001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634653003/90001

[commit-bot: I haz the power, 2016-01-27 23:36:39]

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

[commit-bot: I haz the power, 2016-01-27 23:36:40]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Randy Smith (Not in Mondays), 2016-01-28 17:45:56]

LGTM.

https://codereview.chromium.org/1634653003/diff/50001/chrome/browser/net/sdch...
File chrome/browser/net/sdch_owner_pref_storage.cc (right):

https://codereview.chromium.org/1634653003/diff/50001/chrome/browser/net/sdch...
chrome/browser/net/sdch_owner_pref_storage.cc:105:
init_observer_->OnPrefStorageInitializationComplete(succeeded);
On 2016/01/27 22:23:54, brettw wrote:
> On 2016/01/27 21:48:17, Randy Smith - Not in Fridays wrote:
> > There's an implication in this line about the interface contract (i.e. you
> must
> > call StartObservingInit before the initialization could be complete, and you
> > can't call StopObservingInit() until after it has completed). 
> 
> I changed the comments in the interface to make them a bit more precise and
> added a DCHECK here for null. But I don't agree with what you wrote here. You
> can register or unregister the observer whenever you want as long as there's
> only one. This class will only be a pref store observer when the SdchOwner is
> registered as an observer, so we'll only get here when the SdchOwner is
> registered.

Ah, gotcha.  That makes sense.  Sorry about the misunderstanding.

[brettw, 2016-01-28 23:33:48]

The CQ bit was checked by brettw@chromium.org

[brettw, 2016-01-28 23:33:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
rtenneti@chromium.org, xunjieli@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1634653003/#ps110001
(title: "typo")

[commit-bot: I haz the power, 2016-01-28 23:35:03]

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

[commit-bot: I haz the power, 2016-01-28 23:44:05]

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

[commit-bot: I haz the power, 2016-01-28 23:44:06]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[brettw, 2016-01-28 23:59:41]

The CQ bit was checked by brettw@chromium.org

[brettw, 2016-01-28 23:59:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdsmith@chromium.org, xunjieli@chromium.org, asvitkine@chromium.org,
rtenneti@chromium.org
Link to the patchset: https://codereview.chromium.org/1634653003/#ps130001
(title: "Fix iOS")

[commit-bot: I haz the power, 2016-01-29 00:00:48]

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

[commit-bot: I haz the power, 2016-01-29 01:24:05]

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

[commit-bot: I haz the power, 2016-01-29 01:24:07]

Failed to commit the patch.

[commit-bot: I haz the power, 2016-01-29 01:24:53]

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

[brettw, 2016-01-29 02:32:28]

The CQ bit was checked by brettw@chromium.org

[commit-bot: I haz the power, 2016-01-29 02:34:33]

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

[commit-bot: I haz the power, 2016-01-29 02:40:26]

Description was changed from

==========
Abstract pref storage from net::SdchOwner

This is in preparation for moving prefs out of base, so it can no longer be
used directly by net. This abstract interface gives allows enough of the
storage system to be implemented for SdchOwner's needs by the embedder.

Implementations in terms of the prefs system are provided for Chrome and
Cronet.

The SDCH unit tests changed the most. Since the new API takes ownership of
the storage object, the tests have to do some management of the storage
objects while the old tests just kept the current one in memory.
==========

to

==========
Abstract pref storage from net::SdchOwner

This is in preparation for moving prefs out of base, so it can no longer be
used directly by net. This abstract interface gives allows enough of the
storage system to be implemented for SdchOwner's needs by the embedder.

Implementations in terms of the prefs system are provided for Chrome and
Cronet.

The SDCH unit tests changed the most. Since the new API takes ownership of
the storage object, the tests have to do some management of the storage
objects while the old tests just kept the current one in memory.
==========

[commit-bot: I haz the power, 2016-01-29 02:40:27]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2016-01-29 02:41:29]

Description was changed from

==========
Abstract pref storage from net::SdchOwner

This is in preparation for moving prefs out of base, so it can no longer be
used directly by net. This abstract interface gives allows enough of the
storage system to be implemented for SdchOwner's needs by the embedder.

Implementations in terms of the prefs system are provided for Chrome and
Cronet.

The SDCH unit tests changed the most. Since the new API takes ownership of
the storage object, the tests have to do some management of the storage
objects while the old tests just kept the current one in memory.
==========

to

==========
Abstract pref storage from net::SdchOwner

This is in preparation for moving prefs out of base, so it can no longer be
used directly by net. This abstract interface gives allows enough of the
storage system to be implemented for SdchOwner's needs by the embedder.

Implementations in terms of the prefs system are provided for Chrome and
Cronet.

The SDCH unit tests changed the most. Since the new API takes ownership of
the storage object, the tests have to do some management of the storage
objects while the old tests just kept the current one in memory.

Committed: https://crrev.com/66fdcb70dd8379e558e004da476c3088480aac3d
Cr-Commit-Position: refs/heads/master@{#372261}
==========

[commit-bot: I haz the power, 2016-01-29 02:41:30]

Patchset 8 (id:??) landed as
https://crrev.com/66fdcb70dd8379e558e004da476c3088480aac3d
Cr-Commit-Position: refs/heads/master@{#372261}