[AiS] ImageService

[AiS] ImageService

Add a service to request & cache images for Answers in Suggest.

This service allows the UI thread to request images that are fetched
via http(s) and then decoded in a render process, caching the most
recent images to avoid repeated network requests.

BUG=380916
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278311

[groby-ooo-7-16, 2014-06-05 03:16:43]

Peter - PTAL.

(I'll soonish-ly CC you on a CL that shows this in use, too)

[groby-ooo-7-16, 2014-06-05 03:40:30]

Updated CC list.

[Peter Kasting, 2014-06-05 23:53:00]

Is the "cache" object supposed to be the public interface to this, and it uses
the service object behind the scenes?  I ask because both this change and your
"complete" CL only have a .h for that class, and it seems to have no
implementation or users anywhere.  (Forgot to add file to CLs maybe?)

I'm not sure I'm a fan of putting most of this into c/b/autocomplete/.  I wonder
if instead there should be some sort of c/b/answers/ directory or something
where most of this should go.  Things like fetching and decoding images seem a
bit removed from what most of the rest of autocomplete/ does.  I'm not sure of
this; maybe you can make a strong case for one over the other.

[groby-ooo-7-16, 2014-06-05 23:55:38]

No, the cache object is an accidentally included remnant of the development
process. Will delete.

c/b/answers seems fine to me. (I'm really not tied to a specific directory - I
kept it in autocomplete because answers is part of it. Maybe c/b/a/answers?)

[Peter Kasting, 2014-06-06 00:05:46]

On 2014/06/05 23:55:38, groby wrote:
> No, the cache object is an accidentally included remnant of the development
> process. Will delete.
> 
> c/b/answers seems fine to me. (I'm really not tied to a specific directory - I
> kept it in autocomplete because answers is part of it. Maybe c/b/a/answers?)

Hmm.  It doesn't look like there's really anything "answers-specific" about the
AnswersImageService -- it's really more of a generic image fetch + decode
handler.

Maybe these classes should drop the "answers" parts of their names, and they +
c/b/bitmap_fetcher* should all go in one place -- maybe name this
BitmapFetcherService and put it in c/b/bitmap_fetcher, or something?

I'm normally a little leery of making a single-use class "general-purpose", but
it seems like it might be reasonable in this case.

[Anuj, 2014-06-06 00:09:29]

On 2014/06/06 00:05:46, Peter Kasting wrote:
> On 2014/06/05 23:55:38, groby wrote:
> > No, the cache object is an accidentally included remnant of the development
> > process. Will delete.
> > 
> > c/b/answers seems fine to me. (I'm really not tied to a specific directory -
I
> > kept it in autocomplete because answers is part of it. Maybe c/b/a/answers?)
> 
> Hmm.  It doesn't look like there's really anything "answers-specific" about
the
> AnswersImageService -- it's really more of a generic image fetch + decode
> handler.
> 
> Maybe these classes should drop the "answers" parts of their names, and they +
> c/b/bitmap_fetcher* should all go in one place -- maybe name this
> BitmapFetcherService and put it in c/b/bitmap_fetcher, or something?
> 
> I'm normally a little leery of making a single-use class "general-purpose",
but
> it seems like it might be reasonable in this case.

I also think that this does not need to be Answers specific. This essentially
opens up the possibility for more suggestion types with images.

[groby-ooo-7-16, 2014-06-10 17:55:27]

On 2014/06/06 00:05:46, Peter Kasting wrote:
> On 2014/06/05 23:55:38, groby wrote:
> > No, the cache object is an accidentally included remnant of the development
> > process. Will delete.
> > 
> > c/b/answers seems fine to me. (I'm really not tied to a specific directory -
I
> > kept it in autocomplete because answers is part of it. Maybe c/b/a/answers?)
> 
> Hmm.  It doesn't look like there's really anything "answers-specific" about
the
> AnswersImageService -- it's really more of a generic image fetch + decode
> handler.
> 
> Maybe these classes should drop the "answers" parts of their names, and they +
> c/b/bitmap_fetcher* should all go in one place -- maybe name this
> BitmapFetcherService and put it in c/b/bitmap_fetcher, or something?
> 
> I'm normally a little leery of making a single-use class "general-purpose",
but
> it seems like it might be reasonable in this case.

I was tempted to do that. The main issue I see with that is that re-users might
want a different lifetime policy for cache entries. I suppose we can tackle that
when it comes to it, so let me move/rename it.

[groby-ooo-7-16, 2014-06-13 21:59:19]

Files moved, added sky as reviewer.

[sky, 2014-06-17 00:13:40]

Here's my first set of comments. I'll finish up later on.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
File chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc (right):

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:33: RequestId
request_id_;
const?

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:36: };
DISALLOW...

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:59:
Profile::FromBrowserContext(context);
Why do you need this?

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:89: break;
Any reason you don't erase here then return? Similar to what you do on 70.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:121: RequestId
BitmapFetcherService::RequestImage(const GURL& url,
make order match header.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:122:
BitmapFetcherObserver* observer) {
indentation is off (maybe for all lines that you've wrapped, not sure).

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
File chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h (right):

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h:28: static const int
REQUEST_ID_INVALID = 0;
I would be inclined to make this a member of BitmapFetcherService. Better
scoping then.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h:34: virtual void
OnImageChanged(const SkBitmap& answers_image) = 0;
It's not clear to me (without looking at the code), why there are two methods
here and if this (OnImageChanged) is invoked multiple times. It is possible to
have just one method that is invoked with the id and SkBitmap? If the fetch
fails the SkBitmap could be empty.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h:40: public
chrome::BitmapFetcherDelegate {
nit: indent one more.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h:46: void
CancelRequest(int requestId);
int->RequestId

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h:48: // Requests a new
image. Will either trigger download or satisfy from cache.
Looks like this can fail if there are already too many outstanding requests too.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h:50: RequestId
RequestImage(const GURL& url, BitmapFetcherObserver* observer);
Make this take a scoped_ptr. You should also document notifies, and destroies,
observer if image already downloaded.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h:58: virtual
chrome::BitmapFetcher* CreateFetcher(const GURL& url);
Subclassing for tests always results in introducing weird APIs. How about a
delegate for this? That way no subclassing and separation is clearer.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h:97: };
DISALLOW...

[sky, 2014-06-17 03:00:02]

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
File chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc (right):

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:69:
requests_.erase(iter);
Shouldn't you potentially delete the BitmapFetcher? Otherwise it's unnecessarily
limiting the number of requests that can be outstanding.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:77:
ScopedVector<chrome::BitmapFetcher>::iterator iter = active_fetchers_.begin();
nit: move into for loop.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:118:
(void)EnsureFetcherForUrl(url);
Remove (void)

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:161:
ScopedVector<BitmapFetcherRequest>::iterator iter;
Use for loop and move iter into it.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
File chrome/browser/bitmap_fetcher/bitmap_fetcher_service_factory.h (right):

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service_factory.h:29: };
DISALLOW_...

https://codereview.chromium.org/319623003/diff/60001/chrome/chrome_tests_unit...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/319623003/diff/60001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:589:
'browser/bitmap_fetcher/bitmap_fetcher_service_unittest.cc',
nit: sort

[groby-ooo-7-16, 2014-06-17 18:06:23]

Fixed, PTAL. 

I feel somewhat odd about abusing OnImageChange as an OnRequestFinished, though.

(I do need a finish signal, since that triggers some cleanup on the Java side)

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
File chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc (right):

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:33: RequestId
request_id_;
On 2014/06/17 00:13:40, sky wrote:
> const?

Done.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:36: };
On 2014/06/17 00:13:39, sky wrote:
> DISALLOW...

