Run parsing JSON into ResourceList on blocking pool.

Run parsing JSON into ResourceList on blocking pool.

This CL moves the parsing JSON task from UI thread to blocking pool.
Along the change, thin struct OnParseResourceListCompletedParams is introduced
for memory management and adapt callback arguments.

BUG=165088
TEST=Ran unit_tests and chromeos_unittests.


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

[hidehiko, 2013-01-08 11:16:54]

Thank you for your review in advance,
- hidehiko

[kinaba, 2013-01-08 11:53:16]

lgtm with a nit.

https://codereview.chromium.org/11783025/diff/2001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_api_service.cc (right):

https://codereview.chromium.org/11783025/diff/2001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_api_service.cc:92:
base::Passed(value.Pass()), params),
You can use base::Passed(&value).

[hidehiko, 2013-01-08 13:57:00]

Thank you for your review.

https://codereview.chromium.org/11783025/diff/2001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_api_service.cc (right):

https://codereview.chromium.org/11783025/diff/2001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_api_service.cc:92:
base::Passed(value.Pass()), params),
On 2013/01/08 11:53:16, kinaba wrote:
> You can use base::Passed(&value).

Good to know. Thank you!

[satorux1, 2013-01-09 00:25:19]

https://codereview.chromium.org/11783025/diff/5001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_api_service.cc (right):

https://codereview.chromium.org/11783025/diff/5001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_api_service.cc:70: void
OnParseResourceListCompleted(
Please rename this to DidParseResourceListOnBlockingPool.

This naming convention, just adding "Did" is easier.

https://codereview.chromium.org/11783025/diff/5001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_api_service.cc:83: new
OnParseResourceListCompletedParams;
I think you don't need a struct.

[hidehiko, 2013-01-09 05:21:51]

Could you take another look?

https://codereview.chromium.org/11783025/diff/5001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_api_service.cc (right):

https://codereview.chromium.org/11783025/diff/5001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_api_service.cc:70: void
OnParseResourceListCompleted(
On 2013/01/09 00:25:20, satorux1 wrote:
> Please rename this to DidParseResourceListOnBlockingPool.
> 
> This naming convention, just adding "Did" is easier.

Done.

https://codereview.chromium.org/11783025/diff/5001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_api_service.cc:83: new
OnParseResourceListCompletedParams;
On 2013/01/09 00:25:20, satorux1 wrote:
> I think you don't need a struct.

At the moment we need, IMHO, though there are some alternatives:
1) Run callback if value is NULL here immediately. (though, handling the
callback would be distributed in this method and
DidParseResourceListOnBlockingPool).
2) Overwrite the error code with GDATA_PARSE_ERROR if value is NULL. Then we can
remove error code handling in ParseResourceListOnBlockingPool so that we can
remove params here. (I'm not very sure if we can overwrite it or not, though.)
3) Check if error code is HTTP_SUCCESS or not in
DidParseResourceListOnBlockingPool. If so, and the parsing is failed (i.e.
ParseResourceListOnBlockingPool returns NULL), overwrite error code with
GDATA_PARSE_ERROR.
4) Pass the ownership of |value| to DidParseResourceListOnBlockingPool, instead
give ParseResourceListOnBlockingPool the raw pointer to |value|. So that
DidParseResourceListOnBlockingPool can know if the parsing is failed, or JSON
value was empty (though the handling code may be less clear).
5) Use new for google_apis::GDataErrorCode and 
scoped_ptr<google_apis::ResourceList> individually, here. Then pass their
ownership to the DidParseResourceListOnBlockingPool.
6) Keep the current structure (use the struct).
7) something else?

Which is better in this project?

[satorux1, 2013-01-09 05:34:15]

https://codereview.chromium.org/11783025/diff/5001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_api_service.cc (right):

https://codereview.chromium.org/11783025/diff/5001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_api_service.cc:83: new
OnParseResourceListCompletedParams;
On 2013/01/09 05:21:51, hidehiko wrote:
> On 2013/01/09 00:25:20, satorux1 wrote:
> > I think you don't need a struct.
> 
> At the moment we need, IMHO, though there are some alternatives:
> 1) Run callback if value is NULL here immediately. (though, handling the
> callback would be distributed in this method and
> DidParseResourceListOnBlockingPool).
> 2) Overwrite the error code with GDATA_PARSE_ERROR if value is NULL. Then we
can
> remove error code handling in ParseResourceListOnBlockingPool so that we can
> remove params here. (I'm not very sure if we can overwrite it or not, though.)
> 3) Check if error code is HTTP_SUCCESS or not in
> DidParseResourceListOnBlockingPool. If so, and the parsing is failed (i.e.
> ParseResourceListOnBlockingPool returns NULL), overwrite error code with
> GDATA_PARSE_ERROR.
> 4) Pass the ownership of |value| to DidParseResourceListOnBlockingPool,
instead
> give ParseResourceListOnBlockingPool the raw pointer to |value|. So that
> DidParseResourceListOnBlockingPool can know if the parsing is failed, or JSON
> value was empty (though the handling code may be less clear).
> 5) Use new for google_apis::GDataErrorCode and 
> scoped_ptr<google_apis::ResourceList> individually, here. Then pass their
> ownership to the DidParseResourceListOnBlockingPool.
> 6) Keep the current structure (use the struct).
> 7) something else?
> 
> Which is better in this project?

