Move ImageDecoder to components/image_fetcher/content

Move ImageDecoder to components/image_fetcher/content

This allows moving ImageFetcher's equivalent, ImageDecoderImpl to
components as well. Future CLs should probably clean up the naming
confusion and reduce the number of wrappers.

BUG=624761

[mastiz, 2017-03-21 16:48:11]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 16:48:38]

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

[mastiz, 2017-03-21 16:49:24]

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

[mastiz, 2017-03-21 16:49:25]

Not sure if this is yet ready to be reviewed.

[mastiz, 2017-03-21 16:49:36]

Description was changed from

==========
ima# Enter a description of the change.
Move ImageDecoder to components/image_fetcher/content

BUG=624761
==========

to

==========
Move ImageDecoder to components/image_fetcher/content

BUG=624761
==========

[commit-bot: I haz the power, 2017-03-21 16:55:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 16:55:10]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mastiz, 2017-03-22 09:14:53]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 09:15:17]

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

[mastiz, 2017-03-22 09:17:09]

Description was changed from

==========
Move ImageDecoder to components/image_fetcher/content

BUG=624761
==========

to

==========
Move ImageDecoder to components/image_fetcher/content

This allows moving ImageFetcher's equivalent, ImageDecoderImpl to
components as well. Future CLs should probably clean up the naming
confusion and reduce the number of wrappers.

BUG=624761
==========

[commit-bot: I haz the power, 2017-03-22 09:19:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 09:19:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mastiz, 2017-03-22 09:22:07]

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

[mastiz, 2017-03-22 09:22:08]

rockot@: you seem to be contributing to ImageDecoder? Any objection to moving it
to components/image_fetcher?

[mastiz, 2017-03-22 09:58:22]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 09:58:42]

Dry run: 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-03-22 10:02:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 10:02:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mastiz, 2017-03-22 11:50:30]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 11:50:52]

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

[Ken Rockot(use gerrit already), 2017-03-22 11:55:14]

I don't really care about it, but if it's going to require any amount of
refactoring effort I would prefer we just delete it and replace its usage
with direct image_decoder service usage. That's all it is under the hood
now anyway, with the addition of (AFAICT unnecessary) extra complexity.

On Mar 22, 2017 2:22 AM, <mastiz@chromium.org> wrote:

> rockot@: you seem to be contributing to ImageDecoder? Any objection to
> moving it
> to components/image_fetcher?
>
> https://codereview.chromium.org/2763103002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-03-22 11:55:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 11:55:31]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[mastiz, 2017-03-22 12:46:36]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 12:46:44]

Dry run: 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-03-22 12:56:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 12:56:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mastiz, 2017-03-22 13:20:22]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 13:20:38]

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

[mastiz, 2017-03-22 13:21:14]

The CQ bit was unchecked by mastiz@chromium.org

[mastiz, 2017-03-22 13:22:17]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 13:22:29]

Dry run: 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-03-22 13:36:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 13:37:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mastiz, 2017-03-23 08:30:06]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 08:30:22]

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

[markusheintz_, 2017-03-23 09:02:54]

We need some abstraction in between so that we can plug in a iOS specific
implementation of an image_decoder.

We have a interface (the image_decoder in core/) and a default implementation
for using the image_decoder service that allows to handle multiple requests in
flight.

For the clients of the chrome/browser/image_decoder.* we'll have to go through
each of them and see whether they should use the image_decoder service directly
or the image_fetcher component.

[commit-bot: I haz the power, 2017-03-23 09:53:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 09:53:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Rockot(use gerrit already), 2017-03-23 17:14:16]

On 2017/03/23 at 09:02:54, markusheintz wrote:
> We need some abstraction in between so that we can plug in a iOS specific
implementation of an image_decoder.
> 
> We have a interface (the image_decoder in core/) and a default implementation
for using the image_decoder service that allows to handle multiple requests in
flight.
> 
> For the clients of the chrome/browser/image_decoder.* we'll have to go through
each of them and see whether they should use the image_decoder service directly
or the image_fetcher component.

I see. That seems reasonable.

Would you mind waiting for https://codereview.chromium.org/2774463002 to land
and rebasing onto that? AFAICT this should only require a few small changes to
the moved image_decoder.cc. Otherwise your patch is not quite quite correct,
because image decoding will be broken for any non-Chrome content embedder that
tries using it. Obviously none use it today, so if you really want to get this
landed nothing will break yet.

