Replace SkBitmap with gfx::Image in the ImageFetcher API.

Replace SkBitmap with gfx::Image in the ImageFetcher API and the NTPSnippetsService API.
This will increase the performance on iOS.

BUG=609127, 612760
Committed: https://crrev.com/049dce3d29e33aa1dae4228e0cfb301c391e795b
Cr-Commit-Position: refs/heads/master@{#394719}

[markusheintz_, 2016-05-13 15:35:41]

Description was changed from

==========
Replace SkBitmap with gfx::Image in the ImageFetcher API.
This will increase the performace on iOS.

BUG=609127
==========

to

==========
Replace SkBitmap with gfx::Image in the ImageFetcher API.
This will increase the performace on iOS.

BUG=609127
==========

[markusheintz_, 2016-05-13 15:35:41]

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

[markusheintz_, 2016-05-13 15:36:38]

Marc please take a first look at this CL.

We can also replace the SkBitmap type in the rest of the suggestions code once
this CL was landed.

[markusheintz_, 2016-05-13 15:41:09]

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

[markusheintz_, 2016-05-13 15:41:31]

Eric could you also take a look at this CL? Thanks a lot

[Marc Treib, 2016-05-13 15:54:00]

Generally looks good! Just a bunch of nits below.

https://codereview.chromium.org/1974013002/diff/100001/chrome/browser/search/...
File chrome/browser/search/suggestions/image_fetcher_impl.cc (right):

https://codereview.chromium.org/1974013002/diff/100001/chrome/browser/search/...
chrome/browser/search/suggestions/image_fetcher_impl.cc:75: if (bitmap != NULL)
nit: s/NULL/nullptr

https://codereview.chromium.org/1974013002/diff/100001/chrome/browser/search/...
File chrome/browser/search/suggestions/image_fetcher_impl.h (right):

https://codereview.chromium.org/1974013002/diff/100001/chrome/browser/search/...
chrome/browser/search/suggestions/image_fetcher_impl.h:26: 
nit: remove the extra empty line

https://codereview.chromium.org/1974013002/diff/100001/chrome/browser/search/...
chrome/browser/search/suggestions/image_fetcher_impl.h:29: // ImageFetcher
implementation.
nit: I'd specify "image_fetcher::ImageFetcher"

https://codereview.chromium.org/1974013002/diff/100001/chrome/browser/search/...
File chrome/browser/search/suggestions/image_fetcher_impl_browsertest.cc
(right):

https://codereview.chromium.org/1974013002/diff/100001/chrome/browser/search/...
chrome/browser/search/suggestions/image_fetcher_impl_browsertest.cc:62: } //
namespace
nitty nit: style guide says two spaces before comment

https://codereview.chromium.org/1974013002/diff/100001/chrome/browser/search/...
File chrome/browser/search/thumbnail_source.h (right):

https://codereview.chromium.org/1974013002/diff/100001/chrome/browser/search/...
chrome/browser/search/thumbnail_source.h:15: #include "ui/gfx/image/image.h"
nit: You can forward-declare this, no need to include

https://codereview.chromium.org/1974013002/diff/100001/components/image_fetch...
File components/image_fetcher/image_fetcher.h (right):

https://codereview.chromium.org/1974013002/diff/100001/components/image_fetch...
components/image_fetcher/image_fetcher.h:20: // callback will be called on the
thread which initiated the fetch. An empty
nit: I'd move the new comment to StartOrQueueNetworkRequest

https://codereview.chromium.org/1974013002/diff/100001/components/image_fetch...
File components/image_fetcher/image_fetcher_delegate.h (right):

https://codereview.chromium.org/1974013002/diff/100001/components/image_fetch...
components/image_fetcher/image_fetcher_delegate.h:23: // the image was fetched.
|bitmap| stores image data owned by the caller, and
nit: Update the comment please

https://codereview.chromium.org/1974013002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/BUILD.gn (right):

https://codereview.chromium.org/1974013002/diff/100001/components/ntp_snippet...
components/ntp_snippets/BUILD.gn:40: "//ui/gfx/",
Does the trailing slash do anything? Most of the deps don't have it, but the one
just above does...
(Doesn't really matter, just curious)

https://codereview.chromium.org/1974013002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1974013002/diff/100001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:152: const SkBitmap* bitmap =
NULL;
nit: nullptr

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

https://codereview.chromium.org/1974013002/diff/100001/components/suggestions...
components/suggestions/image_manager.cc:34: // ImageManager callbacks expect
SkBitmaps while ImageFetcher callbacks expecte
nit: s/expecte/expect

https://codereview.chromium.org/1974013002/diff/100001/components/suggestions...
components/suggestions/image_manager.cc:35: // gfx::Images. The ImageFetcher
used to return NULL when the image could not be
Remove the "used to" comment please - no need to document the past :)

