Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 2757643002: components/image_fetcher: Expose RequestMetadata from ImageFetcher (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 1 week ago by Marc Treib
Modified:
4 months ago
Reviewers:
markusheintz_, jkrcal
CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, ntp-dev+reviews_chromium.org, donnd+watch_chromium.org, noyau+watch_chromium.org, jfweitz+watch_chromium.org, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : fix browser_tests #

Patch Set 3 : fix CrOS #

Total comments: 4

Patch Set 4 : rebase #

Patch Set 5 : logo_bridge #

Unified diffs Side-by-side diffs Delta from patch set Stats (+98 lines, -60 lines) Patch
M chrome/browser/android/logo_bridge.h View 1 2 3 4 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/android/logo_bridge.cc View 1 2 3 4 1 chunk +8 lines, -6 lines 0 comments Download
M chrome/browser/chromeos/hats/hats_notification_controller.h View 1 2 2 chunks +4 lines, -1 line 0 comments Download
M chrome/browser/chromeos/hats/hats_notification_controller.cc View 1 2 1 chunk +4 lines, -2 lines 0 comments Download
M chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc View 1 2 5 chunks +14 lines, -11 lines 0 comments Download
M chrome/browser/search/suggestions/image_fetcher_impl_browsertest.cc View 1 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/search/thumbnail_source.h View 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/search/thumbnail_source.cc View 1 chunk +2 lines, -1 line 0 comments Download
M components/image_fetcher/image_fetcher.h View 3 chunks +8 lines, -1 line 0 comments Download
M components/image_fetcher/image_fetcher_impl.h View 3 chunks +5 lines, -7 lines 0 comments Download
M components/image_fetcher/image_fetcher_impl.cc View 3 chunks +7 lines, -5 lines 0 comments Download
M components/ntp_snippets/remote/remote_suggestions_provider_impl.h View 2 chunks +3 lines, -1 line 0 comments Download
M components/ntp_snippets/remote/remote_suggestions_provider_impl.cc View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc View 1 2 3 3 chunks +13 lines, -8 lines 0 comments Download
M components/ntp_tiles/icon_cacher_impl.h View 2 chunks +3 lines, -1 line 0 comments Download
M components/ntp_tiles/icon_cacher_impl.cc View 1 chunk +6 lines, -4 lines 0 comments Download
M components/ntp_tiles/icon_cacher_impl_unittest.cc View 3 chunks +6 lines, -4 lines 0 comments Download
M components/suggestions/image_manager.cc View 1 chunk +2 lines, -1 line 0 comments Download
M components/suggestions/image_manager_unittest.cc View 1 chunk +3 lines, -3 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 36 (21 generated)
Marc Treib
PTAL!
4 months, 1 week ago (2017-03-16 15:46:26 UTC) #2
Marc Treib
Ping!
4 months, 1 week ago (2017-03-17 12:58:50 UTC) #11
Marc Treib
Jan, could you take a look, since Markus is out? Thanks!
4 months ago (2017-03-20 10:18:46 UTC) #15
jkrcal
lgtm, thanks! https://codereview.chromium.org/2757643002/diff/40001/components/image_fetcher/image_fetcher.h File components/image_fetcher/image_fetcher.h (right): https://codereview.chromium.org/2757643002/diff/40001/components/image_fetcher/image_fetcher.h#newcode37 components/image_fetcher/image_fetcher.h:37: const RequestMetadata& metadata)>; A nice cleanup! https://codereview.chromium.org/2757643002/diff/40001/components/image_fetcher/image_fetcher_impl.cc ...
4 months ago (2017-03-20 10:49:59 UTC) #16
Marc Treib
https://codereview.chromium.org/2757643002/diff/40001/components/image_fetcher/image_fetcher_impl.cc File components/image_fetcher/image_fetcher_impl.cc (right): https://codereview.chromium.org/2757643002/diff/40001/components/image_fetcher/image_fetcher_impl.cc#newcode95 components/image_fetcher/image_fetcher_impl.cc:95: callback.Run(request->id, image, metadata); On 2017/03/20 10:49:59, jkrcal wrote: > ...
4 months ago (2017-03-20 10:56:08 UTC) #17
jkrcal
lgtm https://codereview.chromium.org/2757643002/diff/40001/components/image_fetcher/image_fetcher_impl.cc File components/image_fetcher/image_fetcher_impl.cc (right): https://codereview.chromium.org/2757643002/diff/40001/components/image_fetcher/image_fetcher_impl.cc#newcode95 components/image_fetcher/image_fetcher_impl.cc:95: callback.Run(request->id, image, metadata); On 2017/03/20 10:56:08, Marc Treib ...
4 months ago (2017-03-20 11:02:20 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2757643002/80001
4 months ago (2017-03-20 13:04:54 UTC) #26
commit-bot: I haz the power
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/0a6cfc6ed4370aa3f4974ac7ed0e1009e0f9687e
4 months ago (2017-03-20 13:11:16 UTC) #29
markusheintz_
On 2017/03/20 13:11:16, commit-bot: I haz the power wrote: > Committed patchset #5 (id:80001) as ...
4 months ago (2017-03-23 09:13:28 UTC) #30
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: ...
4 months ago (2017-03-23 09:18:43 UTC) #31
jkrcal
On 2017/03/23 09:18:43, Marc Treib wrote: > On 2017/03/23 09:13:28, markusheintz_ wrote: > > On ...
4 months ago (2017-03-23 09:20:34 UTC) #32
markusheintz_
On 2017/03/23 09:20:34, jkrcal wrote: > On 2017/03/23 09:18:43, Marc Treib wrote: > > On ...
4 months ago (2017-03-23 10:22:15 UTC) #33
Marc Treib
On 2017/03/23 10:22:15, markusheintz_ wrote: > On 2017/03/23 09:20:34, jkrcal wrote: > > On 2017/03/23 ...
4 months ago (2017-03-23 10:36:28 UTC) #34
jkrcal
On 2017/03/23 10:36:28, Marc Treib wrote: > On 2017/03/23 10:22:15, markusheintz_ wrote: > > On ...
4 months ago (2017-03-23 12:15:21 UTC) #35
markusheintz_
4 months ago (2017-03-23 12:59:31 UTC) #36
Message was sent while issue was closed.
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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 25c286973