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
The CQ bit was checked by treib@chromium.org to run a CQ dry run
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-generic_chromium_compile_only_ng/builds/301284)
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
==========
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
The CQ bit was checked by treib@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/0a6cfc6ed4370aa3f4974ac7ed0e1009e0f9687e
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
On 2017/03/23 09:13:28, markusheintz_ wrote: > On 2017/03/20 13:11:16, commit-bot: I haz the power 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?
jkrcal
On 2017/03/23 09:18:43, Marc Treib wrote: > On 2017/03/23 09:13:28, markusheintz_ wrote: > > On ...
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_
On 2017/03/23 09:20:34, jkrcal wrote: > On 2017/03/23 09:18:43, Marc Treib wrote: > > On ...
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
On 2017/03/23 10:22:15, markusheintz_ wrote: > On 2017/03/23 09:20:34, jkrcal wrote: > > On 2017/03/23 ...
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
On 2017/03/23 10:36:28, Marc Treib wrote: > On 2017/03/23 10:22:15, markusheintz_ wrote: > > On ...
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_
On 2017/03/23 12:15:21, jkrcal wrote: > On 2017/03/23 10:36:28, Marc Treib wrote: > > On ...
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
Issue 2757643002: components/image_fetcher: Expose RequestMetadata from ImageFetcher
(Closed)
Created 1 year, 1 month ago by Marc Treib
Modified 1 year ago
Reviewers: markusheintz_, jkrcal
Base URL:
Comments: 4
Chromium Code Reviews
