detect preferences errors

detect preferences errors

BUG=38352
TEST=none

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

[Aaron Boodman, 2010-03-20 07:23:28]

very cool

http://codereview.chromium.org/1120006/diff/1/6
File chrome/browser/pref_service.cc (right):

http://codereview.chromium.org/1120006/diff/1/6#newcode68
chrome/browser/pref_service.cc:68: //
Missing comment.

http://codereview.chromium.org/1120006/diff/1/7
File chrome/browser/pref_service.h (right):

http://codereview.chromium.org/1120006/diff/1/7#newcode187
chrome/browser/pref_service.h:187: bool do_not_save() const { return
do_not_save_; }
Nit: What about something a little bit more transparent like read_only_mode()?

[brettw, 2010-03-20 20:17:48]

Seems pretty straightforward to me.

http://codereview.chromium.org/1120006/diff/1/6
File chrome/browser/pref_service.cc (right):

http://codereview.chromium.org/1120006/diff/1/6#newcode93
chrome/browser/pref_service.cc:93: PREF_READ_ERROR_JSON_PARSE,
Since we depend on these values elsewhere, I'd feel better if the first one said
"=0" so the actual values are more explicit.

http://codereview.chromium.org/1120006/diff/1/7
File chrome/browser/pref_service.h (right):

http://codereview.chromium.org/1120006/diff/1/7#newcode187
chrome/browser/pref_service.h:187: bool do_not_save() const { return
do_not_save_; }
I also prefer read_only_mode.

http://codereview.chromium.org/1120006/diff/1/7#newcode221
chrome/browser/pref_service.h:221: // Record if we have an error while reading
the prefs from disk
Don't forget period.

[Erik does not do reviews, 2010-03-28 21:45:32]

New snap uploaded with your comments addressed.

Given your feedback, I'm assuming that you're cool with me submitting this
without further addressing recovery issues?

[brettw, 2010-03-29 16:09:19]

I have to leave now to come to work, so I couldn't actually research the error
string question now.

http://codereview.chromium.org/1120006/diff/6001/7005
File chrome/browser/pref_service.cc (right):

http://codereview.chromium.org/1120006/diff/6001/7005#newcode83
chrome/browser/pref_service.cc:83: // Failing to load prefs on startup is a bad
thing(TM).  Do some diagnosis.
Can you make a helper function for doing this diagnosis rather than adding all
the code to the constructor? I ask because it's a lot of code that isn't
otherwise involved in constructing the object.

http://codereview.chromium.org/1120006/diff/6001/7005#newcode170
chrome/browser/pref_service.cc:170: scoped_ptr<Value>
root(serializer.Deserialize(&read_error_));
It kind of sucks that we have to store this string, and then do string
comparisons above. Do we actually use these error messages for anything useful?
Can we replace them with some JSON enum for the error?

[Erik does not do reviews, 2010-03-29 17:30:59]

I refactored the error handling a bit so that we don't store the error string
and so that the logic isn't in the constructor.

I agree that comparing against error strings sucks, but I was nervous about
changing this API as I wasn't sure exactly what might be depending on it. 
Adding Tony since he probably knows more than either of us.  (Tony - the issue
is about testing particular error codes from the JSON parser)

[brettw, 2010-03-29 17:37:45]

From a grep, it looks like Deserialize has only a few callers, and
most of them are in unit tests. The remaining ones look like the error
message is primarily used for a LOG statement. It looks like
ExtensionUnpacker is the most complex case (it stores the error
message for later logging).

I think we should change the function and have some way to get a text
message easily if one is desired.

Brett

On Mon, Mar 29, 2010 at 10:30 AM,  <erikkay@chromium.org> wrote:
> I refactored the error handling a bit so that we don't store the error
> string
> and so that the logic isn't in the constructor.
>
> I agree that comparing against error strings sucks, but I was nervous about
> changing this API as I wasn't sure exactly what might be depending on it.
> Adding Tony since he probably knows more than either of us.  (Tony - the
> issue
> is about testing particular error codes from the JSON parser)
>
> http://codereview.chromium.org/1120006
>

[Erik does not do reviews, 2010-04-02 23:39:11]

PTAL

OK.  I finally got back to this.  The thing that added complexity is that the
error message has state in it (line and column number).  As a result, I couldn't
just switch the string to an int and add a lookup.  Instead, I added a second
arg so that people could get one or both of the code and the message.

