Support custom icon in search result.

chrome/browser/chromeos/extensions/launcher_search_provider.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/extensions/launcher_search_provider.h"
 
+#include "chrome/browser/chromeos/launcher_search_provider/error_reporter.h"
 #include "chrome/browser/chromeos/launcher_search_provider/service.h"
 #include "chrome/common/extensions/api/launcher_search_provider.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/render_view_host.h"
 
 namespace extensions {
 
 LauncherSearchProviderSetSearchResultsFunction::
     ~LauncherSearchProviderSetSearchResultsFunction() {
 }
 
 bool LauncherSearchProviderSetSearchResultsFunction::RunSync() {

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

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.

[yawano, 2015/04/28 12:11:27]

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:08]

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).

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

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.

   using chromeos::launcher_search_provider::Service;
   using extensions::api::launcher_search_provider::SetSearchResults::Params;
   const scoped_ptr<Params> params(Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params);
 
+  // Either render_view_host or render_frame_host will be set. See
+  // crbug.com/304341.
+  IPC::Sender* sender = nullptr;
+  int routing_id = 0;
+  if (render_view_host()) {
+    sender = render_view_host();
+    routing_id = render_view_host()->GetRoutingID();
+  } else {
+    sender = render_frame_host();
+    routing_id = render_frame_host()->GetRoutingID();
+  }
+  CHECK(sender);

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

CHECK -> DCHECK

[yawano, 2015/05/01 06:09:54]

Done.

+
+  chromeos::launcher_search_provider::ErrorReporter error_reporter(sender,

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

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.

[yawano, 2015/05/01 06:09:54]

Changed this to scoped_refptr since multiple result instances have reference to
an error_reporter.

+                                                                   routing_id);
   Service* const service = Service::Get(GetProfile());
-  service->SetSearchResults(extension(), params->query_id, params->results);
+  service->SetSearchResults(extension(), error_reporter, params->query_id,
+                            params->results);
 
   return true;
 }
 
 }  // namespace extensions