Move favicon callbacks to favicon_base

components/favicon_base/favicon_callback.h

Index: components/favicon_base/favicon_callback.h
diff --git a/components/favicon_base/favicon_callback.h b/components/favicon_base/favicon_callback.h
new file mode 100644
index 0000000000000000000000000000000000000000..90de9f0221eaaee424f74dae50f3070726461b02
--- /dev/null
+++ b/components/favicon_base/favicon_callback.h
@@ -0,0 +1,48 @@
+// 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_FAVICON_BASE_FAVICON_CALLBACK_H_
+#define COMPONENTS_FAVICON_BASE_FAVICON_CALLBACK_H_
+
+#include <vector>
+
+#include "base/callback.h"
+
+namespace favicon_base {
+
+struct FaviconBitmapResult;
+struct FaviconImageResult;
+
+// Callback for FaviconService::GetFaviconImage(),

[blundell, 2014/05/19 07:50:30]

Hmm, these comments are not the right ones now that the functions being
referenced are in another component entirely. Sylvain, could you advise on what
the comments could/should be now? Maybe just generalize to something like
"Callback for functions that have asked for favicons by URL, icon type, desired
size" and etc?

[sdefresne, 2014/05/19 14:36:39]

I'd got with something like:

// Callback for functions returning a |gfx::Image| and the |GURL| it is loaded
// from. They are returned as a |favicon_base::FaviconImageResult| object.

[blundell, 2014/05/19 15:12:59]

s/for functions returning/that can be used to return

here and in the below suggested comments

On 2014/05/19 14:36:39, sdefresne wrote:

[Jiang Jiang, 2014/05/19 15:56:41]

Done.

+// FaviconService::GetFaviconImageForURL() and
+// BookmarkClient::GetFaviconImageForURL().
+// |FaviconImageResult::image| is constructed from the bitmaps for the
+// passed in URL and icon types which most which closely match the passed in
+// |desired_size_in_dip| at the scale factors supported by the current
+// platform (eg MacOS) in addition to 1x.
+// |FaviconImageResult::icon_url| is the favicon that the favicon bitmaps in
+// |image| originate from.
+// TODO(pkotwicz): Enable constructing |image| from bitmaps from several
+// icon URLs.
+typedef base::Callback<void(const FaviconImageResult&)> FaviconImageCallback;
+
+// Callback for GetRawFavicon(), GetRawFaviconForURL() and

[sdefresne, 2014/05/19 14:36:39]

// Callback for functions returning raw data for a favicon.

[blundell, 2014/05/19 15:12:59]

Sylvain,

what is raw data? is there any way to be more specific?

On 2014/05/19 14:36:39, sdefresne wrote:

[sdefresne, 2014/05/19 16:30:04]

FaviconBitmapResult does not store the favicon as a gfx::Image but as a
base::RefCountedMemory. I think this is to allow the code to select a favicon
before loading it as a gfx::Image (which may introduce additional memory
allocation). Maybe adding a reference to FaviconBitmapResult and stating that
the data is not yet converted as a gfx::Image?

+// GetLargestRawFavicon().
+// See function for details on value.
+typedef base::Callback<void(const FaviconBitmapResult&)> FaviconRawCallback;
+
+// Callback for GetFavicon() and GetFaviconForURL().

[sdefresne, 2014/05/19 14:36:39]

// Callback for functions returning raw data for a favicon in multiple
resolution.

[blundell, 2014/05/19 15:12:59]

same question as above for Sylvain

On 2014/05/19 14:36:39, sdefresne wrote:

+//
+// The first argument is the set of bitmaps for the passed in URL and
+// icon types whose pixel sizes best match the passed in
+// |desired_size_in_dip| at the scale factors supported by the current
+// platform (eg MacOS) in addition to 1x. The vector has at most one result
+// for each of the scale factors. There are less entries if a single result
+// is the best bitmap to use for several scale factors.
+typedef base::Callback<void(const std::vector<FaviconBitmapResult>&)>
+    FaviconResultsCallback;
+
+}  // namespace favicon_base
+
+#endif  // COMPONENTS_FAVICON_BASE_FAVICON_CALLBACK_H_