[markusheintz_, 2017-03-23 17:20:13]

On 2017/03/23 17:14:16, Ken Rockot wrote:
> On 2017/03/23 at 09:02:54, markusheintz wrote:
> > We need some abstraction in between so that we can plug in a iOS specific
> implementation of an image_decoder.
> > 
> > We have a interface (the image_decoder in core/) and a default
implementation
> for using the image_decoder service that allows to handle multiple requests in
> flight.
> > 
> > For the clients of the chrome/browser/image_decoder.* we'll have to go
through
> each of them and see whether they should use the image_decoder service
directly
> or the image_fetcher component.
> 
> I see. That seems reasonable.
> 
> Would you mind waiting for https://codereview.chromium.org/2774463002 to land
> and rebasing onto that? AFAICT this should only require a few small changes to
> the moved image_decoder.cc. Otherwise your patch is not quite quite correct,
> because image decoding will be broken for any non-Chrome content embedder that
> tries using it. Obviously none use it today, so if you really want to get this
> landed nothing will break yet.

I can't really speak for mastiz@ but given that we are in Munich, you have at
least one full working day :-). I assume it is fine. You CL is quite a big one.

[Ken Rockot(use gerrit already), 2017-03-23 17:21:15]

Big but largely mechanical and low risk. I anticipate getting it landed
quickly. Thanks!

On Thu, Mar 23, 2017 at 10:20 AM, <markusheintz@chromium.org> wrote:

> On 2017/03/23 17:14:16, Ken Rockot wrote:
> > On 2017/03/23 at 09:02:54, markusheintz wrote:
> > > We need some abstraction in between so that we can plug in a iOS
> specific
> > implementation of an image_decoder.
> > >
> > > We have a interface (the image_decoder in core/) and a default
> implementation
> > for using the image_decoder service that allows to handle multiple
> requests in
> > flight.
> > >
> > > For the clients of the chrome/browser/image_decoder.* we'll have to go
> through
> > each of them and see whether they should use the image_decoder service
> directly
> > or the image_fetcher component.
> >
> > I see. That seems reasonable.
> >
> > Would you mind waiting for https://codereview.chromium.org/2774463002
> to land
> > and rebasing onto that? AFAICT this should only require a few small
> changes to
> > the moved image_decoder.cc. Otherwise your patch is not quite quite
> correct,
> > because image decoding will be broken for any non-Chrome content
> embedder that
> > tries using it. Obviously none use it today, so if you really want to
> get this
> > landed nothing will break yet.
>
> I can't really speak for mastiz@ but given that we are in Munich, you
> have at
> least one full working day :-). I assume it is fine. You CL is quite a big
> one.
>
> https://codereview.chromium.org/2763103002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mastiz, 2017-03-27 12:27:50]

On 2017/03/23 17:21:15, Ken Rockot wrote:
> Big but largely mechanical and low risk. I anticipate getting it landed
> quickly. Thanks!
> 
> On Thu, Mar 23, 2017 at 10:20 AM, <mailto:markusheintz@chromium.org> wrote:
> 
> > On 2017/03/23 17:14:16, Ken Rockot wrote:
> > > On 2017/03/23 at 09:02:54, markusheintz wrote:
> > > > We need some abstraction in between so that we can plug in a iOS
> > specific
> > > implementation of an image_decoder.
> > > >
> > > > We have a interface (the image_decoder in core/) and a default
> > implementation
> > > for using the image_decoder service that allows to handle multiple
> > requests in
> > > flight.
> > > >
> > > > For the clients of the chrome/browser/image_decoder.* we'll have to go
> > through
> > > each of them and see whether they should use the image_decoder service
> > directly
> > > or the image_fetcher component.
> > >
> > > I see. That seems reasonable.
> > >
> > > Would you mind waiting for https://codereview.chromium.org/2774463002
> > to land
> > > and rebasing onto that? AFAICT this should only require a few small
> > changes to
> > > the moved image_decoder.cc. Otherwise your patch is not quite quite
> > correct,
> > > because image decoding will be broken for any non-Chrome content
> > embedder that
> > > tries using it. Obviously none use it today, so if you really want to
> > get this
> > > landed nothing will break yet.
> >
> > I can't really speak for mastiz@ but given that we are in Munich, you
> > have at
> > least one full working day :-). I assume it is fine. You CL is quite a big
> > one.
> >
> > https://codereview.chromium.org/2763103002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Consider there's not much interest in this particular patch, and it's not on my
way, I'm tempted to drop it.