https://codereview.chromium.org/1974013002/diff/100001/components/suggestions...
components/suggestions/image_manager.cc:38: base::Callback<void(const GURL&,
const SkBitmap*)> wrapped_callback,
const Callback&

https://codereview.chromium.org/1974013002/diff/100001/components/suggestions...
components/suggestions/image_manager.cc:41: const SkBitmap* bitmap = NULL;
nit: nullptr

https://codereview.chromium.org/1974013002/diff/100001/components/suggestions...
components/suggestions/image_manager.cc:42: if (!image.IsEmpty()) {
nit: no braces required

https://codereview.chromium.org/1974013002/diff/100001/components/suggestions...
components/suggestions/image_manager.cc:120: SaveImage(url,
*(image.ToSkBitmap()));
nit: no parens required around image.ToSkBitmap()

https://codereview.chromium.org/1974013002/diff/100001/ios/chrome/browser/sug...
File ios/chrome/browser/suggestions/image_fetcher_impl.h (right):

https://codereview.chromium.org/1974013002/diff/100001/ios/chrome/browser/sug...
ios/chrome/browser/suggestions/image_fetcher_impl.h:13: #include
"ui/gfx/image/image.h"
nit: forward-declare; no need to include here

[noyau (Ping after 24h), 2016-05-13 16:10:10]

Two of my comments are because the changes are not deep enough yet. I'm assuming
you are going to follow up with subsequent CL to avoid the conversions, and if
it is the case that's fine.

https://codereview.chromium.org/1974013002/diff/120001/chrome/browser/search/...
File chrome/browser/search/suggestions/image_fetcher_impl.cc (right):

https://codereview.chromium.org/1974013002/diff/120001/chrome/browser/search/...
chrome/browser/search/suggestions/image_fetcher_impl.cc:76: image =
gfx::Image::CreateFrom1xBitmap(*bitmap);
Maybe create just one gfx::Image instead of one per callback and delegate?

https://codereview.chromium.org/1974013002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1974013002/diff/120001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:149: const
NTPSnippetsService::ImageFetchedCallback& callback,
This callback needs to change as well, to carry the gfx::Image up to the point
of usage in the UI. Converting in this function is very expensive on iOS.

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

https://codereview.chromium.org/1974013002/diff/120001/components/suggestions...
components/suggestions/image_manager.cc:38: base::Callback<void(const GURL&,
const SkBitmap*)> wrapped_callback,
Same here.

The rule of thumb is: If you are in components/, this is shared code, and
skBitmap use must be eradicated…

https://codereview.chromium.org/1974013002/diff/120001/ios/chrome/browser/sug...
File ios/chrome/browser/suggestions/image_fetcher_impl.mm (right):

https://codereview.chromium.org/1974013002/diff/120001/ios/chrome/browser/sug...
ios/chrome/browser/suggestions/image_fetcher_impl.mm:54: image =
gfx::Image(ui_image);
Define a local variable here for the image. Right now you're capturing in the
block the variable in the function context, which is doing a needless copy.

Blocks are nasty :)

[markusheintz_, 2016-05-17 13:08:23]

https://codereview.chromium.org/1974013002/diff/100001/chrome/browser/search/...
File chrome/browser/search/suggestions/image_fetcher_impl.cc (right):

https://codereview.chromium.org/1974013002/diff/100001/chrome/browser/search/...
chrome/browser/search/suggestions/image_fetcher_impl.cc:75: if (bitmap != NULL)
On 2016/05/13 15:53:59, Marc Treib wrote:
> nit: s/NULL/nullptr

Done.

https://codereview.chromium.org/1974013002/diff/100001/chrome/browser/search/...
File chrome/browser/search/suggestions/image_fetcher_impl.h (right):

https://codereview.chromium.org/1974013002/diff/100001/chrome/browser/search/...
chrome/browser/search/suggestions/image_fetcher_impl.h:26: 
On 2016/05/13 15:53:59, Marc Treib wrote:
> nit: remove the extra empty line

