components/image_fetcher: Expose RequestMetadata from ImageFetcher

components/image_fetcher: Expose RequestMetadata from ImageFetcher

Before this CL, the metadata was returned by ImageDataFetcher, but not
forwarded by ImageFetcher to its clients. Now it is passed on from
ImageFetcher too.

So far, no client uses the metadata, but components/doodle will use it
soon, to detect if an image request was served from the http cache.
See also https://codereview.chromium.org/2749283002/.

TBRing mechanical changes to hats_notification_controller and logo_bridge
TBR=achuith@chromium.org,bauerb@chromium.org

BUG=690467
Review-Url: https://codereview.chromium.org/2757643002
Cr-Commit-Position: refs/heads/master@{#458048}
Committed: https://chromium.googlesource.com/chromium/src/+/0a6cfc6ed4370aa3f4974ac7ed0e1009e0f9687e

[Marc Treib, 2017-03-16 15:46:25]

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

[Marc Treib, 2017-03-16 15:46:26]

PTAL!

[Marc Treib, 2017-03-16 15:47:23]

Description was changed from

==========
components/image_fetcher: Expose RequestMetadata from ImageFetcher

Before this CL, the metadata was returned by ImageDataFetcher, but not
forwarded by ImageFetcher to its clients. Now it is passed on from
ImageFetcher too.

BUG=690467
==========

to

==========
components/image_fetcher: Expose RequestMetadata from ImageFetcher

Before this CL, the metadata was returned by ImageDataFetcher, but not
forwarded by ImageFetcher to its clients. Now it is passed on from
ImageFetcher too.

So far, no client uses the metadata, but components/doodle will use it
soon, to detect if an image request was served from the http cache.
See also https://codereview.chromium.org/2749283002/.

BUG=690467
==========

[Marc Treib, 2017-03-17 12:10:28]

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

[commit-bot: I haz the power, 2017-03-17 12:10:40]

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-17 12:30:03]

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

[commit-bot: I haz the power, 2017-03-17 12:30:04]

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-...)

[Marc Treib, 2017-03-17 12:56:15]

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

[commit-bot: I haz the power, 2017-03-17 12:56:31]

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

[Marc Treib, 2017-03-17 12:57:58]

Description was changed from

==========
components/image_fetcher: Expose RequestMetadata from ImageFetcher

Before this CL, the metadata was returned by ImageDataFetcher, but not
forwarded by ImageFetcher to its clients. Now it is passed on from
ImageFetcher too.

So far, no client uses the metadata, but components/doodle will use it
soon, to detect if an image request was served from the http cache.
See also https://codereview.chromium.org/2749283002/.

BUG=690467
==========

to

==========
components/image_fetcher: Expose RequestMetadata from ImageFetcher

Before this CL, the metadata was returned by ImageDataFetcher, but not
forwarded by ImageFetcher to its clients. Now it is passed on from
ImageFetcher too.

So far, no client uses the metadata, but components/doodle will use it
soon, to detect if an image request was served from the http cache.
See also https://codereview.chromium.org/2749283002/.

TBRing mechanical changes to hats_notification_controller
TBR=achuith@chromium.org

BUG=690467
==========

[Marc Treib, 2017-03-17 12:58:50]

Ping!

[commit-bot: I haz the power, 2017-03-17 14:02:24]

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

[commit-bot: I haz the power, 2017-03-17 14:02:25]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-03-20 10:18:45]

treib@chromium.org changed reviewers:
+ jkrcal@chromium.org

[Marc Treib, 2017-03-20 10:18:46]

Jan, could you take a look, since Markus is out? Thanks!

[jkrcal, 2017-03-20 10:49:59]

lgtm, thanks!

https://codereview.chromium.org/2757643002/diff/40001/components/image_fetche...
File components/image_fetcher/image_fetcher.h (right):

https://codereview.chromium.org/2757643002/diff/40001/components/image_fetche...
components/image_fetcher/image_fetcher.h:37: const RequestMetadata& metadata)>;
A nice cleanup!

https://codereview.chromium.org/2757643002/diff/40001/components/image_fetche...
File components/image_fetcher/image_fetcher_impl.cc (right):

