Adds async interface method to PersistentPrefStore.

Adds async interface method, which is used by JsonPrefStore. Also see the bug description for more info.

BUG=chromium-os:14289
TEST=JsonPrefStoreTest.*

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

[altimofeev, 2011-04-25 15:06:46]

Ping.

[Bernhard Bauer, 2011-04-25 15:54:10]

(Sorry for the hold-up. Friday and Monday are holidays over here in catholic
Bavaria, so Mattias may not actually look at this until tomorrow.)

http://codereview.chromium.org/6894020/diff/1011/chrome/browser/prefs/pref_se...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/6894020/diff/1011/chrome/browser/prefs/pref_se...
chrome/browser/prefs/pref_service.cc:204: user_pref_store_->GetErrors(&error,
&is_fatal);
Could you pass these values directly to OnInitializationCompleted? Then you
wouldn't need to store them in the user pref store, making reading the
preferences stateless.

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_stor...
File chrome/common/json_pref_store.cc (right):

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_stor...
chrome/common/json_pref_store.cc:265: (*error) = error_;
Nit: the parentheses are not actually necessary, but I don't mind them very much
either.

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_store.h
File chrome/common/json_pref_store.h (right):

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_stor...
chrome/common/json_pref_store.h:30: class Delegate {
Is this interface still used somewhere?

[Mattias Nissler (ping if slow), 2011-04-26 09:04:26]

A first round of mostly high-level comments for your consideration.

http://codereview.chromium.org/6894020/diff/1011/chrome/browser/prefs/pref_se...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/6894020/diff/1011/chrome/browser/prefs/pref_se...
chrome/browser/prefs/pref_service.cc:148: PrefServiceDelegate* delegate)
Why do we still pass this PrefServiceDelegate to the PrefService? There already
is PrefNotifierImpl, which sends out a PREF_INITIALIZATION_COMPLETED
notification. Is it possible to just use the notification and remove
PrefServiceDelegate?