[tony, 2010-04-05 03:31:36]

This overall approach looks good to me.  I just have some small style nits. 
FWIW, Aaron was the one who originally added error messages to the json reader,
so he probably knows the code better than I do.

http://codereview.chromium.org/1120006/diff/16001/17017
File chrome/browser/pref_service.cc (right):

http://codereview.chromium.org/1120006/diff/16001/17017#newcode184
chrome/browser/pref_service.cc:184: bool PrefService::LoadPersistentPrefs(int*
error) {
Maybe this method should just return the error code?  Also, can we use the enum
type rather than int?

http://codereview.chromium.org/1120006/diff/16001/17017#newcode214
chrome/browser/pref_service.cc:214: return true;
Nit: What are the consequences of returning false here?

http://codereview.chromium.org/1120006/diff/16001/17017#newcode223
chrome/browser/pref_service.cc:223: // Lie about our ability to save.
Nit: We're not really lying here since there's no return value.

[brettw, 2010-04-05 16:23:14]

LGTM, just some minor comments.

http://codereview.chromium.org/1120006/diff/16001/17001
File base/json/json_reader.cc (right):

http://codereview.chromium.org/1120006/diff/16001/17001#newcode152
base/json/json_reader.cc:152: return "";
Use std::string() instead of "" since this returns a std::string.

http://codereview.chromium.org/1120006/diff/16001/17002
File base/json/json_reader.h (right):

http://codereview.chromium.org/1120006/diff/16001/17002#newcode135
base/json/json_reader.h:135: std::string error_message();
Since this isn't an inline getter, it should be camel case. Probably like
GetErrorMessage or FormatErrorMessage

http://codereview.chromium.org/1120006/diff/16001/17025
File chrome/common/json_value_serializer.cc (right):

http://codereview.chromium.org/1120006/diff/16001/17025#newcode57
chrome/common/json_value_serializer.cc:57: bool
JSONFileValueSerializer::ReadFileToString(std::string* json_string,
Why not just return the error code, since you already have a code for no error.
This would delete a couple of lines here and looks like it wouldn't make the
caller any more complicated.

http://codereview.chromium.org/1120006/diff/16001/17026
File chrome/common/json_value_serializer.h (right):

http://codereview.chromium.org/1120006/diff/16001/17026#newcode115
chrome/common/json_value_serializer.h:115: static const char* error_message(int
error_code);
This should be called GetErrorMessageForCode or something.

[Erik does not do reviews, 2010-04-05 22:35:00]

OK.  I incorporated all of your comments except for one of Tony's (see below).

http://codereview.chromium.org/1120006/diff/16001/17017
File chrome/browser/pref_service.cc (right):

http://codereview.chromium.org/1120006/diff/16001/17017#newcode184
chrome/browser/pref_service.cc:184: bool PrefService::LoadPersistentPrefs(int*
error) {
On 2010/04/05 03:31:36, tony wrote:
> Maybe this method should just return the error code?  Also, can we use the
enum
> type rather than int?

good point.  fixed.

http://codereview.chromium.org/1120006/diff/16001/17017#newcode214
chrome/browser/pref_service.cc:214: return true;
On 2010/04/05 03:31:36, tony wrote:
> Nit: What are the consequences of returning false here?

Good question.  I had assumed that there were going to be a bunch of places that
had an if check and then went into some catastrophic failure mode.  As it turns
out, there's only one place that checks the return code, and that's a unit test,
so I could do anything here.  

However, I think that the correct thing is still to return true.  The
implication here is that this isn't an error condition and we want callers to
continue as if nothing happened.  I don't want callers to have to worry if this
is read only or not.

http://codereview.chromium.org/1120006/diff/16001/17025
File chrome/common/json_value_serializer.cc (right):

http://codereview.chromium.org/1120006/diff/16001/17025#newcode57
chrome/common/json_value_serializer.cc:57: bool
JSONFileValueSerializer::ReadFileToString(std::string* json_string,
On 2010/04/05 16:23:14, brettw wrote:
> Why not just return the error code, since you already have a code for no
error.
> This would delete a couple of lines here and looks like it wouldn't make the
> caller any more complicated.

good point

[tony, 2010-04-05 23:52:53]

LGTM2.  Thanks for doing this.