https://codereview.chromium.org/2757643002/diff/40001/components/image_fetche...
components/image_fetcher/image_fetcher_impl.cc:95: callback.Run(request->id,
image, metadata);
nit: I do not like the order of args being inconsistent. I see why this private
function needs this order. Is there a reason for the callback having the order
the other way around?

[Marc Treib, 2017-03-20 10:56:08]

https://codereview.chromium.org/2757643002/diff/40001/components/image_fetche...
File components/image_fetcher/image_fetcher_impl.cc (right):

https://codereview.chromium.org/2757643002/diff/40001/components/image_fetche...
components/image_fetcher/image_fetcher_impl.cc:95: callback.Run(request->id,
image, metadata);
On 2017/03/20 10:49:59, jkrcal wrote:
> nit: I do not like the order of args being inconsistent. I see why this
private
> function needs this order. Is there a reason for the callback having the order
> the other way around?

The order in the callback is consistent with the corresponding callback in
ImageDataFetcher:
https://cs.chromium.org/chromium/src/components/image_fetcher/image_data_fetc...

I think this order makes more sense: First the actual result (image), then the
extra info. It's unfortunate that the private function needs things in a
different order, but that should have no influence on the public API.

[jkrcal, 2017-03-20 11:02:20]

lgtm

https://codereview.chromium.org/2757643002/diff/40001/components/image_fetche...
File components/image_fetcher/image_fetcher_impl.cc (right):

https://codereview.chromium.org/2757643002/diff/40001/components/image_fetche...
components/image_fetcher/image_fetcher_impl.cc:95: callback.Run(request->id,
image, metadata);
On 2017/03/20 10:56:08, Marc Treib wrote:
> On 2017/03/20 10:49:59, jkrcal wrote:
> > nit: I do not like the order of args being inconsistent. I see why this
> private
> > function needs this order. Is there a reason for the callback having the
order
> > the other way around?
> 
> The order in the callback is consistent with the corresponding callback in
> ImageDataFetcher:
>
https://cs.chromium.org/chromium/src/components/image_fetcher/image_data_fetc...
> 
> I think this order makes more sense: First the actual result (image), then the
> extra info. It's unfortunate that the private function needs things in a
> different order, but that should have no influence on the public API.

Ok, makes sense. lgtm

[Marc Treib, 2017-03-20 11:49:17]

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

[commit-bot: I haz the power, 2017-03-20 11:49:28]

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

[Marc Treib, 2017-03-20 11:49:32]

Description was changed from

==========
components/image_fetcher: Expose RequestMetadata from ImageFetcher

Before this CL, the metadata was returned by ImageDataFetcher, but not
forwarded by ImageFetcher to its clients. Now it is passed on from
ImageFetcher too.

So far, no client uses the metadata, but components/doodle will use it
soon, to detect if an image request was served from the http cache.
See also https://codereview.chromium.org/2749283002/.

TBRing mechanical changes to hats_notification_controller
TBR=achuith@chromium.org

BUG=690467
==========

to

==========
components/image_fetcher: Expose RequestMetadata from ImageFetcher

Before this CL, the metadata was returned by ImageDataFetcher, but not
forwarded by ImageFetcher to its clients. Now it is passed on from
ImageFetcher too.

So far, no client uses the metadata, but components/doodle will use it
soon, to detect if an image request was served from the http cache.
See also https://codereview.chromium.org/2749283002/.

TBRing mechanical changes to hats_notification_controller and logo_bridge
TBR=achuith@chromium.org,bauerb@chromium.org

BUG=690467
==========

[commit-bot: I haz the power, 2017-03-20 12:51:04]

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

[commit-bot: I haz the power, 2017-03-20 12:51:05]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-03-20 13:04:49]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2017-03-20 13:04:49]

The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org
Link to the patchset: https://codereview.chromium.org/2757643002/#ps80001
(title: "logo_bridge")

[commit-bot: I haz the power, 2017-03-20 13:04:54]

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-20 13:10:34]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490015089040860,
"parent_rev": "e32318796eaf782ee8afe8ec047b1288f756f644", "commit_rev":
"0a6cfc6ed4370aa3f4974ac7ed0e1009e0f9687e"}

[commit-bot: I haz the power, 2017-03-20 13:11:15]

Description was changed from

==========
components/image_fetcher: Expose RequestMetadata from ImageFetcher

Before this CL, the metadata was returned by ImageDataFetcher, but not
forwarded by ImageFetcher to its clients. Now it is passed on from
ImageFetcher too.