Done.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:59:
Profile::FromBrowserContext(context);
On 2014/06/17 00:13:40, sky wrote:
> Why do you need this?

I don't - missed to delete this.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:69:
requests_.erase(iter);
On 2014/06/17 03:00:02, sky wrote:
> Shouldn't you potentially delete the BitmapFetcher? Otherwise it's
unnecessarily
> limiting the number of requests that can be outstanding.

That's actually intentional - we will likely need the data later, anyways. 

This is for "Answers in Suggest" - getting Onebox answers into the Omnibox. The
only images right now are weather images, so unless you only care about LA
weather, you'll need a variety of them. At the same time, there really
_shouldn't_ be too many requests in-flight, since it's triggered by a user query
for which we have high confidence that we know the completion.

Added a short comment that this is deliberate.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:77:
ScopedVector<chrome::BitmapFetcher>::iterator iter = active_fetchers_.begin();
On 2014/06/17 03:00:02, sky wrote:
> nit: move into for loop.

Done. Even though it's ugly :)

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:89: break;
On 2014/06/17 00:13:40, sky wrote:
> Any reason you don't erase here then return? Similar to what you do on 70.

Done.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:118:
(void)EnsureFetcherForUrl(url);
On 2014/06/17 03:00:02, sky wrote:
> Remove (void)

