This CL fixes two bugs:

1) Makes the favicons (tab, bookmarks) look the same in the browser UI as they do in the renderer). This fixes a regression (probably by one of my CLs) since https://codereview.chromium.org/6117006
2) Make the favicons in the tab strip look the same after refreshing. The difference is due to the conversions PNG -> NSImage and PNG -> SkBitmap -> NSImage producing visually different NSImages. In particular, the result is different when the input PNG data has no colorspace information specified. Cocoa defaults to the device colorspace when decoding PNG data with no colorspace information. The generic RGB colorspace is used for converting from SkBitmap to NSImage. 

BUG=242877
TEST=Manual, see bug

Review URL: https://chromiumcodereview.appspot.com/16370006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@207990 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 101 insertions, 5 deletions

diff --git a/base/mac/mac_util.h b/base/mac/mac_util.h
index 2190b24..23b57ed 100644
--- a/base/mac/mac_util.h
+++ b/base/mac/mac_util.h
@@ -47,6 +47,10 @@ BASE_EXPORT bool FSRefFromPath(const std::string& path, FSRef* ref);
 // release it!
 BASE_EXPORT CGColorSpaceRef GetSRGBColorSpace();
 
+// Returns the generic RGB color space. The return value is a static value; do
+// not release it!
+BASE_EXPORT CGColorSpaceRef GetGenericRGBColorSpace();
+
 // Returns the color space being used by the main display.  The return value
 // is a static value; do not release it!
 BASE_EXPORT CGColorSpaceRef GetSystemColorSpace();
diff --git a/base/mac/mac_util.mm b/base/mac/mac_util.mm
index 32dc735..e924f90 100644
--- a/base/mac/mac_util.mm
+++ b/base/mac/mac_util.mm
@@ -144,6 +144,15 @@ bool FSRefFromPath(const std::string& path, FSRef* ref) {
   return status == noErr;
 }
 
