Always select best favicon bitmap

components/favicon/core/favicon_selector.cc

Index: components/favicon/core/favicon_selector.cc
diff --git a/components/favicon/core/favicon_selector.cc b/components/favicon/core/favicon_selector.cc
new file mode 100644
index 0000000000000000000000000000000000000000..81c0b667864e31f4fb34441e844cd46214d33a16
--- /dev/null
+++ b/components/favicon/core/favicon_selector.cc
@@ -0,0 +1,244 @@
+// Copyright (c) 2017 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 "components/favicon/core/favicon_selector.h"
+
+#include <algorithm>
+#include <functional>
+#include <utility>
+
+#include "components/favicon_base/favicon_util.h"
+#include "components/favicon_base/select_favicon_frames.h"
+#include "ui/gfx/favicon_size.h"
+
+namespace favicon {
+namespace {
+
+// Size (along each axis) of a touch icon.
+#if defined(OS_IOS)
+// This currently corresponds to the apple touch icon for iPad.
+const int kLargestIconSizeInPixels = 144;
+#else
+const int kLargestIconSizeInPixels = 192;
+#endif
+
+// Compare function used for std::stable_sort to sort as descend.
+bool CompareScore(const FaviconSelector::Candidate& lhs,
+                  const FaviconSelector::Candidate& rhs) {
+  return lhs.score > rhs.score;
+}
+
+bool ComparePixelSize(const FaviconSelector::TargetSizeSpec& lhs,
+                      const FaviconSelector::TargetSizeSpec& rhs) {
+  return lhs.pixel_size() > rhs.pixel_size();
+}
+
+// Return true if |bitmap_result| is valid.
+bool IsValid(const favicon_base::FaviconRawBitmapResult& bitmap_result) {
+  return bitmap_result.is_valid();
+}
+
+// Returns true if at least one of |bitmap_results| is valid.
+bool HasValidResult(
+    const std::vector<favicon_base::FaviconRawBitmapResult>& bitmap_results) {
+  return std::find_if(bitmap_results.begin(), bitmap_results.end(), IsValid) !=
+         bitmap_results.end();
+}
+
+}  // namespace
+
+// static
+FaviconSelector::TargetSizeSpec FaviconSelector::TargetSizeSpec::ForLargest() {
+  FaviconSelector::TargetSizeSpec target_size_spec;
+  target_size_spec.pixel_size_ = kLargestIconSizeInPixels;
+  // 0 is special-cased in CreateFaviconImageSkiaWithScaleFactors().
+  target_size_spec.scale_factor_ = 0;
+  target_size_spec.ensure_exact_size_ = false;
+  return target_size_spec;
+}
+
+// static
+std::vector<FaviconSelector::TargetSizeSpec>
+FaviconSelector::TargetSizeSpec::For16x16Dips() {
+  std::vector<FaviconSelector::TargetSizeSpec> target_size_specs;
+  std::vector<float> scale_factors = favicon_base::GetFaviconScales();
+  std::sort(scale_factors.begin(), scale_factors.end(), std::greater<float>());
+  for (float scale : scale_factors) {
+    FaviconSelector::TargetSizeSpec target_size_spec;
+    target_size_spec.pixel_size_ = std::ceil(scale * gfx::kFaviconSize);
+    target_size_spec.scale_factor_ = scale;
+    target_size_spec.ensure_exact_size_ = true;
+    target_size_specs.push_back(target_size_spec);
+  }
+  return target_size_specs;
+}
+
+FaviconSelector::TargetSizeSpec::TargetSizeSpec(const TargetSizeSpec&) =
+    default;
+
+FaviconSelector::TargetSizeSpec::~TargetSizeSpec() = default;
+
+FaviconSelector::TargetSizeSpec::TargetSizeSpec(TargetSizeSpec&&) = default;
+
+FaviconSelector::TargetSizeSpec& FaviconSelector::TargetSizeSpec::operator=(
+    FaviconSelector::TargetSizeSpec&&) = default;
+
+bool FaviconSelector::TargetSizeSpec::WantsBestBitmapOnly() const {
+  return !ensure_exact_size_;
+}
+
+bool FaviconSelector::TargetSizeSpec::HasSatisfyingResult(
+    const std::vector<favicon_base::FaviconRawBitmapResult>& bitmap_results)
+    const {
+  if (WantsBestBitmapOnly())
+    return HasValidResult(bitmap_results);
+
+  const gfx::Size desired_size(pixel_size_, pixel_size_);
+  for (const auto& bitmap_result : bitmap_results) {
+    if (IsValid(bitmap_result) && bitmap_result.pixel_size == desired_size)
+      return true;
+  }
+  return false;
+}
+
+std::list<FaviconSelector::Candidate>
+FaviconSelector::TargetSizeSpec::SortAndPruneCandidates(
+    const std::vector<favicon::FaviconURL>& favicon_urls) const {
+  std::vector<Candidate> candidates;
+  for (const favicon::FaviconURL& favicon_url : favicon_urls)
+    candidates.push_back(Candidate::FromFaviconURL(favicon_url, pixel_size_));
+
+  std::stable_sort(candidates.begin(), candidates.end(), CompareScore);
+  std::list<Candidate> result;
+  std::move(candidates.begin(), candidates.end(), std::back_inserter(result));
+  return result;
+}
+
+FaviconSelector::TargetSizeSpec::TargetSizeSpec() = default;
+
+////////////////////////////////////////////////////////////////////////////////
+
+// static
+FaviconSelector::Candidate FaviconSelector::Candidate::FromFaviconURL(
+    const favicon::FaviconURL& favicon_url,
+    int target_pixel_size) {
+  FaviconSelector::Candidate candidate;
+  candidate.icon_url = favicon_url.icon_url;
+  candidate.icon_type = favicon_url.icon_type;
+  candidate.score = 0;
+  for (const gfx::Size& favicon_size : favicon_url.icon_sizes) {

[pkotwicz, 2017/03/20 03:25:16]

If you passed in a vector of target sizes (std::vector<int> target_pixel_sizes)
you could compute the score by summing together the score computed by
GetFaviconCandidateScore()

(Or just use the score computed by SelectFaviconFrameIndices() which does the
summation for you)

[mastiz, 2017/03/20 08:07:28]

This deserves some discussion, because my proposal for FaviconSelector is that
it targets one single pixel size (and doesn't even know about the rest). Hence,
the summation you propose would arguably belong outside, FaviconHandler.

My high level question is whether this summation is actually needed. What's the
actual requirement behind ChooseExactMatchDespiteUpsampling?

Intuitively, assuming most platforms care mostly about the largest supported
scale factor, it looks like undesirable.

+    candidate.score =
+        std::max(candidate.score,
+                 GetFaviconCandidateScore(favicon_size, target_pixel_size));
+  }
+  return candidate;
+}
+
+////////////////////////////////////////////////////////////////////////////////
+
+// static
+std::vector<FaviconSelector> FaviconSelector::BuildMultiple(
+    const std::vector<TargetSizeSpec>& target_size_specs,
+    const std::vector<favicon::FaviconURL>& candidates) {
+  DCHECK(std::is_sorted(target_size_specs.begin(), target_size_specs.end(),
+                        &ComparePixelSize));
+  std::vector<FaviconSelector> selectors;
+  for (const TargetSizeSpec& target_size_spec : target_size_specs)
+    selectors.emplace_back(target_size_spec, candidates);
+  return selectors;
+}
+
+FaviconSelector::FaviconSelector(
+    const FaviconSelector::TargetSizeSpec& target_size_spec,
+    const std::vector<favicon::FaviconURL>& candidates)
+    : target_size_spec_(target_size_spec),
+      pending_candidates_(target_size_spec.SortAndPruneCandidates(candidates)) {
+  DCHECK(!pending_candidates_.empty());
+}
+
+FaviconSelector::~FaviconSelector() = default;
+
+FaviconSelector::FaviconSelector(FaviconSelector&&) = default;
+
+FaviconSelector& FaviconSelector::operator=(FaviconSelector&&) = default;
+
+bool FaviconSelector::IsSatisfied() const {
+  if (!best_candidate_)
+    return false;
+
+  if (target_size_spec_.WantsBestBitmapOnly()) {
+    // The next candidate must have size attributes that are more promising than
+    // the best candidate so far. Otherwise, all favicon without sizes attribute
+    // are downloaded.
+    // TODO(mastiz): This seems useful for sites that have reported the wrong
+    // size. Consider simplifying and returning true here.
+    return pending_candidates_.empty() || pending_candidates_.front().score <=
+                                              best_candidate_->candidate.score;
+  } else {
+    // For exact sizes (i.e. not best only), we download all candidates until an
+    // exact match is found.
+    return best_candidate_->original_size.width() ==
+               target_size_spec_.pixel_size() &&
+           best_candidate_->original_size.height() ==
+               target_size_spec_.pixel_size();
+  }
+}
+
+// Returns the next candidate to be processed by populating |*candidate|.
+// Returns false if the queue is empty.
+base::Optional<FaviconSelector::Candidate> FaviconSelector::DequeueCandidate() {
+  if (pending_candidates_.empty() || IsSatisfied())
+    return base::Optional<Candidate>();
+
+  base::Optional<Candidate> result = std::move(pending_candidates_.front());
+  pending_candidates_.pop_front();
+  return result;
+}
+
+const FaviconSelector::Candidate* FaviconSelector::CurrentCandidate() const {
+  if (pending_candidates_.empty() || IsSatisfied())
+    return nullptr;
+
+  return &pending_candidates_.front();
+}
+
+bool FaviconSelector::ProcessDownloadedImage(
+    const GURL& icon_url,
+    favicon_base::IconType icon_type,
+    const std::vector<SkBitmap>& bitmaps,
+    const std::vector<gfx::Size>& original_sizes) {
+  // Remove potentially queued candidates for |icon_url| (this happens if the
+  // candidate was dequeued from another queue).
+  pending_candidates_.remove_if([&icon_url](const Candidate& candidate) {
+    return candidate.icon_url == icon_url;
+  });
+
+  // Select the best bitmap, which to be relevant needs to be better than the
+  // best known bitmap until now.
+  float best_score = best_candidate_ ? best_candidate_->candidate.score : -1;
+  size_t best_bitmap_index = bitmaps.size();
+  for (size_t i = 0; i < bitmaps.size(); ++i) {
+    const SkBitmap& bitmap = bitmaps[i];
+    const float bitmap_score =
+        GetFaviconCandidateScore(gfx::Size(bitmap.width(), bitmap.height()),
+                                 target_size_spec_.pixel_size());
+    if (bitmap_score > best_score) {
+      best_bitmap_index = i;
+      best_score = bitmap_score;
+    }
+  }
+
+  // No interesting bitmap, nothing to be done (|icon_url| has already been
+  // removed from the queue).
+  if (best_bitmap_index == bitmaps.size())
+    return false;
+
+  Candidate candidate{icon_url, icon_type, best_score};
+  best_candidate_.emplace(BestCandidate{std::move(candidate),
+                                        original_sizes[best_bitmap_index],
+                                        bitmaps[best_bitmap_index]});
+  return true;
+}
+
+}  // namespace favicon