Done.

https://codereview.chromium.org/1974013002/diff/100001/chrome/browser/search/...
chrome/browser/search/suggestions/image_fetcher_impl.h:29: // ImageFetcher
implementation.
On 2016/05/13 15:53:59, Marc Treib wrote:
> nit: I'd specify "image_fetcher::ImageFetcher"

Done.

https://codereview.chromium.org/1974013002/diff/100001/chrome/browser/search/...
File chrome/browser/search/suggestions/image_fetcher_impl_browsertest.cc
(right):

https://codereview.chromium.org/1974013002/diff/100001/chrome/browser/search/...
chrome/browser/search/suggestions/image_fetcher_impl_browsertest.cc:62: } //
namespace
On 2016/05/13 15:53:59, Marc Treib wrote:
> nitty nit: style guide says two spaces before comment

Done.

https://codereview.chromium.org/1974013002/diff/100001/chrome/browser/search/...
File chrome/browser/search/thumbnail_source.h (right):

https://codereview.chromium.org/1974013002/diff/100001/chrome/browser/search/...
chrome/browser/search/thumbnail_source.h:15: #include "ui/gfx/image/image.h"
On 2016/05/13 15:53:59, Marc Treib wrote:
> nit: You can forward-declare this, no need to include

Done.

https://codereview.chromium.org/1974013002/diff/100001/components/image_fetch...
File components/image_fetcher/image_fetcher.h (right):

https://codereview.chromium.org/1974013002/diff/100001/components/image_fetch...
components/image_fetcher/image_fetcher.h:20: // callback will be called on the
thread which initiated the fetch. An empty
On 2016/05/13 15:53:59, Marc Treib wrote:
> nit: I'd move the new comment to StartOrQueueNetworkRequest

Done.

https://codereview.chromium.org/1974013002/diff/100001/components/image_fetch...
File components/image_fetcher/image_fetcher_delegate.h (right):

https://codereview.chromium.org/1974013002/diff/100001/components/image_fetch...
components/image_fetcher/image_fetcher_delegate.h:23: // the image was fetched.
|bitmap| stores image data owned by the caller, and
On 2016/05/13 15:54:00, Marc Treib wrote:
> nit: Update the comment please

Done.

https://codereview.chromium.org/1974013002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/BUILD.gn (right):

https://codereview.chromium.org/1974013002/diff/100001/components/ntp_snippet...
components/ntp_snippets/BUILD.gn:40: "//ui/gfx/",
On 2016/05/13 15:54:00, Marc Treib wrote:
> Does the trailing slash do anything? Most of the deps don't have it, but the
one
> just above does...
> (Doesn't really matter, just curious)

TBH I don't know. Removed the trailing slash

https://codereview.chromium.org/1974013002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1974013002/diff/100001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:152: const SkBitmap* bitmap =
NULL;
On 2016/05/13 15:54:00, Marc Treib wrote:
> nit: nullptr

Done.

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

https://codereview.chromium.org/1974013002/diff/100001/components/suggestions...
components/suggestions/image_manager.cc:34: // ImageManager callbacks expect
SkBitmaps while ImageFetcher callbacks expecte
On 2016/05/13 15:54:00, Marc Treib wrote:
> nit: s/expecte/expect

Done.

https://codereview.chromium.org/1974013002/diff/100001/components/suggestions...
components/suggestions/image_manager.cc:35: // gfx::Images. The ImageFetcher
used to return NULL when the image could not be
On 2016/05/13 15:54:00, Marc Treib wrote:
> Remove the "used to" comment please - no need to document the past :)

Done.

https://codereview.chromium.org/1974013002/diff/100001/components/suggestions...
components/suggestions/image_manager.cc:38: base::Callback<void(const GURL&,
const SkBitmap*)> wrapped_callback,
On 2016/05/13 15:54:00, Marc Treib wrote:
> const Callback&

Done.