[markusheintz_, 2017-03-27 12:55:47]

On 2017/03/27 12:27:50, mastiz wrote:
> On 2017/03/23 17:21:15, Ken Rockot wrote:
> > Big but largely mechanical and low risk. I anticipate getting it landed
> > quickly. Thanks!
> > 
> > On Thu, Mar 23, 2017 at 10:20 AM, <mailto:markusheintz@chromium.org> wrote:
> > 
> > > On 2017/03/23 17:14:16, Ken Rockot wrote:
> > > > On 2017/03/23 at 09:02:54, markusheintz wrote:
> > > > > We need some abstraction in between so that we can plug in a iOS
> > > specific
> > > > implementation of an image_decoder.
> > > > >
> > > > > We have a interface (the image_decoder in core/) and a default
> > > implementation
> > > > for using the image_decoder service that allows to handle multiple
> > > requests in
> > > > flight.
> > > > >
> > > > > For the clients of the chrome/browser/image_decoder.* we'll have to go
> > > through
> > > > each of them and see whether they should use the image_decoder service
> > > directly
> > > > or the image_fetcher component.
> > > >
> > > > I see. That seems reasonable.
> > > >
> > > > Would you mind waiting for https://codereview.chromium.org/2774463002
> > > to land
> > > > and rebasing onto that? AFAICT this should only require a few small
> > > changes to
> > > > the moved image_decoder.cc. Otherwise your patch is not quite quite
> > > correct,
> > > > because image decoding will be broken for any non-Chrome content
> > > embedder that
> > > > tries using it. Obviously none use it today, so if you really want to
> > > get this
> > > > landed nothing will break yet.
> > >
> > > I can't really speak for mastiz@ but given that we are in Munich, you
> > > have at
> > > least one full working day :-). I assume it is fine. You CL is quite a big
> > > one.
> > >
> > > https://codereview.chromium.org/2763103002/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Consider there's not much interest in this particular patch, and it's not on
my
> way, I'm tempted to drop it.

Please don't drop the CL :-) Landing the CL would be much appreciated on my end.

It would be great to get all image_decoder related stuff into the image_fetcher
component. This would help to clean up the code.

[markusheintz_, 2017-03-27 12:55:53]

On 2017/03/27 12:27:50, mastiz wrote:
> On 2017/03/23 17:21:15, Ken Rockot wrote:
> > Big but largely mechanical and low risk. I anticipate getting it landed
> > quickly. Thanks!
> > 
> > On Thu, Mar 23, 2017 at 10:20 AM, <mailto:markusheintz@chromium.org> wrote:
> > 
> > > On 2017/03/23 17:14:16, Ken Rockot wrote:
> > > > On 2017/03/23 at 09:02:54, markusheintz wrote:
> > > > > We need some abstraction in between so that we can plug in a iOS
> > > specific
> > > > implementation of an image_decoder.
> > > > >
> > > > > We have a interface (the image_decoder in core/) and a default
> > > implementation
> > > > for using the image_decoder service that allows to handle multiple
> > > requests in
> > > > flight.
> > > > >
> > > > > For the clients of the chrome/browser/image_decoder.* we'll have to go
> > > through
> > > > each of them and see whether they should use the image_decoder service
> > > directly
> > > > or the image_fetcher component.
> > > >
> > > > I see. That seems reasonable.
> > > >
> > > > Would you mind waiting for https://codereview.chromium.org/2774463002
> > > to land
> > > > and rebasing onto that? AFAICT this should only require a few small
> > > changes to
> > > > the moved image_decoder.cc. Otherwise your patch is not quite quite
> > > correct,
> > > > because image decoding will be broken for any non-Chrome content
> > > embedder that
> > > > tries using it. Obviously none use it today, so if you really want to
> > > get this
> > > > landed nothing will break yet.
> > >
> > > I can't really speak for mastiz@ but given that we are in Munich, you
> > > have at
> > > least one full working day :-). I assume it is fine. You CL is quite a big
> > > one.
> > >
> > > https://codereview.chromium.org/2763103002/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Consider there's not much interest in this particular patch, and it's not on
my
> way, I'm tempted to drop it.

