rAc i18n: implement storage interface for libaddressinput

rAc i18n: implement storage interface for libaddressinput

BUG=325539
TBR=thakis@chromium.org

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

[Evan Stade, 2013-12-20 00:35:19]

+rouslan for libaddressinput
+battre for prefs changes

[please use gerrit instead, 2013-12-20 02:04:40]

libaddressinput overall looks good % a few comments.

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
File third_party/libaddressinput/chromium/chrome_storage_impl.h (right):

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
third_party/libaddressinput/chromium/chrome_storage_impl.h:9: #include
<libaddressinput/util/scoped_ptr.h>
As this is not a part of libaddressinput, you can follow Chrome's include naming
conventions (to fully qualify the include path starting at src/).

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
third_party/libaddressinput/chromium/chrome_storage_impl.h:20: namespace
addressinput {
As this is not a part of libaddressinput, your class should not be in
i18n::addressinput namespace.

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
third_party/libaddressinput/chromium/chrome_storage_impl.h:28:
ChromeStorageImpl(WriteablePrefStore* store);
explicit

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
File third_party/libaddressinput/chromium/chrome_storage_impl_test.cc (right):

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
third_party/libaddressinput/chromium/chrome_storage_impl_test.cc:13: //
limitations under the License.
Use Chrome's license/copyright header, please.

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
third_party/libaddressinput/chromium/chrome_storage_impl_test.cc:19: #include
<gtest/gtest.h>
Since this code is not part of libaddressinput, then you should use Chrome's
include style (by specifying the full path to gtest.h under src/).

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
File third_party/libaddressinput/chromium/cpp/test/storage_test_runner.cc
(right):

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
third_party/libaddressinput/chromium/cpp/test/storage_test_runner.cc:31:
EXPECT_NO_FATAL_FAILURE(GetWithoutPutReturnsEmptyData());
Also GetReturnsWhatWasPut() and SecondPutOverwritesData().

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
File third_party/libaddressinput/chromium/cpp/test/storage_test_runner.h
(right):

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
third_party/libaddressinput/chromium/cpp/test/storage_test_runner.h:29:
StorageTestRunner(Storage* storage);
explicit

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
File third_party/libaddressinput/libaddressinput.gyp (right):

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
third_party/libaddressinput/libaddressinput.gyp:139:
'<(libaddressinput_dir)/cpp/test/storage_test_runner.cc',
Also please add 'test/storage_test_runner.cc' to
src/third_party/libaddressinput/chromium/cpp/libaddressinput.gyp.

[Evan Stade, 2013-12-20 03:21:56]

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
File third_party/libaddressinput/chromium/chrome_storage_impl.h (right):

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
third_party/libaddressinput/chromium/chrome_storage_impl.h:9: #include
<libaddressinput/util/scoped_ptr.h>
On 2013/12/20 02:04:40, Rouslan Solomakhin wrote:
> As this is not a part of libaddressinput, you can follow Chrome's include
naming
> conventions (to fully qualify the include path starting at src/).

Done.

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
third_party/libaddressinput/chromium/chrome_storage_impl.h:20: namespace
addressinput {
On 2013/12/20 02:04:40, Rouslan Solomakhin wrote:
> As this is not a part of libaddressinput, your class should not be in
> i18n::addressinput namespace.

Done.

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
third_party/libaddressinput/chromium/chrome_storage_impl.h:28:
ChromeStorageImpl(WriteablePrefStore* store);
On 2013/12/20 02:04:40, Rouslan Solomakhin wrote:
> explicit

Done.

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
File third_party/libaddressinput/chromium/chrome_storage_impl_test.cc (right):

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
third_party/libaddressinput/chromium/chrome_storage_impl_test.cc:13: //
limitations under the License.
On 2013/12/20 02:04:40, Rouslan Solomakhin wrote:
> Use Chrome's license/copyright header, please.

Done.

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
third_party/libaddressinput/chromium/chrome_storage_impl_test.cc:19: #include
<gtest/gtest.h>
On 2013/12/20 02:04:40, Rouslan Solomakhin wrote:
> Since this code is not part of libaddressinput, then you should use Chrome's
> include style (by specifying the full path to gtest.h under src/).

Done.

also changed the name of this file to match chrome convention.

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
File third_party/libaddressinput/chromium/cpp/test/storage_test_runner.cc
(right):

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
third_party/libaddressinput/chromium/cpp/test/storage_test_runner.cc:31:
EXPECT_NO_FATAL_FAILURE(GetWithoutPutReturnsEmptyData());
On 2013/12/20 02:04:40, Rouslan Solomakhin wrote:
> Also GetReturnsWhatWasPut() and SecondPutOverwritesData().

Done.

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
File third_party/libaddressinput/chromium/cpp/test/storage_test_runner.h
(right):

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
third_party/libaddressinput/chromium/cpp/test/storage_test_runner.h:29:
StorageTestRunner(Storage* storage);
On 2013/12/20 02:04:40, Rouslan Solomakhin wrote:
> explicit

Done.

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
File third_party/libaddressinput/libaddressinput.gyp (right):

https://codereview.chromium.org/98623005/diff/80001/third_party/libaddressinp...
third_party/libaddressinput/libaddressinput.gyp:139:
'<(libaddressinput_dir)/cpp/test/storage_test_runner.cc',
On 2013/12/20 02:04:40, Rouslan Solomakhin wrote:
> Also please add 'test/storage_test_runner.cc' to
> src/third_party/libaddressinput/chromium/cpp/libaddressinput.gyp.

Done.

[battre, 2013-12-20 12:12:12]

https://codereview.chromium.org/98623005/diff/90001/base/prefs/writeable_pref...
File base/prefs/writeable_pref_store.h (right):

https://codereview.chromium.org/98623005/diff/90001/base/prefs/writeable_pref...
base/prefs/writeable_pref_store.h:11: #include "base/memory/ref_counted.h"
this is not needed, right?

https://codereview.chromium.org/98623005/diff/90001/third_party/libaddressinp...
File third_party/libaddressinput/chromium/chrome_storage_impl.cc (right):

https://codereview.chromium.org/98623005/diff/90001/third_party/libaddressinp...
third_party/libaddressinput/chromium/chrome_storage_impl.cc:10:
ChromeStorageImpl::ChromeStorageImpl(WriteablePrefStore* store)
How are you planning to instantiate this class? I am concerned that it gets
linked directly to one of the PrefStores instead of the PrefService which would
be a layering violation.

The PrefService ensures for example that some preferences can be controlled by
policies. If you hook this directly onto the JsonPrefStore, that would not be
possible.

https://codereview.chromium.org/98623005/diff/90001/third_party/libaddressinp...
File third_party/libaddressinput/chromium/chrome_storage_impl_unittest.cc
(right):

https://codereview.chromium.org/98623005/diff/90001/third_party/libaddressinp...
third_party/libaddressinput/chromium/chrome_storage_impl_unittest.cc:19: :
store_(new ValueMapPrefStore()),
I think this leaks.

[please use gerrit instead, 2013-12-20 18:53:48]

libaddressinput looks good % 1 comment.

https://codereview.chromium.org/98623005/diff/90001/third_party/libaddressinp...
File third_party/libaddressinput/chromium/cpp/libaddressinput.gyp (right):

https://codereview.chromium.org/98623005/diff/90001/third_party/libaddressinp...
third_party/libaddressinput/chromium/cpp/libaddressinput.gyp:77:
'test/storage_test_runner.h',
This gyp file does not specify the header files, only the source files.
(libaddressinput style)

[Evan Stade, 2013-12-20 22:34:12]

https://codereview.chromium.org/98623005/diff/90001/base/prefs/writeable_pref...
File base/prefs/writeable_pref_store.h (right):

https://codereview.chromium.org/98623005/diff/90001/base/prefs/writeable_pref...
base/prefs/writeable_pref_store.h:11: #include "base/memory/ref_counted.h"
On 2013/12/20 12:12:13, battre wrote:
> this is not needed, right?

correct. I fixed it but didn't want to upload an entire patchset and restart
tryjobs for it.

https://codereview.chromium.org/98623005/diff/90001/third_party/libaddressinp...
File third_party/libaddressinput/chromium/chrome_storage_impl.cc (right):

https://codereview.chromium.org/98623005/diff/90001/third_party/libaddressinp...
third_party/libaddressinput/chromium/chrome_storage_impl.cc:10:
ChromeStorageImpl::ChromeStorageImpl(WriteablePrefStore* store)
On 2013/12/20 12:12:13, battre wrote:
> How are you planning to instantiate this class? I am concerned that it gets
> linked directly to one of the PrefStores instead of the PrefService which
would
> be a layering violation.
> 
> The PrefService ensures for example that some preferences can be controlled by
> policies. If you hook this directly onto the JsonPrefStore, that would not be
> possible.

A new JsonPrefStore: https://codereview.chromium.org/105823007/

This code actually doesn't have anything to do with prefs, it just wants
on-disk/cached-in-memory json storage.

https://codereview.chromium.org/98623005/diff/90001/third_party/libaddressinp...
File third_party/libaddressinput/chromium/chrome_storage_impl_unittest.cc
(right):

https://codereview.chromium.org/98623005/diff/90001/third_party/libaddressinp...
third_party/libaddressinput/chromium/chrome_storage_impl_unittest.cc:19: :
store_(new ValueMapPrefStore()),
On 2013/12/20 12:12:13, battre wrote:
> I think this leaks.

how so?

[battre, 2013-12-20 22:56:07]

OWNER's LGTM @ base/prefs

https://codereview.chromium.org/98623005/diff/90001/third_party/libaddressinp...
File third_party/libaddressinput/chromium/chrome_storage_impl_unittest.cc
(right):

https://codereview.chromium.org/98623005/diff/90001/third_party/libaddressinp...
third_party/libaddressinput/chromium/chrome_storage_impl_unittest.cc:19: :
store_(new ValueMapPrefStore()),
On 2013/12/20 22:34:12, Evan Stade wrote:
> On 2013/12/20 12:12:13, battre wrote:
> > I think this leaks.
> 
> how so?

never mind. I thought this went into storage_.

[Evan Stade, 2013-12-20 23:59:34]

https://codereview.chromium.org/98623005/diff/90001/third_party/libaddressinp...
File third_party/libaddressinput/chromium/cpp/libaddressinput.gyp (right):

https://codereview.chromium.org/98623005/diff/90001/third_party/libaddressinp...
third_party/libaddressinput/chromium/cpp/libaddressinput.gyp:77:
'test/storage_test_runner.h',
On 2013/12/20 18:53:48, Rouslan Solomakhin wrote:
> This gyp file does not specify the header files, only the source files.
> (libaddressinput style)

Done.

[please use gerrit instead, 2013-12-21 00:00:40]

libaddressinput lgtm

[Evan Stade, 2013-12-21 00:18:23]

TBR'ing base.gyp change.

[commit-bot: I haz the power, 2013-12-21 00:22:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/98623005/350001

[Nico, 2013-12-21 00:28:16]

base.gyp lgtm

[commit-bot: I haz the power, 2013-12-21 00:42:04]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-21 01:26:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/98623005/520001

[commit-bot: I haz the power, 2013-12-21 01:44:49]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-30 22:12:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/98623005/520001

[commit-bot: I haz the power, 2013-12-30 22:28:03]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-31 01:14:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/98623005/540002

[commit-bot: I haz the power, 2014-01-02 21:41:22]

Change committed as 242803