Allow InstallableManager to fetch optional badge icon

chrome/browser/installable/installable_manager.cc

 // Copyright 2016 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/installable/installable_manager.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/stl_util.h"
@@ skipping 124 matching lines @@
   // Return immediately if we're already working on a task. The new task will be
   // looked at once the current task is finished.
   tasks_.push_back({params, callback});
   if (is_active_)
     return;
 
   is_active_ = true;
   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);
 }
 
-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});
+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 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(
     const InstallableParams& params) {
   if (manifest_->error != NO_ERROR_DETECTED)
     return manifest_->error;
 
   if (params.check_installable && installable_->error != NO_ERROR_DETECTED)
     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;
 }
 
 InstallableStatusCode InstallableManager::manifest_error() const {
   return manifest_->error;
 }
 
 InstallableStatusCode InstallableManager::installable_error() const {
   return installable_->error;
 }
 
 void InstallableManager::set_installable_error(
     InstallableStatusCode error_code) {
   installable_->error = error_code;
 }
 
 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();
 }
 
 content::WebContents* InstallableManager::GetWebContents() {
   content::WebContents* contents = web_contents();
   if (!contents || contents->IsBeingDestroyed())
     return nullptr;
   return contents;
 }
 
 bool InstallableManager::IsComplete(const InstallableParams& params) const {
   // Returns true if for all resources:
   //  a. the params did not request it, OR
   //  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() {
   // Prevent any outstanding callbacks to or from this object from being called.
   weak_factory_.InvalidateWeakPtrs();
   tasks_.clear();
   icons_.clear();
 
   manifest_.reset(new ManifestProperty());
   installable_.reset(new InstallableProperty());
 
   is_active_ = false;
 }
 
 void InstallableManager::SetManifestDependentTasksComplete() {
   DCHECK(!tasks_.empty());
   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);
 }
 
 void InstallableManager::StartNextTask() {
   // If there's nothing to do, exit. Resources remain cached so any future calls
   // won't re-fetch anything that has already been retrieved.
   if (tasks_.empty()) {
     is_active_ = false;
@@ skipping 10 matching lines @@
   const InstallableParams& params = task.first;
 
   InstallableStatusCode code = GetErrorCode(params);
   if (code != NO_ERROR_DETECTED || IsComplete(params)) {
     RunCallback(task, code);
     tasks_.erase(tasks_.begin());
     StartNextTask();
     return;
   }
 
   if (!manifest_->fetched)

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

Every line of this conditional needs braces now (there are multi-line else
clauses later on).

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

Done.

     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();
 }
 
 void InstallableManager::FetchManifest() {
   DCHECK(!manifest_->fetched);
 
   content::WebContents* web_contents = GetWebContents();
   DCHECK(web_contents);
 
@@ skipping 97 matching lines @@
     installable_->installable = true;
   } else {
     installable_->installable = false;
     installable_->error = NO_MATCHING_SERVICE_WORKER;
   }
 
   installable_->fetched = true;
   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;
   }
 
   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;
 
   if (bitmap.drawsNothing()) {
     icon.error = NO_ICON_AVAILABLE;
   } else {
     icon.url = icon_url;
     icon.icon.reset(new SkBitmap(bitmap));
   }
@@ skipping 18 matching lines @@
   return manifest_->url;
 }
 
 const content::Manifest& InstallableManager::manifest() const {
   return manifest_->manifest;
 }
 
 bool InstallableManager::is_installable() const {
   return installable_->installable;
 }