Please don't drop the CL :-) Landing the CL would be much appreciated on my end.

It would be great to get all image_decoder related stuff into the image_fetcher
component. This would help to clean up the code.

[markusheintz_, 2017-03-27 12:55:53]

On 2017/03/27 12:27:50, mastiz wrote:
> On 2017/03/23 17:21:15, Ken Rockot wrote:
> > Big but largely mechanical and low risk. I anticipate getting it landed
> > quickly. Thanks!
> > 
> > On Thu, Mar 23, 2017 at 10:20 AM, <mailto:markusheintz@chromium.org> wrote:
> > 
> > > On 2017/03/23 17:14:16, Ken Rockot wrote:
> > > > On 2017/03/23 at 09:02:54, markusheintz wrote:
> > > > > We need some abstraction in between so that we can plug in a iOS
> > > specific
> > > > implementation of an image_decoder.
> > > > >
> > > > > We have a interface (the image_decoder in core/) and a default
> > > implementation
> > > > for using the image_decoder service that allows to handle multiple
> > > requests in
> > > > flight.
> > > > >
> > > > > For the clients of the chrome/browser/image_decoder.* we'll have to go
> > > through
> > > > each of them and see whether they should use the image_decoder service
> > > directly
> > > > or the image_fetcher component.
> > > >
> > > > I see. That seems reasonable.
> > > >
> > > > Would you mind waiting for https://codereview.chromium.org/2774463002
> > > to land
> > > > and rebasing onto that? AFAICT this should only require a few small
> > > changes to
> > > > the moved image_decoder.cc. Otherwise your patch is not quite quite
> > > correct,
> > > > because image decoding will be broken for any non-Chrome content
> > > embedder that
> > > > tries using it. Obviously none use it today, so if you really want to
> > > get this
> > > > landed nothing will break yet.
> > >
> > > I can't really speak for mastiz@ but given that we are in Munich, you
> > > have at
> > > least one full working day :-). I assume it is fine. You CL is quite a big
> > > one.
> > >
> > > https://codereview.chromium.org/2763103002/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Consider there's not much interest in this particular patch, and it's not on
my
> way, I'm tempted to drop it.

Please don't drop the CL :-) Landing the CL would be much appreciated on my end.

It would be great to get all image_decoder related stuff into the image_fetcher
component. This would help to clean up the code.

[markusheintz_, 2017-03-27 12:55:54]

On 2017/03/27 12:27:50, mastiz wrote:
> On 2017/03/23 17:21:15, Ken Rockot wrote:
> > Big but largely mechanical and low risk. I anticipate getting it landed
> > quickly. Thanks!
> > 
> > On Thu, Mar 23, 2017 at 10:20 AM, <mailto:markusheintz@chromium.org> wrote:
> > 
> > > On 2017/03/23 17:14:16, Ken Rockot wrote:
> > > > On 2017/03/23 at 09:02:54, markusheintz wrote:
> > > > > We need some abstraction in between so that we can plug in a iOS
> > > specific
> > > > implementation of an image_decoder.
> > > > >
> > > > > We have a interface (the image_decoder in core/) and a default
> > > implementation
> > > > for using the image_decoder service that allows to handle multiple
> > > requests in
> > > > flight.
> > > > >
> > > > > For the clients of the chrome/browser/image_decoder.* we'll have to go
> > > through
> > > > each of them and see whether they should use the image_decoder service
> > > directly
> > > > or the image_fetcher component.
> > > >
> > > > I see. That seems reasonable.
> > > >
> > > > Would you mind waiting for https://codereview.chromium.org/2774463002
> > > to land
> > > > and rebasing onto that? AFAICT this should only require a few small
> > > changes to
> > > > the moved image_decoder.cc. Otherwise your patch is not quite quite
> > > correct,
> > > > because image decoding will be broken for any non-Chrome content
> > > embedder that
> > > > tries using it. Obviously none use it today, so if you really want to
> > > get this
> > > > landed nothing will break yet.
> > >
> > > I can't really speak for mastiz@ but given that we are in Munich, you
> > > have at
> > > least one full working day :-). I assume it is fine. You CL is quite a big
> > > one.
> > >
> > > https://codereview.chromium.org/2763103002/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Consider there's not much interest in this particular patch, and it's not on
my
> way, I'm tempted to drop it.

