Turn the utility process image decoder into a Mojo service.

Turn the utility process image decoder into a Mojo service.

Also, keep the Chrome IPC based decoder, just in case.

[darin (slow to review), 2015-03-24 16:37:39]

darin@chromium.org changed reviewers:
+ darin@chromium.org

[darin (slow to review), 2015-03-24 16:37:40]

services/*/services/ ??

maybe the second "services" should be "src" or some other term?

[Ken Rockot(use gerrit already), 2015-03-24 20:35:11]

rockot@chromium.org changed reviewers:
+ rockot@chromium.org

[Ken Rockot(use gerrit already), 2015-03-24 20:35:12]

Cool! I might suggest omitting the inner services/ altogether here, letting the
impl live in //services/image_decoder alongside public/

https://codereview.chromium.org/1028543002/diff/20001/services/image_decoder/...
File services/image_decoder/public/interfaces/image.mojom (right):

https://codereview.chromium.org/1028543002/diff/20001/services/image_decoder/...
services/image_decoder/public/interfaces/image.mojom:7: // TODO(amistry):
Consider moving somewhere else.
What about somewhere like //skia/public{/interfaces/image.mojom,
type_converters.h}? It would be nice if these lived in the skia project, but
//skia might be a good compromise for now if OWNERS are OK with it.

Then this could just be module skia, which seems nicer.

https://codereview.chromium.org/1028543002/diff/20001/services/image_decoder/...
File services/image_decoder/public/interfaces/image_decoder.mojom (right):

https://codereview.chromium.org/1028543002/diff/20001/services/image_decoder/...
services/image_decoder/public/interfaces/image_decoder.mojom:5: module
services.image_decoder;
Maybe we could drop the services namespace?

[Anand Mistry (off Chromium), 2015-03-25 03:14:32]

The Chrome convention seems to be explicit public and implicit impl in terms of
directory structure. //content follows this, and the mojo network service also
follows this.

I'm going to add tests, gyp rules, fix up the implementation a bit, and then
sent this out to the various owners.

https://codereview.chromium.org/1028543002/diff/20001/services/image_decoder/...
File services/image_decoder/public/interfaces/image.mojom (right):

https://codereview.chromium.org/1028543002/diff/20001/services/image_decoder/...
services/image_decoder/public/interfaces/image.mojom:7: // TODO(amistry):
Consider moving somewhere else.
On 2015/03/24 20:35:12, Ken Rockot wrote:
> What about somewhere like //skia/public{/interfaces/image.mojom,
> type_converters.h}? It would be nice if these lived in the skia project, but
> //skia might be a good compromise for now if OWNERS are OK with it.
> 
> Then this could just be module skia, which seems nicer.

Done.

https://codereview.chromium.org/1028543002/diff/20001/services/image_decoder/...
File services/image_decoder/public/interfaces/image_decoder.mojom (right):

https://codereview.chromium.org/1028543002/diff/20001/services/image_decoder/...
services/image_decoder/public/interfaces/image_decoder.mojom:5: module
services.image_decoder;
On 2015/03/24 20:35:12, Ken Rockot wrote:
> Maybe we could drop the services namespace?

I kinda want to keep services to minimise namespace pollution. Maybe I can drop
the image_decoder part, similar to content. Thoughts?

[Ken Rockot(use gerrit already), 2015-03-25 22:43:44]

On 2015/03/25 03:14:32, Anand Mistry wrote:
> I kinda want to keep services to minimise namespace pollution. Maybe I can
drop
> the image_decoder part, similar to content. Thoughts?

OK. I don't feel too strongly about it at the moment. If nearly every service
ends up in the services namespace though, it seems like overkill. Most
components in //components have a unique top-level namespace, i.e. there's no
components::foo.

https://codereview.chromium.org/1028543002/diff/40001/skia/public/interfaces/...
File skia/public/interfaces/image.mojom (right):

https://codereview.chromium.org/1028543002/diff/40001/skia/public/interfaces/...
skia/public/interfaces/image.mojom:34: struct Image {
nit: Should this maybe be Bitmap? And bitmap.mojom?

[Anand Mistry (off Chromium), 2015-04-01 07:29:47]

amistry@chromium.org changed reviewers:
- darin@chromium.org, rockot@chromium.org

[Anand Mistry (off Chromium), 2015-04-01 07:43:40]

amistry@chromium.org changed reviewers:
+ edisonn@google.com, jam@chromium.org, rockot@chromium.org, sky@chromium.org

[Anand Mistry (off Chromium), 2015-04-01 07:43:40]

edisonn@google.com: Please review changes in skia/

jam@chromium.org: For new top-level directory

sky@chromium.org: Please review changes in chrome/

rockot@chromium.org: Please review everything!!one!

Another step in making things Mojoier. What do you think? I've left out gyp
rules in case y'all think this is a terrible change. But if you're overall
happy, I'll add them.

[sky, 2015-04-01 15:00:45]

https://codereview.chromium.org/1028543002/diff/60001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/1028543002/diff/60001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:40: ImageDecoder::~ImageDecoder() {}
I'm not familiar with this class. What thread is the destructor run on?

https://codereview.chromium.org/1028543002/diff/60001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:73: void ImageDecoder::OnDecodeImageDone(bool
success, skia::BitmapPtr image) {
Order doesn't match header.

https://codereview.chromium.org/1028543002/diff/60001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/1028543002/diff/60001/chrome/browser/image_de...
chrome/browser/image_decoder.h:81: void OnDecodeImageDone(bool success,
skia::BitmapPtr image);
Do we need to distinguish between failure and an empty image? I'm wondering if
we can get away without the success arg. That you only notify
OnDecodeImageSucceeded on success and non-empty image seems to indicate you
don't know the success boolean.

https://codereview.chromium.org/1028543002/diff/60001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.h (right):

https://codereview.chromium.org/1028543002/diff/60001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.h:50: void MojoRef();
Who is responsible for invoking these?

https://codereview.chromium.org/1028543002/diff/60001/services/image_decoder/...
File services/image_decoder/public/interfaces/image_decoder.mojom (right):

https://codereview.chromium.org/1028543002/diff/60001/services/image_decoder/...
services/image_decoder/public/interfaces/image_decoder.mojom:7: import
"skia/public/interfaces/bitmap.mojom";
Have you considered nuking the public part of the path, eg
ervices/image_decoder/public/interfaces. 'public' is helpful in the mojo repo as
parts of it are exported, but I don't think that applies to the chrome repo. I
would be inclined to make this path: services/image_decoder/interfaces. If we do
want private interfaces I would make that the special case, eg
ervices/image_decoder/private/interfaces.

[Anand Mistry (off Chromium), 2015-04-02 04:29:44]

amistry@chromium.org changed reviewers:
+ sammc@chromium.org

[Anand Mistry (off Chromium), 2015-04-02 04:29:45]

Hold off reviewing chrome/browser/image_decoder.cc because that has substantial
changes I need to rebase and resolve. Everything else is still up for review.

https://codereview.chromium.org/1028543002/diff/60001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/1028543002/diff/60001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:40: ImageDecoder::~ImageDecoder() {}
On 2015/04/01 15:00:45, sky wrote:
> I'm not familiar with this class. What thread is the destructor run on?

Most likely the task_runner_ thread (usually UI) since the last reference to
this should be the success/failed callback. However, this is why
decoder_.reset() exists in the Mojo callback and error handler below. The
response callback holds a reference and ensures this doesn't get deleted unless
either the callback or error handler has run. Subtle,  I know. Maybe I should
split the Mojo part into a helper class to make it less subtle.

https://codereview.chromium.org/1028543002/diff/60001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:73: void ImageDecoder::OnDecodeImageDone(bool
success, skia::BitmapPtr image) {
On 2015/04/01 15:00:45, sky wrote:
> Order doesn't match header.

You mean ordering of functions? Fixed that.

https://codereview.chromium.org/1028543002/diff/60001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/1028543002/diff/60001/chrome/browser/image_de...
chrome/browser/image_decoder.h:81: void OnDecodeImageDone(bool success,
skia::BitmapPtr image);
On 2015/04/01 15:00:45, sky wrote:
> Do we need to distinguish between failure and an empty image? I'm wondering if
> we can get away without the success arg. That you only notify
> OnDecodeImageSucceeded on success and non-empty image seems to indicate you
> don't know the success boolean.

Done.

https://codereview.chromium.org/1028543002/diff/60001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.h (right):

https://codereview.chromium.org/1028543002/diff/60001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.h:50: void MojoRef();
On 2015/04/01 15:00:45, sky wrote:
> Who is responsible for invoking these?

Added a blurb to describe this. At this point, I'm sure I want to refcount the
Mojo connections for establishing the lifetime of the utility process/thread.
However, the exact mechanism for doing so is something I'm still grappling with.
I added some templating to chrome_content_utility_client.cc which does it
implicitly based on a service implementation's lifetime, assuming a
StrongBinding<>. But I've also thought about doing it by passing a callback to
the service. I'm not sure about this yet.

https://codereview.chromium.org/1028543002/diff/60001/services/image_decoder/...
File services/image_decoder/public/interfaces/image_decoder.mojom (right):

https://codereview.chromium.org/1028543002/diff/60001/services/image_decoder/...
services/image_decoder/public/interfaces/image_decoder.mojom:7: import
"skia/public/interfaces/bitmap.mojom";
On 2015/04/01 15:00:45, sky wrote:
> Have you considered nuking the public part of the path, eg
> ervices/image_decoder/public/interfaces. 'public' is helpful in the mojo repo
as
> parts of it are exported, but I don't think that applies to the chrome repo. I
> would be inclined to make this path: services/image_decoder/interfaces. If we
do
> want private interfaces I would make that the special case, eg
> ervices/image_decoder/private/interfaces.

A couple of thoughts about this:
1. These interfaces are public. There are other public things as well, such as
type converters, and maybe helper functions or simple wrappers. I think it makes
sense to keep these together.
2. This is kinda modeled after content/ which explicitly calls out public
things, and keeps private things implicit.

[Anand Mistry (off Chromium), 2015-04-02 07:02:09]

amistry@chromium.org changed reviewers:
+ twellington@chromium.org

[Anand Mistry (off Chromium), 2015-04-02 07:02:10]

+twellington, who's recently made major changes to ImageDecoder

Now it's safe to look at chrome/browser/image_decoder.cc

Also, I strongly suspect there's a race condition in cancellation of a decode
request. It also doesn't appear to handle the utility process crashing
mid-decode. I haven't addressed either of these in this CL.

[sky, 2015-04-02 17:20:10]

https://codereview.chromium.org/1028543002/diff/60001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/1028543002/diff/60001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:40: ImageDecoder::~ImageDecoder() {}
On 2015/04/02 04:29:45, Anand Mistry wrote:
> On 2015/04/01 15:00:45, sky wrote:
> > I'm not familiar with this class. What thread is the destructor run on?
> 
> Most likely the task_runner_ thread (usually UI) since the last reference to
> this should be the success/failed callback. However, this is why
> decoder_.reset() exists in the Mojo callback and error handler below. The
> response callback holds a reference and ensures this doesn't get deleted
unless
> either the callback or error handler has run. Subtle,  I know. Maybe I should
> split the Mojo part into a helper class to make it less subtle.

I'm ok with that, as long as you add DCHECKs and better documentation indicating
this.

https://codereview.chromium.org/1028543002/diff/60001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.h (right):

https://codereview.chromium.org/1028543002/diff/60001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.h:50: void MojoRef();
On 2015/04/02 04:29:45, Anand Mistry wrote:
> On 2015/04/01 15:00:45, sky wrote:
> > Who is responsible for invoking these?
> 
> Added a blurb to describe this. At this point, I'm sure I want to refcount the
> Mojo connections for establishing the lifetime of the utility process/thread.
> However, the exact mechanism for doing so is something I'm still grappling
with.
> I added some templating to chrome_content_utility_client.cc which does it
> implicitly based on a service implementation's lifetime, assuming a
> StrongBinding<>. But I've also thought about doing it by passing a callback to
> the service. I'm not sure about this yet.

Having to explicitly invoke functions like this is error prone. Make it
consumers have to use a RAII class whose constructor does the Ref and destructor
the Unref.

https://codereview.chromium.org/1028543002/diff/60001/services/image_decoder/...
File services/image_decoder/public/interfaces/image_decoder.mojom (right):

https://codereview.chromium.org/1028543002/diff/60001/services/image_decoder/...
services/image_decoder/public/interfaces/image_decoder.mojom:7: import
"skia/public/interfaces/bitmap.mojom";
On 2015/04/02 04:29:45, Anand Mistry wrote:
> On 2015/04/01 15:00:45, sky wrote:
> > Have you considered nuking the public part of the path, eg
> > ervices/image_decoder/public/interfaces. 'public' is helpful in the mojo
repo
> as
> > parts of it are exported, but I don't think that applies to the chrome repo.
I
> > would be inclined to make this path: services/image_decoder/interfaces. If
we
> do
> > want private interfaces I would make that the special case, eg
> > ervices/image_decoder/private/interfaces.
> 
> A couple of thoughts about this:
> 1. These interfaces are public. There are other public things as well, such as
> type converters, and maybe helper functions or simple wrappers. I think it
makes
> sense to keep these together.
> 2. This is kinda modeled after content/ which explicitly calls out public
> things, and keeps private things implicit.

Fair enough.

[Anand Mistry (off Chromium), 2015-04-08 07:14:16]

Removed the utility process ref counting stuff since it's somewhat problematic.

Also, rebase onto https://codereview.chromium.org/1071453002/ where I've fixed
some issues.

https://codereview.chromium.org/1028543002/diff/60001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.h (right):

https://codereview.chromium.org/1028543002/diff/60001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.h:50: void MojoRef();
On 2015/04/02 17:20:10, sky wrote:
> On 2015/04/02 04:29:45, Anand Mistry wrote:
> > On 2015/04/01 15:00:45, sky wrote:
> > > Who is responsible for invoking these?
> > 
> > Added a blurb to describe this. At this point, I'm sure I want to refcount
the
> > Mojo connections for establishing the lifetime of the utility
process/thread.
> > However, the exact mechanism for doing so is something I'm still grappling
> with.
> > I added some templating to chrome_content_utility_client.cc which does it
> > implicitly based on a service implementation's lifetime, assuming a
> > StrongBinding<>. But I've also thought about doing it by passing a callback
to
> > the service. I'm not sure about this yet.
> 
> Having to explicitly invoke functions like this is error prone. Make it
> consumers have to use a RAII class whose constructor does the Ref and
destructor
> the Unref.

On further thought, there are races with this kind of service tracking and
shutdown which can cause spurious failures at the consumers. For now, I think
it's best to leave this out and require the browser side to shut down the
utility process when connecting to Mojo-based services.

Also, if the utility process is untrusted, can we really trust it to shut itself
down?

[Sam McNally, 2015-04-08 08:09:00]

https://codereview.chromium.org/1028543002/diff/120001/chrome/browser/image_d...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/1028543002/diff/120001/chrome/browser/image_d...
chrome/browser/image_decoder.cc:273: utility_process_host_.reset();
3 space indent?

https://codereview.chromium.org/1028543002/diff/120001/chrome/browser/image_d...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/1028543002/diff/120001/chrome/browser/image_d...
chrome/browser/image_decoder.h:153: // Mojo service connection. Must always be
bound and reset on the IO thread.
And only used on the IO thread.

https://codereview.chromium.org/1028543002/diff/120001/services/image_decoder...
File services/image_decoder/image_decoder_impl_unittest.cc (right):

https://codereview.chromium.org/1028543002/diff/120001/services/image_decoder...
services/image_decoder/image_decoder_impl_unittest.cc:38:
Request(ImageDecoderImpl* decoder) : decoder_(decoder) {}
explicit

https://codereview.chromium.org/1028543002/diff/120001/services/image_decoder...
services/image_decoder/image_decoder_impl_unittest.cc:49: skia::BitmapPtr
bitmap;
No public fields in a class.

https://codereview.chromium.org/1028543002/diff/120001/services/image_decoder...
services/image_decoder/image_decoder_impl_unittest.cc:83:
CreateJPEGImage(widths[i], heights[i], SK_ColorRED, &jpg);
ASSERT_TRUE()

https://codereview.chromium.org/1028543002/diff/120001/skia/public/type_conve...
File skia/public/type_converters.cc (right):

https://codereview.chromium.org/1028543002/diff/120001/skia/public/type_conve...
skia/public/type_converters.cc:98: SkBitmap TypeConverter<SkBitmap,
skia::BitmapPtr>::Convert(
Why not take a skia::Bitmap?

[Anand Mistry (off Chromium), 2015-04-09 05:25:03]

https://codereview.chromium.org/1028543002/diff/120001/chrome/browser/image_d...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/1028543002/diff/120001/chrome/browser/image_d...
chrome/browser/image_decoder.cc:273: utility_process_host_.reset();
On 2015/04/08 08:09:00, Sam McNally wrote:
> 3 space indent?

Done.

https://codereview.chromium.org/1028543002/diff/120001/chrome/browser/image_d...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/1028543002/diff/120001/chrome/browser/image_d...
chrome/browser/image_decoder.h:153: // Mojo service connection. Must always be
bound and reset on the IO thread.
On 2015/04/08 08:09:00, Sam McNally wrote:
> And only used on the IO thread.

Done.

https://codereview.chromium.org/1028543002/diff/120001/services/image_decoder...
File services/image_decoder/image_decoder_impl_unittest.cc (right):

https://codereview.chromium.org/1028543002/diff/120001/services/image_decoder...
services/image_decoder/image_decoder_impl_unittest.cc:38:
Request(ImageDecoderImpl* decoder) : decoder_(decoder) {}
On 2015/04/08 08:09:00, Sam McNally wrote:
> explicit

Done.

https://codereview.chromium.org/1028543002/diff/120001/services/image_decoder...
services/image_decoder/image_decoder_impl_unittest.cc:49: skia::BitmapPtr
bitmap;
On 2015/04/08 08:09:00, Sam McNally wrote:
> No public fields in a class.

But... but... it's just a test.

https://codereview.chromium.org/1028543002/diff/120001/services/image_decoder...
services/image_decoder/image_decoder_impl_unittest.cc:83:
CreateJPEGImage(widths[i], heights[i], SK_ColorRED, &jpg);
On 2015/04/08 08:09:00, Sam McNally wrote:
> ASSERT_TRUE()

Done.

https://codereview.chromium.org/1028543002/diff/120001/skia/public/type_conve...
File skia/public/type_converters.cc (right):

https://codereview.chromium.org/1028543002/diff/120001/skia/public/type_conve...
skia/public/type_converters.cc:98: SkBitmap TypeConverter<SkBitmap,
skia::BitmapPtr>::Convert(
On 2015/04/08 08:09:00, Sam McNally wrote:
> Why not take a skia::Bitmap?

Because almost nothing references it (or any other struct) in practice.
Everything takes and gives StructPtr<> wrappers.

FYI, //mojo/converters appears to be all over the place with respect to this.

[Sam McNally, 2015-04-09 08:35:30]

https://codereview.chromium.org/1028543002/diff/120001/skia/public/type_conve...
File skia/public/type_converters.cc (right):

https://codereview.chromium.org/1028543002/diff/120001/skia/public/type_conve...
skia/public/type_converters.cc:98: SkBitmap TypeConverter<SkBitmap,
skia::BitmapPtr>::Convert(
On 2015/04/09 05:25:03, Anand Mistry wrote:
> On 2015/04/08 08:09:00, Sam McNally wrote:
> > Why not take a skia::Bitmap?
> 
> Because almost nothing references it (or any other struct) in practice.
> Everything takes and gives StructPtr<> wrappers.
> 
> FYI, //mojo/converters appears to be all over the place with respect to this.

If the converter doesn't need to handle null input (or something fancy like
moving out of a StructPtr passed by value), I think we should prefer taking the
struct itself.

[Anand Mistry (off Chromium), 2015-04-10 02:56:28]

https://codereview.chromium.org/1028543002/diff/120001/skia/public/type_conve...
File skia/public/type_converters.cc (right):

https://codereview.chromium.org/1028543002/diff/120001/skia/public/type_conve...
skia/public/type_converters.cc:98: SkBitmap TypeConverter<SkBitmap,
skia::BitmapPtr>::Convert(
On 2015/04/09 08:35:30, Sam McNally wrote:
> On 2015/04/09 05:25:03, Anand Mistry wrote:
> > On 2015/04/08 08:09:00, Sam McNally wrote:
> > > Why not take a skia::Bitmap?
> > 
> > Because almost nothing references it (or any other struct) in practice.
> > Everything takes and gives StructPtr<> wrappers.
> > 
> > FYI, //mojo/converters appears to be all over the place with respect to
this.
> 
> If the converter doesn't need to handle null input (or something fancy like
> moving out of a StructPtr passed by value), I think we should prefer taking
the
> struct itself.

Ok. Done.

[Theresa, 2015-04-13 18:52:27]

https://codereview.chromium.org/1028543002/diff/60001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/1028543002/diff/60001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:40: ImageDecoder::~ImageDecoder() {}
On 2015/04/02 17:20:10, sky wrote:
> On 2015/04/02 04:29:45, Anand Mistry wrote:
> > On 2015/04/01 15:00:45, sky wrote:
> > > I'm not familiar with this class. What thread is the destructor run on?
> > 
> > Most likely the task_runner_ thread (usually UI) since the last reference to
> > this should be the success/failed callback. However, this is why
> > decoder_.reset() exists in the Mojo callback and error handler below. The
> > response callback holds a reference and ensures this doesn't get deleted
> unless
> > either the callback or error handler has run. Subtle,  I know. Maybe I
should
> > split the Mojo part into a helper class to make it less subtle.
> 
> I'm ok with that, as long as you add DCHECKs and better documentation
indicating
> this.

I'm pretty sure this method actually never gets run anymore. We explicitly call
AddRef() in the constructor to leak a reference so that the single ImageDecoder
object lives for the life of the program.

I did a really quick, simple test where I added NOTREACHED(); to this destructor
and ran the UserImageManager browser tests, and they all still passed.

https://codereview.chromium.org/1028543002/diff/180001/chrome/browser/image_d...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/1028543002/diff/180001/chrome/browser/image_d...
chrome/browser/image_decoder.cc:308: decoder_.reset();
nit: decoder_ only needs to be reset if mojo is enabled.

[leonhsl(Using Gerrit), 2015-04-16 10:11:03]

Hi,
Sorry to disturb.
I have a concern about skia.Bitmap converter definition:

I'm re-using the bitmap.mojom in the CL
https://codereview.chromium.org/1085783002/.
"array<skia.Bitmap>" is defined in my mojom file, and it will be translated as
"mojo::Array<skia::BitmapPtr>" in generated C++ binding file. 
Now I need to convert mojo::Array<skia::BitmapPtr> to std::vector<SkBitmap>, as
the array converter is defined in array.h as below:
"
template <typename E, typename T>
struct TypeConverter<std::vector<E>, Array<T>> {
  static std::vector<E> Convert(const Array<T>& input)
"
If TypeConverter<SkBitmap, skia::BitmapPtr> is available then I can use above
array converter directly. Otherwise I need to implement
TypeConverter<std::vector<SkBitmap>, Array<skia::Bitmap>> on my own.
So, does it make sense to still provide TypeConverter<SkBitmap, skia::BitmapPtr>
here? 
Thanks in advance.

[Ken Rockot(use gerrit already), 2015-04-16 15:20:17]

On 2015/04/16 10:11:03, leon.han wrote:
> Hi,
> Sorry to disturb.
> I have a concern about skia.Bitmap converter definition:
> 
> I'm re-using the bitmap.mojom in the CL
> https://codereview.chromium.org/1085783002/.
> "array<skia.Bitmap>" is defined in my mojom file, and it will be translated as
> "mojo::Array<skia::BitmapPtr>" in generated C++ binding file. 
> Now I need to convert mojo::Array<skia::BitmapPtr> to std::vector<SkBitmap>,
as
> the array converter is defined in array.h as below:
> "
> template <typename E, typename T>
> struct TypeConverter<std::vector<E>, Array<T>> {
>   static std::vector<E> Convert(const Array<T>& input)
> "
> If TypeConverter<SkBitmap, skia::BitmapPtr> is available then I can use above
> array converter directly. Otherwise I need to implement
> TypeConverter<std::vector<SkBitmap>, Array<skia::Bitmap>> on my own.
> So, does it make sense to still provide TypeConverter<SkBitmap,
skia::BitmapPtr>
> here? 
> Thanks in advance.

Yes it makes sense to provide only TypeConverter<SkBitmap, skia::BitmapPtr>! It
is preferable to do this in fact, specifically because it can then be used for
array and non-array conversions alike, given the generic vector<E>/Array<T>
converter.

[leonhsl(Using Gerrit), 2015-04-17 06:25:13]

@Rockot Thanks a lot for confirmation about the TypeConverters question~ And
I've splitted out /skia/public implementation from here as another CL
https://codereview.chromium.org/1098643002/ to try to submit firstly as my CL
needs them..

@Amistry Thank you very much for kindly consent about this, and sorry for that
it would bring you additional job to re-arrange codes here.

[reed1, 2015-04-20 16:07:10]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2015-04-20 16:07:11]

very intersted to learn more about this (from skia's perspective)

[scroggo, 2015-04-20 18:47:06]

On 2015/04/01 07:43:40, Anand Mistry (away till 30th) wrote:
> mailto:edisonn@google.com: Please review changes in skia/

FYI: edisonn@ is no longer a member of Skia.

> 
> mailto:jam@chromium.org: For new top-level directory
> 
> mailto:sky@chromium.org: Please review changes in chrome/
> 
> mailto:rockot@chromium.org: Please review everything!!one!
> 
> Another step in making things Mojoier. What do you think? I've left out gyp
> rules in case y'all think this is a terrible change. But if you're overall
> happy, I'll add them.

[sky, 2015-04-20 22:58:49]

We had a discussion at the mandoline team about directory structures and decided
on moving the services in mojo/services to components. It didn't seem worth
trying to create another directory structure that implies meaning. It would be
great if Chrome Core could follow the same pattern, eg what you have as
services/images_decoder becomes components/image_decoder.

[Anand Mistry (off Chromium), 2015-05-01 02:32:29]

amistry@chromium.org changed reviewers:
- edisonn@google.com

[Anand Mistry (off Chromium), 2015-05-01 06:49:04]

I've moved this into //components now. I'm still battling with gyp build issues
and some test stuff, but it's pretty much ready to continue reviewing.

https://codereview.chromium.org/1028543002/diff/180001/chrome/browser/image_d...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/1028543002/diff/180001/chrome/browser/image_d...
chrome/browser/image_decoder.cc:308: decoder_.reset();
On 2015/04/13 18:52:27, twellington wrote:
> nit: decoder_ only needs to be reset if mojo is enabled.

Done.

[Sam McNally, 2015-05-06 06:41:39]

https://codereview.chromium.org/1028543002/diff/330001/chrome/browser/image_d...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/1028543002/diff/330001/chrome/browser/image_d...
chrome/browser/image_decoder.h:164:
scoped_ptr<mojo::InterfacePtr<image_decoder::ImageDecoder>> decoder_;
Why a scoped_ptr?

https://codereview.chromium.org/1028543002/diff/330001/chrome/utility/BUILD.gn
File chrome/utility/BUILD.gn (right):

https://codereview.chromium.org/1028543002/diff/330001/chrome/utility/BUILD.g...
chrome/utility/BUILD.gn:24: "//content/common:mojo_bindings",
chrome shouldn't have a dependency on non-public content.

https://codereview.chromium.org/1028543002/diff/330001/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/1028543002/diff/330001/chrome/utility/DEPS#ne...
chrome/utility/DEPS:11: "+skia/public",
Is this still needed?

https://codereview.chromium.org/1028543002/diff/330001/components/image_decod...
File components/image_decoder/image_decoder_impl.cc (right):

https://codereview.chromium.org/1028543002/diff/330001/components/image_decod...
components/image_decoder/image_decoder_impl.cc:20: // TODO(amistry): Replace
with more correct value.
According to
https://code.google.com/p/chromium/codesearch/#chromium/src/content/app/mojo/...,
this should be the correct value.

https://codereview.chromium.org/1028543002/diff/330001/skia/public/type_conve...
File skia/public/type_converters.cc (right):

https://codereview.chromium.org/1028543002/diff/330001/skia/public/type_conve...
skia/public/type_converters.cc:115: const SkBitmap& bitmap) {
How about returning null for a null SkBitmap?

[Anand Mistry (off Chromium), 2015-05-06 07:14:24]

https://codereview.chromium.org/1028543002/diff/330001/chrome/browser/image_d...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/1028543002/diff/330001/chrome/browser/image_d...
chrome/browser/image_decoder.h:164:
scoped_ptr<mojo::InterfacePtr<image_decoder::ImageDecoder>> decoder_;
On 2015/05/06 06:41:39, Sam McNally wrote:
> Why a scoped_ptr?

Let me paraphrase Dorothy:
gyp, and gn, and deps! Oh my!

Or, put another way, using scoped_ptr<> lets me avoid depending on the generated
mojom header (or any part of Mojo) and leaking that dependency.

https://codereview.chromium.org/1028543002/diff/330001/chrome/utility/BUILD.gn
File chrome/utility/BUILD.gn (right):

https://codereview.chromium.org/1028543002/diff/330001/chrome/utility/BUILD.g...
chrome/utility/BUILD.gn:24: "//content/common:mojo_bindings",
On 2015/05/06 06:41:39, Sam McNally wrote:
> chrome shouldn't have a dependency on non-public content.

I can't remember why this is here since it's not used. Removed.

https://codereview.chromium.org/1028543002/diff/330001/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/1028543002/diff/330001/chrome/utility/DEPS#ne...
chrome/utility/DEPS:11: "+skia/public",
On 2015/05/06 06:41:39, Sam McNally wrote:
> Is this still needed?

Don't think so.

https://codereview.chromium.org/1028543002/diff/330001/components/image_decod...
File components/image_decoder/image_decoder_impl.cc (right):

https://codereview.chromium.org/1028543002/diff/330001/components/image_decod...
components/image_decoder/image_decoder_impl.cc:20: // TODO(amistry): Replace
with more correct value.
On 2015/05/06 06:41:39, Sam McNally wrote:
> According to
>
https://code.google.com/p/chromium/codesearch/#chromium/src/content/app/mojo/...,
> this should be the correct value.

Thanks.

https://codereview.chromium.org/1028543002/diff/330001/skia/public/type_conve...
File skia/public/type_converters.cc (right):

https://codereview.chromium.org/1028543002/diff/330001/skia/public/type_conve...
skia/public/type_converters.cc:115: const SkBitmap& bitmap) {
On 2015/05/06 06:41:39, Sam McNally wrote:
> How about returning null for a null SkBitmap?

Done.

[Sam McNally, 2015-05-06 07:20:16]

LGTM

[Ken Rockot(use gerrit already), 2015-05-08 19:39:05]

lgtm. It may help to refresh your request for specific OWNERs reviews given the
age of the CL.

[Theresa, 2015-05-08 20:25:49]

chrome/browser/image_decoder.* changes lgtm

[Anand Mistry (off Chromium), 2015-05-11 07:42:16]

Reviewers, ping!

reed: For //skia
sky: For //chrome
jam: For //content

[sky, 2015-05-11 15:07:17]

jam is an owner of chrome too. He should review both chrome and content as he'll
know the full context.

[jam, 2015-05-12 23:24:57]

lgtm with comment

https://codereview.chromium.org/1028543002/diff/350001/chrome/common/chrome_s...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/1028543002/diff/350001/chrome/common/chrome_s...
chrome/common/chrome_switches.cc:1393: bool MojoUtilityServicesEnabled() {
we don't add methods for checking if one switch is enabled. just put this
directly in the three callers

[Anand Mistry (off Chromium), 2015-05-13 04:35:12]

https://codereview.chromium.org/1028543002/diff/350001/chrome/common/chrome_s...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/1028543002/diff/350001/chrome/common/chrome_s...
chrome/common/chrome_switches.cc:1393: bool MojoUtilityServicesEnabled() {
On 2015/05/12 23:24:57, jam wrote:
> we don't add methods for checking if one switch is enabled. just put this
> directly in the three callers

The main reason I have this here is so that this can be disabled easily if a
regression is found in a release branch (and I can add an enable flag if
needed). The other reason is so that I can apply the same check (with same
ability to back out) to other utility process services I change.  Is that ok?

[Anand Mistry (off Chromium), 2015-05-13 05:44:05]

https://codereview.chromium.org/1028543002/diff/120001/skia/public/type_conve...
File skia/public/type_converters.cc (right):

https://codereview.chromium.org/1028543002/diff/120001/skia/public/type_conve...
skia/public/type_converters.cc:98: SkBitmap TypeConverter<SkBitmap,
skia::BitmapPtr>::Convert(
On 2015/04/10 02:56:27, Anand Mistry wrote:
> On 2015/04/09 08:35:30, Sam McNally wrote:
> > On 2015/04/09 05:25:03, Anand Mistry wrote:
> > > On 2015/04/08 08:09:00, Sam McNally wrote:
> > > > Why not take a skia::Bitmap?
> > > 
> > > Because almost nothing references it (or any other struct) in practice.
> > > Everything takes and gives StructPtr<> wrappers.
> > > 
> > > FYI, //mojo/converters appears to be all over the place with respect to
> this.
> > 
> > If the converter doesn't need to handle null input (or something fancy like
> > moving out of a StructPtr passed by value), I think we should prefer taking
> the
> > struct itself.
> 
> Ok. Done.

Sam: Actually, I had to revert this change. You need to use StructPtr<> in order
to use the type converter with an array, which is what leon's change needs to
do.

[Sam McNally, 2015-05-13 05:52:04]

https://codereview.chromium.org/1028543002/diff/120001/skia/public/type_conve...
File skia/public/type_converters.cc (right):

https://codereview.chromium.org/1028543002/diff/120001/skia/public/type_conve...
skia/public/type_converters.cc:98: SkBitmap TypeConverter<SkBitmap,
skia::BitmapPtr>::Convert(
On 2015/05/13 05:44:05, Anand Mistry wrote:
> On 2015/04/10 02:56:27, Anand Mistry wrote:
> > On 2015/04/09 08:35:30, Sam McNally wrote:
> > > On 2015/04/09 05:25:03, Anand Mistry wrote:
> > > > On 2015/04/08 08:09:00, Sam McNally wrote:
> > > > > Why not take a skia::Bitmap?
> > > > 
> > > > Because almost nothing references it (or any other struct) in practice.
> > > > Everything takes and gives StructPtr<> wrappers.
> > > > 
> > > > FYI, //mojo/converters appears to be all over the place with respect to
> > this.
> > > 
> > > If the converter doesn't need to handle null input (or something fancy
like
> > > moving out of a StructPtr passed by value), I think we should prefer
taking
> > the
> > > struct itself.
> > 
> > Ok. Done.
> 
> Sam: Actually, I had to revert this change. You need to use StructPtr<> in
order
> to use the type converter with an array, which is what leon's change needs to
> do.

OK, please handle the case of a null BitmapPtr.

[Anand Mistry (off Chromium), 2015-05-13 07:36:07]

https://codereview.chromium.org/1028543002/diff/120001/skia/public/type_conve...
File skia/public/type_converters.cc (right):

https://codereview.chromium.org/1028543002/diff/120001/skia/public/type_conve...
skia/public/type_converters.cc:98: SkBitmap TypeConverter<SkBitmap,
skia::BitmapPtr>::Convert(
On 2015/05/13 05:52:04, Sam McNally wrote:
> On 2015/05/13 05:44:05, Anand Mistry wrote:
> > On 2015/04/10 02:56:27, Anand Mistry wrote:
> > > On 2015/04/09 08:35:30, Sam McNally wrote:
> > > > On 2015/04/09 05:25:03, Anand Mistry wrote:
> > > > > On 2015/04/08 08:09:00, Sam McNally wrote:
> > > > > > Why not take a skia::Bitmap?
> > > > > 
> > > > > Because almost nothing references it (or any other struct) in
practice.
> > > > > Everything takes and gives StructPtr<> wrappers.
> > > > > 
> > > > > FYI, //mojo/converters appears to be all over the place with respect
to
> > > this.
> > > > 
> > > > If the converter doesn't need to handle null input (or something fancy
> like
> > > > moving out of a StructPtr passed by value), I think we should prefer
> taking
> > > the
> > > > struct itself.
> > > 
> > > Ok. Done.
> > 
> > Sam: Actually, I had to revert this change. You need to use StructPtr<> in
> order
> > to use the type converter with an array, which is what leon's change needs
to
> > do.
> 
> OK, please handle the case of a null BitmapPtr.

Done.

[reed1, 2015-05-14 15:50:39]

For the short-term I'm sure we can support this. Longer-term, I would feel
better moving the bottom part of this into Skia somehow, so that we can more
easily optimize and expand this as the api and internals of bitmaps evolve.
Would a skia api like this work for chrome?

SkBitmap -> byte_buffer
byte_buffer -> SkBitmap

https://codereview.chromium.org/1028543002/diff/410001/skia/public/type_conve...
File skia/public/type_converters.cc (right):

https://codereview.chromium.org/1028543002/diff/410001/skia/public/type_conve...
skia/public/type_converters.cc:127: SkAutoLockPixels lock(bitmap);
A couple of notes:

1. there is no code to handle Index8 colortable bitmaps here.
2. lockPixels can fail, with bitmap.getPixels() returning NULL.

3. bitmap.readPixels() is the modern way to get a copy of the pixels out of a
bitmap. It requires the caller to allocate space for the pixels ahead of time,
but that should not be a problem here.

[reed1, 2015-05-14 15:52:12]

https://codereview.chromium.org/1028543002/diff/410001/skia/public/type_conve...
File skia/public/type_converters.cc (right):

https://codereview.chromium.org/1028543002/diff/410001/skia/public/type_conve...
skia/public/type_converters.cc:130: memcpy(&result->pixel_data[0],
bitmap.getPixels(), size);
4. this explicitly is forcing a decode if the bitmap was backed by JPEG or PNG
or whatever. Is that required, or could we logically send the encoded form
through mojo? It could be much much smaller.

[Anand Mistry (off Chromium), 2015-05-15 06:51:23]

https://codereview.chromium.org/1028543002/diff/410001/skia/public/type_conve...
File skia/public/type_converters.cc (right):

https://codereview.chromium.org/1028543002/diff/410001/skia/public/type_conve...
skia/public/type_converters.cc:127: SkAutoLockPixels lock(bitmap);
On 2015/05/14 15:50:39, reed1 wrote:
> A couple of notes:
> 
> 1. there is no code to handle Index8 colortable bitmaps here.
> 2. lockPixels can fail, with bitmap.getPixels() returning NULL.
> 
> 3. bitmap.readPixels() is the modern way to get a copy of the pixels out of a
> bitmap. It requires the caller to allocate space for the pixels ahead of time,
> but that should not be a problem here.

The goal here is to serialize the SkBitmap without performing any conversions. I
think it's also worth noting that this code is pretty much a reflection of
what's in
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/ipc/gfx_par...,
which has all the issues you've mentioned. I'm not sure what the correct thing
to do here is. Since no existing IPC uses Index8 (since the current IPC doesn't
support it), I wonder if it's ok to punt on support for that until it's actually
needed.

https://codereview.chromium.org/1028543002/diff/410001/skia/public/type_conve...
skia/public/type_converters.cc:130: memcpy(&result->pixel_data[0],
bitmap.getPixels(), size);
On 2015/05/14 15:52:12, reed1 wrote:
> 4. this explicitly is forcing a decode if the bitmap was backed by JPEG or PNG
> or whatever. Is that required, or could we logically send the encoded form
> through mojo? It could be much much smaller.

Actually, we want to force a decode. The two immediate users of this (this CL
and https://codereview.chromium.org/1085783002/) do untrusted image decoding in
a sandboxed process. Even for any other user, I don't think we can trust passing
a JPEG or PNG here.

[Anand Mistry (off Chromium), 2015-05-15 07:32:50]

Ugh! This change might stall for a while. The android bot is failing, and I have
no idea why, or ability to inspect what's going on. I repo'd it locally and
fixed the issue, but the bot is still failing. If anyone has any insight into
how I can debug this, _please_ let me know.

[reed1, 2015-05-15 13:06:58]

On 2015/05/15 06:51:23, Anand Mistry wrote:
>
https://codereview.chromium.org/1028543002/diff/410001/skia/public/type_conve...
> File skia/public/type_converters.cc (right):
> 
>
https://codereview.chromium.org/1028543002/diff/410001/skia/public/type_conve...
> skia/public/type_converters.cc:127: SkAutoLockPixels lock(bitmap);
> On 2015/05/14 15:50:39, reed1 wrote:
> > A couple of notes:
> > 
> > 1. there is no code to handle Index8 colortable bitmaps here.
> > 2. lockPixels can fail, with bitmap.getPixels() returning NULL.
> > 
> > 3. bitmap.readPixels() is the modern way to get a copy of the pixels out of
a
> > bitmap. It requires the caller to allocate space for the pixels ahead of
time,
> > but that should not be a problem here.
> 
> The goal here is to serialize the SkBitmap without performing any conversions.
I
> think it's also worth noting that this code is pretty much a reflection of
> what's in
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/ipc/gfx_par...,
> which has all the issues you've mentioned. I'm not sure what the correct thing
> to do here is. Since no existing IPC uses Index8 (since the current IPC
doesn't
> support it), I wonder if it's ok to punt on support for that until it's
actually
> needed.
> 
>
https://codereview.chromium.org/1028543002/diff/410001/skia/public/type_conve...
> skia/public/type_converters.cc:130: memcpy(&result->pixel_data[0],
> bitmap.getPixels(), size);
> On 2015/05/14 15:52:12, reed1 wrote:
> > 4. this explicitly is forcing a decode if the bitmap was backed by JPEG or
PNG
> > or whatever. Is that required, or could we logically send the encoded form
> > through mojo? It could be much much smaller.
> 
> Actually, we want to force a decode. The two immediate users of this (this CL
> and https://codereview.chromium.org/1085783002/) do untrusted image decoding
in
> a sandboxed process. Even for any other user, I don't think we can trust
passing
> a JPEG or PNG here.

1. skipping index8 : sgtm, though perhaps it could assert or fail
2. I think the code should check to see if getPixels() is null before proceeding
3. This is a change I can propose later, so I'm fine with no change now (though
readPixels is more correct and just as efficient)
4. Ack.

[Anand Mistry (off Chromium), 2015-06-01 05:59:03]

On 2015/05/15 13:06:58, reed1 wrote:
> On 2015/05/15 06:51:23, Anand Mistry wrote:
> >
>
https://codereview.chromium.org/1028543002/diff/410001/skia/public/type_conve...
> > File skia/public/type_converters.cc (right):
> > 
> >
>
https://codereview.chromium.org/1028543002/diff/410001/skia/public/type_conve...
> > skia/public/type_converters.cc:127: SkAutoLockPixels lock(bitmap);
> > On 2015/05/14 15:50:39, reed1 wrote:
> > > A couple of notes:
> > > 
> > > 1. there is no code to handle Index8 colortable bitmaps here.
> > > 2. lockPixels can fail, with bitmap.getPixels() returning NULL.
> > > 
> > > 3. bitmap.readPixels() is the modern way to get a copy of the pixels out
of
> a
> > > bitmap. It requires the caller to allocate space for the pixels ahead of
> time,
> > > but that should not be a problem here.
> > 
> > The goal here is to serialize the SkBitmap without performing any
conversions.
> I
> > think it's also worth noting that this code is pretty much a reflection of
> > what's in
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/ipc/gfx_par...,
> > which has all the issues you've mentioned. I'm not sure what the correct
thing
> > to do here is. Since no existing IPC uses Index8 (since the current IPC
> doesn't
> > support it), I wonder if it's ok to punt on support for that until it's
> actually
> > needed.
> > 
> >
>
https://codereview.chromium.org/1028543002/diff/410001/skia/public/type_conve...
> > skia/public/type_converters.cc:130: memcpy(&result->pixel_data[0],
> > bitmap.getPixels(), size);
> > On 2015/05/14 15:52:12, reed1 wrote:
> > > 4. this explicitly is forcing a decode if the bitmap was backed by JPEG or
> PNG
> > > or whatever. Is that required, or could we logically send the encoded form
> > > through mojo? It could be much much smaller.
> > 
> > Actually, we want to force a decode. The two immediate users of this (this
CL
> > and https://codereview.chromium.org/1085783002/) do untrusted image decoding
> in
> > a sandboxed process. Even for any other user, I don't think we can trust
> passing
> > a JPEG or PNG here.
> 
> 1. skipping index8 : sgtm, though perhaps it could assert or fail
> 2. I think the code should check to see if getPixels() is null before
proceeding
> 3. This is a change I can propose later, so I'm fine with no change now
(though
> readPixels is more correct and just as efficient)
> 4. Ack.

Done. I'm going to split the skia stuff off this and send to you separately.
Until I can figure out what's happening with the Android bot, I can't submit the
rest.