Done.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:121: RequestId
BitmapFetcherService::RequestImage(const GURL& url,
On 2014/06/17 00:13:40, sky wrote:
> make order match header.

Done.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:122:
BitmapFetcherObserver* observer) {
On 2014/06/17 00:13:40, sky wrote:
> indentation is off (maybe for all lines that you've wrapped, not sure).

Done.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:161:
ScopedVector<BitmapFetcherRequest>::iterator iter;
On 2014/06/17 03:00:02, sky wrote:
> Use for loop and move iter into it.

Can't use for(), since iter is only incremented if the request isn't erased.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
File chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h (right):

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h:28: static const int
REQUEST_ID_INVALID = 0;
On 2014/06/17 00:13:40, sky wrote:
> I would be inclined to make this a member of BitmapFetcherService. Better
> scoping then.

Done.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h:34: virtual void
OnImageChanged(const SkBitmap& answers_image) = 0;
On 2014/06/17 00:13:40, sky wrote:
> It's not clear to me (without looking at the code), why there are two methods
> here and if this (OnImageChanged) is invoked multiple times. It is possible to
> have just one method that is invoked with the id and SkBitmap? If the fetch
> fails the SkBitmap could be empty.

I can probably roll this into one. Giving it a spin.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h:40: public
chrome::BitmapFetcherDelegate {
On 2014/06/17 00:13:40, sky wrote:
> nit: indent one more.

Done.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h:46: void
CancelRequest(int requestId);
On 2014/06/17 00:13:40, sky wrote:
> int->RequestId

Done.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h:48: // Requests a new
image. Will either trigger download or satisfy from cache.
On 2014/06/17 00:13:40, sky wrote:
> Looks like this can fail if there are already too many outstanding requests
too.
Clarified docs.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h:50: RequestId
RequestImage(const GURL& url, BitmapFetcherObserver* observer);
On 2014/06/17 00:13:40, sky wrote:
> Make this take a scoped_ptr. You should also document notifies, and destroies,
> observer if image already downloaded.

The destruction is kind of implicit in "takes ownership" - I'd rather not commit
to a _specific_ lifetime here. Clarified notification.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h:58: virtual
chrome::BitmapFetcher* CreateFetcher(const GURL& url);
On 2014/06/17 00:13:40, sky wrote:
> Subclassing for tests always results in introducing weird APIs. How about a
> delegate for this? That way no subclassing and separation is clearer.

How's a subclassable factory method a weird API? I thought about a delegate, but
that's an additional indirection, extra data storage.

If you still feel bad about the API as-is, I'll look at changing it, but I
prefer to keep this.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h:97: };
On 2014/06/17 00:13:40, sky wrote:
> DISALLOW...

Done.

https://codereview.chromium.org/319623003/diff/60001/chrome/chrome_tests_unit...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/319623003/diff/60001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:589:
'browser/bitmap_fetcher/bitmap_fetcher_service_unittest.cc',
On 2014/06/17 03:00:02, sky wrote:
> nit: sort

Done.

[groby-ooo-7-16, 2014-06-17 19:05:23]

Heads up - keeping a forwarding version of the old include, since downstream
code needs to be patched :(

[sky, 2014-06-17 19:19:47]

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
File chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc (right):

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:69:
requests_.erase(iter);
On 2014/06/17 18:06:22, groby wrote:
> On 2014/06/17 03:00:02, sky wrote:
> > Shouldn't you potentially delete the BitmapFetcher? Otherwise it's
> unnecessarily
> > limiting the number of requests that can be outstanding.
> 
> That's actually intentional - we will likely need the data later, anyways. 
> 
> This is for "Answers in Suggest" - getting Onebox answers into the Omnibox.
The
> only images right now are weather images, so unless you only care about LA
> weather, you'll need a variety of them. At the same time, there really
> _shouldn't_ be too many requests in-flight, since it's triggered by a user
query
> for which we have high confidence that we know the completion.
> 
> Added a short comment that this is deliberate.
> 
> 

Isn't this is a general service though that could be used anywhere else there
are similar needs?

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
File chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h (right):

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h:50: RequestId
RequestImage(const GURL& url, BitmapFetcherObserver* observer);
On 2014/06/17 18:06:23, groby wrote:
> On 2014/06/17 00:13:40, sky wrote:
> > Make this take a scoped_ptr. You should also document notifies, and
destroies,
> > observer if image already downloaded.
> 
> The destruction is kind of implicit in "takes ownership" - I'd rather not
commit
> to a _specific_ lifetime here. Clarified notification.

I wasn't clear. I think the interesting bit is that you may end up notifying
synchronously, before the function returns, that often times can be unexpected.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h:58: virtual
chrome::BitmapFetcher* CreateFetcher(const GURL& url);
On 2014/06/17 18:06:23, groby wrote:
> On 2014/06/17 00:13:40, sky wrote:
> > Subclassing for tests always results in introducing weird APIs. How about a
> > delegate for this? That way no subclassing and separation is clearer.
> 
> How's a subclassable factory method a weird API? I thought about a delegate,
but
> that's an additional indirection, extra data storage.

Having a pure virtual delegate is a lot cleaner and better separation, in my
opinion. Once you head down the subclass route it's all too easy to start
opening up API in weird ways that makes the code harder to understand.

> 
> If you still feel bad about the API as-is, I'll look at changing it, but I
> prefer to keep this.

I'm ok with what you have.

[groby-ooo-7-16, 2014-06-17 21:24:30]

PTAL.

Tried working on removing the fetcher if all associated requests are gone, but
it turns out that BitmapFetcher is not cancelable :(

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
File chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc (right):

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:69:
requests_.erase(iter);
On 2014/06/17 19:19:47, sky wrote:
> On 2014/06/17 18:06:22, groby wrote:
> > On 2014/06/17 03:00:02, sky wrote:
> > > Shouldn't you potentially delete the BitmapFetcher? Otherwise it's
> > unnecessarily
> > > limiting the number of requests that can be outstanding.
> > 
> > That's actually intentional - we will likely need the data later, anyways. 
> > 
> > This is for "Answers in Suggest" - getting Onebox answers into the Omnibox.
> The
> > only images right now are weather images, so unless you only care about LA
> > weather, you'll need a variety of them. At the same time, there really
> > _shouldn't_ be too many requests in-flight, since it's triggered by a user
> query
> > for which we have high confidence that we know the completion.
> > 
> > Added a short comment that this is deliberate.
> > 
> > 
> 
> Isn't this is a general service though that could be used anywhere else there
> are similar needs?

Yes, but

1) There are currently no consumers requiring a different behavior
2) BitmapFetchers are shared across multiple requests. I could ref-count them,
but that seems overkill from here.
3) Worst part, BitmapFetcher is not cancelable. If you issue a fetch request,
you better want that image.

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
File chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h (right):

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h:50: RequestId
RequestImage(const GURL& url, BitmapFetcherObserver* observer);
On 2014/06/17 19:19:47, sky wrote:
> On 2014/06/17 18:06:23, groby wrote:
> > On 2014/06/17 00:13:40, sky wrote:
> > > Make this take a scoped_ptr. You should also document notifies, and
> destroies,
> > > observer if image already downloaded.
> > 
> > The destruction is kind of implicit in "takes ownership" - I'd rather not
> commit
> > to a _specific_ lifetime here. Clarified notification.
> 
> I wasn't clear. I think the interesting bit is that you may end up notifying
> synchronously, before the function returns, that often times can be
unexpected.

Clarified comment some more :)

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
File chrome/browser/bitmap_fetcher/bitmap_fetcher_service_factory.h (right):

