Change the signature of JSONReader::Read() and related methods to be more

Change the signature of JSONReader::Read() and related 
methods to be more friendly to use with scoped_ptr. Change 
all the callsites.

[Aaron Boodman, 2008-12-26 07:08:01]

Sorry about the mind-numbing review.

[tony, 2008-12-29 10:02:03]

This is awesome; much cleaner.  Thanks for doing this refactoring.

LGTM, some comments below.  Please watch the purify bots to make sure nothing
regresses.

http://codereview.chromium.org/16270/diff/222/224
File base/json_reader.h (right):

http://codereview.chromium.org/16270/diff/222/224#newcode91
Line 91: // commas in objects and arrays even though this goes against the RFC.
Can you add a comment saying that the caller owns the returned Value and is
responsible for cleaning it up?

http://codereview.chromium.org/16270/diff/222/225
File base/json_reader_unittest.cc (right):

http://codereview.chromium.org/16270/diff/222/225#newcode8
Line 8: #include "base/scoped_ptr.h"
Nit: alphabetize

http://codereview.chromium.org/16270/diff/222/227
File chrome/browser/autocomplete/search_provider.cc (right):

http://codereview.chromium.org/16270/diff/222/227#newcode138
Line 138: have_suggest_results_ = true;
Nit: This could just be:
have_suggest_results_ = root_val.get() && ParseSuggestResults(root_val.get());

http://codereview.chromium.org/16270/diff/222/233
File chrome/browser/page_state.cc (right):

http://codereview.chromium.org/16270/diff/222/233#newcode48
Line 48: if (root->GetType() == Value::TYPE_DICTIONARY) {
Do we still need a null check here? Maybe we should add a return after the
NOTREACHED() above?

http://codereview.chromium.org/16270/diff/222/242
File chrome/test/automation/tab_proxy.cc (left):

http://codereview.chromium.org/16270/diff/222/242#oldcode560
Line 560: delete response;
I think we still want to delete the response (the IPC::Message).

[Aaron Boodman, 2008-12-29 19:57:28]

Thanks for the quick feedback. All fixed, submitting, will watch purify bot.

http://codereview.chromium.org/16270/diff/222/224
File base/json_reader.h (right):

http://codereview.chromium.org/16270/diff/222/224#newcode91
Line 91: // commas in objects and arrays even though this goes against the RFC.
On 2008/12/29 10:02:04, tony wrote:
> Can you add a comment saying that the caller owns the returned Value and is
> responsible for cleaning it up?

Done.

http://codereview.chromium.org/16270/diff/222/225
File base/json_reader_unittest.cc (right):

http://codereview.chromium.org/16270/diff/222/225#newcode8
Line 8: #include "base/scoped_ptr.h"
On 2008/12/29 10:02:04, tony wrote:
> Nit: alphabetize

Done.

http://codereview.chromium.org/16270/diff/222/227
File chrome/browser/autocomplete/search_provider.cc (right):

http://codereview.chromium.org/16270/diff/222/227#newcode138
Line 138: have_suggest_results_ = true;
On 2008/12/29 10:02:04, tony wrote:
> Nit: This could just be:
> have_suggest_results_ = root_val.get() && ParseSuggestResults(root_val.get());

Done.

http://codereview.chromium.org/16270/diff/222/233
File chrome/browser/page_state.cc (right):

http://codereview.chromium.org/16270/diff/222/233#newcode48
Line 48: if (root->GetType() == Value::TYPE_DICTIONARY) {
On 2008/12/29 10:02:04, tony wrote:
> Do we still need a null check here? Maybe we should add a return after the
> NOTREACHED() above?

Done.

http://codereview.chromium.org/16270/diff/222/242
File chrome/test/automation/tab_proxy.cc (left):

http://codereview.chromium.org/16270/diff/222/242#oldcode560
Line 560: delete response;
On 2008/12/29 10:02:04, tony wrote:
> I think we still want to delete the response (the IPC::Message).

Done.

[eroman, 2009-01-03 04:39:05]

looks great to me, thanks for doing this!

I have some nits, sending them as:

http://codereview.chromium.org/17047

http://codereview.chromium.org/16270/diff/257/424
File chrome/common/json_value_serializer.cc (right):

http://codereview.chromium.org/16270/diff/257/424#newcode26
Line 26: return false;
return NULL

http://codereview.chromium.org/16270/diff/257/424#newcode54
Line 54: return false;
return NULL