[Icons NTP] Refactor large_icon_source to extract the logic shared between desktop and Android to f…

components/favicon_base/favicon_types.h

Index: components/favicon_base/favicon_types.h
diff --git a/components/favicon_base/favicon_types.h b/components/favicon_base/favicon_types.h
index ea6475429d93d931fb9f511fbe24adaa16ccd38e..562fbcec070f008ee95d675f8a07e95c5f8cd580 100644
--- a/components/favicon_base/favicon_types.h
+++ b/components/favicon_base/favicon_types.h
@@ -5,6 +5,8 @@
 #ifndef COMPONENTS_FAVICON_BASE_FAVICON_TYPES_H_
 #define COMPONENTS_FAVICON_BASE_FAVICON_TYPES_H_
 
+#include <base/memory/scoped_ptr.h>
+
 #include "base/memory/ref_counted_memory.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/image/image.h"
@@ -12,6 +14,8 @@
 
 namespace favicon_base {
 
+struct FallbackIconStyle;
+
 typedef int64 FaviconID;
 
 // Defines the icon types. They are also stored in icon_type field of favicons
@@ -72,6 +76,20 @@ struct FaviconRawBitmapResult {
 // HistoryBackend::SetFavicons().
 typedef FaviconRawBitmapResult FaviconRawBitmapData;
 
+// Result returned when accessing a large icon, which is either a bitmap if
+// one was available; or the style of the fallback icon otherwise.
+struct LargeIconResult {
+  LargeIconResult();
+  ~LargeIconResult();
+
+  // The bitmap of the large icon if available.
+  FaviconRawBitmapResult bitmap;
+
+  // The fallback icon style if a large icon isn't available. This uses the
+  // dominant color of a smaller icon as the background if available.
+  scoped_ptr<FallbackIconStyle> fallback_icon_style;

[pkotwicz, 2015/04/21 18:19:47]

I wonder whether the API would be easier to understand if this class had a
single member: "scoped_refptr<base::RefCountedMemory> bitmap_data"

|bitmap_data| would have the PNG bytes for the favicon if it is large enough and
the PNG bytes of the rendered fallback icon otherwise.

[pkotwicz, 2015/04/21 20:13:28]

Ping on this comment

[beaudoin, 2015/04/21 20:24:05]

Sorry missed that comment. Even though I only ever use bitmap_data I'm tempted
to push back on your proposal. I find it would be bad practice to discard the
pixel_size or the icon_type or anything else in FaviconRawBitmapResult. The only
thing that makes this struct hard to understand, IMHO, is the naming convention
where we use *Result for classes that contain information about a favicon or a
large icon. If it was called FaviconRawBitmap and LargeIcon the composition
pattern used here would make perfect sense.

+};
+
 }  // namespace favicon_base
 
 #endif  // COMPONENTS_FAVICON_BASE_FAVICON_TYPES_H_