gdrive: Get JSON feeds parsing off the UI thread

gdrive: Get JSON feeds parsing off the UI thread.

BUG=128378
TEST=GDataOperationsTest.GetDataOperationParseJson passes.

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

[yoshiki, 2012-07-09 23:59:00]

hashimoto: Could you take a look?

[hashimoto, 2012-07-10 03:48:38]

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_errorcode.h (right):

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_errorcode.h:27: GDATA_NO_STATUS            =
0,
Do we really need to add this?
Can we use HTTP_SUCCESS instead?

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_operations.cc (right):

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations.cc:489: default:
Shouldn't we call the callback on error cases?

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations.cc:503: bool
GetDataOperation::ParseResponse(GDataErrorCode code,
Please rename |code| to more informative name, see the comment on
gdata_operations.h.

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations.cc:509: scoped_ptr<base::Value>*
parsed_value = new scoped_ptr<base::Value>();
I've once tried to introduce this pointer-to-scoped_ptr pattern, but got strong
opposition from reviewers (See
http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda...
if you like).
Can't we use base::Value::DeepCopy() to avoid this pattern?
Maybe it will not cost so much.

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations.cc:522: void
GetDataOperation::RunCallback(GDataErrorCode code,
ditto as ParseResponse().

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations.cc:529: void
GetDataOperation::OnDataParsed(GDataErrorCode code,
ditto as ParseResponse().

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_operations.h (right):

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations.h:188: public
base::SupportsWeakPtr<GetDataOperation> {
SupportsWeakPtr exposes AsWeakPtr() for public.
In this case, you don't need it to be public so you can use base::WeakPtrFactory
instead.

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations.h:196: virtual bool
ParseResponse(GDataErrorCode code, const std::string& data);
|code| is too ambiguous, please rename this to more informative name like
|fetch_error_code| or something.

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations.h:204: void
RunCallback(GDataErrorCode code, scoped_ptr<base::Value> value);
ditto as ParseResponse().

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations.h:207: void
OnDataParsed(GDataErrorCode code, scoped_ptr<base::Value>* value);
ditto as ParseResponse().

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_operations_unittest.cc (right):

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations_unittest.cc:8: #include
"chrome/browser/chromeos/cros/cros_library.h"
Could you remove this?

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations_unittest.cc:12: #include
"chrome/browser/chromeos/gdata/gdata_util.h"
ditto.

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations_unittest.cc:16: #include
"testing/gmock/include/gmock/gmock.h"
ditto.

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations_unittest.cc:21: class
GetDataOperationJsonParseTest : public GetDataOperation {
Two comments:
1) Could you move this class into a nameless namespace?
2) Class name ending with "-Test" is generally used for names of tests.  I think
this class should be renamed to "JsonParseTestGetDataOperation" or something.

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations_unittest.cc:40: virtual GURL
GetURL() const OVERRIDE {
Two comments:
1) Add method comment "GetDataOperation override."
2) Since UrlFetchOperationBase::GetURL is protected, could you make this
protected?

[yoshiki, 2012-07-11 00:47:14]

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_errorcode.h (right):

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_errorcode.h:27: GDATA_NO_STATUS            =
0,
On 2012/07/10 03:48:39, hashimoto wrote:
> Do we really need to add this?
> Can we use HTTP_SUCCESS instead?

Done.

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_operations.cc (right):

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations.cc:489: default:
On 2012/07/10 03:48:39, hashimoto wrote:
> Shouldn't we call the callback on error cases?

Done.

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations.cc:503: bool
GetDataOperation::ParseResponse(GDataErrorCode code,
On 2012/07/10 03:48:39, hashimoto wrote:
> Please rename |code| to more informative name, see the comment on
> gdata_operations.h.

Done.

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations.cc:509: scoped_ptr<base::Value>*
parsed_value = new scoped_ptr<base::Value>();
As I added the comment, sometimes json might be so big that deep-copy takes a
lot of time. So I think it is better to keep this way.

On 2012/07/10 03:48:39, hashimoto wrote:
> I've once tried to introduce this pointer-to-scoped_ptr pattern, but got
strong
> opposition from reviewers (See
>
http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda...
> if you like).
> Can't we use base::Value::DeepCopy() to avoid this pattern?
> Maybe it will not cost so much.

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations.cc:522: void
GetDataOperation::RunCallback(GDataErrorCode code,
On 2012/07/10 03:48:39, hashimoto wrote:
> ditto as ParseResponse().

Done.

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations.cc:529: void
GetDataOperation::OnDataParsed(GDataErrorCode code,
On 2012/07/10 03:48:39, hashimoto wrote:
> ditto as ParseResponse().

Done.

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_operations.h (right):

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations.h:188: public
base::SupportsWeakPtr<GetDataOperation> {
On 2012/07/10 03:48:39, hashimoto wrote:
> SupportsWeakPtr exposes AsWeakPtr() for public.
> In this case, you don't need it to be public so you can use
base::WeakPtrFactory
> instead.

Done.

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations.h:196: virtual bool
ParseResponse(GDataErrorCode code, const std::string& data);
On 2012/07/10 03:48:39, hashimoto wrote:
> |code| is too ambiguous, please rename this to more informative name like
> |fetch_error_code| or something.

Done.

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations.h:204: void
RunCallback(GDataErrorCode code, scoped_ptr<base::Value> value);
On 2012/07/10 03:48:39, hashimoto wrote:
> ditto as ParseResponse().

Done.

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations.h:207: void
OnDataParsed(GDataErrorCode code, scoped_ptr<base::Value>* value);
On 2012/07/10 03:48:39, hashimoto wrote:
> ditto as ParseResponse().

Done.

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_operations_unittest.cc (right):

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations_unittest.cc:8: #include
"chrome/browser/chromeos/cros/cros_library.h"
On 2012/07/10 03:48:39, hashimoto wrote:
> Could you remove this?

Done.

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations_unittest.cc:12: #include
"chrome/browser/chromeos/gdata/gdata_util.h"
On 2012/07/10 03:48:39, hashimoto wrote:
> ditto.

Done.

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations_unittest.cc:16: #include
"testing/gmock/include/gmock/gmock.h"
On 2012/07/10 03:48:39, hashimoto wrote:
> ditto.

Done.

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations_unittest.cc:21: class
GetDataOperationJsonParseTest : public GetDataOperation {
On 2012/07/10 03:48:39, hashimoto wrote:
> Two comments:
> 1) Could you move this class into a nameless namespace?
> 2) Class name ending with "-Test" is generally used for names of tests.  I
think
> this class should be renamed to "JsonParseTestGetDataOperation" or something.

Done x2.

http://codereview.chromium.org/10749015/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_operations_unittest.cc:40: virtual GURL
GetURL() const OVERRIDE {
On 2012/07/10 03:48:39, hashimoto wrote:
> Two comments:
> 1) Add method comment "GetDataOperation override."
> 2) Since UrlFetchOperationBase::GetURL is protected, could you make this
> protected?

Done x2.

[hashimoto, 2012-07-11 03:32:08]

lgtm

http://codereview.chromium.org/10749015/diff/12001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_operations.cc (left):

http://codereview.chromium.org/10749015/diff/12001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_operations.cc:477: return root_value.get()
!= NULL;
Look like this code had a bug where the return value is always false whenever
|callback_| is not null.
Good to see this bug is also fixed :)

[commit-bot: I haz the power, 2012-07-11 10:30:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/10749015/8004

[commit-bot: I haz the power, 2012-07-11 11:56:49]

Change committed as 146104