Please don't drop the CL :-) Landing the CL would be much appreciated on my end.

It would be great to get all image_decoder related stuff into the image_fetcher
component. This would help to clean up the code.

[mastiz, 2017-03-27 14:39:16]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 14:39:38]

Dry run: 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-03-27 15:35:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 15:35:37]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2017-03-27 18:27:36]

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

[Ken Rockot(use gerrit already), 2017-03-27 18:27:37]

Maybe this is a discussion for another time, but I wonder if we should just have
the in-process Service Manager running on iOS. The SM and its client library are
lightweight and depend only on //mojo and //base, both of which are already used
on iOS, and this presents an opportunity to simplify how we approach the iOS
port of various features.

+blundell@ as the originator (or one of the originators) of the whole iOS
componentization effort for thoughts.

We could trivially configure the data_decoder service to run in-process on iOS,
and it could have its own private iOS implementation of its public ImageDecoder
interface.

Then all code which needs to decode an image can do so by connecting to the
data_decoder service, and we simplify the layering.

With this CL we'd end up having:

(all platforms) image_fetcher::ImageDecoder
  |
  |_ (non-iOS) Content image_fetcher::ImageDecoder impl
  |  |
  |  |_ Connect to data_decoder service, request ImageDecoder interface
  |     |
  |     |_ Blink data_decoder::ImageDecoder impl
  |
  |_ (iOS) iOS image_fetcher::ImageDecoder impl


and this would instead become:


(all platforms) Connect to data_decoder service, request ImageDecoder interface
  |
  |_ (non-iOS) Blink data_decoder::ImageDecoder impl
  |
  |_ (iOS) iOS data_decoder::ImageDecoder impl


The layered components approach makes a lot of sense when content/ is still the
chokepoint through which most non-iOS browser features are exposed, but we're
trying to remove content from the picture as much as possible. It certainly
doesn't make sense for the concept of image decoding to be tied to content if we
can avoid it.

[mastiz, 2017-03-27 19:57:40]

On 2017/03/27 18:27:37, Ken Rockot wrote:
> Maybe this is a discussion for another time, but I wonder if we should just
have
> the in-process Service Manager running on iOS. The SM and its client library
are
> lightweight and depend only on //mojo and //base, both of which are already
used
> on iOS, and this presents an opportunity to simplify how we approach the iOS
> port of various features.
> 
> +blundell@ as the originator (or one of the originators) of the whole iOS
> componentization effort for thoughts.
> 
> We could trivially configure the data_decoder service to run in-process on
iOS,
> and it could have its own private iOS implementation of its public
ImageDecoder
> interface.
> 
> Then all code which needs to decode an image can do so by connecting to the
> data_decoder service, and we simplify the layering.
> 
> With this CL we'd end up having:
> 
> (all platforms) image_fetcher::ImageDecoder
>   |
>   |_ (non-iOS) Content image_fetcher::ImageDecoder impl
>   |  |
>   |  |_ Connect to data_decoder service, request ImageDecoder interface
>   |     |
>   |     |_ Blink data_decoder::ImageDecoder impl
>   |
>   |_ (iOS) iOS image_fetcher::ImageDecoder impl
> 
> 
> and this would instead become:
> 
> 
> (all platforms) Connect to data_decoder service, request ImageDecoder
interface
>   |
>   |_ (non-iOS) Blink data_decoder::ImageDecoder impl
>   |
>   |_ (iOS) iOS data_decoder::ImageDecoder impl
> 
> 
> The layered components approach makes a lot of sense when content/ is still
the
> chokepoint through which most non-iOS browser features are exposed, but we're
> trying to remove content from the picture as much as possible. It certainly
> doesn't make sense for the concept of image decoding to be tied to content if
we
> can avoid it.

Thanks for the detailed proposal and blundell@ to follow up!

