[Suggestions] Move ImageManager to the component

[Suggestions] Move ImageManager to the component

Keeps the fetcher functionality as ImageFetcherImpl in chrome/browser/search/suggestions.

BUG=387751
TBR=blundell
TEST=ImageManagerTest,ImageFetcherBrowserTest

Committed: https://crrev.com/5a25f30080dd57d200679546c970d70f2247def9
Cr-Commit-Position: refs/heads/master@{#296503}

[Mathieu, 2014-09-22 21:09:31]

Patchset #1 (id:1) has been deleted

[Mathieu, 2014-09-22 21:09:38]

Patchset #1 (id:20001) has been deleted

[Mathieu, 2014-09-22 21:09:44]

Patchset #1 (id:40001) has been deleted

[Mathieu, 2014-09-22 21:09:54]

Patchset #1 (id:60001) has been deleted

[Mathieu, 2014-09-22 21:11:59]

mathp@chromium.org changed reviewers:
+ huangs@chromium.org

[Mathieu, 2014-09-22 21:12:54]

Hi Sam, 

Sorry this is a big change. It's meant to be functionality neutral, but I've had
to create a delegate, interfaces, etc.

Test coverage may not be final.

Have a look?

[huangs, 2014-09-22 21:19:51]

Is there a counterpart to image_manager_impl_browsertest.cc ?

[Mathieu, 2014-09-22 21:22:44]

On 2014/09/22 21:19:51, huangs1 wrote:
> Is there a counterpart to image_manager_impl_browsertest.cc ?

As mentioned the test coverage is not final. Some of the tests that were in the
browser_test can be found in the image_manager_unittest, and the fetcher needs
to be tested (not here now). Currently my setup to run browser_tests is broken
so I may add later.

[huangs, 2014-09-22 21:51:14]

First pass.

https://codereview.chromium.org/543753002/diff/80001/chrome/browser/search/su...
File chrome/browser/search/suggestions/image_fetcher_impl.h (right):

https://codereview.chromium.org/543753002/diff/80001/chrome/browser/search/su...
chrome/browser/search/suggestions/image_fetcher_impl.h:40: virtual void
OnFetchComplete(const GURL image_url,
const GURL or const GURL& ?  Also check .cc file.

https://codereview.chromium.org/543753002/diff/80001/chrome/browser/search/su...
chrome/browser/search/suggestions/image_fetcher_impl.h:50: explicit
ImageRequest(chrome::BitmapFetcher* f);
Comment on the fact that the class takes ownership of |f|.

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
File components/suggestions/image_fetcher.h (right):

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
components/suggestions/image_fetcher.h:5: #ifndef
COMPONENTS_SUGGESTIONS_IMAGE_FETCHER_IMPL_H_
Remove _IMPL_

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
components/suggestions/image_fetcher.h:8: #include <map>
These STD includes are not needed.

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
components/suggestions/image_fetcher.h:15: #include "ui/gfx/image/image_skia.h"
Can forward declare SkBitmap ?

What's basictypes.h used for?

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
components/suggestions/image_fetcher.h:19: class URLRequestContextGetter;
Unused.

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
components/suggestions/image_fetcher.h:35: };
DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
File components/suggestions/image_fetcher_delegate.h (right):

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
components/suggestions/image_fetcher_delegate.h:8: #include "url/gurl.h"
Note: GURL can also be pre-declared instead of #included; up to you.

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
File components/suggestions/image_manager.cc (right):

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
components/suggestions/image_manager.cc:10: #include "net/base/load_flags.h"
Don't need ref_counted_memory.h ?
Don't need load_flags.h ?

Don't need url_request_context_getter.h .

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
File components/suggestions/image_manager.h (right):

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
components/suggestions/image_manager.h:9: #include <utility>
#include <string>

[Mathieu, 2014-09-23 14:59:07]

Will work on browsertest now.

https://codereview.chromium.org/543753002/diff/80001/chrome/browser/search/su...
File chrome/browser/search/suggestions/image_fetcher_impl.h (right):

https://codereview.chromium.org/543753002/diff/80001/chrome/browser/search/su...
chrome/browser/search/suggestions/image_fetcher_impl.h:40: virtual void
OnFetchComplete(const GURL image_url,
On 2014/09/22 21:51:13, huangs1 wrote:
> const GURL or const GURL& ?  Also check .cc file.

The bitmap_fetcher_delegate is not controlled by me, and they have no &.

https://codereview.chromium.org/543753002/diff/80001/chrome/browser/search/su...
chrome/browser/search/suggestions/image_fetcher_impl.h:50: explicit
ImageRequest(chrome::BitmapFetcher* f);
On 2014/09/22 21:51:13, huangs1 wrote:
> Comment on the fact that the class takes ownership of |f|.

Done.

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
File components/suggestions/image_fetcher.h (right):

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
components/suggestions/image_fetcher.h:5: #ifndef
COMPONENTS_SUGGESTIONS_IMAGE_FETCHER_IMPL_H_
On 2014/09/22 21:51:13, huangs1 wrote:
> Remove _IMPL_

Good catch

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
components/suggestions/image_fetcher.h:8: #include <map>
On 2014/09/22 21:51:14, huangs1 wrote:
> These STD includes are not needed.

Done.

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
components/suggestions/image_fetcher.h:15: #include "ui/gfx/image/image_skia.h"
On 2014/09/22 21:51:13, huangs1 wrote:
> Can forward declare SkBitmap ?
> 
> What's basictypes.h used for?

Done.

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
components/suggestions/image_fetcher.h:19: class URLRequestContextGetter;
On 2014/09/22 21:51:13, huangs1 wrote:
> Unused.

Done.

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
components/suggestions/image_fetcher.h:35: };
On 2014/09/22 21:51:13, huangs1 wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
File components/suggestions/image_fetcher_delegate.h (right):

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
components/suggestions/image_fetcher_delegate.h:8: #include "url/gurl.h"
On 2014/09/22 21:51:14, huangs1 wrote:
> Note: GURL can also be pre-declared instead of #included; up to you.

Done.

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
File components/suggestions/image_manager.cc (right):

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
components/suggestions/image_manager.cc:10: #include "net/base/load_flags.h"
On 2014/09/22 21:51:14, huangs1 wrote:
> Don't need ref_counted_memory.h ?
> Don't need load_flags.h ?
> 
> Don't need url_request_context_getter.h .

Done.

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
File components/suggestions/image_manager.h (right):

https://codereview.chromium.org/543753002/diff/80001/components/suggestions/i...
components/suggestions/image_manager.h:9: #include <utility>
On 2014/09/22 21:51:14, huangs1 wrote:
> #include <string>

Done.

[Mathieu, 2014-09-23 17:01:33]

Patchset #3 (id:120001) has been deleted

[Mathieu, 2014-09-23 17:03:32]

Now with more browsertest!

Please take a look.

[huangs, 2014-09-24 15:46:58]

LGTM with nits.

https://codereview.chromium.org/543753002/diff/160001/chrome/browser/search/s...
File chrome/browser/search/suggestions/image_fetcher_impl.cc (right):

https://codereview.chromium.org/543753002/diff/160001/chrome/browser/search/s...
chrome/browser/search/suggestions/image_fetcher_impl.cc:6: 
#include <string>

https://codereview.chromium.org/543753002/diff/160001/chrome/browser/search/s...
File chrome/browser/search/suggestions/image_fetcher_impl_browsertest.cc
(right):

https://codereview.chromium.org/543753002/diff/160001/chrome/browser/search/s...
chrome/browser/search/suggestions/image_fetcher_impl_browsertest.cc:7: #include
<string>
Doesn't look like these are used.

https://codereview.chromium.org/543753002/diff/160001/components/suggestions/...
File components/suggestions/DEPS (right):

https://codereview.chromium.org/543753002/diff/160001/components/suggestions/...
components/suggestions/DEPS:2: "+components/leveldb_proto",
Sort alphabetically.

https://codereview.chromium.org/543753002/diff/160001/components/suggestions/...
File components/suggestions/image_fetcher.h (right):

https://codereview.chromium.org/543753002/diff/160001/components/suggestions/...
components/suggestions/image_fetcher.h:22: virtual void
SetImageFetcherDelegate(ImageFetcherDelegate* delegate) = 0;
Can forward-declare ImageFetcherDelegate (and also URL) and remove #include.

https://codereview.chromium.org/543753002/diff/160001/components/suggestions/...
File components/suggestions/image_fetcher_delegate.h (right):

https://codereview.chromium.org/543753002/diff/160001/components/suggestions/...
components/suggestions/image_fetcher_delegate.h:22: virtual
~ImageFetcherDelegate() {}
Please comment on why destructor is in private.

https://codereview.chromium.org/543753002/diff/160001/components/suggestions/...
File components/suggestions/image_manager.h (right):

https://codereview.chromium.org/543753002/diff/160001/components/suggestions/...
components/suggestions/image_manager.h:10: #include <utility>
What's <utility> used for?

https://codereview.chromium.org/543753002/diff/160001/components/suggestions/...
components/suggestions/image_manager.h:85: typedef std::map<const GURL,
ImageCacheRequest> ImageCacheRequestMap;
Is there reason why you're using std::map here but base::hash_map for ImageMap?

[Mathieu, 2014-09-24 19:20:38]

Patchset #5 (id:180001) has been deleted

[Mathieu, 2014-09-24 19:30:49]

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

[Mathieu, 2014-09-24 19:31:31]

Thanks, submitting...

https://codereview.chromium.org/543753002/diff/160001/chrome/browser/search/s...
File chrome/browser/search/suggestions/image_fetcher_impl.cc (right):

https://codereview.chromium.org/543753002/diff/160001/chrome/browser/search/s...
chrome/browser/search/suggestions/image_fetcher_impl.cc:6: 
On 2014/09/24 15:46:57, huangs1 wrote:
> #include <string>

Done.

https://codereview.chromium.org/543753002/diff/160001/chrome/browser/search/s...
File chrome/browser/search/suggestions/image_fetcher_impl_browsertest.cc
(right):

https://codereview.chromium.org/543753002/diff/160001/chrome/browser/search/s...
chrome/browser/search/suggestions/image_fetcher_impl_browsertest.cc:7: #include
<string>
On 2014/09/24 15:46:57, huangs1 wrote:
> Doesn't look like these are used.

Done.

https://codereview.chromium.org/543753002/diff/160001/components/suggestions/...
File components/suggestions/DEPS (right):

https://codereview.chromium.org/543753002/diff/160001/components/suggestions/...
components/suggestions/DEPS:2: "+components/leveldb_proto",
On 2014/09/24 15:46:57, huangs1 wrote:
> Sort alphabetically.

Done.

https://codereview.chromium.org/543753002/diff/160001/components/suggestions/...
File components/suggestions/image_fetcher.h (right):

https://codereview.chromium.org/543753002/diff/160001/components/suggestions/...
components/suggestions/image_fetcher.h:22: virtual void
SetImageFetcherDelegate(ImageFetcherDelegate* delegate) = 0;
On 2014/09/24 15:46:57, huangs1 wrote:
> Can forward-declare ImageFetcherDelegate (and also URL) and remove #include.

Turns out I can't do this because of the mock in image_manager_unittest

https://codereview.chromium.org/543753002/diff/160001/components/suggestions/...
File components/suggestions/image_fetcher_delegate.h (right):

https://codereview.chromium.org/543753002/diff/160001/components/suggestions/...
components/suggestions/image_fetcher_delegate.h:22: virtual
~ImageFetcherDelegate() {}
On 2014/09/24 15:46:57, huangs1 wrote:
> Please comment on why destructor is in private.

Acknowledged.

https://codereview.chromium.org/543753002/diff/160001/components/suggestions/...
File components/suggestions/image_manager.h (right):

https://codereview.chromium.org/543753002/diff/160001/components/suggestions/...
components/suggestions/image_manager.h:10: #include <utility>
On 2014/09/24 15:46:58, huangs1 wrote:
> What's <utility> used for?

Done.

https://codereview.chromium.org/543753002/diff/160001/components/suggestions/...
components/suggestions/image_manager.h:85: typedef std::map<const GURL,
ImageCacheRequest> ImageCacheRequestMap;
On 2014/09/24 15:46:58, huangs1 wrote:
> Is there reason why you're using std::map here but base::hash_map for
ImageMap?

GURL is not hashable

[Mathieu, 2014-09-24 19:31:58]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2014-09-24 19:32:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/543753002/220001

[commit-bot: I haz the power, 2014-09-24 20:19:45]

Committed patchset #6 (id:220001) as 7a814504a984216577ba7829fa597ffdf9aa5689

[commit-bot: I haz the power, 2014-09-24 20:20:40]

Patchset 6 (id:??) landed as
https://crrev.com/5a25f30080dd57d200679546c970d70f2247def9
Cr-Commit-Position: refs/heads/master@{#296503}

[Lei Zhang, 2014-09-24 23:00:35]

A revert of this CL (patchset #6 id:220001) has been created in
https://codereview.chromium.org/600203002/ by thestig@chromium.org.

The reason for reverting is: One of the new unit tests failed with uninit memory
access:
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28v....

[Lei Zhang, 2014-09-24 23:03:41]

BTW, you should tell blundell@ what you are TBRing.