Add a static method to WebPDecoder to decode WebP

Add a static method to WebPDecoder to decode WebP

This CL adds a method to the WEBPDecoder to allow easier decoding.
Before each class has to implement its own delegate.

BUG=687921
Review-Url: https://codereview.chromium.org/2689213010
Cr-Commit-Position: refs/heads/master@{#451279}
Committed: https://chromium.googlesource.com/chromium/src/+/e94a602b2adc854bc7f8accffef20fcf5b1f54e0

[gambard, 2017-02-16 15:34:54]

gambard@chromium.org changed reviewers:
+ rohitrao@chromium.org, sdefresne@chromium.org

[gambard, 2017-02-16 15:34:55]

PTAL
rohit@:PTAL for ios/chrome/browser/suggestions.

[sdefresne, 2017-02-16 17:10:32]

https://codereview.chromium.org/2689213010/diff/20001/components/image_fetche...
File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm (right):

https://codereview.chromium.org/2689213010/diff/20001/components/image_fetche...
components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:79:
task_runner.get(), FROM_HERE, base::BindBlockArc(^NSData* {
I think this should be

  task_runner.get(), FROM_HERE, base::BindBlockArc(^NSData*() {

https://codereview.chromium.org/2689213010/diff/20001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/ios_image_decoder_impl.mm (right):

https://codereview.chromium.org/2689213010/diff/20001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/ios_image_decoder_impl.mm:61: [NSData
dataWithBytesNoCopy:const_cast<char*>(image_data.c_str())
I'm surprised this works. You're asking the NSData* not to copy the data, then
post a task to be executed later to decode it. If the caller data is released
before the task is executed, then the NSData will point at released memory, thus
undefined behaviour.

I think you should either copy the data or call
webp_transcode::WebpDecoder::DecodeWebpImage(data) in the current method (i.e.
not as PostTaskAndReplyWithResult...

[gambard, 2017-02-17 08:31:00]

Thanks, PTAL.

https://codereview.chromium.org/2689213010/diff/20001/components/image_fetche...
File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm (right):

https://codereview.chromium.org/2689213010/diff/20001/components/image_fetche...
components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:79:
task_runner.get(), FROM_HERE, base::BindBlockArc(^NSData* {
On 2017/02/16 17:10:32, sdefresne wrote:
> I think this should be
> 
>   task_runner.get(), FROM_HERE, base::BindBlockArc(^NSData*() {

Why?

https://codereview.chromium.org/2689213010/diff/20001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/ios_image_decoder_impl.mm (right):

https://codereview.chromium.org/2689213010/diff/20001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/ios_image_decoder_impl.mm:61: [NSData
dataWithBytesNoCopy:const_cast<char*>(image_data.c_str())
On 2017/02/16 17:10:32, sdefresne wrote:
> I'm surprised this works. You're asking the NSData* not to copy the data, then
> post a task to be executed later to decode it. If the caller data is released
> before the task is executed, then the NSData will point at released memory,
thus
> undefined behaviour.
> 
> I think you should either copy the data or call
> webp_transcode::WebpDecoder::DecodeWebpImage(data) in the current method (i.e.
> not as PostTaskAndReplyWithResult...

I am not sure this code path has ever been used.
Done.

[gambard, 2017-02-17 10:17:20]

Block formatted :)

[sdefresne, 2017-02-17 10:22:55]

lgtm

https://codereview.chromium.org/2689213010/diff/20001/components/image_fetche...
File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm (right):

https://codereview.chromium.org/2689213010/diff/20001/components/image_fetche...
components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:79:
task_runner.get(), FROM_HERE, base::BindBlockArc(^NSData* {
On 2017/02/17 08:30:59, gambard wrote:
> On 2017/02/16 17:10:32, sdefresne wrote:
> > I think this should be
> > 
> >   task_runner.get(), FROM_HERE, base::BindBlockArc(^NSData*() {
> 
> Why?

What you wrote is technically correct, however, it quite uncommon to omit the
argument list but not the return type in a block declaration. It's frequent to
omit the return type to have it inferred, or both if the argument list is empty
and you want the return type inferred. Your usage is quite uncommon though.

I'd prefer either you completely omit the return type (^{...}) and argument list
or list them both (^NSData*(){...}).

https://codereview.chromium.org/2689213010/diff/10006/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/ios_image_decoder_impl.mm (right):

https://codereview.chromium.org/2689213010/diff/10006/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/ios_image_decoder_impl.mm:59: // Convert the
|image_data| std::string to an NSData buffer.
nit: can you expand comment to say that the data is copied as it may have to
outlive the caller (in the case there is a posttaskandreply)? That way no one
would try to optimise this to -dataWithBytesNoCopy:length:freeWhenDone:.

[gambard, 2017-02-17 10:46:02]

Thanks!

https://codereview.chromium.org/2689213010/diff/10006/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/ios_image_decoder_impl.mm (right):

https://codereview.chromium.org/2689213010/diff/10006/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/ios_image_decoder_impl.mm:59: // Convert the
|image_data| std::string to an NSData buffer.
On 2017/02/17 10:22:55, sdefresne wrote:
> nit: can you expand comment to say that the data is copied as it may have to
> outlive the caller (in the case there is a posttaskandreply)? That way no one
> would try to optimise this to -dataWithBytesNoCopy:length:freeWhenDone:.

Done.

[gambard, 2017-02-17 10:46:08]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-02-17 10:46:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2689213010/#ps70001
(title: "Address comment")

[commit-bot: I haz the power, 2017-02-17 10:46:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 11:05:21]

CQ is committing da patch.

Bot data: {"patchset_id": 70001, "attempt_start_ts": 1487328368640720,
"parent_rev": "5b32d74c7168caa99fd78792166b0c7e03ac1515", "commit_rev":
"e94a602b2adc854bc7f8accffef20fcf5b1f54e0"}

[commit-bot: I haz the power, 2017-02-17 11:05:55]

Description was changed from

==========
Add a static method to WebPDecoder to decode WebP

This CL adds a method to the WEBPDecoder to allow easier decoding.
Before each class has to implement its own delegate.

BUG=687921
==========

to

==========
Add a static method to WebPDecoder to decode WebP

This CL adds a method to the WEBPDecoder to allow easier decoding.
Before each class has to implement its own delegate.

BUG=687921

Review-Url: https://codereview.chromium.org/2689213010
Cr-Commit-Position: refs/heads/master@{#451279}
Committed:
https://chromium.googlesource.com/chromium/src/+/e94a602b2adc854bc7f8accffef2...
==========

[commit-bot: I haz the power, 2017-02-17 11:05:56]

Committed patchset #5 (id:70001) as
https://chromium.googlesource.com/chromium/src/+/e94a602b2adc854bc7f8accffef2...