[Suggestions] Create ImageEncoder, to abstract away image encode/decode.

[Suggestions] Create ImageEncoder, to abstract away image encode/decode.

iOS can't depend on JPEG encoder.

TBR=jam
BUG=387751, 409156
Committed: https://crrev.com/f5e2e2fa7303d699027095cd9b7cfb9462b585f6
Cr-Commit-Position: refs/heads/master@{#298510}

[Mathieu, 2014-10-06 15:43:51]

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

[Mathieu, 2014-10-06 15:44:40]

Hi, 

This is ready for review.

Sam: there should be no functionality change, but can you review?
Colin: Not sure how to arrange build rules so that ios doesn't depend on what it
shouldn't. Do I need to create a new target?

[huangs, 2014-10-06 17:14:18]

Looks good. First round of comments.

https://chromiumcodereview.appspot.com/630073002/diff/1/components/suggestion...
File components/suggestions/image_manager.h (right):

https://chromiumcodereview.appspot.com/630073002/diff/1/components/suggestion...
components/suggestions/image_manager.h:41: ImageManager(scoped_ptr<ImageFetcher>
image_fetcher,
Add comments to state that ownerships of |image_fetcher|, |image_encoder|, and
|database| are transferred to the class.

https://chromiumcodereview.appspot.com/630073002/diff/1/components/suggestion...
File components/suggestions/image_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/630073002/diff/1/components/suggestion...
components/suggestions/image_manager_unittest.cc:6: 
#include <vector>

https://chromiumcodereview.appspot.com/630073002/diff/1/components/suggestion...
components/suggestions/image_manager_unittest.cc:193:
image_manager_.reset(CreateImageManager(fake_db, image_encoder));
Inline rather than create temp variable, since ownership is taken by
|image_manager_|.

[Mathieu, 2014-10-06 18:13:35]

Thanks!

https://chromiumcodereview.appspot.com/630073002/diff/1/components/suggestion...
File components/suggestions/image_manager.h (right):

https://chromiumcodereview.appspot.com/630073002/diff/1/components/suggestion...
components/suggestions/image_manager.h:41: ImageManager(scoped_ptr<ImageFetcher>
image_fetcher,
On 2014/10/06 17:14:18, huangs1 wrote:
> Add comments to state that ownerships of |image_fetcher|, |image_encoder|, and
> |database| are transferred to the class.

Done.

https://chromiumcodereview.appspot.com/630073002/diff/1/components/suggestion...
File components/suggestions/image_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/630073002/diff/1/components/suggestion...
components/suggestions/image_manager_unittest.cc:6: 
On 2014/10/06 17:14:18, huangs1 wrote:
> #include <vector>

Done.

https://chromiumcodereview.appspot.com/630073002/diff/1/components/suggestion...
components/suggestions/image_manager_unittest.cc:193:
image_manager_.reset(CreateImageManager(fake_db, image_encoder));
On 2014/10/06 17:14:18, huangs1 wrote:
> Inline rather than create temp variable, since ownership is taken by
> |image_manager_|.

Done.

[huangs, 2014-10-06 18:24:47]

lgtm

[blundell, 2014-10-07 11:59:23]

This is along the lines of what I had envisioned. ImageEncoder should be a pure
virtual interface, and the jpeg impl should be called JpegImageEncoder or
something like that. Putting JpegImageEncoder in its own target makes sense to
me as the cleanest way to cut the dependency on iOS

[Mathieu, 2014-10-07 13:55:21]

Patchset #3 (id:40001) has been deleted

[Mathieu, 2014-10-07 13:56:26]

Hi Colin, 

Can you have a look?

[blundell, 2014-10-07 14:14:00]

LGTM

I think for clarity it makes sense to put the jpeg files in their own
subdirectory. Maybe components/suggestions/jpeg? (Note: investigate
tools/gyp/move_source_file.py if you're not familiar with it to make this sort
of thing a snap).

https://codereview.chromium.org/630073002/diff/60001/chrome/browser/search/su...
File chrome/browser/search/suggestions/suggestions_service_factory.cc (right):

https://codereview.chromium.org/630073002/diff/60001/chrome/browser/search/su...
chrome/browser/search/suggestions/suggestions_service_factory.cc:82:
image_encoder.PassAs<ImageEncoder>(),
hmm, I think there was a chromium-dev@ thread about not needing the PassAs()
anymore, mumblemumblemumble C++11

https://codereview.chromium.org/630073002/diff/60001/components/suggestions.gypi
File components/suggestions.gypi (right):

https://codereview.chromium.org/630073002/diff/60001/components/suggestions.g...
components/suggestions.gypi:50: ['OS != "ios"', {
You (or Justin) should have a comment to add here, as it's not clear why this
target should be !iOS.

[Mathieu, 2014-10-07 15:05:39]

Patchset #5 (id:90001) has been deleted

[Mathieu, 2014-10-07 15:31:28]

Patchset #5 (id:90001) has been deleted

[Mathieu, 2014-10-07 15:31:35]

Patchset #6 (id:130001) has been deleted

[Mathieu, 2014-10-07 15:31:41]

Patchset #5 (id:110001) has been deleted

[Mathieu, 2014-10-07 15:31:50]

Patchset #4 (id:80001) has been deleted

[Mathieu, 2014-10-07 15:42:39]

Patchset #4 (id:150001) has been deleted

[Mathieu, 2014-10-07 16:00:29]

Thanks!

https://codereview.chromium.org/630073002/diff/60001/chrome/browser/search/su...
File chrome/browser/search/suggestions/suggestions_service_factory.cc (right):

https://codereview.chromium.org/630073002/diff/60001/chrome/browser/search/su...
chrome/browser/search/suggestions/suggestions_service_factory.cc:82:
image_encoder.PassAs<ImageEncoder>(),
On 2014/10/07 14:13:59, blundell wrote:
> hmm, I think there was a chromium-dev@ thread about not needing the PassAs()
> anymore, mumblemumblemumble C++11

Done.

https://codereview.chromium.org/630073002/diff/60001/components/suggestions.gypi
File components/suggestions.gypi (right):

https://codereview.chromium.org/630073002/diff/60001/components/suggestions.g...
components/suggestions.gypi:50: ['OS != "ios"', {
On 2014/10/07 14:13:59, blundell wrote:
> You (or Justin) should have a comment to add here, as it's not clear why this
> target should be !iOS.

Done.

[Mathieu, 2014-10-07 16:00:32]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2014-10-07 16:01:28]

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

[commit-bot: I haz the power, 2014-10-07 16:09:35]

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

[commit-bot: I haz the power, 2014-10-07 16:09:36]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Mathieu, 2014-10-07 16:40:10]

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

[Mathieu, 2014-10-07 16:40:46]

On 2014/10/07 16:09:36, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

TBR jam for chrome/browser build file changes.

[Mathieu, 2014-10-07 16:40:56]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2014-10-07 16:42:18]

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

[commit-bot: I haz the power, 2014-10-07 17:12:22]

Committed patchset #4 (id:170001) as 23b020c3aa523003d35c90b4a9ad1d7fef2dd354

[commit-bot: I haz the power, 2014-10-07 17:13:03]

Patchset 4 (id:??) landed as
https://crrev.com/f5e2e2fa7303d699027095cd9b7cfb9462b585f6
Cr-Commit-Position: refs/heads/master@{#298510}

[blundell, 2014-10-07 18:40:31]

https://codereview.chromium.org/630073002/diff/170001/components/suggestions/...
File components/suggestions/BUILD.gn (right):

https://codereview.chromium.org/630073002/diff/170001/components/suggestions/...
components/suggestions/BUILD.gn:36: static_library("jpeg_image_encoder") {
nit: the same TODO for justincohen should be here, right?

[Mathieu, 2014-10-08 14:50:55]

A revert of this CL (patchset #4 id:170001) has been created in
https://codereview.chromium.org/634273002/ by mathp@chromium.org.

The reason for reverting is: It is no longer needed, we decided on a different
approach..