https://codereview.chromium.org/1974013002/diff/100001/components/suggestions...
components/suggestions/image_manager.cc:41: const SkBitmap* bitmap = NULL;
On 2016/05/13 15:54:00, Marc Treib wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/1974013002/diff/100001/components/suggestions...
components/suggestions/image_manager.cc:42: if (!image.IsEmpty()) {
On 2016/05/13 15:54:00, Marc Treib wrote:
> nit: no braces required

Done.

https://codereview.chromium.org/1974013002/diff/100001/components/suggestions...
components/suggestions/image_manager.cc:120: SaveImage(url,
*(image.ToSkBitmap()));
On 2016/05/13 15:54:00, Marc Treib wrote:
> nit: no parens required around image.ToSkBitmap()

Done.

https://codereview.chromium.org/1974013002/diff/100001/ios/chrome/browser/sug...
File ios/chrome/browser/suggestions/image_fetcher_impl.h (right):

https://codereview.chromium.org/1974013002/diff/100001/ios/chrome/browser/sug...
ios/chrome/browser/suggestions/image_fetcher_impl.h:13: #include
"ui/gfx/image/image.h"
On 2016/05/13 15:54:00, Marc Treib wrote:
> nit: forward-declare; no need to include here

Done.

https://codereview.chromium.org/1974013002/diff/120001/chrome/browser/search/...
File chrome/browser/search/suggestions/image_fetcher_impl.cc (right):

https://codereview.chromium.org/1974013002/diff/120001/chrome/browser/search/...
chrome/browser/search/suggestions/image_fetcher_impl.cc:76: image =
gfx::Image::CreateFrom1xBitmap(*bitmap);
On 2016/05/13 16:10:09, noyau wrote:
> Maybe create just one gfx::Image instead of one per callback and delegate?

Done.

https://codereview.chromium.org/1974013002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1974013002/diff/120001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:149: const
NTPSnippetsService::ImageFetchedCallback& callback,
On 2016/05/13 16:10:09, noyau wrote:
> This callback needs to change as well, to carry the gfx::Image up to the point
> of usage in the UI. Converting in this function is very expensive on iOS.

Absolutely. The same is true for the suggestions service. I will create more
CLs. Just want to keep them nice and short :-)

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

https://codereview.chromium.org/1974013002/diff/120001/components/suggestions...
components/suggestions/image_manager.cc:38: base::Callback<void(const GURL&,
const SkBitmap*)> wrapped_callback,
On 2016/05/13 16:10:09, noyau wrote:
> Same here.
> 
> The rule of thumb is: If you are in components/, this is shared code, and
> skBitmap use must be eradicated…

Same as the other. This will be changed. Just in a separate CL.

https://codereview.chromium.org/1974013002/diff/120001/ios/chrome/browser/sug...
File ios/chrome/browser/suggestions/image_fetcher_impl.mm (right):

https://codereview.chromium.org/1974013002/diff/120001/ios/chrome/browser/sug...
ios/chrome/browser/suggestions/image_fetcher_impl.mm:54: image =
gfx::Image(ui_image);
On 2016/05/13 16:10:09, noyau wrote:
> Define a local variable here for the image. Right now you're capturing in the
> block the variable in the function context, which is doing a needless copy.
> 
> Blocks are nasty :)

I was hoping that this copy is very cheep.

Done.

[Marc Treib, 2016-05-17 14:12:01]

LGTM, thanks!

https://codereview.chromium.org/1974013002/diff/140001/chrome/browser/search/...
File chrome/browser/search/suggestions/image_fetcher_impl.cc (right):

