Replace the usage of SkBitmap with gfx::Image in the suggestion service.

Replace the usage of SkBitmap with gfx::Image in the suggestion service API.

BUG=612469
Committed: https://crrev.com/55ae116c1f0be2fadf1695706ebd7a11f91e3a06
Cr-Commit-Position: refs/heads/master@{#395830}

[markusheintz_, 2016-05-20 14:06:42]

Description was changed from

==========
Replace the usage of SkBitmap with gfx::Image in the suggestion service.

BUG=
==========

to

==========
Replace the usage of SkBitmap with gfx::Image in the suggestion service API.

BUG=612469
==========

[markusheintz_, 2016-05-20 14:21:40]

markusheintz@chromium.org changed reviewers:
+ noyau@chromium.org, treib@chromium.org

[markusheintz_, 2016-05-20 14:22:18]

On 2016/05/20 14:21:40, markusheintz_ wrote:
> mailto:markusheintz@chromium.org changed reviewers:
> + mailto:noyau@chromium.org, mailto:treib@chromium.org

I'm trying to get rid of the SkBitmap in the SuggestionsService too. Here is a
first CL PTAL

[Marc Treib, 2016-05-20 14:40:04]

LGTM with just a few nits below. Thanks for doing this!

https://codereview.chromium.org/2000653002/diff/20001/chrome/browser/android/...
File chrome/browser/android/ntp/most_visited_sites.cc (left):

https://codereview.chromium.org/2000653002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites.cc:260: if (!bitmap.get()) {
Wow, this was confusing. The new code is much better, thanks for cleaning this
up!

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
File components/suggestions/image_manager.cc (right):

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
components/suggestions/image_manager.cc:88: callback.Run(url, empty_image);
Any reason for declaring an empty_image variable, instead of just passing
gfx::Image() to the callback?

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
components/suggestions/image_manager.cc:140: const gfx::Image
image(gfx::Image::CreateFrom1xBitmap(*bitmap.get()));
I think the .get() isn't needed, you can directly dereference the unique_ptr

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
components/suggestions/image_manager.cc:141: callback.Run(url, image);
Also here: I'd just inline the
gfx::Image::CreateFrom1xBitmap(*bitmap.get())
into the callback.Run

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
components/suggestions/image_manager.cc:171: url, image_url, callback);
This should fit on the previous line now :)

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

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
components/suggestions/image_manager.h:24: #include "ui/gfx/image/image_skia.h"
Not your doing, but is this include needed?

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
File components/suggestions/suggestions_service.h (left):

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
components/suggestions/suggestions_service.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
nit: We don't update the copyright headers.

[noyau (Ping after 24h), 2016-05-20 15:15:59]

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

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
components/suggestions/image_manager.h:46: class ImageManager : public
image_fetcher::ImageFetcherDelegate {
I'm lost in this code, but from what I read, this class caches the image on disk
by converting it to jpeg to save in the cache, and then converted back when
loaded. That's a lot of conversions, and lossy conversions to boot. I suggest
that this code should be revised to provide native encoding/decoding, with no
conversion from and to a bitmap…

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
components/suggestions/image_manager.h:112: base::Callback<void(const GURL&,
const gfx::Image&)> callback);
You are using this callback type in 4 places just in this header. Maybe worth
giving it a name?

[markusheintz_, 2016-05-23 13:20:47]

https://codereview.chromium.org/2000653002/diff/20001/chrome/browser/android/...
File chrome/browser/android/ntp/most_visited_sites.cc (left):

https://codereview.chromium.org/2000653002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites.cc:260: if (!bitmap.get()) {
On 2016/05/20 14:40:03, Marc Treib wrote:
> Wow, this was confusing. The new code is much better, thanks for cleaning this
> up!

Thanks. :-)

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
File components/suggestions/image_manager.cc (right):

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
components/suggestions/image_manager.cc:88: callback.Run(url, empty_image);
On 2016/05/20 14:40:04, Marc Treib wrote:
> Any reason for declaring an empty_image variable, instead of just passing
> gfx::Image() to the callback?

Done.

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
components/suggestions/image_manager.cc:140: const gfx::Image
image(gfx::Image::CreateFrom1xBitmap(*bitmap.get()));
On 2016/05/20 14:40:04, Marc Treib wrote:
> I think the .get() isn't needed, you can directly dereference the unique_ptr