So far, no client uses the metadata, but components/doodle will use it
soon, to detect if an image request was served from the http cache.
See also https://codereview.chromium.org/2749283002/.

TBRing mechanical changes to hats_notification_controller and logo_bridge
TBR=achuith@chromium.org,bauerb@chromium.org

BUG=690467
==========

to

==========
components/image_fetcher: Expose RequestMetadata from ImageFetcher

Before this CL, the metadata was returned by ImageDataFetcher, but not
forwarded by ImageFetcher to its clients. Now it is passed on from
ImageFetcher too.

So far, no client uses the metadata, but components/doodle will use it
soon, to detect if an image request was served from the http cache.
See also https://codereview.chromium.org/2749283002/.

TBRing mechanical changes to hats_notification_controller and logo_bridge
TBR=achuith@chromium.org,bauerb@chromium.org

BUG=690467

Review-Url: https://codereview.chromium.org/2757643002
Cr-Commit-Position: refs/heads/master@{#458048}
Committed:
https://chromium.googlesource.com/chromium/src/+/0a6cfc6ed4370aa3f4974ac7ed0e...
==========

[commit-bot: I haz the power, 2017-03-20 13:11:16]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/0a6cfc6ed4370aa3f4974ac7ed0e...

[markusheintz_, 2017-03-23 09:13:28]

On 2017/03/20 13:11:16, commit-bot: I haz the power wrote:
> Committed patchset #5 (id:80001) as
>
https://chromium.googlesource.com/chromium/src/+/0a6cfc6ed4370aa3f4974ac7ed0e...

Sorry for beeing late to the party.

I wonder whether we can simply expose a flag served_from_cache or something
similar instead of the whole metadata.
Is there anything that would prevent this?

[Marc Treib, 2017-03-23 09:18:43]

On 2017/03/23 09:13:28, markusheintz_ wrote:
> On 2017/03/20 13:11:16, commit-bot: I haz the power wrote:
> > Committed patchset #5 (id:80001) as
> >
>
https://chromium.googlesource.com/chromium/src/+/0a6cfc6ed4370aa3f4974ac7ed0e...
> 
> Sorry for beeing late to the party.
> 
> I wonder whether we can simply expose a flag served_from_cache or something
> similar instead of the whole metadata.
> Is there anything that would prevent this?

Reasons for exposing all the request metadata:
- Future proof: When someone needs one more flag in the future, we won't have to
update all clients again.
- Consistency with what ImageDataFetcher does.

And I don't see a reason for *not* handing out the whole metadata - do you have
any particular concerns?

[jkrcal, 2017-03-23 09:20:34]

On 2017/03/23 09:18:43, Marc Treib wrote:
> On 2017/03/23 09:13:28, markusheintz_ wrote:
> > On 2017/03/20 13:11:16, commit-bot: I haz the power wrote:
> > > Committed patchset #5 (id:80001) as
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/0a6cfc6ed4370aa3f4974ac7ed0e...
> > 
> > Sorry for beeing late to the party.
> > 
> > I wonder whether we can simply expose a flag served_from_cache or something
> > similar instead of the whole metadata.
> > Is there anything that would prevent this?
> 
> Reasons for exposing all the request metadata:
> - Future proof: When someone needs one more flag in the future, we won't have
to
> update all clients again.
> - Consistency with what ImageDataFetcher does.
> 
> And I don't see a reason for *not* handing out the whole metadata - do you
have
> any particular concerns?

I prefer the current way as I will need to expose response headers to the
client. 
Exposing RequestMetadata would simplify my job heavily.

[markusheintz_, 2017-03-23 10:22:15]

On 2017/03/23 09:20:34, jkrcal wrote:
> On 2017/03/23 09:18:43, Marc Treib wrote:
> > On 2017/03/23 09:13:28, markusheintz_ wrote:
> > > On 2017/03/20 13:11:16, commit-bot: I haz the power wrote:
> > > > Committed patchset #5 (id:80001) as
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/0a6cfc6ed4370aa3f4974ac7ed0e...
> > > 
> > > Sorry for beeing late to the party.
> > > 
> > > I wonder whether we can simply expose a flag served_from_cache or
something
> > > similar instead of the whole metadata.
> > > Is there anything that would prevent this?
> > 
> > Reasons for exposing all the request metadata:
> > - Future proof: When someone needs one more flag in the future, we won't
have
> to
> > update all clients again.
> > - Consistency with what ImageDataFetcher does.
> > 
> > And I don't see a reason for *not* handing out the whole metadata - do you
> have
> > any particular concerns?
> 
> I prefer the current way as I will need to expose response headers to the
> client. 
> Exposing RequestMetadata would simplify my job heavily.

@Marc:
No need to be consistent with the ImageDataFetcher. It's a different layer.
Re-Future proof. You never know what the future brings ;-) I don't think this
will make anything more ore less future proof.

