Add ios_image_decoder_impl.*

Add ios_image_decoder_impl.* and pass an instance to the NTPSnippetsService when it is instantiated on iOS.

BUG=609127
Committed: https://crrev.com/da47f994b774d716523f119b9b655ad0ebb22765
Cr-Commit-Position: refs/heads/master@{#403198}

[markusheintz_, 2016-06-29 16:37:42]

Use IOSImageDecoderImpl

[markusheintz_, 2016-06-29 16:47:28]

Description was changed from

==========
Add ios_image_decoder_impl.*

BUG=TBD
==========

to

==========
Add ios_image_decoder_impl.*

BUG=609127
==========

[markusheintz_, 2016-06-29 16:49:10]

Description was changed from

==========
Add ios_image_decoder_impl.*

BUG=609127
==========

to

==========
Add ios_image_decoder_impl.* and pass an instance to the NTPSnippetsService when
it is instantiated on iOS.

BUG=609127
==========

[markusheintz_, 2016-06-29 16:49:11]

markusheintz@chromium.org changed reviewers:
+ noyau@chromium.org

[markusheintz_, 2016-06-29 16:51:58]

Hey Eric

it would be great if you could take a look at this CL and let me know whether
this is heading in the right direction.

In CL https://codereview.chromium.org/2074153002/ I already moved the platform
independent image fetcher to the components directory. In a followup CL I would
start using components/image_fetcher/image_fetcher_impl.* and get rid of
ios/chrome/browser/suggestions/image_fetcher_impl.*

[markusheintz_, 2016-06-29 16:55:48]

"

[markusheintz_, 2016-06-29 17:07:53]

Modify the std::string to NSData conversion

[noyau (Ping after 24h), 2016-06-30 08:28:53]

Yup, this is fine. Couple of questions:

* Any reasons why the ios decoder is not in components as well? There is no
dependency on anything in Chrome…

* Ideally I'd like to see the WebP decoder make its way into this, so that this
image_fetcher can subsume the one in ios/chrome/browser/net. But this doesn't
need to happen in this CL

* I'm assuming that a subsequent CL will remove the image_fetcher_impl from
ios/c/b/suggestions, right?

https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/ios_image_decoder_impl.h (right):

https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/ios_image_decoder_impl.h:19: const std::string&
image_data,
I'm just curious why this api uses a string here and not simply a vector/array
of bytes?

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

https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/ios_image_decoder_impl.mm:22: NSData* nsData =
We tend to drop the prefix for variables. s/nsData/data

https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/ios_image_decoder_impl.mm:23: [NSData
dataWithBytes:image_data.c_str() length:image_data.length()];
This causes a copy of the string buffer.

If you use -dataWithBytesNoCopy:length:freeWhenDone:NO you'll get the exact same
behavior, but without the copy. As the string is guaranteed not to be freed
until the method returns, this is safe.

[markusheintz_, 2016-06-30 09:15:07]

Address some comments from noyau & other will be clarified

[markusheintz_, 2016-06-30 09:15:15]

Answers to your three questions:

1) No particular reason why iosDecoder does not live int he component. I'll add
an iOS folder there and will move it there.

2) Will be done. I'll file a bug for this to track the work.

3) Correct I love small CLs :-)

https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/ios_image_decoder_impl.h (right):

https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/ios_image_decoder_impl.h:19: const std::string&
image_data,
On 2016/06/30 08:28:53, noyau wrote:
> I'm just curious why this api uses a string here and not simply a vector/array
> of bytes?

I want to get rid of the std::string as byte buffer. This is on the TODO list in
my head. I filed a bug for this to track the work
(https://bugs.chromium.org/p/chromium/issues/detail?id=624720)

Three reasons why I have not does this yet:
1) The URL Fetcher returns the response as string so this was the common
denominator to start with (see
https://cs.chromium.org/chromium/src/components/image_fetcher/image_data_fetc...)
2) This work was blocking other so I needed to move.
3) I have not compared the different options yet. I first wanted to see how many
different byte buffers or options already exist in the Chrome code base before I
add mine ;-)

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