Uups sorry forgot to remove done.

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
components/suggestions/image_manager.cc:141: callback.Run(url, image);
On 2016/05/20 14:40:03, Marc Treib wrote:
> Also here: I'd just inline the
> gfx::Image::CreateFrom1xBitmap(*bitmap.get())
> into the callback.Run

Done.

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
components/suggestions/image_manager.cc:171: url, image_url, callback);
On 2016/05/20 14:40:03, Marc Treib wrote:
> This should fit on the previous line now :)

Done.

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

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
components/suggestions/image_manager.h:24: #include "ui/gfx/image/image_skia.h"
On 2016/05/20 14:40:04, Marc Treib wrote:
> Not your doing, but is this include needed?

Replaced with an include for SkBitmap

This requires adding a new DEP. SkBitmap was pulled in indirectly by
ui/gfx/image/image_skia.h

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
components/suggestions/image_manager.h:46: class ImageManager : public
image_fetcher::ImageFetcherDelegate {
On 2016/05/20 15:15:59, noyau wrote:
> I'm lost in this code, but from what I read, this class caches the image on
disk
> by converting it to jpeg to save in the cache, and then converted back when
> loaded. That's a lot of conversions, and lossy conversions to boot. I suggest
> that this code should be revised to provide native encoding/decoding, with no
> conversion from and to a bitmap…

While I'm cleaning this up I'll try to identify such things. I'll put this on my
list. Marc and me synced about these things too. So expect more CLs.

Once the SkBitmap is gone from the API I'll start working on splitting t the
code for fetching and decoding.

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
components/suggestions/image_manager.h:112: base::Callback<void(const GURL&,
const gfx::Image&)> callback);
On 2016/05/20 15:15:59, noyau wrote:
> You are using this callback type in 4 places just in this header. Maybe worth
> giving it a name?

Done

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
File components/suggestions/suggestions_service.h (left):

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
components/suggestions/suggestions_service.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2016/05/20 14:40:04, Marc Treib wrote:
> nit: We don't update the copyright headers.

Done.

[Marc Treib, 2016-05-23 13:44:03]

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

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
components/suggestions/image_manager.h:24: #include "ui/gfx/image/image_skia.h"
On 2016/05/23 13:20:47, markusheintz_ wrote:
> On 2016/05/20 14:40:04, Marc Treib wrote:
> > Not your doing, but is this include needed?
> 
> Replaced with an include for SkBitmap
> 
> This requires adding a new DEP. SkBitmap was pulled in indirectly by
> ui/gfx/image/image_skia.h 
> 

Ah, okay. If this requires extra approvals for the new DEP, then I'm also fine
with leaving it as-is, since it'll soon go away anyway.

[markusheintz_, 2016-05-23 14:53:10]

On 2016/05/23 13:44:03, Marc Treib wrote:
>
https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
> File components/suggestions/image_manager.h (right):
> 
>
https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
> components/suggestions/image_manager.h:24: #include
"ui/gfx/image/image_skia.h"
> On 2016/05/23 13:20:47, markusheintz_ wrote:
> > On 2016/05/20 14:40:04, Marc Treib wrote:
> > > Not your doing, but is this include needed?
> > 
> > Replaced with an include for SkBitmap
> > 
> > This requires adding a new DEP. SkBitmap was pulled in indirectly by
> > ui/gfx/image/image_skia.h 
> > 
> 
> Ah, okay. If this requires extra approvals for the new DEP, then I'm also fine
> with leaving it as-is, since it'll soon go away anyway.

Done

[markusheintz_, 2016-05-24 08:32:32]

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

https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
components/suggestions/image_manager.h:24: #include "ui/gfx/image/image_skia.h"
On 2016/05/23 13:44:03, Marc Treib wrote:
> On 2016/05/23 13:20:47, markusheintz_ wrote:
> > On 2016/05/20 14:40:04, Marc Treib wrote:
> > > Not your doing, but is this include needed?
> > 
> > Replaced with an include for SkBitmap
> > 
> > This requires adding a new DEP. SkBitmap was pulled in indirectly by
> > ui/gfx/image/image_skia.h 
> > 
> 
> Ah, okay. If this requires extra approvals for the new DEP, then I'm also fine
> with leaving it as-is, since it'll soon go away anyway.

