Migrate LauncherSearchResult to use SetBadgeIcon.

Migrate LauncherSearchResult to use SetBadgeIcon.

Renamed ExtensionBadgedIconImage to LauncherSearchIconImageLoader.
Logics to load and select icons are kept in separated class from
LauncherSearchResult to keep it easily testable.

BUG=497897
TEST=unit_tests:LauncherSearchIconImageLoaderTest.*

Committed: https://crrev.com/fa271c3b0a42ae476ca43d6e6f45ad96a61bf761
Cr-Commit-Position: refs/heads/master@{#336993}

[yawano, 2015-06-22 10:13:00]

yawano@chromium.org changed reviewers:
+ mgiuca@chromium.org

[yawano, 2015-06-22 10:13:00]

PTAL. Thank you!

[Matt Giuca, 2015-06-24 08:26:07]

This is most excellent! I'm proposing a small refactor, sorry, and an optional
large refactor. Otherwise good.

https://codereview.chromium.org/1200543003/diff/1/chrome/browser/ui/app_list/...
File
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h
(right):

https://codereview.chromium.org/1200543003/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h:26:
LauncherSearchIconImageLoader* image_loader) = 0;
I think this should be split into two callbacks, OnIconImageChanged and
OnBadgeIconImageChanged.

When either of these changes, it is only necessary to update one of them in the
caller.

Also: I'm not sure about this (only if you think it's appropriate), but maybe
this whole class (LauncherSearchIconImageLoader) should be split into two, one
for icon image and one for badge icon image. You would only do that if there is
no real relationship between the two parts.

[yawano, 2015-06-24 08:43:11]

Thank you for the review!

https://codereview.chromium.org/1200543003/diff/1/chrome/browser/ui/app_list/...
File
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h
(right):

https://codereview.chromium.org/1200543003/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h:26:
LauncherSearchIconImageLoader* image_loader) = 0;
For splitting a callback into two, I'll create a patch set.

For splitting this class into two, I want to keep this as a single class. The
reason is that I want to keep the icon selection and error handling logic in
this class for testing purpose. The actual image loads are done by
extensions::IconImage and extensions::ImageLoader. The role of this class is to
call these loaders properly and selecting/passing the images to the search
results with handling errors.

[Matt Giuca, 2015-06-25 08:01:28]

lgtm

https://codereview.chromium.org/1200543003/diff/1/chrome/browser/ui/app_list/...
File
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h
(right):

https://codereview.chromium.org/1200543003/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h:26:
LauncherSearchIconImageLoader* image_loader) = 0;
OK if you create a second patch set to do the callback splitting.

I don't mind if you keep this in one class.

[yawano, 2015-06-25 08:20:40]

@mgiuca: Could you take a look at the new patch set? I changed significant
amount of logic. Thank you!

The key changes are
- When it fails to load custom icon, it doesn't update it (*1).
- Extension icon image cannot be null.
- Hold images as main icon image and badge icon image (previously it holds them
as extension icon image and custom icon image and selected them in getters).

*1: In the previous patch set, when it fails to load new custom icon, it falls
back to extension icon only style, i.e. once it goes to badged icon style, it
doesn't go back to extension icon only style.

[Matt Giuca, 2015-06-25 08:29:37]

Hmm OK, i thought you were going to land this and make a new CL to do that.
Sorry, I think I'm too tired to review a new patch set now, so it will have to
be tomorrow.

Either revert PS2 and land PS1 or wait until tomorrow.

Thanks

Matt

[yawano, 2015-06-25 08:34:01]

Sorry, the word "patch set" might be confusing.
I'll wait until tomorrow.

Thank you.
Have a good day!

[Matt Giuca, 2015-06-25 08:40:20]

Sorry, I must be very tired. You said "patch set" and I replied with "patch set"
but somehow I thought you meant CL. My fault :)

[Matt Giuca, 2015-06-29 06:33:17]

https://codereview.chromium.org/1200543003/diff/20001/chrome/browser/ui/app_l...
File
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc
(right):