https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/ios_image_decoder_impl.mm:22: NSData* nsData =
On 2016/06/30 08:28:53, noyau wrote:
> We tend to drop the prefix for variables. s/nsData/data

Done.

https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/ios_image_decoder_impl.mm:23: [NSData
dataWithBytes:image_data.c_str() length:image_data.length()];
On 2016/06/30 08:28:53, noyau wrote:
> This causes a copy of the string buffer.
> 
> If you use -dataWithBytesNoCopy:length:freeWhenDone:NO you'll get the exact
same
> behavior, but without the copy. As the string is guaranteed not to be freed
> until the method returns, this is safe.

c_str or data are returning a const char* . But dataWithBytesNoCopy expects a
non const pointer.

Assume I'm a noob when it comes to objective C and in particular to the
libraries they offer. Maybe I'm missing something obvious.

I could do a cast here.

[markusheintz_, 2016-06-30 10:20:26]

On 2016/06/30 09:15:15, markusheintz_ wrote:
> Answers to your three questions:
> 
> 1) No particular reason why iosDecoder does not live int he component. I'll
add
> an iOS folder there and will move it there.
> 
> 2) Will be done. I'll file a bug for this to track the work.
> 
> 3) Correct I love small CLs :-)
> 
>
https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
> File ios/chrome/browser/suggestions/ios_image_decoder_impl.h (right):
> 
>
https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
> ios/chrome/browser/suggestions/ios_image_decoder_impl.h:19: const std::string&
> image_data,
> On 2016/06/30 08:28:53, noyau wrote:
> > I'm just curious why this api uses a string here and not simply a
vector/array
> > of bytes?
> 
> I want to get rid of the std::string as byte buffer. This is on the TODO list
in
> my head. I filed a bug for this to track the work
> (https://bugs.chromium.org/p/chromium/issues/detail?id=624720)
> 
> Three reasons why I have not does this yet:
> 1) The URL Fetcher returns the response as string so this was the common
> denominator to start with (see
>
https://cs.chromium.org/chromium/src/components/image_fetcher/image_data_fetc...)
> 2) This work was blocking other so I needed to move.
> 3) I have not compared the different options yet. I first wanted to see how
many
> different byte buffers or options already exist in the Chrome code base before
I
> add mine ;-)
> 
>
https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
> File ios/chrome/browser/suggestions/ios_image_decoder_impl.mm (right):
> 
>
https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
> ios/chrome/browser/suggestions/ios_image_decoder_impl.mm:22: NSData* nsData =
> On 2016/06/30 08:28:53, noyau wrote:
> > We tend to drop the prefix for variables. s/nsData/data
> 
> Done.
> 
>
https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
> ios/chrome/browser/suggestions/ios_image_decoder_impl.mm:23: [NSData
> dataWithBytes:image_data.c_str() length:image_data.length()];
> On 2016/06/30 08:28:53, noyau wrote:
> > This causes a copy of the string buffer.
> > 
> > If you use -dataWithBytesNoCopy:length:freeWhenDone:NO you'll get the exact
> same
> > behavior, but without the copy. As the string is guaranteed not to be freed
> > until the method returns, this is safe.
> 
> c_str or data are returning a const char* . But dataWithBytesNoCopy expects a
> non const pointer.
> 
> Assume I'm a noob when it comes to objective C and in particular to the
> libraries they offer. Maybe I'm missing something obvious.
> 
> I could do a cast here.

After taking a look at our component dir I would love to make a plan first
before I move the IOSImage Decoder there.
Would you be fine with leaving the decoder were it is for this CL.

I would fill a bug and summarize the plan in a 1 pager to let You, Bernhard, Tim
and the rest of the team comment on it.