https://codereview.chromium.org/1974013002/diff/140001/chrome/browser/search/...
chrome/browser/search/suggestions/image_fetcher_impl.cc:79: callback_iter !=
request->callbacks.end(); ++callback_iter) {
optional nit: You could replace this with a range-based for, like this:
for (const auto& callback : request->callbacks)
  callback.Run(...);

https://codereview.chromium.org/1974013002/diff/140001/ios/chrome/browser/sug...
File ios/chrome/browser/suggestions/image_fetcher_impl.mm (right):

https://codereview.chromium.org/1974013002/diff/140001/ios/chrome/browser/sug...
ios/chrome/browser/suggestions/image_fetcher_impl.mm:54: gfx::Image image =
gfx::Image(ui_image);
nit: You could just write
gfx::Image image(ui_image);

https://codereview.chromium.org/1974013002/diff/140001/ios/chrome/browser/sug...
ios/chrome/browser/suggestions/image_fetcher_impl.mm:57:
delegate_->OnImageFetched(page_url, gfx_image);
s/gfx_image/image/

[markusheintz_, 2016-05-17 14:26:45]

https://codereview.chromium.org/1974013002/diff/140001/chrome/browser/search/...
File chrome/browser/search/suggestions/image_fetcher_impl.cc (right):

https://codereview.chromium.org/1974013002/diff/140001/chrome/browser/search/...
chrome/browser/search/suggestions/image_fetcher_impl.cc:79: callback_iter !=
request->callbacks.end(); ++callback_iter) {
On 2016/05/17 14:12:01, Marc Treib wrote:
> optional nit: You could replace this with a range-based for, like this:
> for (const auto& callback : request->callbacks)
>   callback.Run(...);

Good idea. Done

https://codereview.chromium.org/1974013002/diff/140001/ios/chrome/browser/sug...
File ios/chrome/browser/suggestions/image_fetcher_impl.mm (right):

https://codereview.chromium.org/1974013002/diff/140001/ios/chrome/browser/sug...
ios/chrome/browser/suggestions/image_fetcher_impl.mm:54: gfx::Image image =
gfx::Image(ui_image);
On 2016/05/17 14:12:01, Marc Treib wrote:
> nit: You could just write
> gfx::Image image(ui_image);

Done.

https://codereview.chromium.org/1974013002/diff/140001/ios/chrome/browser/sug...
ios/chrome/browser/suggestions/image_fetcher_impl.mm:57:
delegate_->OnImageFetched(page_url, gfx_image);
On 2016/05/17 14:12:01, Marc Treib wrote:
> s/gfx_image/image/

Done.

[markusheintz_, 2016-05-17 14:43:01]

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

[noyau (Ping after 24h), 2016-05-17 14:44:06]

lgtm with minor issues.

https://codereview.chromium.org/1974013002/diff/160001/ios/chrome/browser/sug...
File ios/chrome/browser/suggestions/image_fetcher_impl.mm (right):

https://codereview.chromium.org/1974013002/diff/160001/ios/chrome/browser/sug...
ios/chrome/browser/suggestions/image_fetcher_impl.mm:54: gfx::Image image =
gfx::Image(ui_image);
Can't you do:

gfx::Image image(ui_image);

instead ?

https://codereview.chromium.org/1974013002/diff/160001/ios/chrome/browser/sug...
ios/chrome/browser/suggestions/image_fetcher_impl.mm:57:
delegate_->OnImageFetched(page_url, gfx_image);
s/gfx_image/image/ ?

Or better, use gfx_image instead of image in the block.

https://codereview.chromium.org/1974013002/diff/160001/ios/chrome/browser/sug...
ios/chrome/browser/suggestions/image_fetcher_impl.mm:59: return;
Nit, and optional as it predates your changes. In case of early return, I always
favor putting the shorter code first. This is not the case here, as the empty
image case is definitely smaller but is relegated at the end. Can you reverse
the condition and treat the empty case first? It would help readability.

https://codereview.chromium.org/1974013002/diff/160001/ios/chrome/browser/sug...
ios/chrome/browser/suggestions/image_fetcher_impl.mm:62: callback.Run(page_url,
empty_image);
Please create a different, local, empty_image here, to avoid capturing and
copying the variable from outside the block.

[markusheintz_, 2016-05-17 14:44:55]

Adding Dana as reviewer.

I'm adding a dep on ui/gfx in this CL. Could you please take a look or dispatch
to another ui/gfx OWNER? Thanks a lot

[markusheintz_, 2016-05-17 15:00:43]

https://codereview.chromium.org/1974013002/diff/160001/ios/chrome/browser/sug...
File ios/chrome/browser/suggestions/image_fetcher_impl.mm (right):

https://codereview.chromium.org/1974013002/diff/160001/ios/chrome/browser/sug...
ios/chrome/browser/suggestions/image_fetcher_impl.mm:54: gfx::Image image =
gfx::Image(ui_image);
On 2016/05/17 14:44:05, noyau wrote:
> Can't you do:
> 
> gfx::Image image(ui_image);
> 
> instead ?

Yes :-) Sorry.

https://codereview.chromium.org/1974013002/diff/160001/ios/chrome/browser/sug...
ios/chrome/browser/suggestions/image_fetcher_impl.mm:57:
delegate_->OnImageFetched(page_url, gfx_image);
On 2016/05/17 14:44:05, noyau wrote:
> s/gfx_image/image/ ?
> 
> Or better, use gfx_image instead of image in the block.

Done.