Jan gave one example - exposing technical details like HTTP headers.

@Jan: What HTTP response headers do you need? 

The reason why I'm challenging this is that returning a bag of things is usually
a easy way for exposing details that may not need to be exposed.

[Marc Treib, 2017-03-23 10:36:28]

On 2017/03/23 10:22:15, markusheintz_ wrote:
> On 2017/03/23 09:20:34, jkrcal wrote:
> > On 2017/03/23 09:18:43, Marc Treib wrote:
> > > On 2017/03/23 09:13:28, markusheintz_ wrote:
> > > > On 2017/03/20 13:11:16, commit-bot: I haz the power wrote:
> > > > > Committed patchset #5 (id:80001) as
> > > > >
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/0a6cfc6ed4370aa3f4974ac7ed0e...
> > > > 
> > > > Sorry for beeing late to the party.
> > > > 
> > > > I wonder whether we can simply expose a flag served_from_cache or
> something
> > > > similar instead of the whole metadata.
> > > > Is there anything that would prevent this?
> > > 
> > > Reasons for exposing all the request metadata:
> > > - Future proof: When someone needs one more flag in the future, we won't
> have
> > to
> > > update all clients again.
> > > - Consistency with what ImageDataFetcher does.
> > > 
> > > And I don't see a reason for *not* handing out the whole metadata - do you
> > have
> > > any particular concerns?
> > 
> > I prefer the current way as I will need to expose response headers to the
> > client. 
> > Exposing RequestMetadata would simplify my job heavily.
> 
> @Marc:
> No need to be consistent with the ImageDataFetcher. It's a different layer.

But it's still the same overall API. I'd find it weird if data is exposed from
ImageDataFetcher, but ImageFetcher keeps it to itself.

> Re-Future proof. You never know what the future brings ;-) I don't think this
> will make anything more ore less future proof.
> 
> Jan gave one example - exposing technical details like HTTP headers.

IMO even if we only have these two things, it's worth packing them into a struct
rather than handing out individual random things.

FWIW, we went through the same process with ImageDataFetcher: First expose one
flag (and update all clients), then another (and update all clients), until we
added the struct.

> @Jan: What HTTP response headers do you need? 
> 
> The reason why I'm challenging this is that returning a bag of things is
usually
> a easy way for exposing details that may not need to be exposed.

Well, we do have concrete use cases for the things we expose. IMO packing them
all into a struct is really a minor details, which just reduces churn.

[jkrcal, 2017-03-23 12:15:21]