https://codereview.chromium.org/1200543003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc:117:
if (!is_badged_) {
This won't be necessary if you take my advice.

https://codereview.chromium.org/1200543003/diff/20001/chrome/browser/ui/app_l...
File
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h
(right):

https://codereview.chromium.org/1200543003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h:29:
// GetbadgeIconImage method.
nit: GetBadgeIconImage

https://codereview.chromium.org/1200543003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h:90:
scoped_ptr<chromeos::launcher_search_provider::ErrorReporter> error_reporter_;
Nit: Why did this line move up there?

https://codereview.chromium.org/1200543003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h:93:
gfx::ImageSkia badge_icon_image_;
It looks like the logic behind these fields is:
- If no custom image supplied: icon_image is extension image, badge_icon_image
is empty.
- If custom image supplied: icon_image is custom image, badge_icon_image is
extension image.

This seems unnecessarily complicated (I see you have logic to dance images
between these two fields). You could just keep extension_icon_image and
custom_icon_image, and have the GetIconImage() and GetBadgeIconImage() getters
apply that logic to decide which image to return.

You'll just have to be careful about calling the Notify functions correctly.

https://codereview.chromium.org/1200543003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h:94:
bool is_badged_;
This bool will not be necessary after that suggestion, just check whether
custom_icon_image is non-empty. (Maybe add a getter IsBadged() that checks for
you.)

[yawano, 2015-06-29 07:27:03]

PTAL. Thank you!

https://codereview.chromium.org/1200543003/diff/20001/chrome/browser/ui/app_l...
File
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc
(right):

https://codereview.chromium.org/1200543003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc:117:
if (!is_badged_) {
On 2015/06/29 06:33:16, Matt Giuca wrote:
> This won't be necessary if you take my advice.

Done.

https://codereview.chromium.org/1200543003/diff/20001/chrome/browser/ui/app_l...
File
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h
(right):

https://codereview.chromium.org/1200543003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h:29:
// GetbadgeIconImage method.
On 2015/06/29 06:33:16, Matt Giuca wrote:
> nit: GetBadgeIconImage

Done.

https://codereview.chromium.org/1200543003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h:90:
scoped_ptr<chromeos::launcher_search_provider::ErrorReporter> error_reporter_;
I just wanted to bring up the variables initialized in the initialization list
first.

https://codereview.chromium.org/1200543003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h:93:
gfx::ImageSkia badge_icon_image_;
SGTM. Thank you! Done.

https://codereview.chromium.org/1200543003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h:94:
bool is_badged_;
On 2015/06/29 06:33:16, Matt Giuca wrote:
> This bool will not be necessary after that suggestion, just check whether
> custom_icon_image is non-empty. (Maybe add a getter IsBadged() that checks for
> you.)

Done.

[Matt Giuca, 2015-06-30 01:53:48]

https://codereview.chromium.org/1200543003/diff/20001/chrome/browser/ui/app_l...
File
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h
(right):

https://codereview.chromium.org/1200543003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h:90:
scoped_ptr<chromeos::launcher_search_provider::ErrorReporter> error_reporter_;
On 2015/06/29 07:27:03, yawano wrote:
> I just wanted to bring up the variables initialized in the initialization list
> first.

Acknowledged.

https://codereview.chromium.org/1200543003/diff/50015/chrome/browser/ui/app_l...
File
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc
(right):

https://codereview.chromium.org/1200543003/diff/50015/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc:44:
CHECK(custom_icon_image_.isNull());
Do these need to be CHECKs? Can they just be DCHECKs? (Is there any security
reason to crash if these are false.)

https://codereview.chromium.org/1200543003/diff/50015/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc:84:
if (custom_icon_image_.isNull())
// If no custom icon is supplied, return the extension icon.

https://codereview.chromium.org/1200543003/diff/50015/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc:86:
else
No else after return.

https://codereview.chromium.org/1200543003/diff/50015/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc:91:
if (custom_icon_image_.isNull())
// If a custom icon is supplied, badge it with the extension icon.

https://codereview.chromium.org/1200543003/diff/50015/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc:93:
else
No else after return.

https://codereview.chromium.org/1200543003/diff/50015/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc:104:
// When custom_icon_image_ is not set, extension icon image will be shown as
|custom_icon_image_|

https://codereview.chromium.org/1200543003/diff/50015/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc:126:
const bool first_custom_icon = custom_icon_image_.isNull();
Can we call this |previously_unbadged|?

[yawano, 2015-07-01 04:53:32]

PTAL. Thank you!

https://codereview.chromium.org/1200543003/diff/50015/chrome/browser/ui/app_l...
File
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc
(right):

https://codereview.chromium.org/1200543003/diff/50015/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc:44:
CHECK(custom_icon_image_.isNull());
For this custom_icon_image_ check, it shouldn't be.

For other checks of extension_icon_image_, I think they should be. If we show
custom_icon_image_ without extension_icon_image_, it shows unbadged custom icon.
It should not happen from security perspective.

https://codereview.chromium.org/1200543003/diff/50015/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc:84:
if (custom_icon_image_.isNull())
On 2015/06/30 01:53:48, Matt Giuca wrote:
> // If no custom icon is supplied, return the extension icon.

Done.

https://codereview.chromium.org/1200543003/diff/50015/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc:86:
else
On 2015/06/30 01:53:48, Matt Giuca wrote:
> No else after return.

Done.

https://codereview.chromium.org/1200543003/diff/50015/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc:91:
if (custom_icon_image_.isNull())
On 2015/06/30 01:53:48, Matt Giuca wrote:
> // If a custom icon is supplied, badge it with the extension icon.

Done.

https://codereview.chromium.org/1200543003/diff/50015/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc:93:
else
On 2015/06/30 01:53:48, Matt Giuca wrote:
> No else after return.

Done.

https://codereview.chromium.org/1200543003/diff/50015/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc:104:
// When custom_icon_image_ is not set, extension icon image will be shown as
On 2015/06/30 01:53:48, Matt Giuca wrote:
> |custom_icon_image_|

Done.

https://codereview.chromium.org/1200543003/diff/50015/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc:126:
const bool first_custom_icon = custom_icon_image_.isNull();
On 2015/06/30 01:53:48, Matt Giuca wrote:
> Can we call this |previously_unbadged|?

Done.

[Matt Giuca, 2015-07-01 07:13:08]

LGTM, thanks!

https://codereview.chromium.org/1200543003/diff/50015/chrome/browser/ui/app_l...
File
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc
(right):

https://codereview.chromium.org/1200543003/diff/50015/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc:44:
CHECK(custom_icon_image_.isNull());
On 2015/07/01 04:53:32, yawano wrote:
> For this custom_icon_image_ check, it shouldn't be.
> 
> For other checks of extension_icon_image_, I think they should be. If we show
> custom_icon_image_ without extension_icon_image_, it shows unbadged custom
icon.
> It should not happen from security perspective.

Acknowledged.

[yawano, 2015-07-01 07:13:47]

On 2015/07/01 07:13:08, Matt Giuca wrote:
> LGTM, thanks!
> 
>
https://codereview.chromium.org/1200543003/diff/50015/chrome/browser/ui/app_l...
> File
>
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc
> (right):
> 
>
https://codereview.chromium.org/1200543003/diff/50015/chrome/browser/ui/app_l...
>
chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.cc:44:
> CHECK(custom_icon_image_.isNull());
> On 2015/07/01 04:53:32, yawano wrote:
> > For this custom_icon_image_ check, it shouldn't be.
> > 
> > For other checks of extension_icon_image_, I think they should be. If we
show
> > custom_icon_image_ without extension_icon_image_, it shows unbadged custom
> icon.
> > It should not happen from security perspective.
> 
> Acknowledged.

Thank you!

[yawano, 2015-07-01 07:13:52]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2015-07-01 07:14:02]

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

[commit-bot: I haz the power, 2015-07-01 07:58:00]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2015-07-01 07:59:05]

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