Allow InstallableManager to fetch optional badge icon

chrome/browser/installable/installable_manager.cc

Index: chrome/browser/installable/installable_manager.cc
diff --git a/chrome/browser/installable/installable_manager.cc b/chrome/browser/installable/installable_manager.cc
index 95bffb9c9d4b8cc4be7a57de0efeb93cba78a29f..528cc87fb733cf3195ad935e5769c0f483b453fc 100644
--- a/chrome/browser/installable/installable_manager.cc
+++ b/chrome/browser/installable/installable_manager.cc
@@ -142,20 +142,27 @@ void InstallableManager::GetData(const InstallableParams& params,
   StartNextTask();
 }
 
-InstallableManager::IconProperty& InstallableManager::GetIcon(
-    const InstallableParams& params) {
-  return icons_[{params.ideal_primary_icon_size_in_px,
-                 params.minimum_primary_icon_size_in_px}];
+InstallableManager::IconParams InstallableManager::ParamsForPrimaryIcon(
+    const InstallableParams& params) const {
+  return std::make_tuple(params.ideal_primary_icon_size_in_px,
+                         params.minimum_primary_icon_size_in_px,
+                         IconPurpose::ANY);
+}
+
+InstallableManager::IconParams InstallableManager::ParamsForBadgeIcon(
+    const InstallableParams& params) const {
+  return std::make_tuple(params.ideal_badge_icon_size_in_px,
+                         params.minimum_badge_icon_size_in_px,
+                         IconPurpose::BADGE);
 }
 
-bool InstallableManager::IsIconFetched(const InstallableParams& params) const {
-  const auto it = icons_.find({params.ideal_primary_icon_size_in_px,
-                               params.minimum_primary_icon_size_in_px});
+bool InstallableManager::IsIconFetched(const IconParams& params) const {
+  const auto it = icons_.find(params);
   return it != icons_.end() && it->second.fetched;
 }
 
-void InstallableManager::SetIconFetched(const InstallableParams& params) {
-  GetIcon(params).fetched = true;
+void InstallableManager::SetIconFetched(const IconParams& params) {
+  icons_[params].fetched = true;
 }
 
 InstallableStatusCode InstallableManager::GetErrorCode(
@@ -167,11 +174,12 @@ InstallableStatusCode InstallableManager::GetErrorCode(
     return installable_->error;
 
   if (params.fetch_valid_primary_icon) {
-    IconProperty& icon = GetIcon(params);
+    IconProperty& icon = icons_[ParamsForPrimaryIcon(params)];
     if (icon.error != NO_ERROR_DETECTED)
       return icon.error;
   }
 
+  // Do not report badge icon's error because badge icon is optional.
   return NO_ERROR_DETECTED;
 }
 
@@ -189,17 +197,15 @@ void InstallableManager::set_installable_error(
 }
 
 InstallableStatusCode InstallableManager::icon_error(
-    const InstallableManager::IconParams& icon_params) {
+    const IconParams& icon_params) {
   return icons_[icon_params].error;
 }
 
-GURL& InstallableManager::icon_url(
-    const InstallableManager::IconParams& icon_params) {
+GURL& InstallableManager::icon_url(const IconParams& icon_params) {
   return icons_[icon_params].url;
 }
 
-const SkBitmap* InstallableManager::icon(
-    const InstallableManager::IconParams& icon_params) {
+const SkBitmap* InstallableManager::icon(const IconParams& icon_params) {
   return icons_[icon_params].icon.get();
 }
 
@@ -216,7 +222,10 @@ bool InstallableManager::IsComplete(const InstallableParams& params) const {
   //  b. the resource has been fetched/checked.
   return manifest_->fetched &&
          (!params.check_installable || installable_->fetched) &&
-         (!params.fetch_valid_primary_icon || IsIconFetched(params));
+         (!params.fetch_valid_primary_icon ||
+             IsIconFetched(ParamsForPrimaryIcon(params))) &&
+         (!params.fetch_valid_badge_icon ||
+             IsIconFetched(ParamsForBadgeIcon(params)));
 }
 
 void InstallableManager::Reset() {
@@ -236,19 +245,32 @@ void InstallableManager::SetManifestDependentTasksComplete() {
   const InstallableParams& params = tasks_[0].first;
 
   installable_->fetched = true;
-  SetIconFetched(params);
+  SetIconFetched(ParamsForPrimaryIcon(params));
+  SetIconFetched(ParamsForBadgeIcon(params));
 }
 
 void InstallableManager::RunCallback(const Task& task,
                                      InstallableStatusCode code) {
   const InstallableParams& params = task.first;
-  IconProperty& icon = GetIcon(params);
+  IconProperty null_icon;
+  IconProperty* primary_icon = &null_icon;
+  IconProperty* badge_icon = &null_icon;
+
+  if (params.fetch_valid_primary_icon &&
+      base::ContainsKey(icons_, ParamsForPrimaryIcon(params)))
+    primary_icon = &icons_[ParamsForPrimaryIcon(params)];
+  if (params.fetch_valid_badge_icon &&
+      base::ContainsKey(icons_, ParamsForBadgeIcon(params)))
+    badge_icon = &icons_[ParamsForBadgeIcon(params)];
+
   InstallableData data = {
       code,
       manifest_url(),
       manifest(),
-      params.fetch_valid_primary_icon ? icon.url : GURL::EmptyGURL(),
-      params.fetch_valid_primary_icon ? icon.icon.get() : nullptr,
+      primary_icon->url,
+      primary_icon->icon.get(),
+      badge_icon->url,
+      badge_icon->icon.get(),
       params.check_installable ? is_installable() : false};
 
   task.second.Run(data);
@@ -283,8 +305,12 @@ void InstallableManager::WorkOnTask() {
     FetchManifest();
   else if (params.check_installable && !installable_->fetched)
     CheckInstallable();
-  else if (params.fetch_valid_primary_icon && !IsIconFetched(params))
-    CheckAndFetchBestIcon();
+  else if (params.fetch_valid_primary_icon &&
+      !IsIconFetched(ParamsForPrimaryIcon(params)))

[dominickn, 2017/01/27 06:40:42]

This repeated construction of the params throughout this cc file is a little
upsetting. It would be good if we could find a way to avoid that. Let me think
on what a good solution might be. Possibly introducing a new internal struct for
each Task (instead of reusing InstallableParams internally) so that we construct
the params once and are done with it.

[F, 2017/01/27 20:11:17]

I agree that the repeated construction may seem a bit too much. However, I don't
think it's a bad idea. Previously, each step would fetch the InstallableParams
from the task queue, and invoke GetIcon, thereby constructing IconParams. The
current approach is basically the same, except that it centralizes
InstallableParams into WorkOnTask.

I prefer reusing InstallableParams over introducing new struct. WDUT?

[dominickn, 2017/01/30 00:51:27]

Fair enough. Let's leave this as is for now. It's not *that* much more
repetitions, and it's less elegant to handle this some other way.

+    CheckAndFetchBestIcon(ParamsForPrimaryIcon(params));
+  else if (params.fetch_valid_badge_icon &&
+      !IsIconFetched(ParamsForBadgeIcon(params)))
+    CheckAndFetchBestIcon(ParamsForBadgeIcon(params));
   else
     NOTREACHED();
 }
@@ -402,26 +428,35 @@ void InstallableManager::OnDidCheckHasServiceWorker(bool has_service_worker) {
   WorkOnTask();
 }
 
-void InstallableManager::CheckAndFetchBestIcon() {
+void InstallableManager::CheckAndFetchBestIcon(const IconParams& params) {
   DCHECK(!manifest().IsEmpty());
-  DCHECK(!tasks_.empty());
 
-  const InstallableParams& params = tasks_[0].first;
-  IconProperty& icon = GetIcon(params);
+  int ideal_icon_size_in_px = std::get<0>(params);
+  int minimum_icon_size_in_px = std::get<1>(params);
+  IconPurpose icon_purpose = std::get<2>(params);
+
+  IconProperty& icon = icons_[params];
   icon.fetched = true;
 
+  // Filter by icon purpose.

[dominickn, 2017/01/27 06:40:42]

I think a better idea here is to make ManifestIconSelector take an icon purpose
so we avoid this extra (needless) copy. Perhaps an initial CL that does that?

(That also reminds me that I should get myself added to chrome/browser/manifest
OWNERs at some point)

[F, 2017/01/27 20:11:17]

I agree that it is a better idea. Previously I explored this and found that
ManifestIconSelector would also create an extra copy so eventually decided to
put the extra copy here.

I'm going to try combining the type filter and the purpose filter in
ManifestIconSelector and let you decide :)

[dominickn, 2017/01/30 00:51:26]

I'm confused why ManifestIconSelector would make an extra copy. Why not change
FindBestMatchingIcon to take an IconPurpose as an extra argument, and add an
IconContainsRequiredPurpose() method that you check before accepting an icon? Is
there something I'm missing? :)

[F, 2017/01/30 16:50:05]

At the moment, FilterIconsByType creates an extra copy of icons:
https://cs.chromium.org/chromium/src/chrome/browser/manifest/manifest_icon_se...
So previously I thought I might write a similar function FilterIconsByPurpose.

I'm going to refactor ManifestIconSelector to get rid of the extra copy of
icons. WDUT?

[dominickn, 2017/01/30 18:07:41]

I didn't realise there was already a copy in there. There should be no reason
for ManifestIconSelector to be making copies, so +1 to a refactor.

I would prefer that you didn't have this additional copy here now, but if you do
that refactor and add a purpose filter asap I'll be okay with that.

+  std::vector<content::Manifest::Icon> filtered_icons;
+  std::copy_if(manifest().icons.begin(), manifest().icons.end(),
+               std::back_inserter(filtered_icons),
+               [icon_purpose](const content::Manifest::Icon& icon) {
+                 return base::ContainsValue(icon.purpose, icon_purpose);
+               });
+
   GURL icon_url = ManifestIconSelector::FindBestMatchingIcon(
-      manifest().icons, params.ideal_primary_icon_size_in_px,
-      params.minimum_primary_icon_size_in_px);
+      filtered_icons, ideal_icon_size_in_px, minimum_icon_size_in_px);
 
   if (icon_url.is_empty()) {
     icon.error = NO_ACCEPTABLE_ICON;
   } else {
     bool can_download_icon = ManifestIconDownloader::Download(
-        GetWebContents(), icon_url, params.ideal_primary_icon_size_in_px,
-        params.minimum_primary_icon_size_in_px,
+        GetWebContents(), icon_url, ideal_icon_size_in_px,
+        minimum_icon_size_in_px,
         base::Bind(&InstallableManager::OnAppIconFetched,
-                   weak_factory_.GetWeakPtr(), icon_url));
+                   weak_factory_.GetWeakPtr(), icon_url, params));
     if (can_download_icon)
       return;
     icon.error = CANNOT_DOWNLOAD_ICON;
@@ -430,11 +465,9 @@ void InstallableManager::CheckAndFetchBestIcon() {
   WorkOnTask();
 }
 
-void InstallableManager::OnAppIconFetched(const GURL icon_url,
-                                          const SkBitmap& bitmap) {
-  DCHECK(!tasks_.empty());
-  const InstallableParams& params = tasks_[0].first;
-  IconProperty& icon = GetIcon(params);
+void InstallableManager::OnAppIconFetched(
+    const GURL icon_url, const IconParams& params, const SkBitmap& bitmap) {
+  IconProperty& icon = icons_[params];
 
   if (!GetWebContents())
     return;