In-process JSON parsing for web resources on iOS

In-process JSON parsing for web resources on iOS

BUG=447575
Committed: https://crrev.com/37deaaa2ccfe7e71b0f13536bb58b7cdec1c4b8b
Cr-Commit-Position: refs/heads/master@{#313063}

[droger, 2015-01-26 10:35:12]

Patchset #1 (id:1) has been deleted

[droger, 2015-01-26 11:45:45]

droger@chromium.org changed reviewers:
+ bauerb@chromium.org

[droger, 2015-01-26 11:45:45]

The implementation is based on the old WebResourceUnpacker implementation.

[Bernhard Bauer, 2015-01-26 12:05:43]

https://codereview.chromium.org/880433002/diff/30001/ios/chrome/browser/web_r...
File ios/chrome/browser/web_resource/ios_web_resource_service.cc (right):

https://codereview.chromium.org/880433002/diff/30001/ios/chrome/browser/web_r...
ios/chrome/browser/web_resource/ios_web_resource_service.cc:76:
base::Passed(&data_copy), success_callback, error_callback),
Could you just pass the data directly?

https://codereview.chromium.org/880433002/diff/30001/ios/chrome/browser/web_r...
File ios/chrome/browser/web_resource/ios_web_resource_service.h (right):

https://codereview.chromium.org/880433002/diff/30001/ios/chrome/browser/web_r...
ios/chrome/browser/web_resource/ios_web_resource_service.h:31: static
base::Closure ParseJSONOnBackgroundThread(
If this method is static and private, it can just live in the anonymous
namespace in the .cc file.

[droger, 2015-01-26 12:14:24]

Thanks for the review.
I have some questions/comments on your comments.

https://codereview.chromium.org/880433002/diff/30001/ios/chrome/browser/web_r...
File ios/chrome/browser/web_resource/ios_web_resource_service.cc (right):

https://codereview.chromium.org/880433002/diff/30001/ios/chrome/browser/web_r...
ios/chrome/browser/web_resource/ios_web_resource_service.cc:76:
base::Passed(&data_copy), success_callback, error_callback),
On 2015/01/26 12:05:43, Bernhard Bauer wrote:
> Could you just pass the data directly?

I used base::Passed to avoid making a copy. I am not sure what would be the
benefits of that, is it to make the code clearer?

https://codereview.chromium.org/880433002/diff/30001/ios/chrome/browser/web_r...
File ios/chrome/browser/web_resource/ios_web_resource_service.h (right):

https://codereview.chromium.org/880433002/diff/30001/ios/chrome/browser/web_r...
ios/chrome/browser/web_resource/ios_web_resource_service.h:31: static
base::Closure ParseJSONOnBackgroundThread(
On 2015/01/26 12:05:43, Bernhard Bauer wrote:
> If this method is static and private, it can just live in the anonymous
> namespace in the .cc file.

I put the method in the class because the SuccessCallback and ErrorCallback
types are protected.
I can make them public and move the function out of the class if you think it is
better.

[Bernhard Bauer, 2015-01-26 12:22:49]

https://codereview.chromium.org/880433002/diff/30001/ios/chrome/browser/web_r...
File ios/chrome/browser/web_resource/ios_web_resource_service.cc (right):

https://codereview.chromium.org/880433002/diff/30001/ios/chrome/browser/web_r...
ios/chrome/browser/web_resource/ios_web_resource_service.cc:76:
base::Passed(&data_copy), success_callback, error_callback),
On 2015/01/26 12:14:23, droger wrote:
> On 2015/01/26 12:05:43, Bernhard Bauer wrote:
> > Could you just pass the data directly?
> 
> I used base::Passed to avoid making a copy. I am not sure what would be the
> benefits of that, is it to make the code clearer?

Yeah, pretty much. You do already make a copy right before that :)

https://codereview.chromium.org/880433002/diff/30001/ios/chrome/browser/web_r...
File ios/chrome/browser/web_resource/ios_web_resource_service.h (right):

https://codereview.chromium.org/880433002/diff/30001/ios/chrome/browser/web_r...
ios/chrome/browser/web_resource/ios_web_resource_service.h:31: static
base::Closure ParseJSONOnBackgroundThread(
On 2015/01/26 12:14:23, droger wrote:
> On 2015/01/26 12:05:43, Bernhard Bauer wrote:
> > If this method is static and private, it can just live in the anonymous
> > namespace in the .cc file.
> 
> I put the method in the class because the SuccessCallback and ErrorCallback
> types are protected.
> I can make them public and move the function out of the class if you think it
is
> better.

Oh, I see.

Hm, in that case either approach is fine with me.

[droger, 2015-01-26 12:28:19]

https://codereview.chromium.org/880433002/diff/30001/ios/chrome/browser/web_r...
File ios/chrome/browser/web_resource/ios_web_resource_service.cc (right):

https://codereview.chromium.org/880433002/diff/30001/ios/chrome/browser/web_r...
ios/chrome/browser/web_resource/ios_web_resource_service.cc:76:
base::Passed(&data_copy), success_callback, error_callback),
On 2015/01/26 12:22:49, Bernhard Bauer wrote:
> On 2015/01/26 12:14:23, droger wrote:
> > On 2015/01/26 12:05:43, Bernhard Bauer wrote:
> > > Could you just pass the data directly?
> > 
> > I used base::Passed to avoid making a copy. I am not sure what would be the
> > benefits of that, is it to make the code clearer?
> 
> Yeah, pretty much. You do already make a copy right before that :)

Done.

https://codereview.chromium.org/880433002/diff/30001/ios/chrome/browser/web_r...
File ios/chrome/browser/web_resource/ios_web_resource_service.h (right):

https://codereview.chromium.org/880433002/diff/30001/ios/chrome/browser/web_r...
ios/chrome/browser/web_resource/ios_web_resource_service.h:31: static
base::Closure ParseJSONOnBackgroundThread(
On 2015/01/26 12:22:49, Bernhard Bauer wrote:
> On 2015/01/26 12:14:23, droger wrote:
> > On 2015/01/26 12:05:43, Bernhard Bauer wrote:
> > > If this method is static and private, it can just live in the anonymous
> > > namespace in the .cc file.
> > 
> > I put the method in the class because the SuccessCallback and ErrorCallback
> > types are protected.
> > I can make them public and move the function out of the class if you think
it
> is
> > better.
> 
> Oh, I see.
> 
> Hm, in that case either approach is fine with me.

Ok, I left it as is then, to minimize the size of the CL.

[Bernhard Bauer, 2015-01-26 13:37:31]

lgtm

[droger, 2015-01-26 13:39:24]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2015-01-26 13:40:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/880433002/50001

[commit-bot: I haz the power, 2015-01-26 14:28:50]

Committed patchset #3 (id:50001)

[commit-bot: I haz the power, 2015-01-26 14:29:41]

Patchset 3 (id:??) landed as
https://crrev.com/37deaaa2ccfe7e71b0f13536bb58b7cdec1c4b8b
Cr-Commit-Position: refs/heads/master@{#313063}