[noyau (Ping after 24h), 2016-06-30 10:50:18]

On 2016/06/30 09:15:15, markusheintz_ wrote:
> Answers to your three questions:
> 
> 1) No particular reason why iosDecoder does not live int he component. I'll
add
> an iOS folder there and will move it there.
> 
> 2) Will be done. I'll file a bug for this to track the work.
> 
> 3) Correct I love small CLs :-)
> 
>
https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
> File ios/chrome/browser/suggestions/ios_image_decoder_impl.h (right):
> 
>
https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
> ios/chrome/browser/suggestions/ios_image_decoder_impl.h:19: const std::string&
> image_data,
> On 2016/06/30 08:28:53, noyau wrote:
> > I'm just curious why this api uses a string here and not simply a
vector/array
> > of bytes?
> 
> I want to get rid of the std::string as byte buffer. This is on the TODO list
in
> my head. I filed a bug for this to track the work
> (https://bugs.chromium.org/p/chromium/issues/detail?id=624720)
> 
> Three reasons why I have not does this yet:
> 1) The URL Fetcher returns the response as string so this was the common
> denominator to start with (see
>
https://cs.chromium.org/chromium/src/components/image_fetcher/image_data_fetc...)
> 2) This work was blocking other so I needed to move.
> 3) I have not compared the different options yet. I first wanted to see how
many
> different byte buffers or options already exist in the Chrome code base before
I
> add mine ;-)
> 
>
https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
> File ios/chrome/browser/suggestions/ios_image_decoder_impl.mm (right):
> 
>
https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
> ios/chrome/browser/suggestions/ios_image_decoder_impl.mm:22: NSData* nsData =
> On 2016/06/30 08:28:53, noyau wrote:
> > We tend to drop the prefix for variables. s/nsData/data
> 
> Done.
> 
>
https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
> ios/chrome/browser/suggestions/ios_image_decoder_impl.mm:23: [NSData
> dataWithBytes:image_data.c_str() length:image_data.length()];
> On 2016/06/30 08:28:53, noyau wrote:
> > This causes a copy of the string buffer.
> > 
> > If you use -dataWithBytesNoCopy:length:freeWhenDone:NO you'll get the exact
> same
> > behavior, but without the copy. As the string is guaranteed not to be freed
> > until the method returns, this is safe.
> 
> c_str or data are returning a const char* . But dataWithBytesNoCopy expects a
> non const pointer.
> 
> Assume I'm a noob when it comes to objective C and in particular to the
> libraries they offer. Maybe I'm missing something obvious.
> 
> I could do a cast here.

These API are old, and predate C++ usage. I'm pretty sure a cast would be fine.
But this said, yuck, this API stinks.

[noyau (Ping after 24h), 2016-06-30 10:51:13]