http://codereview.chromium.org/6894020/diff/1011/chrome/browser/prefs/pref_se...
chrome/browser/prefs/pref_service.cc:197: void
PrefService::OnInitializationCompleted() {
If we use the PREF_INITIALIZATION_COMPLETED notification instead, this can go
away and the error handling here can really be moved either down to
JsonPrefStore, or over to PrefNotifier, which is the designated home for all
signaling to the outer world.

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_stor...
File chrome/common/json_pref_store.cc (right):

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_stor...
chrome/common/json_pref_store.cc:235: void JsonPrefStore::ReadPrefsAsync() {
I was wondering before if we could just merge ReadPrefs() and ReadPrefsAsync()
into one. After all, doing I/O on the main thread is a bad thing (TM), and
always trying to do our I/O on the FILE thread and only falling back if that's
impossible seems like a sane strategy to me.

Would you be willing to give that a try? We'd just execute the asynchronous path
till the PostTask() call in FileThreadDeserializer::Start(). If that  fails
(indicating the associated threads are not yet spinning), we could just read
synchronously.

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_stor...
File chrome/common/json_pref_store_unittest.cc (right):

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_stor...
chrome/common/json_pref_store_unittest.cc:20: #include
"content/browser/browser_thread.h"
Are we allowed to include content/browser from chrome/common? It looks like the
original browser_thread.h include you added to json_pref_store.cc is the first
instance of this. Note that the JsonPrefStore constructor takes the file message
loop proxy for exactly the reason to avoid BrowserThread, but it was introduced
a long time ago when there was no content/ yet...

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_stor...
chrome/common/json_pref_store_unittest.cc:22: #include
"testing/gtest/include/gtest/gtest.h"
newline

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_stor...
chrome/common/json_pref_store_unittest.cc:26: public:
indentation

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_stor...
chrome/common/json_pref_store_unittest.cc:175: // Tests asynchronous reading of
the file case when there is no file.
remove "case"

[altimofeev, 2011-04-27 10:32:08]

Tried to satisfy all the comments. Please take a look.

http://codereview.chromium.org/6894020/diff/1011/chrome/browser/prefs/pref_se...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/6894020/diff/1011/chrome/browser/prefs/pref_se...
chrome/browser/prefs/pref_service.cc:148: PrefServiceDelegate* delegate)
On 2011/04/26 09:04:26, Mattias Nissler wrote:
> Why do we still pass this PrefServiceDelegate to the PrefService? There
already
> is PrefNotifierImpl, which sends out a PREF_INITIALIZATION_COMPLETED
> notification. Is it possible to just use the notification and remove
> PrefServiceDelegate?

Done.

http://codereview.chromium.org/6894020/diff/1011/chrome/browser/prefs/pref_se...
chrome/browser/prefs/pref_service.cc:197: void
PrefService::OnInitializationCompleted() {
On 2011/04/26 09:04:26, Mattias Nissler wrote:
> If we use the PREF_INITIALIZATION_COMPLETED notification instead, this can go
> away and the error handling here can really be moved either down to
> JsonPrefStore, or over to PrefNotifier, which is the designated home for all
> signaling to the outer world.

I have leaved "PrefStore::Observer" here, because "PrefStore::Observer" and
"PrefNotifier" both have OnInitializationCompleted() method, so
"PrefNotiferImpl" can't simply inherit from both of this interfaces.

http://codereview.chromium.org/6894020/diff/1011/chrome/browser/prefs/pref_se...
chrome/browser/prefs/pref_service.cc:204: user_pref_store_->GetErrors(&error,
&is_fatal);
On 2011/04/25 15:54:10, Bernhard Bauer wrote:
> Could you pass these values directly to OnInitializationCompleted? Then you
> wouldn't need to store them in the user pref store, making reading the
> preferences stateless.

The problem is that "OnInitializationCompleted" is part of the generic
"PrefStore::Observer" interface, which has other users as well.

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_stor...
File chrome/common/json_pref_store.cc (right):

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_stor...
chrome/common/json_pref_store.cc:235: void JsonPrefStore::ReadPrefsAsync() {
On 2011/04/26 09:04:26, Mattias Nissler wrote:
> I was wondering before if we could just merge ReadPrefs() and ReadPrefsAsync()
> into one. After all, doing I/O on the main thread is a bad thing (TM), and
> always trying to do our I/O on the FILE thread and only falling back if that's
> impossible seems like a sane strategy to me.
> 
> Would you be willing to give that a try? We'd just execute the asynchronous
path
> till the PostTask() call in FileThreadDeserializer::Start(). If that  fails
> (indicating the associated threads are not yet spinning), we could just read
> synchronously.

I remember, and actually thought about it. And the main thing which I don't like
about this idea is that the caller of the ReadPrefs() will not know what version
(sync or async) will be used. At the same time, there are situations, when the
caller want this function to be synchronous (e.g. unittest, browser init). So it
is either overkill to change these parts to support asynchronous reading or
unclear code, which assumes that ReadPrefs() will read synchronously.

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_stor...
chrome/common/json_pref_store.cc:265: (*error) = error_;
On 2011/04/25 15:54:10, Bernhard Bauer wrote:
> Nit: the parentheses are not actually necessary, but I don't mind them very
much
> either.

This is the matter of taste, I think. But grep says that your variant is more
common in Chromium code (1497 vs 9). So will not use unnecessary parentheses.

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_store.h
File chrome/common/json_pref_store.h (right):

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_stor...
chrome/common/json_pref_store.h:30: class Delegate {
On 2011/04/25 15:54:10, Bernhard Bauer wrote:
> Is this interface still used somewhere?

Oops. Deleted.

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_stor...
File chrome/common/json_pref_store_unittest.cc (right):

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_stor...
chrome/common/json_pref_store_unittest.cc:20: #include
"content/browser/browser_thread.h"
On 2011/04/26 09:04:26, Mattias Nissler wrote:
> Are we allowed to include content/browser from chrome/common? It looks like
the
> original browser_thread.h include you added to json_pref_store.cc is the first
> instance of this. Note that the JsonPrefStore constructor takes the file
message
> loop proxy for exactly the reason to avoid BrowserThread, but it was
introduced
> a long time ago when there was no content/ yet...

Good point. If I understand correctly, you suggesting me to use
|file_message_loop_proxy|, which is passed to constructor of "JsonPrefStore"
class. It will make unnecessary using of browser threads in the unittest.

Done.

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_stor...
chrome/common/json_pref_store_unittest.cc:22: #include
"testing/gtest/include/gtest/gtest.h"
On 2011/04/26 09:04:26, Mattias Nissler wrote:
> newline

Done.

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_stor...
chrome/common/json_pref_store_unittest.cc:26: public:
On 2011/04/26 09:04:26, Mattias Nissler wrote:
> indentation

Done.

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_stor...
chrome/common/json_pref_store_unittest.cc:175: // Tests asynchronous reading of
the file case when there is no file.
On 2011/04/26 09:04:26, Mattias Nissler wrote:
> remove "case"

Done.

[Bernhard Bauer, 2011-04-27 10:52:16]

http://codereview.chromium.org/6894020/diff/1011/chrome/browser/prefs/pref_se...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/6894020/diff/1011/chrome/browser/prefs/pref_se...
chrome/browser/prefs/pref_service.cc:204: user_pref_store_->GetErrors(&error,
&is_fatal);
On 2011/04/27 10:32:08, altimofeev wrote:
> On 2011/04/25 15:54:10, Bernhard Bauer wrote:
> > Could you pass these values directly to OnInitializationCompleted? Then you
> > wouldn't need to store them in the user pref store, making reading the
> > preferences stateless.
> 
> The problem is that "OnInitializationCompleted" is part of the generic
> "PrefStore::Observer" interface, which has other users as well.

I think passing (PREF_READ_ERROR_NONE, false) at the other call sites should be
fine. Mattias, WDYT?

Another idea would be to add an OnInitializationError method which gets these
values. The contract would then be that eventually one of these two methods is
called.

[altimofeev, 2011-04-27 11:26:58]

On 2011/04/27 10:52:16, Bernhard Bauer wrote:
>
http://codereview.chromium.org/6894020/diff/1011/chrome/browser/prefs/pref_se...
> File chrome/browser/prefs/pref_service.cc (right):
> 
>
http://codereview.chromium.org/6894020/diff/1011/chrome/browser/prefs/pref_se...
> chrome/browser/prefs/pref_service.cc:204: user_pref_store_->GetErrors(&error,
> &is_fatal);
> On 2011/04/27 10:32:08, altimofeev wrote:
> > On 2011/04/25 15:54:10, Bernhard Bauer wrote:
> > > Could you pass these values directly to OnInitializationCompleted? Then
you
> > > wouldn't need to store them in the user pref store, making reading the
> > > preferences stateless.
> > 
> > The problem is that "OnInitializationCompleted" is part of the generic
> > "PrefStore::Observer" interface, which has other users as well.
> 
> I think passing (PREF_READ_ERROR_NONE, false) at the other call sites should
be
> fine. Mattias, WDYT?
> 
> Another idea would be to add an OnInitializationError method which gets these
> values. The contract would then be that eventually one of these two methods is
> called.

Both of this need PrefStore::Observer interface to be change, which I think is
not better than current approach, which only deals with PersistentPrefStore
[subtype of PrefStore]. Anyway, what exactly do you think is bad in having
GetErrors() method?

[Mattias Nissler (ping if slow), 2011-04-27 12:16:30]

Some more comments. Let me know if something is unclear and thanks for being
patient with us.

http://codereview.chromium.org/6894020/diff/1011/chrome/browser/prefs/pref_se...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/6894020/diff/1011/chrome/browser/prefs/pref_se...
chrome/browser/prefs/pref_service.cc:197: void
PrefService::OnInitializationCompleted() {
On 2011/04/27 10:32:08, altimofeev wrote:
> On 2011/04/26 09:04:26, Mattias Nissler wrote:
> > If we use the PREF_INITIALIZATION_COMPLETED notification instead, this can
go
> > away and the error handling here can really be moved either down to
> > JsonPrefStore, or over to PrefNotifier, which is the designated home for all
> > signaling to the outer world.
> 
> I have leaved "PrefStore::Observer" here, because "PrefStore::Observer" and
> "PrefNotifier" both have OnInitializationCompleted() method, so
> "PrefNotiferImpl" can't simply inherit from both of this interfaces.

Yes, PrefNotifier already has OnInitializationCompleted(). Signaling the outer
world is a PrefNotifier responsibility, which is why I'd like to see the
signaling code moved there. You can change the interface between PrefValueStore
and PrefNotifier to what you need, it's internal to the pref system.

Here is more details on how I imagine this to work:

1. PrefService::CreatePrefService() gets called and PrefStores get constructed
2. ReadPrefs or ReadPrefsAsync() gets called
3. All the pref stores (including JsonPrefStore) notify PrefValueStore about
their initialization status by calling OnInitializationCompleted().
4. Once PrefValueStore has heard back from all of them (or decided there is an
error), it delegates to the PrefNotifier to tell the outside world.
5. PrefNotifierImpl sends the notification.

http://codereview.chromium.org/6894020/diff/1011/chrome/browser/prefs/pref_se...
chrome/browser/prefs/pref_service.cc:204: user_pref_store_->GetErrors(&error,
&is_fatal);
On 2011/04/27 10:52:16, Bernhard Bauer wrote:
> On 2011/04/27 10:32:08, altimofeev wrote:
> > On 2011/04/25 15:54:10, Bernhard Bauer wrote:
> > > Could you pass these values directly to OnInitializationCompleted? Then
you
> > > wouldn't need to store them in the user pref store, making reading the
> > > preferences stateless.
> > 
> > The problem is that "OnInitializationCompleted" is part of the generic
> > "PrefStore::Observer" interface, which has other users as well.
> 
> I think passing (PREF_READ_ERROR_NONE, false) at the other call sites should
be
> fine. Mattias, WDYT?
> 
> Another idea would be to add an OnInitializationError method which gets these
> values. The contract would then be that eventually one of these two methods is
> called.

I think passing the parameters makes sense. At the moment, your only use for the
GetErrors() function is so you can call it when you get notified. As long as
there's no reason to call GetErrors() in other context, it doesn't seem useful
to have it separately from the callback.

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_stor...
File chrome/common/json_pref_store.cc (right):

http://codereview.chromium.org/6894020/diff/1011/chrome/common/json_pref_stor...
chrome/common/json_pref_store.cc:235: void JsonPrefStore::ReadPrefsAsync() {
On 2011/04/27 10:32:08, altimofeev wrote:
> On 2011/04/26 09:04:26, Mattias Nissler wrote:
> > I was wondering before if we could just merge ReadPrefs() and
ReadPrefsAsync()
> > into one. After all, doing I/O on the main thread is a bad thing (TM), and
> > always trying to do our I/O on the FILE thread and only falling back if
that's
> > impossible seems like a sane strategy to me.
> > 
> > Would you be willing to give that a try? We'd just execute the asynchronous
> path
> > till the PostTask() call in FileThreadDeserializer::Start(). If that  fails
> > (indicating the associated threads are not yet spinning), we could just read
> > synchronously.
> 
> I remember, and actually thought about it. And the main thing which I don't
like
> about this idea is that the caller of the ReadPrefs() will not know what
version
> (sync or async) will be used. At the same time, there are situations, when the
> caller want this function to be synchronous (e.g. unittest, browser init). So
it
> is either overkill to change these parts to support asynchronous reading or
> unclear code, which assumes that ReadPrefs() will read synchronously.

If we do it, we should make sure we don't put additional requirements on client
code. You are right in that we'd probably need some unit test changes (probably
calling MessageLoop::RunAllPending() at some places). I think your reasoning
about unclear expectations is valid, so let's go with the async flag then.

http://codereview.chromium.org/6894020/diff/7001/chrome/browser/prefs/pref_no...
File chrome/browser/prefs/pref_notifier_impl.cc (right):

http://codereview.chromium.org/6894020/diff/7001/chrome/browser/prefs/pref_no...
chrome/browser/prefs/pref_notifier_impl.cc:82: void
PrefNotifierImpl::OnPersistentPrefsRead(bool success) {
We should really get this trigger through OnInitializationCompleted(). If you
need a status code, I suggest adding an enum to PrefNotifier that you can pass
as a  parameter.

Once you have done so, you also don't need the ready_ flag, since PrefValueStore
can make sure to call OnInitializationCompleted only once.

http://codereview.chromium.org/6894020/diff/7001/chrome/browser/prefs/pref_no...
chrome/browser/prefs/pref_notifier_impl.cc:102: void
PrefNotifierImpl::NotifyInitializationFailed() {
We shouldn't have a second notification, but rather a Details parameter for the
PREF_INITIALIZATION_COMPLETED notification that indicates the status.

http://codereview.chromium.org/6894020/diff/7001/chrome/common/json_pref_stor...
File chrome/common/json_pref_store.cc (right):

http://codereview.chromium.org/6894020/diff/7001/chrome/common/json_pref_stor...
chrome/common/json_pref_store.cc:86: scoped_refptr<base::MessageLoopProxy>
ui_loop_proxy_;
ui_loop_proxy is a misnomer IMHO, the service process uses it, but doesn't have
UI AFAIK. Evidence gather by grep indicates that it seems to be common to name
this |origin_loop_|.

http://codereview.chromium.org/6894020/diff/7001/chrome/common/persistent_pre...
File chrome/common/persistent_pref_store.h (right):

http://codereview.chromium.org/6894020/diff/7001/chrome/common/persistent_pre...
chrome/common/persistent_pref_store.h:66: // "OnInitializationCompleted" when
done.
Maybe write PrefStore::Observer::OnInitializationCompleted to make it a bit
clearer? Also below.

http://codereview.chromium.org/6894020/diff/7001/content/common/notification_...
File content/common/notification_type.h (right):

http://codereview.chromium.org/6894020/diff/7001/content/common/notification_...
content/common/notification_type.h:751: PREF_INITIALIZATION_FAILED,
As mention elsewhere, I think it's much better to pass a Details parameter
instead.

[altimofeev, 2011-05-04 13:46:14]

Sorry, that reply takes a long time. 
In a nutshell, I have implemented scheme suggested by Mattias:
- JsonPrefStore sends OnInitializationCompleted via PrefStoreKeeper to
PrefValueStore, which, as usual, reports to PrefNotifierImpl.
- OnInitializationCompleted takes one argument - whether initialization
succeeeded.
- Also, JsonPrefService calls newly added ReadErrorDelegate about read errors
(if any). ReadErrorHandler, which implements ReadErrorDelegate, handles errors
and shows corresponding notifications.

Please take a look.

http://codereview.chromium.org/6894020/diff/7001/chrome/browser/prefs/pref_no...
File chrome/browser/prefs/pref_notifier_impl.cc (right):

http://codereview.chromium.org/6894020/diff/7001/chrome/browser/prefs/pref_no...
chrome/browser/prefs/pref_notifier_impl.cc:82: void
PrefNotifierImpl::OnPersistentPrefsRead(bool success) {
On 2011/04/27 12:16:30, Mattias Nissler wrote:
> We should really get this trigger through OnInitializationCompleted(). If you
> need a status code, I suggest adding an enum to PrefNotifier that you can pass
> as a  parameter.
> 
> Once you have done so, you also don't need the ready_ flag, since
PrefValueStore
> can make sure to call OnInitializationCompleted only once.

Done.

http://codereview.chromium.org/6894020/diff/7001/chrome/browser/prefs/pref_no...
chrome/browser/prefs/pref_notifier_impl.cc:102: void
PrefNotifierImpl::NotifyInitializationFailed() {
On 2011/04/27 12:16:30, Mattias Nissler wrote:
> We shouldn't have a second notification, but rather a Details parameter for
the
> PREF_INITIALIZATION_COMPLETED notification that indicates the status.

Done.

http://codereview.chromium.org/6894020/diff/7001/chrome/common/json_pref_stor...
File chrome/common/json_pref_store.cc (right):

http://codereview.chromium.org/6894020/diff/7001/chrome/common/json_pref_stor...
chrome/common/json_pref_store.cc:86: scoped_refptr<base::MessageLoopProxy>
ui_loop_proxy_;
On 2011/04/27 12:16:30, Mattias Nissler wrote:
> ui_loop_proxy is a misnomer IMHO, the service process uses it, but doesn't
have
> UI AFAIK. Evidence gather by grep indicates that it seems to be common to name
> this |origin_loop_|.

Done.

http://codereview.chromium.org/6894020/diff/7001/chrome/common/persistent_pre...
File chrome/common/persistent_pref_store.h (right):

http://codereview.chromium.org/6894020/diff/7001/chrome/common/persistent_pre...
chrome/common/persistent_pref_store.h:66: // "OnInitializationCompleted" when
done.
On 2011/04/27 12:16:30, Mattias Nissler wrote:
> Maybe write PrefStore::Observer::OnInitializationCompleted to make it a bit
> clearer? Also below.

Done.

http://codereview.chromium.org/6894020/diff/7001/content/common/notification_...
File content/common/notification_type.h (right):

http://codereview.chromium.org/6894020/diff/7001/content/common/notification_...
content/common/notification_type.h:751: PREF_INITIALIZATION_FAILED,
On 2011/04/27 12:16:30, Mattias Nissler wrote:
> As mention elsewhere, I think it's much better to pass a Details parameter
> instead.

Done.

[Mattias Nissler (ping if slow), 2011-05-04 23:33:08]

Thanks for addressing all the previous comments, I think we're pretty close now.

http://codereview.chromium.org/6894020/diff/14032/chrome/browser/prefs/pref_n...
File chrome/browser/prefs/pref_notifier_impl.cc (right):

http://codereview.chromium.org/6894020/diff/14032/chrome/browser/prefs/pref_n...
chrome/browser/prefs/pref_notifier_impl.cc:83: Details<PrefService>(succeeded ?
pref_service_ : NULL));
That's a weird contract. Why not have an enum and use
Details<PrefService::InitializationStatus> here?

http://codereview.chromium.org/6894020/diff/14032/chrome/browser/prefs/pref_v...
File chrome/browser/prefs/pref_value_store.cc (right):

http://codereview.chromium.org/6894020/diff/14032/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store.cc:243: if (!succeeded) {
There's a problem here: If you have a PrefStore reporting initialization failed
and afterwards some other store reports successful initialization, you'll first
report the failure and then success. I think the fix is to introduce some flag
here, or make the PrefStoreKeeper keep a status flag.

http://codereview.chromium.org/6894020/diff/14032/chrome/common/json_pref_sto...
File chrome/common/json_pref_store_unittest.cc (right):

http://codereview.chromium.org/6894020/diff/14032/chrome/common/json_pref_sto...
chrome/common/json_pref_store_unittest.cc:180: // Tests asynchronous reading of
the file when there is no file.
Can you please also add a test case for the positive case, i.e. successful async
reading?

http://codereview.chromium.org/6894020/diff/14032/chrome/common/persistent_pr...
File chrome/common/persistent_pref_store.h (right):

http://codereview.chromium.org/6894020/diff/14032/chrome/common/persistent_pr...
chrome/common/persistent_pref_store.h:77: //
"PrefStore::OnInitializationCompleted" when done. Also it fires
So we still have two callback mechanisms? Seems like this is a hassle to
implementors. Can't we just make OnInitializationCompleted take a PrefReadError
parameter? And move the error handling code from PrefService to
PrefNotifierImpl?

[altimofeev, 2011-05-05 12:59:28]

Thanks for your comments.

http://codereview.chromium.org/6894020/diff/14032/chrome/browser/prefs/pref_n...
File chrome/browser/prefs/pref_notifier_impl.cc (right):

http://codereview.chromium.org/6894020/diff/14032/chrome/browser/prefs/pref_n...
chrome/browser/prefs/pref_notifier_impl.cc:83: Details<PrefService>(succeeded ?
pref_service_ : NULL));
On 2011/05/04 23:33:08, Mattias Nissler wrote:
> That's a weird contract. Why not have an enum and use
> Details<PrefService::InitializationStatus> here?

I mistakenly thought that detail's pointer should outlive function end. Since it
is not the case, I think Details<bool> will be sufficient.

http://codereview.chromium.org/6894020/diff/14032/chrome/browser/prefs/pref_v...
File chrome/browser/prefs/pref_value_store.cc (right):

http://codereview.chromium.org/6894020/diff/14032/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store.cc:243: if (!succeeded) {
On 2011/05/04 23:33:08, Mattias Nissler wrote:
> There's a problem here: If you have a PrefStore reporting initialization
failed
> and afterwards some other store reports successful initialization, you'll
first
> report the failure and then success. I think the fix is to introduce some flag
> here, or make the PrefStoreKeeper keep a status flag.

Actually, to prevent it, IsInitializationComplete() returns false in case when
OnInitializationCompleted was called with "false" (initialization unsucceeded).

http://codereview.chromium.org/6894020/diff/14032/chrome/common/json_pref_sto...
File chrome/common/json_pref_store_unittest.cc (right):

http://codereview.chromium.org/6894020/diff/14032/chrome/common/json_pref_sto...
chrome/common/json_pref_store_unittest.cc:180: // Tests asynchronous reading of
the file when there is no file.
On 2011/05/04 23:33:08, Mattias Nissler wrote:
> Can you please also add a test case for the positive case, i.e. successful
async
> reading?

Done.

http://codereview.chromium.org/6894020/diff/14032/chrome/common/persistent_pr...
File chrome/common/persistent_pref_store.h (right):

http://codereview.chromium.org/6894020/diff/14032/chrome/common/persistent_pr...
chrome/common/persistent_pref_store.h:77: //
"PrefStore::OnInitializationCompleted" when done. Also it fires
On 2011/05/04 23:33:08, Mattias Nissler wrote:
> So we still have two callback mechanisms? Seems like this is a hassle to
> implementors. Can't we just make OnInitializationCompleted take a
PrefReadError
> parameter? And move the error handling code from PrefService to
> PrefNotifierImpl?

Yes, there are two of them. But the only reason why ReadErrorDelegate was
created was that JsonPrefService can't handle error (show notification etc). So
owner of JsonPrefService provides this delegate to extend JsonPrefService if
this extensions is needed. At the same time, OnInitializationCompleted() is
general interface for all PrefStores, not only the ones which read initial data
from the file. It brings us to my first objection - OnInitializationCompleted()
is part of the PrefStore, not PersistentPrefStore. My second objection is that
moving error handling to PrefNotifierImpl implies passing error through the
chain - JsonPrefStore => PrefStoreKeeper => PrefValueStore =>
PrefNotificationImpl.

Note, in the future, if this interface will be broadly used, one can make
ReadErrorHandler public to be used by other PersistentPrefs clients.

[Mattias Nissler (ping if slow), 2011-05-07 00:24:14]

First of all, sorry for the delay. I've spent the last couple of days in MTV in
meetings...

What you have now looks pretty good, there are just a couple nits left at this
point. Thanks again for your patience.

http://codereview.chromium.org/6894020/diff/14032/chrome/browser/prefs/pref_v...
File chrome/browser/prefs/pref_value_store.cc (right):

http://codereview.chromium.org/6894020/diff/14032/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store.cc:243: if (!succeeded) {
On 2011/05/05 12:59:28, altimofeev wrote:
> On 2011/05/04 23:33:08, Mattias Nissler wrote:
> > There's a problem here: If you have a PrefStore reporting initialization
> failed
> > and afterwards some other store reports successful initialization, you'll
> first
> > report the failure and then success. I think the fix is to introduce some
flag
> > here, or make the PrefStoreKeeper keep a status flag.
> 
> Actually, to prevent it, IsInitializationComplete() returns false in case when
> OnInitializationCompleted was called with "false" (initialization
unsucceeded).

But then IsInitializationComplete() is a misnomer, since it did complete, just
not successfully. Note that PrefService can still work perfectly fine, e.g. if
the file was corrupt, so I think it makes sense to be able to distinguish
between initialization done and failures. Long story short: I think the correct
behavior would be to have IsInitializationComplete() return true when init is
done, even if there is an error.

http://codereview.chromium.org/6894020/diff/14032/chrome/common/persistent_pr...
File chrome/common/persistent_pref_store.h (right):

http://codereview.chromium.org/6894020/diff/14032/chrome/common/persistent_pr...
chrome/common/persistent_pref_store.h:77: //
"PrefStore::OnInitializationCompleted" when done. Also it fires
On 2011/05/05 12:59:28, altimofeev wrote:
> On 2011/05/04 23:33:08, Mattias Nissler wrote:
> > So we still have two callback mechanisms? Seems like this is a hassle to
> > implementors. Can't we just make OnInitializationCompleted take a
> PrefReadError
> > parameter? And move the error handling code from PrefService to
> > PrefNotifierImpl?
> 
> Yes, there are two of them. But the only reason why ReadErrorDelegate was
> created was that JsonPrefService can't handle error (show notification etc).
So
> owner of JsonPrefService provides this delegate to extend JsonPrefService if
> this extensions is needed. At the same time, OnInitializationCompleted() is
> general interface for all PrefStores, not only the ones which read initial
data
> from the file. It brings us to my first objection -
OnInitializationCompleted()
> is part of the PrefStore, not PersistentPrefStore. My second objection is that
> moving error handling to PrefNotifierImpl implies passing error through the
> chain - JsonPrefStore => PrefStoreKeeper => PrefValueStore =>
> PrefNotificationImpl.
> 
> Note, in the future, if this interface will be broadly used, one can make
> ReadErrorHandler public to be used by other PersistentPrefs clients.

You are very right, in this case it makes sense to have two callback mechanisms,
since we don't want to pollute the PrefStore interface with PersistentPrefStore
stuff. Sorry for not thinking it through when writing the first comment.

http://codereview.chromium.org/6894020/diff/15012/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/6894020/diff/15012/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.cc:213: user_pref_store_->ReadPrefs();
Need to call the error handler here!

http://codereview.chromium.org/6894020/diff/15012/chrome/common/json_pref_sto...
File chrome/common/json_pref_store_unittest.cc (right):

http://codereview.chromium.org/6894020/diff/15012/chrome/common/json_pref_sto...
chrome/common/json_pref_store_unittest.cc:89: // This function is used to avoid
code duplication while testing synchronios and
s/synchronios/synchronous/

[altimofeev, 2011-05-10 11:10:22]

As always, thank you for your comments.

http://codereview.chromium.org/6894020/diff/14032/chrome/browser/prefs/pref_v...
File chrome/browser/prefs/pref_value_store.cc (right):

http://codereview.chromium.org/6894020/diff/14032/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store.cc:243: if (!succeeded) {
On 2011/05/07 00:24:14, Mattias Nissler wrote:
> On 2011/05/05 12:59:28, altimofeev wrote:
> > On 2011/05/04 23:33:08, Mattias Nissler wrote:
> > > There's a problem here: If you have a PrefStore reporting initialization
> > failed
> > > and afterwards some other store reports successful initialization, you'll
> > first
> > > report the failure and then success. I think the fix is to introduce some
> flag
> > > here, or make the PrefStoreKeeper keep a status flag.
> > 
> > Actually, to prevent it, IsInitializationComplete() returns false in case
when
> > OnInitializationCompleted was called with "false" (initialization
> unsucceeded).
> 
> But then IsInitializationComplete() is a misnomer, since it did complete, just
> not successfully. Note that PrefService can still work perfectly fine, e.g. if
> the file was corrupt, so I think it makes sense to be able to distinguish
> between initialization done and failures. Long story short: I think the
correct
> behavior would be to have IsInitializationComplete() return true when init is
> done, even if there is an error.

Agree, misnomer is no good. So now PrevValueStore stores whether one of the
PrefStores has failed initialization, and if it is the case, only one
OnInitializaionCompleted(false) is called.

http://codereview.chromium.org/6894020/diff/15012/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/6894020/diff/15012/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.cc:213: user_pref_store_->ReadPrefs();
On 2011/05/07 00:24:14, Mattias Nissler wrote:
> Need to call the error handler here!

Good point, thanks! 
Done.

http://codereview.chromium.org/6894020/diff/15012/chrome/common/json_pref_sto...
File chrome/common/json_pref_store_unittest.cc (right):

http://codereview.chromium.org/6894020/diff/15012/chrome/common/json_pref_sto...
chrome/common/json_pref_store_unittest.cc:89: // This function is used to avoid
code duplication while testing synchronios and
On 2011/05/07 00:24:14, Mattias Nissler wrote:
> s/synchronios/synchronous/

Done.

[Mattias Nissler (ping if slow), 2011-05-10 11:27:50]

LGTM pending a comment to be resolved.

http://codereview.chromium.org/6894020/diff/33002/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/6894020/diff/33002/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.cc:230: new ReadErrorHandler()));
It seems like this ReadErrorHandler instance gets leaked. 
You cloud solve this e.g. by making OnError delete the ReadErrorHandler and
changing the if branch above to

(new ReadErrorHandler).OnError(user_pref_store_->ReadPrefs());

[altimofeev, 2011-05-10 12:01:18]

http://codereview.chromium.org/6894020/diff/33002/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/6894020/diff/33002/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.cc:230: new ReadErrorHandler()));
On 2011/05/10 11:27:50, Mattias Nissler wrote:
> It seems like this ReadErrorHandler instance gets leaked. 
> You cloud solve this e.g. by making OnError delete the ReadErrorHandler and
> changing the if branch above to
> 
> (new ReadErrorHandler).OnError(user_pref_store_->ReadPrefs());

Excuse me, but I didn't get you.
ReadPrefsAsync owns ReadErrorDelegate (delegate is saved as scoped_ptr<>). Also
note, it is mentioned in the corresponding comments.

[Mattias Nissler (ping if slow), 2011-05-10 12:16:41]

http://codereview.chromium.org/6894020/diff/33002/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/6894020/diff/33002/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.cc:230: new ReadErrorHandler()));
On 2011/05/10 12:01:18, altimofeev wrote:
> On 2011/05/10 11:27:50, Mattias Nissler wrote:
> > It seems like this ReadErrorHandler instance gets leaked. 
> > You cloud solve this e.g. by making OnError delete the ReadErrorHandler and
> > changing the if branch above to
> > 
> > (new ReadErrorHandler).OnError(user_pref_store_->ReadPrefs());
> 
> Excuse me, but I didn't get you.
> ReadPrefsAsync owns ReadErrorDelegate (delegate is saved as scoped_ptr<>).
Also
> note, it is mentioned in the corresponding comments.

I'm not talking about the ReadErrorDelegate, but the ReadErrorHandler instance
you create in line 230. I couldn't find where that object gets deleted. Maybe I
miss something?

[altimofeev, 2011-05-10 14:24:19]

http://codereview.chromium.org/6894020/diff/33002/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/6894020/diff/33002/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.cc:230: new ReadErrorHandler()));
On 2011/05/10 12:16:41, Mattias Nissler wrote:
> On 2011/05/10 12:01:18, altimofeev wrote:
> > On 2011/05/10 11:27:50, Mattias Nissler wrote:
> > > It seems like this ReadErrorHandler instance gets leaked. 
> > > You cloud solve this e.g. by making OnError delete the ReadErrorHandler
and
> > > changing the if branch above to
> > > 
> > > (new ReadErrorHandler).OnError(user_pref_store_->ReadPrefs());
> > 
> > Excuse me, but I didn't get you.
> > ReadPrefsAsync owns ReadErrorDelegate (delegate is saved as scoped_ptr<>).
> Also
> > note, it is mentioned in the corresponding comments.
> 
> I'm not talking about the ReadErrorDelegate, but the ReadErrorHandler instance
> you create in line 230. I couldn't find where that object gets deleted. Maybe
I
> miss something?

ReadErrorDelegate is an interface which is implemented by ReadErrorHandler. Also
this interface is provided to the ReadPrefsAsync to be called, if reading error
occurs.

So, as I have said, ReadErrorHandler is owned by |user_pref_store_| and will be
deleted by it in the end.

But you are right - I have forgotten to delete it in some PersistenPrefStore
realizations (non-JsonPrefStore ones) - fixed.

[Mattias Nissler (ping if slow), 2011-05-10 14:43:54]

LGTM!

http://codereview.chromium.org/6894020/diff/33002/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/6894020/diff/33002/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.cc:230: new ReadErrorHandler()));
On 2011/05/10 14:24:19, altimofeev wrote:
> On 2011/05/10 12:16:41, Mattias Nissler wrote:
> > On 2011/05/10 12:01:18, altimofeev wrote:
> > > On 2011/05/10 11:27:50, Mattias Nissler wrote:
> > > > It seems like this ReadErrorHandler instance gets leaked. 
> > > > You cloud solve this e.g. by making OnError delete the ReadErrorHandler
> and
> > > > changing the if branch above to
> > > > 
> > > > (new ReadErrorHandler).OnError(user_pref_store_->ReadPrefs());
> > > 
> > > Excuse me, but I didn't get you.
> > > ReadPrefsAsync owns ReadErrorDelegate (delegate is saved as scoped_ptr<>).
> > Also
> > > note, it is mentioned in the corresponding comments.
> > 
> > I'm not talking about the ReadErrorDelegate, but the ReadErrorHandler
instance
> > you create in line 230. I couldn't find where that object gets deleted.
Maybe
> I
> > miss something?
> 
> ReadErrorDelegate is an interface which is implemented by ReadErrorHandler.
Also
> this interface is provided to the ReadPrefsAsync to be called, if reading
error
> occurs.
> 
> So, as I have said, ReadErrorHandler is owned by |user_pref_store_| and will
be
> deleted by it in the end.

Ah, sorry, I didn't realize ReadPrefAsync takes ownership. What you do is
correct.

On a side-note: In this particular case, you could also use the standard
Callback infrastructure (see base/callback.h and base/callback_old.h,
respectively), instead of coding up an ad-hoc interface class. Not a review
comment, feel free to decide yourself :)

> 
> But you are right - I have forgotten to delete it in some PersistenPrefStore
> realizations (non-JsonPrefStore ones) - fixed.

Thanks!