That discussion is certainly beyond my knowledge and CL scope. Depending on the
expected timing, I believe this patch would make sense as a transitional state,
since right now the component is half-layered (introducing ios/ was important to
break cyclic dependencies). Once the proposal above was finalized, the layering
could always be reverted.

On the other hand, as I mentioned earlier, dropping this patch is not something
I care about much.

blundell@, let us know what you think :-)

[blundell, 2017-03-28 14:24:48]

On 2017/03/27 18:27:37, Ken Rockot wrote:
> Maybe this is a discussion for another time, but I wonder if we should just
have
> the in-process Service Manager running on iOS. The SM and its client library
are
> lightweight and depend only on //mojo and //base, both of which are already
used
> on iOS, and this presents an opportunity to simplify how we approach the iOS
> port of various features.
> 
> +blundell@ as the originator (or one of the originators) of the whole iOS
> componentization effort for thoughts.
> 
> We could trivially configure the data_decoder service to run in-process on
iOS,
> and it could have its own private iOS implementation of its public
ImageDecoder
> interface.
> 
> Then all code which needs to decode an image can do so by connecting to the
> data_decoder service, and we simplify the layering.
> 
> With this CL we'd end up having:
> 
> (all platforms) image_fetcher::ImageDecoder
>   |
>   |_ (non-iOS) Content image_fetcher::ImageDecoder impl
>   |  |
>   |  |_ Connect to data_decoder service, request ImageDecoder interface
>   |     |
>   |     |_ Blink data_decoder::ImageDecoder impl
>   |
>   |_ (iOS) iOS image_fetcher::ImageDecoder impl
> 
> 
> and this would instead become:
> 
> 
> (all platforms) Connect to data_decoder service, request ImageDecoder
interface
>   |
>   |_ (non-iOS) Blink data_decoder::ImageDecoder impl
>   |
>   |_ (iOS) iOS data_decoder::ImageDecoder impl
> 
> 
> The layered components approach makes a lot of sense when content/ is still
the
> chokepoint through which most non-iOS browser features are exposed, but we're
> trying to remove content from the picture as much as possible. It certainly
> doesn't make sense for the concept of image decoding to be tied to content if
we
> can avoid it.

This is all 100% correct. Getting started on this has been in the back of my
mind for a while, but I didn't realize how quickly we'd approach the point of
components becoming layered simply due to connecting to a service. Filed
crbug.com/705982 to track, and am going to start some discussions about how to
prioritize.

[blundell, 2017-03-28 14:26:34]

On 2017/03/27 19:57:40, mastiz wrote:
> On 2017/03/27 18:27:37, Ken Rockot wrote:
> > Maybe this is a discussion for another time, but I wonder if we should just
> have
> > the in-process Service Manager running on iOS. The SM and its client library
> are
> > lightweight and depend only on //mojo and //base, both of which are already
> used
> > on iOS, and this presents an opportunity to simplify how we approach the iOS
> > port of various features.
> > 
> > +blundell@ as the originator (or one of the originators) of the whole iOS
> > componentization effort for thoughts.
> > 
> > We could trivially configure the data_decoder service to run in-process on
> iOS,
> > and it could have its own private iOS implementation of its public
> ImageDecoder
> > interface.
> > 
> > Then all code which needs to decode an image can do so by connecting to the
> > data_decoder service, and we simplify the layering.
> > 
> > With this CL we'd end up having:
> > 
> > (all platforms) image_fetcher::ImageDecoder
> >   |
> >   |_ (non-iOS) Content image_fetcher::ImageDecoder impl
> >   |  |
> >   |  |_ Connect to data_decoder service, request ImageDecoder interface
> >   |     |
> >   |     |_ Blink data_decoder::ImageDecoder impl
> >   |
> >   |_ (iOS) iOS image_fetcher::ImageDecoder impl
> > 
> > 
> > and this would instead become:
> > 
> > 
> > (all platforms) Connect to data_decoder service, request ImageDecoder
> interface
> >   |
> >   |_ (non-iOS) Blink data_decoder::ImageDecoder impl
> >   |
> >   |_ (iOS) iOS data_decoder::ImageDecoder impl
> > 
> > 
> > The layered components approach makes a lot of sense when content/ is still
> the
> > chokepoint through which most non-iOS browser features are exposed, but
we're
> > trying to remove content from the picture as much as possible. It certainly
> > doesn't make sense for the concept of image decoding to be tied to content
if
> we
> > can avoid it.
> 
> Thanks for the detailed proposal and blundell@ to follow up!
> 
> That discussion is certainly beyond my knowledge and CL scope. Depending on
the
> expected timing, I believe this patch would make sense as a transitional
state,
> since right now the component is half-layered (introducing ios/ was important
to
> break cyclic dependencies). Once the proposal above was finalized, the
layering
> could always be reverted.
> 
> On the other hand, as I mentioned earlier, dropping this patch is not
something
> I care about much.
> 
> blundell@, let us know what you think :-)