On 2016/06/30 10:20:26, markusheintz_ wrote:
> On 2016/06/30 09:15:15, markusheintz_ wrote:
> > Answers to your three questions:
> > 
> > 1) No particular reason why iosDecoder does not live int he component. I'll
> add
> > an iOS folder there and will move it there.
> > 
> > 2) Will be done. I'll file a bug for this to track the work.
> > 
> > 3) Correct I love small CLs :-)
> > 
> >
>
https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
> > File ios/chrome/browser/suggestions/ios_image_decoder_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
> > ios/chrome/browser/suggestions/ios_image_decoder_impl.h:19: const
std::string&
> > image_data,
> > On 2016/06/30 08:28:53, noyau wrote:
> > > I'm just curious why this api uses a string here and not simply a
> vector/array
> > > of bytes?
> > 
> > I want to get rid of the std::string as byte buffer. This is on the TODO
list
> in
> > my head. I filed a bug for this to track the work
> > (https://bugs.chromium.org/p/chromium/issues/detail?id=624720)
> > 
> > Three reasons why I have not does this yet:
> > 1) The URL Fetcher returns the response as string so this was the common
> > denominator to start with (see
> >
>
https://cs.chromium.org/chromium/src/components/image_fetcher/image_data_fetc...)
> > 2) This work was blocking other so I needed to move.
> > 3) I have not compared the different options yet. I first wanted to see how
> many
> > different byte buffers or options already exist in the Chrome code base
before
> I
> > add mine ;-)
> > 
> >
>
https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
> > File ios/chrome/browser/suggestions/ios_image_decoder_impl.mm (right):
> > 
> >
>
https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
> > ios/chrome/browser/suggestions/ios_image_decoder_impl.mm:22: NSData* nsData
=
> > On 2016/06/30 08:28:53, noyau wrote:
> > > We tend to drop the prefix for variables. s/nsData/data
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2111573002/diff/80001/ios/chrome/browser/sugg...
> > ios/chrome/browser/suggestions/ios_image_decoder_impl.mm:23: [NSData
> > dataWithBytes:image_data.c_str() length:image_data.length()];
> > On 2016/06/30 08:28:53, noyau wrote:
> > > This causes a copy of the string buffer.
> > > 
> > > If you use -dataWithBytesNoCopy:length:freeWhenDone:NO you'll get the
exact
> > same
> > > behavior, but without the copy. As the string is guaranteed not to be
freed
> > > until the method returns, this is safe.
> > 
> > c_str or data are returning a const char* . But dataWithBytesNoCopy expects
a
> > non const pointer.
> > 
> > Assume I'm a noob when it comes to objective C and in particular to the
> > libraries they offer. Maybe I'm missing something obvious.
> > 
> > I could do a cast here.
> 
> After taking a look at our component dir I would love to make a plan first
> before I move the IOSImage Decoder there.
> Would you be fine with leaving the decoder were it is for this CL.
> 
> I would fill a bug and summarize the plan in a 1 pager to let You, Bernhard,
Tim
> and the rest of the team comment on it.

Ok.

[markusheintz_, 2016-06-30 14:57:24]

Addressed final comments noyau

[markusheintz_, 2016-06-30 15:18:14]

On 2016/06/30 14:57:24, markusheintz_ wrote:
> Addressed final comments noyau

Added the const_cast and filed
https://bugs.chromium.org/p/chromium/issues/detail?id=624761

[markusheintz_, 2016-06-30 15:21:19]

Fix typo in ios_chrome.gyp

[noyau (Ping after 24h), 2016-06-30 15:42:19]

lgtm

[markusheintz_, 2016-06-30 16:10:57]

The CQ bit was checked by markusheintz@chromium.org

[commit-bot: I haz the power, 2016-06-30 16:11:17]

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, 2016-06-30 16:48:44]

Description was changed from

==========
Add ios_image_decoder_impl.* and pass an instance to the NTPSnippetsService when
it is instantiated on iOS.

BUG=609127
==========

to

==========
Add ios_image_decoder_impl.* and pass an instance to the NTPSnippetsService when
it is instantiated on iOS.

BUG=609127
==========

[commit-bot: I haz the power, 2016-06-30 16:48:45]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-30 16:49:01]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 16:50:07]

Description was changed from

==========
Add ios_image_decoder_impl.* and pass an instance to the NTPSnippetsService when
it is instantiated on iOS.

BUG=609127
==========

to

==========
Add ios_image_decoder_impl.* and pass an instance to the NTPSnippetsService when
it is instantiated on iOS.

BUG=609127

Committed: https://crrev.com/da47f994b774d716523f119b9b655ad0ebb22765
Cr-Commit-Position: refs/heads/master@{#403198}
==========

[commit-bot: I haz the power, 2016-06-30 16:50:08]

Patchset 8 (id:??) landed as
https://crrev.com/da47f994b774d716523f119b9b655ad0ebb22765
Cr-Commit-Position: refs/heads/master@{#403198}