On 2017/03/23 10:36:28, Marc Treib wrote:
> On 2017/03/23 10:22:15, markusheintz_ wrote:
> > On 2017/03/23 09:20:34, jkrcal wrote:
> > > On 2017/03/23 09:18:43, Marc Treib wrote:
> > > > On 2017/03/23 09:13:28, markusheintz_ wrote:
> > > > > On 2017/03/20 13:11:16, commit-bot: I haz the power wrote:
> > > > > > Committed patchset #5 (id:80001) as
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/0a6cfc6ed4370aa3f4974ac7ed0e...
> > > > > 
> > > > > Sorry for beeing late to the party.
> > > > > 
> > > > > I wonder whether we can simply expose a flag served_from_cache or
> > something
> > > > > similar instead of the whole metadata.
> > > > > Is there anything that would prevent this?
> > > > 
> > > > Reasons for exposing all the request metadata:
> > > > - Future proof: When someone needs one more flag in the future, we won't
> > have
> > > to
> > > > update all clients again.
> > > > - Consistency with what ImageDataFetcher does.
> > > > 
> > > > And I don't see a reason for *not* handing out the whole metadata - do
you
> > > have
> > > > any particular concerns?
> > > 
> > > I prefer the current way as I will need to expose response headers to the
> > > client. 
> > > Exposing RequestMetadata would simplify my job heavily.
> > 
> > @Marc:
> > No need to be consistent with the ImageDataFetcher. It's a different layer.
> 
> But it's still the same overall API. I'd find it weird if data is exposed from
> ImageDataFetcher, but ImageFetcher keeps it to itself.
> 
> > Re-Future proof. You never know what the future brings ;-) I don't think
this
> > will make anything more ore less future proof.
> > 
> > Jan gave one example - exposing technical details like HTTP headers.
> 
> IMO even if we only have these two things, it's worth packing them into a
struct
> rather than handing out individual random things.
> 
> FWIW, we went through the same process with ImageDataFetcher: First expose one
> flag (and update all clients), then another (and update all clients), until we
> added the struct.
> 
> > @Jan: What HTTP response headers do you need? 
> > 
> > The reason why I'm challenging this is that returning a bag of things is
> usually
> > a easy way for exposing details that may not need to be exposed.
> 
> Well, we do have concrete use cases for the things we expose. IMO packing them
> all into a struct is really a minor details, which just reduces churn.

@Markus:
I need the response headers for integration with Google favicon server.
We agreed that they send an additional piece of info (the original URL of the
icon being returned) via the "Content-Location" HTTP header.

[markusheintz_, 2017-03-23 12:59:31]

On 2017/03/23 12:15:21, jkrcal wrote:
> On 2017/03/23 10:36:28, Marc Treib wrote:
> > On 2017/03/23 10:22:15, markusheintz_ wrote:
> > > On 2017/03/23 09:20:34, jkrcal wrote:
> > > > On 2017/03/23 09:18:43, Marc Treib wrote:
> > > > > On 2017/03/23 09:13:28, markusheintz_ wrote:
> > > > > > On 2017/03/20 13:11:16, commit-bot: I haz the power wrote:
> > > > > > > Committed patchset #5 (id:80001) as
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/0a6cfc6ed4370aa3f4974ac7ed0e...
> > > > > > 
> > > > > > Sorry for beeing late to the party.
> > > > > > 
> > > > > > I wonder whether we can simply expose a flag served_from_cache or
> > > something
> > > > > > similar instead of the whole metadata.
> > > > > > Is there anything that would prevent this?
> > > > > 
> > > > > Reasons for exposing all the request metadata:
> > > > > - Future proof: When someone needs one more flag in the future, we
won't
> > > have
> > > > to
> > > > > update all clients again.
> > > > > - Consistency with what ImageDataFetcher does.
> > > > > 
> > > > > And I don't see a reason for *not* handing out the whole metadata - do
> you
> > > > have
> > > > > any particular concerns?
> > > > 
> > > > I prefer the current way as I will need to expose response headers to
the
> > > > client. 
> > > > Exposing RequestMetadata would simplify my job heavily.
> > > 
> > > @Marc:
> > > No need to be consistent with the ImageDataFetcher. It's a different
layer.
> > 
> > But it's still the same overall API. I'd find it weird if data is exposed
from
> > ImageDataFetcher, but ImageFetcher keeps it to itself.
> > 
> > > Re-Future proof. You never know what the future brings ;-) I don't think
> this
> > > will make anything more ore less future proof.
> > > 
> > > Jan gave one example - exposing technical details like HTTP headers.
> > 
> > IMO even if we only have these two things, it's worth packing them into a
> struct
> > rather than handing out individual random things.
> > 
> > FWIW, we went through the same process with ImageDataFetcher: First expose
one
> > flag (and update all clients), then another (and update all clients), until
we
> > added the struct.
> > 
> > > @Jan: What HTTP response headers do you need? 
> > > 
> > > The reason why I'm challenging this is that returning a bag of things is
> > usually
> > > a easy way for exposing details that may not need to be exposed.
> > 
> > Well, we do have concrete use cases for the things we expose. IMO packing
them
> > all into a struct is really a minor details, which just reduces churn.
> 
> @Markus:
> I need the response headers for integration with Google favicon server.
> We agreed that they send an additional piece of info (the original URL of the
> icon being returned) via the "Content-Location" HTTP header.

Ok I'm convinced :-) 

LGTM