https://codereview.chromium.org/1974013002/diff/160001/ios/chrome/browser/sug...
ios/chrome/browser/suggestions/image_fetcher_impl.mm:59: return;
On 2016/05/17 14:44:05, noyau wrote:
> Nit, and optional as it predates your changes. In case of early return, I
always
> favor putting the shorter code first. This is not the case here, as the empty
> image case is definitely smaller but is relegated at the end. Can you reverse
> the condition and treat the empty case first? It would help readability.

I have no strong feelings and I'm happy to revers this is you prefer.

The rule I'm following here is: Let's have a positive (non negated conditions:
if (data) { ....  because they are easy to read (in this context this is trivial
of course). If I reverse it then I have if (!data) { ...   .

As an alternative I could also use an else

if (data) {
} else {
}

then I don't need the early return and the code is also very readable.

As I said I have no strong feelings. So feel free to pick your favorite
solution.

https://codereview.chromium.org/1974013002/diff/160001/ios/chrome/browser/sug...
ios/chrome/browser/suggestions/image_fetcher_impl.mm:62: callback.Run(page_url,
empty_image);
On 2016/05/17 14:44:05, noyau wrote:
> Please create a different, local, empty_image here, to avoid capturing and
> copying the variable from outside the block.

I moved the "gfx::Image empty_image;" inside the if block and create a new local
instance with the same name here now.

Blocks are really crazy.

[markusheintz_, 2016-05-17 20:56:44]

markusheintz@chromium.org changed reviewers:
+ sky@chromium.org
- danakj@chromium.org

[markusheintz_, 2016-05-17 20:59:51]

Replacing danakj@ with sky@ after talking to Dana.

Scott I'm adding ui/gfx as dependency for our component. 
Would you mind reviewing the Deps change? Thanks a lot.

[sky, 2016-05-17 21:25:40]

I'm ok with the dep, but doesn't you also need a dep on skia too?

[noyau (Ping after 24h), 2016-05-18 05:48:53]

https://codereview.chromium.org/1974013002/diff/160001/ios/chrome/browser/sug...
File ios/chrome/browser/suggestions/image_fetcher_impl.mm (right):

https://codereview.chromium.org/1974013002/diff/160001/ios/chrome/browser/sug...
ios/chrome/browser/suggestions/image_fetcher_impl.mm:59: return;
On 2016/05/17 15:00:43, markusheintz_ wrote:
> On 2016/05/17 14:44:05, noyau wrote:
> > Nit, and optional as it predates your changes. In case of early return, I
> always
> > favor putting the shorter code first. This is not the case here, as the
empty
> > image case is definitely smaller but is relegated at the end. Can you
reverse
> > the condition and treat the empty case first? It would help readability.
> 
> I have no strong feelings and I'm happy to revers this is you prefer.
> 
> The rule I'm following here is: Let's have a positive (non negated conditions:
> if (data) { ....  because they are easy to read (in this context this is
trivial
> of course). If I reverse it then I have if (!data) { ...   .
> 
> As an alternative I could also use an else
> 
> if (data) {
> } else {
> }
> 
> then I don't need the early return and the code is also very readable.
> 
> As I said I have no strong feelings. So feel free to pick your favorite
> solution.

Leave it as it is. I'll revisit this code later anyway (I don't like the scale
set to 1 by default there).

[markusheintz_, 2016-05-18 09:58:32]

On 2016/05/17 21:25:40, sky wrote:
> I'm ok with the dep, but doesn't you also need a dep on skia too?

True because of the Changes in ntp_snippets_service.cc.
The component is not supposed to depend on SKIA. I'll replace the lines in
https://codereview.chromium.org/1995493002/.

I guess I'll merge https://codereview.chromium.org/1995493002/ in this CL.

[markusheintz_, 2016-05-18 11:22:32]

On 2016/05/18 09:58:32, markusheintz_ wrote:
> On 2016/05/17 21:25:40, sky wrote:
> > I'm ok with the dep, but doesn't you also need a dep on skia too?
> 
> True because of the Changes in ntp_snippets_service.cc.
> The component is not supposed to depend on SKIA. I'll replace the lines in
> <crxref style="background-image:
url("chrome-extension://ppnlfijacbhfpjgkafbeiaklmgoljooo/res/type/codereview.png");">https://codereview.chromium.org/1995493002/</crxref>.
> 
> I guess I'll merge <crxref style="background-image:
url("chrome-extension://ppnlfijacbhfpjgkafbeiaklmgoljooo/res/type/codereview.png");">https://codereview.chromium.org/1995493002/</crxref>
in this CL.

@sky: I merged https://codereview.chromium.org/1995493002/ into this CL. This
way I don't need to add Skia DEPS. Thanks for the catch

@noyau,treib: Since both CL are LGTMed I guess your LGTM for this one still
holds :-)

[markusheintz_, 2016-05-18 11:23:08]

Description was changed from

==========
Replace SkBitmap with gfx::Image in the ImageFetcher API.
This will increase the performace on iOS.

BUG=609127
==========

to

==========
Replace SkBitmap with gfx::Image in the ImageFetcher API.
This will increase the performace on iOS.

BUG=609127,612760
==========

[markusheintz_, 2016-05-18 11:25:00]

Description was changed from

==========
Replace SkBitmap with gfx::Image in the ImageFetcher API.
This will increase the performace on iOS.

BUG=609127,612760
==========

to

==========
Replace SkBitmap with gfx::Image in the ImageFetcher API and the
NTPSnippetsService API.
This will increase the performace on iOS.

BUG=609127,612760
==========

[markusheintz_, 2016-05-18 11:25:21]

Description was changed from

==========
Replace SkBitmap with gfx::Image in the ImageFetcher API and the
NTPSnippetsService API.
This will increase the performace on iOS.

BUG=609127,612760
==========

to

==========
Replace SkBitmap with gfx::Image in the ImageFetcher API and the
NTPSnippetsService API.
This will increase the performance on iOS.

BUG=609127,612760
==========

[Marc Treib, 2016-05-18 11:34:30]

Yup, still LGTM :)

[sky, 2016-05-18 16:46:44]

LGTM

[markusheintz_, 2016-05-18 16:49:29]

The CQ bit was checked by markusheintz@chromium.org

[markusheintz_, 2016-05-18 16:49:29]

The patchset sent to the CQ was uploaded after l-g-t-m from noyau@chromium.org
Link to the patchset: https://codereview.chromium.org/1974013002/#ps220001
(title: "Merge https://codereview.chromium.org/1995493002/")

[commit-bot: I haz the power, 2016-05-18 16:49:46]

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

[commit-bot: I haz the power, 2016-05-18 16:56:49]

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

[commit-bot: I haz the power, 2016-05-18 16:56:51]

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

[markusheintz_, 2016-05-18 17:00:09]

The CQ bit was checked by markusheintz@chromium.org

[markusheintz_, 2016-05-18 17:00:10]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
noyau@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1974013002/#ps240001
(title: "Sync")

[commit-bot: I haz the power, 2016-05-18 17:00:24]

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

[commit-bot: I haz the power, 2016-05-18 17:06:18]

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

[commit-bot: I haz the power, 2016-05-18 17:06:20]

Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[sdefresne, 2016-05-18 18:16:35]

The ios failure should have been fixed, no need to update, you can just send it
back to the bots.

[noyau (Ping after 24h), 2016-05-19 08:15:19]

The CQ bit was checked by noyau@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
noyau@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1974013002/#ps260001
(title: "Sync again")

[commit-bot: I haz the power, 2016-05-19 08:15:36]

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

[commit-bot: I haz the power, 2016-05-19 08:43:39]

Description was changed from

==========
Replace SkBitmap with gfx::Image in the ImageFetcher API and the
NTPSnippetsService API.
This will increase the performance on iOS.

BUG=609127,612760
==========

to

==========
Replace SkBitmap with gfx::Image in the ImageFetcher API and the
NTPSnippetsService API.
This will increase the performance on iOS.

BUG=609127,612760
==========

[commit-bot: I haz the power, 2016-05-19 08:43:41]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-05-19 08:44:48]

Description was changed from

==========
Replace SkBitmap with gfx::Image in the ImageFetcher API and the
NTPSnippetsService API.
This will increase the performance on iOS.

BUG=609127,612760
==========

to

==========
Replace SkBitmap with gfx::Image in the ImageFetcher API and the
NTPSnippetsService API.
This will increase the performance on iOS.

BUG=609127,612760

Committed: https://crrev.com/049dce3d29e33aa1dae4228e0cfb301c391e795b
Cr-Commit-Position: refs/heads/master@{#394719}
==========

[commit-bot: I haz the power, 2016-05-19 08:44:50]

Patchset 14 (id:??) landed as
https://crrev.com/049dce3d29e33aa1dae4228e0cfb301c391e795b
Cr-Commit-Position: refs/heads/master@{#394719}