Support custom icon in search result.

Support custom icon in search result.

In chrome.launcherSearchProvider API, support iconUrl field in search result.
Currently only chrome-extension:// url is supported.

BUG=440649, 486236
TEST=out/Release/unit_tests --gtest_filter=ExtensionBadgedIconImageTest.*

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

[yawano, 2015-04-21 06:45:44]

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

[yawano, 2015-04-21 06:45:44]

PTAL.

@satorux: service.cc under chrome/browser/chromeos. Sorry for asking this. I
added a TODO not to forget the comment in
https://codereview.chromium.org/1095793002/diff/20001/chrome/browser/chromeos...

@mgiuca: the rest of files.

Thank you!

[satorux1, 2015-04-22 01:44:53]

c/b/chromeos LGTM with a suggestion

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/launcher_search_provider/service.cc (right):

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/launcher_search_provider/service.cc:137: //
TODO(yawano): Cache this result for optimization.
also add a link to crbug.com?

[Matt Giuca, 2015-04-22 03:31:38]

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

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:30:
canvas->DrawImageInt(custom_icon_, 0, 0);
Have you tested what happens if the custom icon has the wrong size? It probably
should get scaled to |icon_size|.

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:35:
gfx::ImageSkia resized_extension_icon =
Can you do this in the constructor and store the resized_extension_icon instead
of storing the extension icon?

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:71:
icon_url.host() == extension->id()) {
I think you should differentiate between |icon_url.is_empty()| (which is
documented as being valid, meaning we just want the extension icon), and
|icon_url| being an invalid URL (e.g., not in the chrome-extension scheme).

In the latter case, some error should be communicated, even if it is just
printed to the JavaScript console. Is it possible to do that? But you should not
print an error simply if the URL is empty.

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:106:
custom_icon_image_ = image.AsImageSkia();
Have you tested what happens if the file could not be loaded? Just from looking
at the code, it seems that it will result in a null ImageSkia (which I see you
check for later).

But as above, I think there should be some error communicated (possibly to the
JavaScript console) if this happens. Maybe you can check for that here (but I'm
not sure how hard it is to get into the extension's JS console from here).

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

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.h:28:
ExtensionBadgedIconImage(const GURL& icon_url,
custom_icon_url

Also add comment:
// If |icon_url| is empty, uses the extension icon.

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.h:41:
// Called by |extension_icon_image_| when extension icon image is changed.
Convention is not to document overrides. Just replace this comment with
// extensions::IconImage::Observer overrides:

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.h:45:
gfx::ImageSkia GetIconImage() const;
const gfx::ImageSkia&

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.h:60:
// Badged icon image.
Remove this comment.

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.h:63:
std::set<Observer*> observers_;
nit: Blank line after.

Also, maybe swap this with |icon_size_| since it is the only field that doesn't
have anything to do with the images. Best to group those all together.

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

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:61:
ExtensionBadgedIconImage* icon_image) {
Don't rename this to icon_image. Keeping it as image makes the DCHECK less
confusing.

[yawano, 2015-04-22 12:23:47]

Thank you for the review! PTAL.

@satorux: I added new files and modified files under chrome/browser/chromeos to
show warning messages in developer console of the extension. PTAL files under
chrome/browser/chromeos again (5 files in total). Thank you.

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/launcher_search_provider/service.cc (right):

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/launcher_search_provider/service.cc:137: //
TODO(yawano): Cache this result for optimization.
On 2015/04/22 01:44:53, satorux1 wrote:
> also add a link to crbug.com?

Done.

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

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:30:
canvas->DrawImageInt(custom_icon_, 0, 0);
Fixed the call of extensions::ImageLoader to resize custom icon to |icon_size|.

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:35:
gfx::ImageSkia resized_extension_icon =
On 2015/04/22 03:31:37, Matt Giuca wrote:
> Can you do this in the constructor and store the resized_extension_icon
instead
> of storing the extension icon?

Done.

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:71:
icon_url.host() == extension->id()) {
On 2015/04/22 03:31:37, Matt Giuca wrote:
> I think you should differentiate between |icon_url.is_empty()| (which is
> documented as being valid, meaning we just want the extension icon), and
> |icon_url| being an invalid URL (e.g., not in the chrome-extension scheme).
> 
> In the latter case, some error should be communicated, even if it is just
> printed to the JavaScript console. Is it possible to do that? But you should
not
> print an error simply if the URL is empty.

Done.

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:106:
custom_icon_image_ = image.AsImageSkia();
I tested this and confirmed that empty image is returned for a case that it
failed to load an image. I added code to show warning message in developer
console for that case.

Since this is not a fatal error, it shows message as warning. Should it be error
message?

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

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.h:28:
ExtensionBadgedIconImage(const GURL& icon_url,
On 2015/04/22 03:31:37, Matt Giuca wrote:
> custom_icon_url
> 
> Also add comment:
> // If |icon_url| is empty, uses the extension icon.

Done.

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.h:41:
// Called by |extension_icon_image_| when extension icon image is changed.
On 2015/04/22 03:31:37, Matt Giuca wrote:
> Convention is not to document overrides. Just replace this comment with
> // extensions::IconImage::Observer overrides:

Done.

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.h:45:
gfx::ImageSkia GetIconImage() const;
On 2015/04/22 03:31:37, Matt Giuca wrote:
> const gfx::ImageSkia&

Done.

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.h:60:
// Badged icon image.
On 2015/04/22 03:31:37, Matt Giuca wrote:
> Remove this comment.

Done.

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.h:63:
std::set<Observer*> observers_;
On 2015/04/22 03:31:37, Matt Giuca wrote:
> nit: Blank line after.
> 
> Also, maybe swap this with |icon_size_| since it is the only field that
doesn't
> have anything to do with the images. Best to group those all together.

Done.

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

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:61:
ExtensionBadgedIconImage* icon_image) {
On 2015/04/22 03:31:38, Matt Giuca wrote:
> Don't rename this to icon_image. Keeping it as image makes the DCHECK less
> confusing.

Done.

[satorux1, 2015-04-24 01:31:18]

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/launcher_search_provider.cc (right):

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/launcher_search_provider.cc:36:
CHECK(sender);
CHECK -> DCHECK

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/launcher_search_provider/error_reporter.h (right):

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/launcher_search_provider/error_reporter.h:15: class
ErrorReporter {
class comment is missing

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/launcher_search_provider/error_reporter.h:23:
IPC::Sender* sender_;
is this owned by the class? if so, add "// Owned". otherwise "// Not owned".

[Matt Giuca, 2015-04-24 04:26:46]

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

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:71:
icon_url.host() == extension->id()) {
Thanks for adding error checking!

https://codereview.chromium.org/1060733003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:106:
custom_icon_image_ = image.AsImageSkia();
A warning is fine.

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/launcher_search_provider.cc (right):

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/launcher_search_provider.cc:19: bool
LauncherSearchProviderSetSearchResultsFunction::RunSync() {
I think this is getting complex enough that it really needs tests. I don't think
the launcher search provider really has tests yet, does it?

There are too many new edge cases being introduced in this CL (invalid icon
URLs, icon loading failures, error reporting, URL truncation) that I think they
really should be behind a test suite.

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/launcher_search_provider.cc:38:
chromeos::launcher_search_provider::ErrorReporter error_reporter(sender,
Have you tested the loading error case (the error in OnCustomIconLoaded)? Just
tracing through with my eyes, it seems like this is a use-after-free on the
ErrorReporter: it is a local variable in RunSync, yet it gets stored by
reference in a bunch of classes which will remain alive over an async call to
load the icon. If the icon load fails (asynchronously), this ErrorReporter will
be used.

You can make the ErrorReporter a by-value class that is copyable, or pass around
via scoped_ptr.

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/launcher_search_provider/error_reporter.h (right):

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/launcher_search_provider/error_reporter.h:22: private:
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/ui/app_l...
File
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc
(right):

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:77:
if (!icon_url.is_empty()) {
Could you move this entire function *except* for Update() to a separate private
method? Then you can rewrite it with early exits like this:

if (icon_url.is_empty())
  return;

if (!icon_url.is_valid() || ... || ...) {
  // Warn.
  return;
}

// Main loading code.

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:98:
"Invalid url is provided. Only chrome-extension:// url is valid. " +
This message is a bit hard to read.

What about "Invalid icon URL: $URL. Must be a valid URL within
chrome-extension://$EXTENSION_ID."

Also, use kExtensionScheme instead of hard-coding "chrome-extension" here.

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:129:
"Failed to load icon url. " +
nit: "Failed to load icon URL: "

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:131:
} else {
No else; just return early in the error case.

[yawano, 2015-04-28 12:11:28]

Thank you for the review.

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/launcher_search_provider.cc (right):

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/launcher_search_provider.cc:19: bool
LauncherSearchProviderSetSearchResultsFunction::RunSync() {
LauncherSearchProvider API has no test at now. While it's still work in
progress, I added a new patch set to confirm that I'm not going to a wrong way.

I tried to write an unittest of ExtensionBadgedIconImage at first since the
class is one of the most complex part of this API. However, I noticed that it
was difficult since the class depends on many other classes. I decided to
implement the test as API test. It would be the best time to implement test
framework for launcherSearchProvider, now.

[Matt Giuca, 2015-04-30 01:34:09]

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/launcher_search_provider.cc (right):

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/launcher_search_provider.cc:19: bool
LauncherSearchProviderSetSearchResultsFunction::RunSync() {
I think writing a unit test for ExtensionBadgedIconImage is the right way to go,
since it is a complex component in its own right. Having an end-to-end API test
is not a substitute for a proper unit test of EBII. I have written some notes in
the EBII constructor that might be helpful in getting to a unit test.

It looks like you've done a lot of good work writing an end-to-end API test for
LauncherSearchProvider. That's great, but it certainly does not belong in this
CL. It has almost doubled the size of this CL, and the API testing is not
relevant to supporting custom icons. Please remove the new code added by Patch
Set 6 and move it to a different CL (either before this or after this).

https://codereview.chromium.org/1060733003/diff/100001/chrome/browser/ui/app_...
File
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc
(right):

https://codereview.chromium.org/1060733003/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:92:
extensions::file_util::ExtensionURLToRelativeFilePath(icon_url);
I think you could make this testable (without having to create an entire
extension) by abstracting this part out into a function.

virtual bool GetExtensionIconResource(const GURL& icon_url,
extensions::ExtensionResource*) = 0;

And then make a subclass ExtensionBadgedIconImageImpl with the concrete
implementation. Then, you can make a testing subclass with a dummy
implementation that just returns a dummy resource.

You would have to move this logic out of the constructor (as you can't call a
virtual method from a base class constructor).

https://codereview.chromium.org/1060733003/diff/100001/chrome/common/extensio...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/1060733003/diff/100001/chrome/common/extensio...
chrome/common/extensions/api/_permission_features.json:546:
"5065C83E84321221675D20FBE4DC43C655CC8C6F"   // Test Extension
We do not globally whitelist test extensions. Instead, you can use switches to
temporarily whitelist the extension from within the test. See:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

[yawano, 2015-04-30 03:38:52]

Thank you for the advise.

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/launcher_search_provider.cc (right):

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/launcher_search_provider.cc:19: bool
LauncherSearchProviderSetSearchResultsFunction::RunSync() {
I'm going to move the patch set 6 to different CL. Since patch set 6 relies on
this CL, I'm going to focus on this CL first.

https://codereview.chromium.org/1060733003/diff/100001/chrome/browser/ui/app_...
File
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc
(right):

https://codereview.chromium.org/1060733003/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:92:
extensions::file_util::ExtensionURLToRelativeFilePath(icon_url);
Okay, I'll extract it as a method. I'm also going to extract
extension_icon_image_ since it also relies on other classes. I'll create two
methods.

virtual const gfx::ImageSkia& GetExtensionIcon(Profile* profile, const
Extension* extension, const int icon_dimension) = 0;
virtual bool GetExtensionIconResource(const GURL& icon_Url,
extensions::ExtensionResource* resource) = 0;

[yawano, 2015-05-01 06:09:55]

@mgiuca, @satorux: PTAL. Thank you!

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/launcher_search_provider.cc (right):

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/launcher_search_provider.cc:36:
CHECK(sender);
On 2015/04/24 01:31:18, satorux1 wrote:
> CHECK -> DCHECK

Done.

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/launcher_search_provider.cc:38:
chromeos::launcher_search_provider::ErrorReporter error_reporter(sender,
Changed this to scoped_refptr since multiple result instances have reference to
an error_reporter.

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/launcher_search_provider/error_reporter.h (right):

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/launcher_search_provider/error_reporter.h:15: class
ErrorReporter {
On 2015/04/24 01:31:18, satorux1 wrote:
> class comment is missing

Done.

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/launcher_search_provider/error_reporter.h:22: private:
On 2015/04/24 04:26:46, Matt Giuca wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/launcher_search_provider/error_reporter.h:23:
IPC::Sender* sender_;
On 2015/04/24 01:31:18, satorux1 wrote:
> is this owned by the class? if so, add "// Owned". otherwise "// Not owned".

Done.

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/ui/app_l...
File
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc
(right):

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:77:
if (!icon_url.is_empty()) {
Moved this to LoadResources. We needed to extract this logic outside of
constructor since we cannot call virtual method from constructor.

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:98:
"Invalid url is provided. Only chrome-extension:// url is valid. " +
On 2015/04/24 04:26:46, Matt Giuca wrote:
> This message is a bit hard to read.
> 
> What about "Invalid icon URL: $URL. Must be a valid URL within
> chrome-extension://$EXTENSION_ID."
> 
> Also, use kExtensionScheme instead of hard-coding "chrome-extension" here.

Done.

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:129:
"Failed to load icon url. " +
On 2015/04/24 04:26:46, Matt Giuca wrote:
> nit: "Failed to load icon URL: "

Done.

https://codereview.chromium.org/1060733003/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:131:
} else {
On 2015/04/24 04:26:46, Matt Giuca wrote:
> No else; just return early in the error case.

Done.

https://codereview.chromium.org/1060733003/diff/100001/chrome/common/extensio...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/1060733003/diff/100001/chrome/common/extensio...
chrome/common/extensions/api/_permission_features.json:546:
"5065C83E84321221675D20FBE4DC43C655CC8C6F"   // Test Extension
On 2015/04/30 01:34:09, Matt Giuca wrote:
> We do not globally whitelist test extensions. Instead, you can use switches to
> temporarily whitelist the extension from within the test. See:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

Acknowledged.

[satorux1, 2015-05-03 23:51:24]

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/launcher_search_provider/error_reporter.h (right):

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/launcher_search_provider/error_reporter.h:17: class
ErrorReporter : public base::RefCounted<ErrorReporter> {
Why does it have to be a ref counted object? Ref counting should generally be
avoided. IIRC, this wasn't a ref counted class when I originally saw the patch.

[Matt Giuca, 2015-05-05 04:25:31]

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/launcher_search_provider/error_reporter.cc (right):

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/launcher_search_provider/error_reporter.cc:21: if
(sender_ == nullptr)
DCHECK(sender);

The only time sender should be null is in MockErrorReporter, and that overrides
Warn. So it is incorrect to allow nullptr here.

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/launcher_search_provider/error_reporter.h (right):

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/launcher_search_provider/error_reporter.h:17: class
ErrorReporter : public base::RefCounted<ErrorReporter> {
There was a use-after-free in the older patch set.

Can you make it by-value and copyable instead? ErrorReporter is essentially just
a struct that can be freely copied around (since it doesn't own any objects).
Much simpler than refcounted.

(See the earlier discussion in launcher_search_provider.cc.)

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
File
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.h
(right):

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.h:53:
// Loads |extension| icon and returns it as sync if possible. When it loads
What does it return if it decides to use async? A null ImageSkia?

(I can't see any logic for this in
ExtensionBadgedIconImageImpl::LoadExtensionIcon; it always seems to be sync.)

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.h:60:
virtual void LoadExtensionIconResource() = 0;
So LoadExtensionIcon loads the extension's icon, whereas
LoadExtensionIconResource loads an arbitrary icon resource from an extension
URL, correct?

The naming is very confusing (they seem so similar, it seems at first glance
that they are both loading an extension icon in different ways).

Can we rename this to LoadIconResourceFromExtension?

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
File
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_impl.h
(right):

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_impl.h:15:
class ExtensionBadgedIconImageImpl : public ExtensionBadgedIconImage,
Class comment.

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
File
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_unittest.cc
(right):

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_unittest.cc:21:
namespace {
nit: blank line above

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_unittest.cc:23:
const char kFakeExtensionId[] = "foo";
kTestExtensionId
kTestCustomIconURL

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_unittest.cc:27:
// # |primary_fill_color|
I found it pretty hard to understand this comment at first. Can you write it
like this:

// This image source generates following image:
//
// ###
// #**
// #**
//
// where # is the primary fill color, and * is the secondary fill color.

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_unittest.cc:68:
using ExtensionBadgedIconImage::ExtensionBadgedIconImage;  // Use base class
Nit: Don't wrap inline comments like this, just place it on the line above.

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_unittest.cc:104:
class MockErrorReporter : public ErrorReporter {
nit: This is a fake, not a mock. FakeErrorReporter.

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_unittest.cc:121:
scoped_refptr<extensions::Extension> CreateFakeExtension(
CreateTestExtension

(It's not a fake extension, it's a test extension. A fake is the class stub
you've created above.)

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_unittest.cc:156:
GURL icon_url;
GURL icon_url;  // No custom icon.

(Add this comment to the end of the icon_url declaration line, not in /**/
comments inside the function call.

Same below.

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_unittest.cc:208:
std::string invalid_url =
// Use a really long URL (for testing the string truncation).
// The URL is from the wrong extension (foo2), so should be rejected.

[yawano, 2015-05-07 02:27:17]

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/launcher_search_provider/error_reporter.h (right):

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/launcher_search_provider/error_reporter.h:17: class
ErrorReporter : public base::RefCounted<ErrorReporter> {
This wasn't ref counted object before.

The reason that I changed this to ref counted object is for testing. We cannot
simply copy the value since we inherits this class in unit test (the value will
be copied as original class, right?). Also we need to reference same instance
from test code and production code to check the error message.

I'm going to find a way not to use ref counted object here.

[Matt Giuca, 2015-05-07 03:24:03]

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/launcher_search_provider/error_reporter.h (right):

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/launcher_search_provider/error_reporter.h:17: class
ErrorReporter : public base::RefCounted<ErrorReporter> {
I see. Right, you can't directly copy an object that has a subclass (or you will
be calling the base class copy constructor with the subclass, and that will
throw away any fields of the subclass). You also can't directly store an object
of this type; it must be pointed to.

The usual solution to this is to use scoped_ptr everywhere (using .Pass() where
possible to avoid copying), and have a method "virtual scoped_ptr<ErrorReporter>
Duplicate() const" that creates a proper copy of the object, overriding it in
each subclass to correctly duplicate.

You might want the subclass (in the test) to internally have a
linked_ptr<std::string> (rather than directly having a std::string), so that if
the MockErrorReporter gets duplicated, all of the copies share the same string.
(It may sound complicated, but I prefer this because it constrains the reference
counting to the tests which is the reason we needed it in the first place.)

[yawano, 2015-05-07 06:24:55]

Thank you for the review! PTAL.

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/launcher_search_provider/error_reporter.cc (right):

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/launcher_search_provider/error_reporter.cc:21: if
(sender_ == nullptr)
On 2015/05/05 04:25:30, Matt Giuca wrote:
> DCHECK(sender);
> 
> The only time sender should be null is in MockErrorReporter, and that
overrides
> Warn. So it is incorrect to allow nullptr here.

Done.

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/launcher_search_provider/error_reporter.h (right):

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/launcher_search_provider/error_reporter.h:17: class
ErrorReporter : public base::RefCounted<ErrorReporter> {
On 2015/05/07 03:24:03, Matt Giuca wrote:
> I see. Right, you can't directly copy an object that has a subclass (or you
will
> be calling the base class copy constructor with the subclass, and that will
> throw away any fields of the subclass). You also can't directly store an
object
> of this type; it must be pointed to.
> 
> The usual solution to this is to use scoped_ptr everywhere (using .Pass()
where
> possible to avoid copying), and have a method "virtual
scoped_ptr<ErrorReporter>
> Duplicate() const" that creates a proper copy of the object, overriding it in
> each subclass to correctly duplicate.
> 
> You might want the subclass (in the test) to internally have a
> linked_ptr<std::string> (rather than directly having a std::string), so that
if
> the MockErrorReporter gets duplicated, all of the copies share the same
string.
> (It may sound complicated, but I prefer this because it constrains the
reference
> counting to the tests which is the reason we needed it in the first place.)

Done. Thank you for the idea.

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
File
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.h
(right):

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.h:53:
// Loads |extension| icon and returns it as sync if possible. When it loads
In the constructor of extension:IconImage, it loads default icon if it's
provided. In IconImage, it uses ImageLoader to load extension resource async,
and notify observers when it completed (ExtensionBadgedIconImageImpl is one of
the observers).

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
Note: Source class is defined in the above of same file.

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.h:60:
virtual void LoadExtensionIconResource() = 0;
On 2015/05/05 04:25:30, Matt Giuca wrote:
> So LoadExtensionIcon loads the extension's icon, whereas
> LoadExtensionIconResource loads an arbitrary icon resource from an extension
> URL, correct?
> 
> The naming is very confusing (they seem so similar, it seems at first glance
> that they are both loading an extension icon in different ways).
> 
> Can we rename this to LoadIconResourceFromExtension?

Done.

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
File
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_impl.h
(right):

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_impl.h:15:
class ExtensionBadgedIconImageImpl : public ExtensionBadgedIconImage,
On 2015/05/05 04:25:30, Matt Giuca wrote:
> Class comment.

Done.

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
File
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_unittest.cc
(right):

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_unittest.cc:21:
namespace {
On 2015/05/05 04:25:30, Matt Giuca wrote:
> nit: blank line above

Done.

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_unittest.cc:23:
const char kFakeExtensionId[] = "foo";
On 2015/05/05 04:25:30, Matt Giuca wrote:
> kTestExtensionId
> kTestCustomIconURL

Done.

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_unittest.cc:27:
// # |primary_fill_color|
On 2015/05/05 04:25:30, Matt Giuca wrote:
> I found it pretty hard to understand this comment at first. Can you write it
> like this:
> 
> // This image source generates following image:
> //
> // ###
> // #**
> // #**
> //
> // where # is the primary fill color, and * is the secondary fill color.

Done.

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_unittest.cc:68:
using ExtensionBadgedIconImage::ExtensionBadgedIconImage;  // Use base class
On 2015/05/05 04:25:30, Matt Giuca wrote:
> Nit: Don't wrap inline comments like this, just place it on the line above.

Done.

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_unittest.cc:104:
class MockErrorReporter : public ErrorReporter {
On 2015/05/05 04:25:30, Matt Giuca wrote:
> nit: This is a fake, not a mock. FakeErrorReporter.

Done.

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_unittest.cc:121:
scoped_refptr<extensions::Extension> CreateFakeExtension(
On 2015/05/05 04:25:30, Matt Giuca wrote:
> CreateTestExtension
> 
> (It's not a fake extension, it's a test extension. A fake is the class stub
> you've created above.)

Done.

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_unittest.cc:156:
GURL icon_url;
On 2015/05/05 04:25:30, Matt Giuca wrote:
> GURL icon_url;  // No custom icon.
> 
> (Add this comment to the end of the icon_url declaration line, not in /**/
> comments inside the function call.
> 
> Same below.

Done.

https://codereview.chromium.org/1060733003/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_unittest.cc:208:
std::string invalid_url =
On 2015/05/05 04:25:30, Matt Giuca wrote:
> // Use a really long URL (for testing the string truncation).
> // The URL is from the wrong extension (foo2), so should be rejected.

Done.

[Matt Giuca, 2015-05-11 03:46:40]

lgtm! Thanks Yuki.

[yawano, 2015-05-11 03:49:21]

On 2015/05/11 03:46:40, Matt Giuca wrote:
> lgtm! Thanks Yuki.

Thank you!

[yawano, 2015-05-11 03:52:43]

@satorux: We've added significant amount of changes after your first lgtm. PTAL
files under chrome/browser/chromeos. Thank you!

[Matt Giuca, 2015-05-11 05:52:26]

Please add 486236 to the BUG= line (comma-separated), so that that bug gets
updated when this lands.

[yawano, 2015-05-11 05:56:47]

On 2015/05/11 05:52:26, Matt Giuca wrote:
> Please add 486236 to the BUG= line (comma-separated), so that that bug gets
> updated when this lands.

Thank you. I added the issue in the description.
The unit_tests fails to build on other OSs since codes of launcher search
provider is available only in Chrome OS. I'm currently working on fixing this by
adding new source set(?). (I'm going to add
chrome_unit_tests_app_list_chromeos_sources).

[Matt Giuca, 2015-05-11 06:25:01]

I think that a simple fix is to just surround the offending code by #if
defined(OS_CHROMEOS).

[satorux1, 2015-05-11 06:32:27]

c/b/chromeos LGTM with a request

https://codereview.chromium.org/1060733003/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/launcher_search_provider/error_reporter.h (right):

https://codereview.chromium.org/1060733003/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/launcher_search_provider/error_reporter.h:25: virtual
scoped_ptr<ErrorReporter> Duplicate();
add a comment and explains when you need to use this?

[yawano, 2015-05-11 06:40:29]

@satorux: Thank you!

https://codereview.chromium.org/1060733003/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/launcher_search_provider/error_reporter.h (right):

https://codereview.chromium.org/1060733003/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/launcher_search_provider/error_reporter.h:25: virtual
scoped_ptr<ErrorReporter> Duplicate();
On 2015/05/11 06:32:27, satorux1 wrote:
> add a comment and explains when you need to use this?

Done.

[yawano, 2015-05-11 06:44:50]

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

[yawano, 2015-05-11 06:44:52]

@thestig: PTAL chrome_tests_unit.gypi since it's not just adding or renaming
files.

@mgiuca: I added a patch set to change gypi file. I prefer to change gypi file
since it seems to be more natural (straight).

[Matt Giuca, 2015-05-11 07:05:03]

I really want to avoid locking this feature into ChromeOS only. During the
initial reviews, you argued that the feature should not be compiled onto
platforms where it is not currently used, but as I was saying at the time, there
is nothing ChromeOS-specific about it (and we have talked with the Apps people
about making it available on Windows etc). I would like to limit the amount of
work that would be required to open this up to other platforms, and so I would
really like to keep this behind #ifdefs rather than segregating source files out
in the build system.

Also, I didn't realise earlier that launcher_search_provider is under the
chromeos namespace. That's unfortunate as I think we should not be building this
as a ChromeOS-only feature, but I guess we can leave it as-is for the time
being.

[yawano, 2015-05-11 07:22:27]

I don't think it becomes big work if we change gypi file. Just changing and
deleting a few lines would make it available for other OSs. In addition, if we
are going to make this feature available in other OSs, I want to avoid #ifdef.
When we add more unit tests, more #ifdefs are added in the code, and we need to
remove all of them when we make this available to other OSs.

For the namespace, it would be better to extract the code from chromeos
namespace after this feature has landed as whiltelisted version.

[Matt Giuca, 2015-05-11 07:38:24]

OK slgtm.

[Lei Zhang, 2015-05-11 19:48:07]

https://codereview.chromium.org/1060733003/diff/260001/chrome/chrome_tests_un...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1060733003/diff/260001/chrome/chrome_tests_un...
chrome/chrome_tests_unit.gypi:2612: ['enable_app_list==1 and chromeos==1', {
You need to make the same change in chrome/test/BUILD.gn.

[yawano, 2015-05-12 02:37:23]

@thestig: Thank you for the review! PTAL.

https://codereview.chromium.org/1060733003/diff/260001/chrome/chrome_tests_un...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1060733003/diff/260001/chrome/chrome_tests_un...
chrome/chrome_tests_unit.gypi:2612: ['enable_app_list==1 and chromeos==1', {
On 2015/05/11 19:48:07, Lei Zhang wrote:
> You need to make the same change in chrome/test/BUILD.gn.

Done.

[Lei Zhang, 2015-05-12 04:49:00]

lgtm

[yawano, 2015-05-12 07:08:23]

On 2015/05/12 04:49:00, Lei Zhang wrote:
> lgtm

Thank you!

[yawano, 2015-05-12 07:08:33]

The CQ bit was checked by yawano@chromium.org

[yawano, 2015-05-12 07:08:33]

The patchset sent to the CQ was uploaded after l-g-t-m from mgiuca@chromium.org,
satorux@chromium.org
Link to the patchset: https://codereview.chromium.org/1060733003/#ps280001
(title: "Fix BUILD.gn.")

[commit-bot: I haz the power, 2015-05-12 07:08:46]

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

[commit-bot: I haz the power, 2015-05-12 08:07:43]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-05-12 08:08:24]

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