+CGColorSpaceRef GetGenericRGBColorSpace() {
+  // Leaked. That's OK, it's scoped to the lifetime of the application.
+  static CGColorSpaceRef g_color_space_generic_rgb(
+      CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB));
+  DLOG_IF(ERROR, !g_color_space_generic_rgb) <<
+      "Couldn't get the generic RGB color space";
+  return g_color_space_generic_rgb;
+}
+
 CGColorSpaceRef GetSRGBColorSpace() {
   // Leaked.  That's OK, it's scoped to the lifetime of the application.
   static CGColorSpaceRef g_color_space_sRGB =
diff --git a/chrome/browser/favicon/favicon_handler.cc b/chrome/browser/favicon/favicon_handler.cc
index de667b0..881008d 100644
--- a/chrome/browser/favicon/favicon_handler.cc
+++ b/chrome/browser/favicon/favicon_handler.cc
@@ -323,7 +323,10 @@ void FaviconHandler::UpdateFavicon(NavigationEntry* entry,
   if (image.IsEmpty())
     return;
 
-  entry->GetFavicon().image = image;
+  gfx::Image image_with_adjusted_colorspace = image;
+  FaviconUtil::SetFaviconColorSpace(&image_with_adjusted_colorspace);
+
+  entry->GetFavicon().image = image_with_adjusted_colorspace;
   delegate_->NotifyFaviconUpdated(icon_url_changed);
 }
 
diff --git a/chrome/browser/favicon/favicon_service.cc b/chrome/browser/favicon/favicon_service.cc
index 37d691a..35875ff 100644
--- a/chrome/browser/favicon/favicon_service.cc
+++ b/chrome/browser/favicon/favicon_service.cc
@@ -314,6 +314,8 @@ void FaviconService::RunFaviconImageCallbackWithBitmapResults(
       favicon_bitmap_results,
       FaviconUtil::GetFaviconScaleFactors(),
       desired_size_in_dip);
+  FaviconUtil::SetFaviconColorSpace(&image_result.image);
+
   image_result.icon_url = image_result.image.IsEmpty() ?
       GURL() : favicon_bitmap_results[0].icon_url;
   callback.Run(image_result);
diff --git a/chrome/browser/favicon/favicon_util.cc b/chrome/browser/favicon/favicon_util.cc
index df96a47..70a2dbe5f 100644
--- a/chrome/browser/favicon/favicon_util.cc
+++ b/chrome/browser/favicon/favicon_util.cc
@@ -16,6 +16,10 @@
 #include "ui/gfx/image/image_skia.h"
 #include "webkit/glue/image_decoder.h"
 
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+#include "base/mac/mac_util.h"
+#endif  // defined(OS_MACOSX) && !defined(OS_IOS)
+
 namespace {
 
 // Creates image reps of DIP size |favicon_size| for the subset of
@@ -105,6 +109,13 @@ std::vector<ui::ScaleFactor> FaviconUtil::GetFaviconScaleFactors() {
 }
 
 // static
+void FaviconUtil::SetFaviconColorSpace(gfx::Image* image) {
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+  image->SetSourceColorSpace(base::mac::GetSystemColorSpace());
+#endif  // defined(OS_MACOSX) && !defined(OS_IOS)
+}
+
+// static
 gfx::Image FaviconUtil::SelectFaviconFramesFromPNGs(
       const std::vector<chrome::FaviconBitmapResult>& png_data,
       const std::vector<ui::ScaleFactor>& scale_factors,
diff --git a/chrome/browser/favicon/favicon_util.h b/chrome/browser/favicon/favicon_util.h
index 32dc6e0..89474e9 100644
--- a/chrome/browser/favicon/favicon_util.h
+++ b/chrome/browser/favicon/favicon_util.h
@@ -34,6 +34,11 @@ class FaviconUtil {
   // the default favicon.
   static std::vector<ui::ScaleFactor> GetFaviconScaleFactors();
 
+  // Sets the color space used for converting |image| to an NSImage to the
+  // system colorspace. This makes the favicon look the same in the browser UI
+  // as it does in the renderer.
+  static void SetFaviconColorSpace(gfx::Image* image);
+
   // Takes a vector of png-encoded frames, decodes them, and converts them to
   // a favicon of size favicon_size (in DIPs) at the desired ui scale factors.
   static gfx::Image SelectFaviconFramesFromPNGs(
diff --git a/ui/gfx/image/image.cc b/ui/gfx/image/image.cc
index 875ca20..b0733b7 100644
--- a/ui/gfx/image/image.cc
+++ b/ui/gfx/image/image.cc
@@ -124,7 +124,8 @@ gfx::Size UIImageSize(UIImage* image);
 scoped_refptr<base::RefCountedMemory> Get1xPNGBytesFromNSImage(
     NSImage* nsimage);
 // Caller takes ownership of the returned NSImage.
-NSImage* NSImageFromPNG(const std::vector<gfx::ImagePNGRep>& image_png_reps);
+NSImage* NSImageFromPNG(const std::vector<gfx::ImagePNGRep>& image_png_reps,
+                        CGColorSpaceRef color_space);
 gfx::Size NSImageSize(NSImage* image);
 #endif // defined(OS_MACOSX)
 
@@ -472,6 +473,10 @@ class ImageStorage : public base::RefCounted<ImageStorage> {
  public:
   ImageStorage(gfx::Image::RepresentationType default_type)
       : default_representation_type_(default_type),
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+        default_representation_color_space_(
+            base::mac::GetGenericRGBColorSpace()),
+#endif  // defined(OS_MACOSX) && !defined(OS_IOS)
         representations_deleter_(&representations_) {
   }
 
@@ -480,6 +485,15 @@ class ImageStorage : public base::RefCounted<ImageStorage> {
   }
   gfx::Image::RepresentationMap& representations() { return representations_; }
 
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+  void set_default_representation_color_space(CGColorSpaceRef color_space) {
+    default_representation_color_space_ = color_space;
+  }
+  CGColorSpaceRef default_representation_color_space() {
+    return default_representation_color_space_;
+  }
+#endif  // defined(OS_MACOSX) && !defined(OS_IOS)
+
  private:
   friend class base::RefCounted<ImageStorage>;
 
@@ -489,6 +503,14 @@ class ImageStorage : public base::RefCounted<ImageStorage> {
   // exist in the |representations_| map.
   gfx::Image::RepresentationType default_representation_type_;
 
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+  // The default representation's colorspace. This is used for converting to
+  // NSImage. This field exists to compensate for PNGCodec not writing or
+  // reading colorspace ancillary chunks. (sRGB, iCCP).
+  // Not owned.
+  CGColorSpaceRef default_representation_color_space_;
+#endif  // defined(OS_MACOSX) && !defined(OS_IOS)
+
   // All the representations of an Image. Size will always be at least one, with
   // more for any converted representations.
   gfx::Image::RepresentationMap representations_;
@@ -710,18 +732,22 @@ UIImage* Image::ToUIImage() const {
 NSImage* Image::ToNSImage() const {
   internal::ImageRep* rep = GetRepresentation(kImageRepCocoa, false);
   if (!rep) {
+    CGColorSpaceRef default_representation_color_space =
+        storage_->default_representation_color_space();
+
     switch (DefaultRepresentationType()) {
       case kImageRepPNG: {
         internal::ImageRepPNG* png_rep =
             GetRepresentation(kImageRepPNG, true)->AsImageRepPNG();
         rep = new internal::ImageRepCocoa(internal::NSImageFromPNG(
-            png_rep->image_reps()));
+            png_rep->image_reps(), default_representation_color_space));
         break;
       }
       case kImageRepSkia: {
         internal::ImageRepSkia* skia_rep =
             GetRepresentation(kImageRepSkia, true)->AsImageRepSkia();
-        NSImage* image = NSImageFromImageSkia(*skia_rep->image());
+        NSImage* image = NSImageFromImageSkiaWithColorSpace(*skia_rep->image(),
+            default_representation_color_space);
         base::mac::NSObjectRetain(image);
         rep = new internal::ImageRepCocoa(image);
         break;
@@ -896,6 +922,13 @@ void Image::SwapRepresentations(gfx::Image* other) {
   storage_.swap(other->storage_);
 }
 
+#if defined(OS_MACOSX)  && !defined(OS_IOS)
+void Image::SetSourceColorSpace(CGColorSpaceRef color_space) {
+  if (storage_.get())
+    storage_->set_default_representation_color_space(color_space);
+}
+#endif  // defined(OS_MACOSX) && !defined(OS_IOS)
+
 Image::RepresentationType Image::DefaultRepresentationType() const {
   CHECK(storage_.get());
   RepresentationType default_type = storage_->default_representation_type();
diff --git a/ui/gfx/image/image.h b/ui/gfx/image/image.h
index ab13e84..e9619dc 100644
--- a/ui/gfx/image/image.h
+++ b/ui/gfx/image/image.h
@@ -28,6 +28,10 @@
 #include "ui/base/ui_export.h"
 #include "ui/gfx/native_widget_types.h"
 
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+typedef struct CGColorSpace* CGColorSpaceRef;
+#endif
+
 class SkBitmap;
 
 namespace {
@@ -175,6 +179,13 @@ class UI_EXPORT Image {
   // Swaps this image's internal representations with |other|.
   void SwapRepresentations(gfx::Image* other);
 
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+  // Set the default representation's color space. This is used for converting
+  // to NSImage. This is used to compensate for PNGCodec not writing or reading
+  // colorspace ancillary chunks. (sRGB, iCCP).
+  void SetSourceColorSpace(CGColorSpaceRef color_space);
+#endif  // defined(OS_MACOSX) && !defined(OS_IOS)
+
  private:
   // Returns the type of the default representation.
   RepresentationType DefaultRepresentationType() const;
diff --git a/ui/gfx/image/image_mac.mm b/ui/gfx/image/image_mac.mm
index c79100c..0158521 100644
--- a/ui/gfx/image/image_mac.mm
+++ b/ui/gfx/image/image_mac.mm
@@ -48,7 +48,8 @@ scoped_refptr<base::RefCountedMemory> Get1xPNGBytesFromNSImage(
   return refcounted_bytes;
 }
 
-NSImage* NSImageFromPNG(const std::vector<gfx::ImagePNGRep>& image_png_reps) {
+NSImage* NSImageFromPNG(const std::vector<gfx::ImagePNGRep>& image_png_reps,
+                        CGColorSpaceRef color_space) {
   if (image_png_reps.empty()) {
     LOG(ERROR) << "Unable to decode PNG.";
     return GetErrorNSImage();
@@ -69,6 +70,23 @@ NSImage* NSImageFromPNG(const std::vector<gfx::ImagePNGRep>& image_png_reps) {
       return GetErrorNSImage();
     }
 
+    // PNGCodec ignores colorspace related ancillary chunks (sRGB, iCCP). Ignore
+    // colorspace information when decoding directly from PNG to an NSImage so
+    // that the conversions: PNG -> SkBitmap -> NSImage and PNG -> NSImage
+    // produce visually similar results.
+    CGColorSpaceModel decoded_color_space_model = CGColorSpaceGetModel(
+        [[ns_image_rep colorSpace] CGColorSpace]);
+    CGColorSpaceModel color_space_model = CGColorSpaceGetModel(color_space);
+    if (decoded_color_space_model == color_space_model) {
+      scoped_nsobject<NSColorSpace> ns_color_space(
+          [[NSColorSpace alloc] initWithCGColorSpace:color_space]);
+      NSBitmapImageRep* ns_retagged_image_rep =
+          [ns_image_rep
+              bitmapImageRepByRetaggingWithColorSpace:ns_color_space];
+      if (ns_retagged_image_rep && ns_retagged_image_rep != ns_image_rep)
+        ns_image_rep.reset([ns_retagged_image_rep retain]);
+    }
+
     if (!image.get()) {
       float scale = ui::GetScaleFactorScale(image_png_reps[i].scale_factor);
       NSSize image_size = NSMakeSize([ns_image_rep pixelsWide] / scale,