https://codereview.chromium.org/319623003/diff/60001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service_factory.h:29: };
On 2014/06/17 03:00:02, sky wrote:
> DISALLOW_...

Done.

[sky, 2014-06-17 23:59:27]

LGTM - one meta question though. How do you know the images you're requesting
never change? And how did you decide on 5?

https://codereview.chromium.org/319623003/diff/140001/chrome/browser/bitmap_f...
File chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc (right):

https://codereview.chromium.org/319623003/diff/140001/chrome/browser/bitmap_f...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:155:
ScopedVector<chrome::BitmapFetcher>::iterator iter = active_fetchers_.begin();
nit: move into for loop (a typedef would likely make this line and others
shorter).

[groby-ooo-7-16, 2014-06-18 00:13:40]

On 2014/06/17 23:59:27, sky wrote:
> LGTM - one meta question though. How do you know the images you're requesting
> never change? 

I don't in the general case, and there's a follow-up CL for that coming soon.
The current use-case, however, is OK with this. Not perfect, but OK.

> And how did you decide on 5?

I had to pick a number. Basically, eyeballing what will work for the current use
case. I didn't want it too large, since it's used on mobile, and I didn't want
it too small so it doesn't thrash all the time. I expect the policy to become a
little bit more complex as additional consumers are added.