Done.

[markusheintz_, 2016-05-24 08:38:27]

markusheintz@chromium.org changed reviewers:
+ jochen@chromium.org

[markusheintz_, 2016-05-24 08:39:31]

On 2016/05/24 08:32:32, markusheintz_ wrote:
>
https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
> File components/suggestions/image_manager.h (right):
> 
>
https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
> components/suggestions/image_manager.h:24: #include
"ui/gfx/image/image_skia.h"
> On 2016/05/23 13:44:03, Marc Treib wrote:
> > On 2016/05/23 13:20:47, markusheintz_ wrote:
> > > On 2016/05/20 14:40:04, Marc Treib wrote:
> > > > Not your doing, but is this include needed?
> > > 
> > > Replaced with an include for SkBitmap
> > > 
> > > This requires adding a new DEP. SkBitmap was pulled in indirectly by
> > > ui/gfx/image/image_skia.h 
> > > 
> > 
> > Ah, okay. If this requires extra approvals for the new DEP, then I'm also
fine
> > with leaving it as-is, since it'll soon go away anyway.
> 
> Done.

Adding Jochen for OWNERS approval for
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc

Jochen could you PTAL

[markusheintz_, 2016-05-24 10:12:21]

markusheintz@chromium.org changed reviewers:
+ tapted@chromium.org
- jochen@chromium.org

[markusheintz_, 2016-05-24 10:13:51]

On 2016/05/24 08:39:31, markusheintz_ wrote:
> On 2016/05/24 08:32:32, markusheintz_ wrote:
> >
>
https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
> > File components/suggestions/image_manager.h (right):
> > 
> >
>
https://codereview.chromium.org/2000653002/diff/20001/components/suggestions/...
> > components/suggestions/image_manager.h:24: #include
> "ui/gfx/image/image_skia.h"
> > On 2016/05/23 13:44:03, Marc Treib wrote:
> > > On 2016/05/23 13:20:47, markusheintz_ wrote:
> > > > On 2016/05/20 14:40:04, Marc Treib wrote:
> > > > > Not your doing, but is this include needed?
> > > > 
> > > > Replaced with an include for SkBitmap
> > > > 
> > > > This requires adding a new DEP. SkBitmap was pulled in indirectly by
> > > > ui/gfx/image/image_skia.h 
> > > > 
> > > 
> > > Ah, okay. If this requires extra approvals for the new DEP, then I'm also
> fine
> > > with leaving it as-is, since it'll soon go away anyway.
> > 
> > Done.
> 
> Adding Jochen for OWNERS approval for
> chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc
> 
> Jochen could you PTAL

Replacing Jochen with Trent since I missed the OWNERS file in
chrome/browser/ui/app_list.

Trent could you pls take a look at the changes in
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc thanks a
lot

[noyau (Ping after 24h), 2016-05-24 12:58:47]

lgtm

lgtm.

[tapted, 2016-05-25 00:06:09]

app_list lgtm (sorry for the delay - #timezones)

[markusheintz_, 2016-05-25 08:16:46]

The CQ bit was checked by markusheintz@chromium.org

[markusheintz_, 2016-05-25 08:16:47]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2000653002/#ps100001
(title: "Fix Chrome OS builds by updating url_suggestion_result.h")

[commit-bot: I haz the power, 2016-05-25 08:17:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2000653002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2000653002/100001

[commit-bot: I haz the power, 2016-05-25 08:21:10]

Description was changed from

==========
Replace the usage of SkBitmap with gfx::Image in the suggestion service API.

BUG=612469
==========

to

==========
Replace the usage of SkBitmap with gfx::Image in the suggestion service API.

BUG=612469
==========

[commit-bot: I haz the power, 2016-05-25 08:21:12]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-25 08:22:26]

Description was changed from

==========
Replace the usage of SkBitmap with gfx::Image in the suggestion service API.

BUG=612469
==========

to

==========
Replace the usage of SkBitmap with gfx::Image in the suggestion service API.

BUG=612469

Committed: https://crrev.com/55ae116c1f0be2fadf1695706ebd7a11f91e3a06
Cr-Commit-Position: refs/heads/master@{#395830}
==========

[commit-bot: I haz the power, 2016-05-25 08:22:27]

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