I thought we could do something like:

  google_apis::GDataErrorCode* error = new
google_apis::GDataErrorCode(in_error);
  base::PostTaskAndReplyWithResult(
      BrowserThread::GetBlockingPool(),
      FROM_HERE,
      base::Bind(&ParseResourceListOnBlockingPool, base::Passed(&value), error),
      base::Bind(&DidParseResourceListOnBlockingPool, callback,
base::Owned(error)));

scoped_ptr<ResourceList> ParseResourceListOnBlockingPool(
    scoped_ptr<base::Value> value,
    GDataErrorCode* error) {
  ...

  return resource_list.Pass();
}


void DidParseResourceListOnBlockingPool(
    const google_apis::GetResourceListCallback& callback,
    GDataErrorCode* error,
    scoped_ptr<ResourceList> resource_list)
  callback.Run(*error, resource_list.Pass());
}

[hidehiko, 2013-01-09 07:34:07]

Couold you take another look?

https://codereview.chromium.org/11783025/diff/5001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_api_service.cc (right):

https://codereview.chromium.org/11783025/diff/5001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_api_service.cc:83: new
OnParseResourceListCompletedParams;
On 2013/01/09 05:34:15, satorux1 wrote:
> On 2013/01/09 05:21:51, hidehiko wrote:
> > On 2013/01/09 00:25:20, satorux1 wrote:
> > > I think you don't need a struct.
> > 
> > At the moment we need, IMHO, though there are some alternatives:
> > 1) Run callback if value is NULL here immediately. (though, handling the
> > callback would be distributed in this method and
> > DidParseResourceListOnBlockingPool).
> > 2) Overwrite the error code with GDATA_PARSE_ERROR if value is NULL. Then we
> can
> > remove error code handling in ParseResourceListOnBlockingPool so that we can
> > remove params here. (I'm not very sure if we can overwrite it or not,
though.)
> > 3) Check if error code is HTTP_SUCCESS or not in
> > DidParseResourceListOnBlockingPool. If so, and the parsing is failed (i.e.
> > ParseResourceListOnBlockingPool returns NULL), overwrite error code with
> > GDATA_PARSE_ERROR.
> > 4) Pass the ownership of |value| to DidParseResourceListOnBlockingPool,
> instead
> > give ParseResourceListOnBlockingPool the raw pointer to |value|. So that
> > DidParseResourceListOnBlockingPool can know if the parsing is failed, or
JSON
> > value was empty (though the handling code may be less clear).
> > 5) Use new for google_apis::GDataErrorCode and 
> > scoped_ptr<google_apis::ResourceList> individually, here. Then pass their
> > ownership to the DidParseResourceListOnBlockingPool.
> > 6) Keep the current structure (use the struct).
> > 7) something else?
> > 
> > Which is better in this project?
> 
> I thought we could do something like:
> 
>   google_apis::GDataErrorCode* error = new
> google_apis::GDataErrorCode(in_error);
>   base::PostTaskAndReplyWithResult(
>       BrowserThread::GetBlockingPool(),
>       FROM_HERE,
>       base::Bind(&ParseResourceListOnBlockingPool, base::Passed(&value),
error),
>       base::Bind(&DidParseResourceListOnBlockingPool, callback,
> base::Owned(error)));
> 
> scoped_ptr<ResourceList> ParseResourceListOnBlockingPool(
>     scoped_ptr<base::Value> value,
>     GDataErrorCode* error) {
>   ...
> 
>   return resource_list.Pass();
> }
> 
> 
> void DidParseResourceListOnBlockingPool(
>     const google_apis::GetResourceListCallback& callback,
>     GDataErrorCode* error,
>     scoped_ptr<ResourceList> resource_list)
>   callback.Run(*error, resource_list.Pass());
> }
> 

I see. Done.

[satorux1, 2013-01-09 08:13:05]

LGTM. Thanks!

[hidehiko, 2013-01-09 11:31:01]

Thank you, Satoru, for you review.

Rebased. Kazuhiro, could you kindly take a look, too?

[kinaba, 2013-01-09 11:36:00]

On 2013/01/09 11:31:01, hidehiko wrote:
> Thank you, Satoru, for you review.
> 
> Rebased. Kazuhiro, could you kindly take a look, too?

Still LGTM. Thanks!

[commit-bot: I haz the power, 2013-01-09 11:37:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/11783025/18001

[commit-bot: I haz the power, 2013-01-09 13:45:57]

Change committed as 175793