LargeIconService returns icon of the desired size

components/favicon/core/large_icon_service.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 "components/favicon/core/large_icon_service.h"
 
 #include <memory>
 
 #include "base/bind.h"
 #include "base/feature_list.h"
@@ skipping 290 matching lines @@
 
 }  // namespace
 
 LargeIconService::LargeIconService(
     FaviconService* favicon_service,
     const scoped_refptr<base::TaskRunner>& background_task_runner,
     std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher)
     : favicon_service_(favicon_service),
       background_task_runner_(background_task_runner),
       image_fetcher_(std::move(image_fetcher)) {
   large_icon_types_.push_back(favicon_base::IconType::FAVICON);

[jkrcal, 2017/05/18 12:33:55]

Actually another fix for the issue could be using only:
large_icon_types_.push_back(
    favicon_base::IconType::FAVICON
    | favicon_base::IconType::TOUCH_ICON
    | favicon_base::IconType::TOUCH_PRECOMPOSED_ICON);

Then, we would specify no preference for icon type and thus could use minimum
size below as 0.

In general, this seems simpler, to mee. 
WDYT?

[gambard, 2017/05/18 13:43:59]

Yes but I think we want to have a preference. I remember some people saying we
should prefer FAVICON over TOUCH, if the size is big enough.

[jkrcal, 2017/05/18 13:50:59]

Okay, let's keep it as it is now.

   large_icon_types_.push_back(favicon_base::IconType::TOUCH_ICON);
   large_icon_types_.push_back(favicon_base::IconType::TOUCH_PRECOMPOSED_ICON);
 }
 
 LargeIconService::~LargeIconService() {
 }
 
 base::CancelableTaskTracker::TaskId
 LargeIconService::GetLargeIconOrFallbackStyle(
     const GURL& page_url,
@@ skipping 56 matching lines @@
     const favicon_base::LargeIconCallback& raw_bitmap_callback,
     const favicon_base::LargeIconImageCallback& image_callback,
     base::CancelableTaskTracker* tracker) {
   DCHECK_LE(1, min_source_size_in_pixel);
   DCHECK_LE(0, desired_size_in_pixel);
 
   scoped_refptr<LargeIconWorker> worker = new LargeIconWorker(
       min_source_size_in_pixel, desired_size_in_pixel, raw_bitmap_callback,
       image_callback, background_task_runner_, tracker);
 
+  int max_size_in_pixel =
+      std::max(desired_size_in_pixel, min_source_size_in_pixel);
   // TODO(beaudoin): For now this is just a wrapper around
   //   GetLargestRawFaviconForPageURL. Add the logic required to select the best
   //   possible large icon. Also add logic to fetch-on-demand when the URL of
   //   a large icon is known but its bitmap is not available.
   return favicon_service_->GetLargestRawFaviconForPageURL(
-      page_url, large_icon_types_, min_source_size_in_pixel,
+      page_url, large_icon_types_, max_size_in_pixel,

[jkrcal, 2017/05/18 12:33:55]

nit: I do not like the naming. Passing in a max_size_in_pixel variable seems
misleading to me as the function does not give any guarantee about maximum size
of what is returned.

What about
desired_size_in_pixel =
      std::max(desired_size_in_pixel, min_source_size_in_pixel);
and using desired_size_in_pixel here?

[gambard, 2017/05/18 13:43:59]

I don't like changing the value of arguments. but maybe
ideal_size_in_pixel = std::max(desired_size_in_pixel, min_source_size_in_pixel);
is better?

[jkrcal, 2017/05/18 13:50:59]

I see. ideal is better to me than max. Maybe size_in_pixel_to_request fits even
better? (as this is the size we request from the service).

       base::Bind(&LargeIconWorker::OnIconLookupComplete, worker), tracker);
 }
 
 }  // namespace favicon