Implement setSearchResults.

chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc

Index: chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc
diff --git a/chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc b/chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc
new file mode 100644
index 0000000000000000000000000000000000000000..51c413677c6bfcedffd768a327eec23f47308e0f
--- /dev/null
+++ b/chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc
@@ -0,0 +1,62 @@
+// 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/ui/app_list/search/launcher_search/launcher_search_result.h"
+
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/extensions/extension_util.h"
+#include "extensions/common/manifest_handlers/icons_handler.h"
+
+namespace app_list {
+
+LauncherSearchResult::LauncherSearchResult(
+    const std::string& id,
+    scoped_ptr<std::string> icon_url,
+    const int discrete_value_relevance,
+    Profile* profile,
+    const extensions::Extension* extension)
+    : discrete_value_relevance_(discrete_value_relevance),
+      profile_(profile),
+      extension_(extension) {
+  set_id(id);

[Matt Giuca, 2015/04/10 13:29:14]

I think there will be a problem of ID conflicts between different extensions and
search providers.

A malicious extension could set an ID that matches, for example, one of the IDs
in the app provider. We should namespace them by extension ID (e.g.,
"<YOUR-EXTENSION-ID>:<WHATEVER-THE-EXTENSION-WANTS>").

I *think* that should be done here. So the |id| parameter to
LauncherSearchResult is "<WHATEVER-THE-EXTENSION-WANTS>", and we prepend the
extension-id on to the front before calling set_id (so the ID seen by the Mixer
is "<YOUR-EXTENSION-ID>:<WHATEVER-THE-EXTENSION-WANTS>").

(You could also do this in Service::SetSearchResults but I think it makes sense
here.)

[yawano, 2015/04/13 07:30:52]

Done.

+  set_relevance(static_cast<double>(discrete_value_relevance) / 4.0);

[Matt Giuca, 2015/04/10 13:29:14]

Above, I suggested creating kMaxSearchResultScore = 4.

Please use that here (static_cast<double>(kMaxSearchResultScore)).

Also please add DCHECKs testing discrete_value_relevance's range.

[yawano, 2015/04/13 07:30:52]

Done.

+  set_details(base::UTF8ToUTF16(extension->name()));
+
+  // TODO(yawano) Decode passed icon url and show it baddged with extension

[Matt Giuca, 2015/04/10 13:29:14]

badged

[yawano, 2015/04/13 07:30:52]

Done.

+  // icon.
+  icon_url_ = icon_url.Pass();
+
+  extension_icon_image_.reset(new extensions::IconImage(
+      profile, extension, extensions::IconsInfo::GetIcons(extension),
+      GetPreferredIconDimension(), extensions::util::GetDefaultExtensionIcon(),
+      this));
+  UpdateIcon();
+}
+
+LauncherSearchResult::~LauncherSearchResult() {
+}
+
+scoped_ptr<SearchResult> LauncherSearchResult::Duplicate() const {
+  std::string* icon_url =
+      icon_url_ == NULL ? nullptr : new std::string(icon_url_->c_str());

[Matt Giuca, 2015/04/10 13:29:14]

s/NULL/nullptr

No need for c_str; just new std::string(*icon_url_).

This will be much cleaner if icon_url is not a scoped_ptr.

[yawano, 2015/04/13 07:30:52]

Done. Changed not to use scoped_ptr here.

+
+  LauncherSearchResult* duplicated_result =
+      new LauncherSearchResult(id(), make_scoped_ptr(icon_url),

[Matt Giuca, 2015/04/10 13:29:14]

I think this is too expensive (given that you use Duplicate on every result
every time new results come in).

It looks like the LauncherSearchResult constructor creates a new IconImage from
the extension (rather than copying the existing one). I'm not sure how much work
there is involved in creating a new IconImage but I clicked through a bit and it
looked like quite a lot of ImageSkias and Images are getting created. (I assume
it isn't copying pixel data here, but it's still quite a bit of objects being
created.)

Could we make Duplicate copy the existing extension_icon_image_?

Ideally we wouldn't have to call Duplicate at all (but I can't see a good way of
avoiding that, given that Add expects a scoped_ptr and you need to hold onto the
LauncherSearchResults in the map for future use).

[yawano, 2015/04/13 07:30:52]

Since icon image loading is done as async, I think it's difficult to simply copy
the result. How about to share icon_image with duplicated results? Custom icon
loading will also be done as async. i.e. Shares icon_image (loader) between
duplicated results, pass the image (pixel data) if image loading is completed.

[Matt Giuca, 2015/04/14 03:15:43]

You mean actually share the extension::IconImage using a linked_ptr across
duplicates of the same LauncherSearchResult? Yeah I would be happy with that.

[yawano, 2015/04/14 05:41:59]

Yes. I did it with the new patch set.

+                               discrete_value_relevance_, profile_, extension_);
+  duplicated_result->set_title(title());
+  return make_scoped_ptr(duplicated_result);
+}
+
+void LauncherSearchResult::OnExtensionIconImageChanged(
+    extensions::IconImage* image) {
+  DCHECK_EQ(image, extension_icon_image_.get());
+  UpdateIcon();
+}
+
+void LauncherSearchResult::UpdateIcon() {
+  if (!extension_icon_image_->image_skia().isNull())

[Matt Giuca, 2015/04/10 13:29:14]

Use AsImageSkia instead of image_skia(). Then if it is some other
representation, it will be converted. (You should still check isNull.)

[Matt Giuca, 2015/04/13 07:25:14]

As discussed, ignore this comment. (I just checked that extension_icon_image_
always has an ImageSkia.)

+    SetIcon(extension_icon_image_->image_skia());
+}
+
+}  // namespace app_list