If you don't need to land this patch for any concrete reason, I'd prefer we
didn't. I'd like to avoid as much as possible introducing the notion that
"connecting to services" means "should be in content/ portion of a layered
component."

If you have a driving need for this patch (which it sounds like you don't?), you
should feel free to go ahead though, as it's going to take some time to get to
the state that we'd like to be in (i.e., what Ken outlined above).

[markusheintz_, 2017-03-28 17:25:22]

On 2017/03/28 14:26:34, blundell wrote:
> On 2017/03/27 19:57:40, mastiz wrote:
> > On 2017/03/27 18:27:37, Ken Rockot wrote:
> > > Maybe this is a discussion for another time, but I wonder if we should
just
> > have
> > > the in-process Service Manager running on iOS. The SM and its client
library
> > are
> > > lightweight and depend only on //mojo and //base, both of which are
already
> > used
> > > on iOS, and this presents an opportunity to simplify how we approach the
iOS
> > > port of various features.
> > > 
> > > +blundell@ as the originator (or one of the originators) of the whole iOS
> > > componentization effort for thoughts.
> > > 
> > > We could trivially configure the data_decoder service to run in-process on
> > iOS,
> > > and it could have its own private iOS implementation of its public
> > ImageDecoder
> > > interface.
> > > 
> > > Then all code which needs to decode an image can do so by connecting to
the
> > > data_decoder service, and we simplify the layering.
> > > 
> > > With this CL we'd end up having:
> > > 
> > > (all platforms) image_fetcher::ImageDecoder
> > >   |
> > >   |_ (non-iOS) Content image_fetcher::ImageDecoder impl
> > >   |  |
> > >   |  |_ Connect to data_decoder service, request ImageDecoder interface
> > >   |     |
> > >   |     |_ Blink data_decoder::ImageDecoder impl
> > >   |
> > >   |_ (iOS) iOS image_fetcher::ImageDecoder impl
> > > 
> > > 
> > > and this would instead become:
> > > 
> > > 
> > > (all platforms) Connect to data_decoder service, request ImageDecoder
> > interface
> > >   |
> > >   |_ (non-iOS) Blink data_decoder::ImageDecoder impl
> > >   |
> > >   |_ (iOS) iOS data_decoder::ImageDecoder impl
> > > 
> > > 
> > > The layered components approach makes a lot of sense when content/ is
still
> > the
> > > chokepoint through which most non-iOS browser features are exposed, but
> we're
> > > trying to remove content from the picture as much as possible. It
certainly
> > > doesn't make sense for the concept of image decoding to be tied to content
> if
> > we
> > > can avoid it.
> > 
> > Thanks for the detailed proposal and blundell@ to follow up!
> > 
> > That discussion is certainly beyond my knowledge and CL scope. Depending on
> the
> > expected timing, I believe this patch would make sense as a transitional
> state,
> > since right now the component is half-layered (introducing ios/ was
important
> to
> > break cyclic dependencies). Once the proposal above was finalized, the
> layering
> > could always be reverted.
> > 
> > On the other hand, as I mentioned earlier, dropping this patch is not
> something
> > I care about much.
> > 
> > blundell@, let us know what you think :-)
> 
> If you don't need to land this patch for any concrete reason, I'd prefer we
> didn't. I'd like to avoid as much as possible introducing the notion that
> "connecting to services" means "should be in content/ portion of a layered
> component."
> 
> If you have a driving need for this patch (which it sounds like you don't?),
you
> should feel free to go ahead though, as it's going to take some time to get to
> the state that we'd like to be in (i.e., what Ken outlined above).

Ok then we will hold off and not land this CL