[groby-ooo-7-16, 2014-06-18 00:13:47]

https://codereview.chromium.org/319623003/diff/140001/chrome/browser/bitmap_f...
File chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc (right):

https://codereview.chromium.org/319623003/diff/140001/chrome/browser/bitmap_f...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:155:
ScopedVector<chrome::BitmapFetcher>::iterator iter = active_fetchers_.begin();
On 2014/06/17 23:59:27, sky wrote:
> nit: move into for loop (a typedef would likely make this line and others
> shorter).

It's not much shorter, but typedef'd anyways.

[groby-ooo-7-16, 2014-06-18 00:13:55]

The CQ bit was checked by groby@chromium.org

[commit-bot: I haz the power, 2014-06-18 00:16:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/groby@chromium.org/319623003/160001

[commit-bot: I haz the power, 2014-06-18 06:37:13]

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

[commit-bot: I haz the power, 2014-06-18 06:37:14]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[groby-ooo-7-16, 2014-06-18 18:31:02]

The CQ bit was checked by groby@chromium.org

[groby-ooo-7-16, 2014-06-18 18:31:26]

Heads up: Fixed a signed/unsigned compile error. (Beats me why only linux flags
that...)

[commit-bot: I haz the power, 2014-06-18 18:32:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/groby@chromium.org/319623003/180001

[commit-bot: I haz the power, 2014-06-19 09:04:44]

Change committed as 278311

[tfarina, 2014-12-10 17:46:36]

https://codereview.chromium.org/319623003/diff/180001/chrome/browser/bitmap_f...
File chrome/browser/bitmap_fetcher.h (right):

https://codereview.chromium.org/319623003/diff/180001/chrome/browser/bitmap_f...
chrome/browser/bitmap_fetcher.h:5: #include
"chrome/browser/bitmap_fetcher/bitmap_fetcher.h"
Weird that rietveld marked this file as Deleted. Because it was deleted, it is
still in the tree: 
https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/bitmap...

Could you add a TODO(groby) here to delete chrome/browser/bitmap_fetcher.h when
all the includes are updated?

[tfarina, 2014-12-10 17:49:27]

Could you also move
https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/bitmap...
into chrome/browser/bitmap_fetcher/?

[groby-ooo-7-16, 2014-12-11 01:35:28]

Fixed headers in https://codereview.chromium.org/796693002/ instead