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

components/favicon_base/favicon_types.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_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"
 #include "url/gurl.h"
 
 namespace favicon_base {
 
+struct FallbackIconStyle;
+
 typedef int64 FaviconID;
 
 // Defines the icon types. They are also stored in icon_type field of favicons
 // table.
 // The values of the IconTypes are used to select the priority in which favicon
 // data is returned in HistoryBackend and ThumbnailDatabase. Data for the
 // largest IconType takes priority if data for multiple IconTypes is available.
 enum IconType {
   INVALID_ICON = 0x0,
   FAVICON = 1 << 0,
@@ skipping 40 matching lines @@
   GURL icon_url;
 
   // The icon type of the containing favicon.
   IconType icon_type;
 };
 
 // Define type with same structure as FaviconRawBitmapResult for passing data to
 // 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_