[Refactor] Change FaviconService API to take in a desired pixel size

components/bookmarks/browser/bookmark_client.h

 // Copyright 2014 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.
 
 #ifndef COMPONENTS_BOOKMARKS_BROWSER_BOOKMARK_CLIENT_H_
 #define COMPONENTS_BOOKMARKS_BROWSER_BOOKMARK_CLIENT_H_
 
 #include <set>
 #include <utility>
 #include <vector>
@@ skipping 21 matching lines @@
   // Types representing a set of BookmarkNode and a mapping from BookmarkNode
   // to the number of time the corresponding URL has been typed by the user in
   // the Omnibox.
   typedef std::set<const BookmarkNode*> NodeSet;
   typedef std::pair<const BookmarkNode*, int> NodeTypedCountPair;
   typedef std::vector<NodeTypedCountPair> NodeTypedCountPairs;
 
   // Returns true if the embedder favors touch icons over favicons.
   virtual bool PreferTouchIcon();
 
-  // Requests the favicon of any of |icon_types| whose pixel sizes most
-  // closely match |desired_size_in_dip| (if value is 0, the largest favicon
-  // is returned) and desired scale factor for |page_url|. |callback| is run
-  // when the bits have been fetched. |icon_types| can be any combination of
-  // IconType value, but only one icon will be returned.
-  virtual base::CancelableTaskTracker::TaskId GetFaviconImageForURL(
+  // Requests a multi-resolution favicon from the history cache of type
+  // favicon_base::FAVICON for the web page at |page_url|. The retrieved
+  // data is resized such that the image passed to |callback| has
+  // gfx::kFaviconSize width and height.
+  // |callback| is run when the favicon has been fetched.
+  virtual base::CancelableTaskTracker::TaskId GetFaviconImageForPageURL(
       const GURL& page_url,
-      int icon_types,
-      int desired_size_in_dip,
       const favicon_base::FaviconImageCallback& callback,
       base::CancelableTaskTracker* tracker);
 
+  // Requests the largest favicon of type favicon_base::TOUCH_ICON in the

[sky, 2014/07/10 00:03:39]

Can we pass in favicon_base::IconType so that we only have one method?

[pkotwicz, 2014/07/11 01:23:08]

The main reason that I split the method into two is because it was confusing to
explain that for a request of
favicon_base::FAVICON the returned gfx::Image is a multi-resolution 16x16 DIP
favicon
favicon_base::TOUCH_ICON the returned gfx::Image is a single resolution image
which is of the largest size available in the database.

[sky, 2014/07/11 16:26:51]

This interface is very narrowly used, so I'm fine with cryptic wording.

[pkotwicz, 2014/07/11 18:47:44]

Done!

+  // history cache for the web page at |page_url|.
+  // |callback| is run when the favicon has been fetched.
+  virtual base::CancelableTaskTracker::TaskId GetTouchFaviconImageForPageURL(
+     const GURL& page_url,
+     const favicon_base::FaviconImageCallback& callback,
+     base::CancelableTaskTracker* tracker);
+
   // Returns true if the embedder supports typed count for URL.
   virtual bool SupportsTypedCountForNodes();
 
   // Retrieves the number of time each BookmarkNode URL has been typed in
   // the Omnibox by the user.
   virtual void GetTypedCountForNodes(
       const NodeSet& nodes,
       NodeTypedCountPairs* node_typed_count_pairs);
 
   // Returns whether the embedder wants permanent node |node|
@@ skipping 20 matching lines @@
   // http://crbug.com/49598
   virtual bool CanBeEditedByUser(const BookmarkNode* node) = 0;
 
  protected:
   virtual ~BookmarkClient() {}
 };
 
 }  // namespace bookmarks
 
 #endif  // COMPONENTS_BOOKMARKS_BROWSER_BOOKMARK_CLIENT_H_