From 477f7f4fbee00103bf512362656cfd6143ee082b Mon Sep 17 00:00:00 2001 From: "avi@google.com" Date: Thu, 18 Jun 2009 20:23:44 +0000 Subject: Add favicons to tabs on the Mac. Also moved SkBitmapToNSImage() to skia/ext/skia_utils_mac.h and removed chrome/browser/cocoa/cocoa_utils.h. Patch by rsesek. BUG=13565 Review URL: http://codereview.chromium.org/131018 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@18743 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/cocoa/bookmark_bar_controller.mm | 8 +- chrome/browser/cocoa/cocoa_utils.h | 19 ----- chrome/browser/cocoa/cocoa_utils.mm | 100 ------------------------ chrome/browser/cocoa/cocoa_utils_unittest.mm | 83 -------------------- chrome/browser/cocoa/tab_strip_controller.mm | 26 +++++- chrome/chrome.gyp | 3 - skia/ext/skia_utils_mac.h | 3 + skia/ext/skia_utils_mac.mm | 88 +++++++++++++++++++++ skia/ext/skia_utils_mac_unittest.mm | 83 ++++++++++++++++++++ webkit/tools/test_shell/test_shell.gyp | 5 +- 10 files changed, 204 insertions(+), 214 deletions(-) delete mode 100644 chrome/browser/cocoa/cocoa_utils.h delete mode 100644 chrome/browser/cocoa/cocoa_utils.mm delete mode 100644 chrome/browser/cocoa/cocoa_utils_unittest.mm create mode 100644 skia/ext/skia_utils_mac_unittest.mm diff --git a/chrome/browser/cocoa/bookmark_bar_controller.mm b/chrome/browser/cocoa/bookmark_bar_controller.mm index e30a1a2..ad92820 100644 --- a/chrome/browser/cocoa/bookmark_bar_controller.mm +++ b/chrome/browser/cocoa/bookmark_bar_controller.mm @@ -10,12 +10,10 @@ #import "chrome/browser/cocoa/bookmark_bar_controller.h" #import "chrome/browser/cocoa/bookmark_bar_view.h" #import "chrome/browser/cocoa/bookmark_button_cell.h" -#import "chrome/browser/cocoa/cocoa_utils.h" #include "chrome/browser/profile.h" #include "chrome/common/pref_names.h" #include "chrome/common/pref_service.h" - -using namespace CocoaUtils; +#include "skia/ext/skia_utils_mac.h" @interface BookmarkBarController(Private) - (void)applyContentAreaOffset:(BOOL)apply; @@ -167,7 +165,7 @@ const CGFloat kBookmarkHorizontalPadding = 8.0; // (and their icons) are loaded on the IO thread to speed launch. const SkBitmap& favicon = bookmarkModel_->GetFavIcon(node); if (!favicon.isNull()) { - NSImage* image = SkBitmapToNSImage(favicon); + NSImage* image = gfx::SkBitmapToNSImage(favicon); if (image) { [cell setImage:image]; [cell setImagePosition:NSImageLeft]; @@ -295,7 +293,7 @@ const CGFloat kBookmarkHorizontalPadding = 8.0; void* pointer = [[cell representedObject] pointerValue]; BookmarkNode* cellnode = static_cast(pointer); if (cellnode == node) { - NSImage* image = SkBitmapToNSImage(bookmarkModel_->GetFavIcon(node)); + NSImage* image = gfx::SkBitmapToNSImage(bookmarkModel_->GetFavIcon(node)); if (image) { [cell setImage:image]; [cell setImagePosition:NSImageLeft]; diff --git a/chrome/browser/cocoa/cocoa_utils.h b/chrome/browser/cocoa/cocoa_utils.h deleted file mode 100644 index 9f721d1..0000000 --- a/chrome/browser/cocoa/cocoa_utils.h +++ /dev/null @@ -1,19 +0,0 @@ -// Copyright (c) 2009 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. - -#include -#include "third_party/skia/include/core/SkBitmap.h" - -// Generic Cocoa utilities which fit nowhere else. - -namespace CocoaUtils { - -// Given an SkBitmap, return an autoreleased NSImage. This function -// was not placed in -// third_party/skia/src/utils/mac/SkCreateCGImageRef.cpp since that -// would make Skia dependent on Cocoa. -NSImage* SkBitmapToNSImage(const SkBitmap& icon); - - -}; // namespace CocoaUtils diff --git a/chrome/browser/cocoa/cocoa_utils.mm b/chrome/browser/cocoa/cocoa_utils.mm deleted file mode 100644 index 4bc65a3..0000000 --- a/chrome/browser/cocoa/cocoa_utils.mm +++ /dev/null @@ -1,100 +0,0 @@ -// Copyright (c) 2009 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. - -#include -#include "chrome/browser/cocoa/cocoa_utils.h" -#import -#include "third_party/skia/include/utils/mac/SkCGUtils.h" -#include "third_party/skia/include/core/SkColorPriv.h" - -namespace { - -// Callback passed to CGDataProviderCreateWithData() -void ReleaseData(void* info, const void* pixelData, size_t size) { - // info is a non-const pixelData - if (info) - free(info); -} - -} - -namespace CocoaUtils { - -NSImage* SkBitmapToNSImage(const SkBitmap& icon) { - // First convert SkBitmap to CGImageRef. - CGImageRef cgimage; - if (icon.config() != SkBitmap::kARGB_8888_Config) { - cgimage = SkCreateCGImageRef(icon); - } else { - // The above code returns a valid NSImage even in the - // kARGB_8888_Config case. As an example, the unit test which - // draws a blue SkBitmap can lockPixels, NSReadPixel, and pull out - // a single pixel from the NSImage and see it blue. However, the - // NSImage returned will be in ABGR format. Although Cocoa is - // otherwise happy with that format (as seen in simple tests - // outside Chromium), Chromium is NOT happy. In Chromium, B and R - // are swapped. - // - // As a hint, CIImage supports a few formats, such as ARGB. - // Interestingly, it does NOT support ABGR. I speculate there is - // some way we set up our drawing context which has the format - // specified wrong (in skia/ext/bitmap_platform_device_mac.cc), - // but I have not been able to solve this yet. - // - // TODO(jrg): track down the disconnect. - // TODO(jrg): Remove byte conversion. - // TODO(jrg): Fix unit tests to NOT swap bytes. - // http://crbug.com/14020 - CGBitmapInfo info = (kCGImageAlphaPremultipliedFirst | - kCGBitmapByteOrder32Host); - int width = icon.width(); - int height = icon.height(); - int rowbytes = icon.rowBytes(); - int rowwords = rowbytes/4; - unsigned length = rowbytes * height; - DCHECK(length > 0); - uint32_t* rawptr = static_cast(malloc(length)); - DCHECK(rawptr); - if (!rawptr || !length) - return nil; - - // Convert ABGR to ARGB - icon.lockPixels(); - uint32_t* rawbitmap = static_cast(icon.getPixels()); - uint32_t rawbit; - int offset; - for (int y = 0; y < height; y++) { - for (int x = 0; x < width; x++) { - offset = x + y*rowwords; - rawbit = rawbitmap[offset]; - rawptr[offset] = SkPackARGB32(SkGetPackedA32(rawbit), - SkGetPackedR32(rawbit), - SkGetPackedG32(rawbit), - SkGetPackedB32(rawbit)); - } - } - icon.unlockPixels(); - - CGDataProviderRef dataRef = - CGDataProviderCreateWithData(rawptr, rawptr, length, ReleaseData); - CGColorSpaceRef space = CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB); - cgimage = CGImageCreate(width, height, 8, - icon.bytesPerPixel() * 8, - rowbytes, space, info, dataRef, - NULL, false, kCGRenderingIntentDefault); - CGColorSpaceRelease(space); - CGDataProviderRelease(dataRef); - } - - // Now convert to NSImage. - NSBitmapImageRep* bitmap = [[[NSBitmapImageRep alloc] - initWithCGImage:cgimage] autorelease]; - CFRelease(cgimage); - NSImage* image = [[[NSImage alloc] init] autorelease]; - [image addRepresentation:bitmap]; - return image; -} - -} // namespace CocoaUtils - diff --git a/chrome/browser/cocoa/cocoa_utils_unittest.mm b/chrome/browser/cocoa/cocoa_utils_unittest.mm deleted file mode 100644 index 7812f2c..0000000 --- a/chrome/browser/cocoa/cocoa_utils_unittest.mm +++ /dev/null @@ -1,83 +0,0 @@ -// Copyright (c) 2009 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. - -#import "chrome/browser/cocoa/cocoa_utils.h" -#import "chrome/browser/cocoa/cocoa_test_helper.h" -#include "testing/gtest/include/gtest/gtest.h" - -namespace { - -class CocoaUtilsTest : public testing::Test { - public: - CocoaTestHelper cocoa_helper_; // Inits Cocoa, creates window, etc... - - // If not red, is blue. - // If not tfbit (twenty-four-bit), is 444. - // If swap, swap R and B before testing (see TODOs in cocoa_utils.mm) - void ShapeHelper(int width, int height, bool isred, bool tfbit, bool swap); -}; - -void CocoaUtilsTest::ShapeHelper(int width, int height, - bool isred, bool tfbit, bool swap) { - SkBitmap thing; - - if (tfbit) - thing.setConfig(SkBitmap::kARGB_8888_Config, width, height); - else - thing.setConfig(SkBitmap::kARGB_4444_Config, width, height); - thing.allocPixels(); - - if (isred) - thing.eraseRGB(0xff, 0, 0); - else - thing.eraseRGB(0, 0, 0xff); - - // Confirm size - NSImage* image = CocoaUtils::SkBitmapToNSImage(thing); - EXPECT_DOUBLE_EQ([image size].width, (double)width); - EXPECT_DOUBLE_EQ([image size].height, (double)height); - - // Get the color of a pixel and make sure it looks fine - [image lockFocus]; - - int x = width > 17 ? 17 : 0; - int y = height > 17 ? 17 : 0; - NSColor* color = NSReadPixel(NSMakePoint(x, y)); - CGFloat red = 0, green = 0, blue = 0, alpha = 0; - [image unlockFocus]; - [color getRed:&red green:&green blue:&blue alpha:&alpha]; - - if (swap) { - CGFloat tmp = red; - red = blue; - blue = tmp; - } - - // Be tolerant of floating point rounding, gamma, etc. - if (isred) { - EXPECT_GT(red, 0.8); - EXPECT_LT(blue, 0.2); - } else { - EXPECT_LT(red, 0.2); - EXPECT_GT(blue, 0.8); - } - EXPECT_LT(green, 0.2); - EXPECT_GT(alpha, 0.9); -} - - -TEST_F(CocoaUtilsTest, BitmapToNSImage_RedSquare64x64) { - ShapeHelper(64, 64, true, true, true); -} - -TEST_F(CocoaUtilsTest, BitmapToNSImage_BlueRectangle199x19) { - ShapeHelper(199, 19, false, true, true); -} - -TEST_F(CocoaUtilsTest, BitmapToNSImage_BlueRectangle444) { - ShapeHelper(200, 200, false, false, false); -} - - -} // namespace diff --git a/chrome/browser/cocoa/tab_strip_controller.mm b/chrome/browser/cocoa/tab_strip_controller.mm index b81b455..56eb7d0 100644 --- a/chrome/browser/cocoa/tab_strip_controller.mm +++ b/chrome/browser/cocoa/tab_strip_controller.mm @@ -5,6 +5,7 @@ #import "chrome/browser/cocoa/tab_strip_controller.h" #include "app/l10n_util.h" +#include "base/mac_util.h" #include "base/sys_string_conversions.h" #include "chrome/app/chrome_dll_resource.h" #include "chrome/browser/browser.h" @@ -17,10 +18,13 @@ #import "chrome/browser/cocoa/tab_strip_model_observer_bridge.h" #import "chrome/browser/cocoa/tab_view.h" #import "chrome/browser/cocoa/throbber_view.h" +#include "chrome/browser/tab_contents/navigation_controller.h" +#include "chrome/browser/tab_contents/navigation_entry.h" #include "chrome/browser/tab_contents/tab_contents.h" #include "chrome/browser/tab_contents/tab_contents_view.h" #include "chrome/browser/tabs/tab_strip_model.h" #include "grit/generated_resources.h" +#include "skia/ext/skia_utils_mac.h" NSString* const kTabStripNumberOfTabsChanged = @"kTabStripNumberOfTabsChanged"; @@ -414,13 +418,29 @@ NSString* const kTabStripNumberOfTabsChanged = @"kTabStripNumberOfTabsChanged"; // A helper routine for creating an NSImageView to hold the fav icon for // |contents|. -// TODO(pinkerton): fill in with code to use the real favicon, not the default -// for all cases. - (NSImageView*)favIconImageViewForContents:(TabContents*)contents { NSRect iconFrame = NSMakeRect(0, 0, 16, 16); NSImageView* view = [[[NSImageView alloc] initWithFrame:iconFrame] autorelease]; - [view setImage:[NSImage imageNamed:@"nav"]]; + + NSImage* image = nil; + + NavigationEntry* navEntry = contents->controller().GetLastCommittedEntry(); + NavigationEntry::FaviconStatus favIcon = navEntry->favicon(); + const SkBitmap& bitmap = favIcon.bitmap(); + if (favIcon.is_valid() && !bitmap.isNull()) + image = gfx::SkBitmapToNSImage(bitmap); + + // Either we don't have a valid favicon or there was some issue converting it + // from an SkBitmap. Either way, just show the default. + if (!image) { + NSBundle* bundle = mac_util::MainAppBundle(); + image = [[NSImage alloc] initByReferencingFile: + [bundle pathForResource:@"nav" ofType:@"pdf"]]; + [image autorelease]; + } + + [view setImage:image]; return view; } diff --git a/chrome/chrome.gyp b/chrome/chrome.gyp index ad0d097..e8d43cf 100644 --- a/chrome/chrome.gyp +++ b/chrome/chrome.gyp @@ -718,8 +718,6 @@ 'browser/cocoa/clear_browsing_data_controller.h', 'browser/cocoa/clear_browsing_data_controller.mm', 'browser/cocoa/cocoa_test_helper.h', - 'browser/cocoa/cocoa_utils.h', - 'browser/cocoa/cocoa_utils.mm', 'browser/cocoa/command_observer_bridge.h', 'browser/cocoa/command_observer_bridge.mm', 'browser/cocoa/custom_home_pages_model.h', @@ -3361,7 +3359,6 @@ 'browser/cocoa/bookmark_menu_cocoa_controller_unittest.mm', 'browser/cocoa/browser_window_cocoa_unittest.mm', 'browser/cocoa/browser_window_controller_unittest.mm', - 'browser/cocoa/cocoa_utils_unittest.mm', 'browser/cocoa/command_observer_bridge_unittest.mm', 'browser/cocoa/custom_home_pages_model_unittest.mm', 'browser/cocoa/find_bar_bridge_unittest.mm', diff --git a/skia/ext/skia_utils_mac.h b/skia/ext/skia_utils_mac.h index 14cd17f..06cb151 100644 --- a/skia/ext/skia_utils_mac.h +++ b/skia/ext/skia_utils_mac.h @@ -57,6 +57,9 @@ SkBitmap CGImageToSkBitmap(CGImageRef image); #ifdef __OBJC__ // Draws an NSImage with a given size into a SkBitmap. SkBitmap NSImageToSkBitmap(NSImage* image, NSSize size, bool is_opaque); + +// Given an SkBitmap, return an autoreleased NSImage. +NSImage* SkBitmapToNSImage(const SkBitmap& icon); #endif } // namespace gfx diff --git a/skia/ext/skia_utils_mac.mm b/skia/ext/skia_utils_mac.mm index 6880ea8..3295be0 100644 --- a/skia/ext/skia_utils_mac.mm +++ b/skia/ext/skia_utils_mac.mm @@ -10,6 +10,19 @@ #include "base/scoped_cftyperef.h" #include "base/scoped_ptr.h" #include "skia/ext/bitmap_platform_device_mac.h" +#include "third_party/skia/include/utils/mac/SkCGUtils.h" +#include "third_party/skia/include/core/SkColorPriv.h" + +namespace { + +// Callback passed to CGDataProviderCreateWithData() +void ReleaseData(void* info, const void* pixelData, size_t size) { + // info is a non-const pixelData + if (info) + free(info); +} + +} namespace gfx { @@ -156,4 +169,79 @@ SkBitmap NSImageToSkBitmap(NSImage* image, NSSize size, bool is_opaque) { return bitmap; } +NSImage* SkBitmapToNSImage(const SkBitmap& icon) { + // First convert SkBitmap to CGImageRef. + CGImageRef cgimage; + if (icon.config() != SkBitmap::kARGB_8888_Config) { + cgimage = SkCreateCGImageRef(icon); + } else { + // The above code returns a valid NSImage even in the + // kARGB_8888_Config case. As an example, the unit test which + // draws a blue SkBitmap can lockPixels, NSReadPixel, and pull out + // a single pixel from the NSImage and see it blue. However, the + // NSImage returned will be in ABGR format. Although Cocoa is + // otherwise happy with that format (as seen in simple tests + // outside Chromium), Chromium is NOT happy. In Chromium, B and R + // are swapped. + // + // As a hint, CIImage supports a few formats, such as ARGB. + // Interestingly, it does NOT support ABGR. I speculate there is + // some way we set up our drawing context which has the format + // specified wrong (in skia/ext/bitmap_platform_device_mac.cc), + // but I have not been able to solve this yet. + // + // TODO(jrg): track down the disconnect. + // TODO(jrg): Remove byte conversion. + // TODO(jrg): Fix unit tests to NOT swap bytes. + // http://crbug.com/14020 + CGBitmapInfo info = (kCGImageAlphaPremultipliedFirst | + kCGBitmapByteOrder32Host); + int width = icon.width(); + int height = icon.height(); + int rowbytes = icon.rowBytes(); + int rowwords = rowbytes/4; + unsigned length = rowbytes * height; + DCHECK(length > 0); + uint32_t* rawptr = static_cast(malloc(length)); + DCHECK(rawptr); + if (!rawptr || !length) + return nil; + + // Convert ABGR to ARGB + icon.lockPixels(); + uint32_t* rawbitmap = static_cast(icon.getPixels()); + uint32_t rawbit; + int offset; + for (int y = 0; y < height; y++) { + for (int x = 0; x < width; x++) { + offset = x + y*rowwords; + rawbit = rawbitmap[offset]; + rawptr[offset] = SkPackARGB32(SkGetPackedA32(rawbit), + SkGetPackedR32(rawbit), + SkGetPackedG32(rawbit), + SkGetPackedB32(rawbit)); + } + } + icon.unlockPixels(); + + CGDataProviderRef dataRef = + CGDataProviderCreateWithData(rawptr, rawptr, length, ReleaseData); + CGColorSpaceRef space = CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB); + cgimage = CGImageCreate(width, height, 8, + icon.bytesPerPixel() * 8, + rowbytes, space, info, dataRef, + NULL, false, kCGRenderingIntentDefault); + CGColorSpaceRelease(space); + CGDataProviderRelease(dataRef); + } + + // Now convert to NSImage. + NSBitmapImageRep* bitmap = [[[NSBitmapImageRep alloc] + initWithCGImage:cgimage] autorelease]; + CFRelease(cgimage); + NSImage* image = [[[NSImage alloc] init] autorelease]; + [image addRepresentation:bitmap]; + return image; +} + } // namespace gfx diff --git a/skia/ext/skia_utils_mac_unittest.mm b/skia/ext/skia_utils_mac_unittest.mm new file mode 100644 index 0000000..ce1e7c2 --- /dev/null +++ b/skia/ext/skia_utils_mac_unittest.mm @@ -0,0 +1,83 @@ +// Copyright (c) 2009 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. + +#import "chrome/browser/cocoa/cocoa_test_helper.h" +#include "skia/ext/skia_utils_mac.mm" +#include "testing/gtest/include/gtest/gtest.h" + +namespace { + +class SkiaUtilsMacTest : public testing::Test { + public: + CocoaTestHelper cocoa_helper_; // Inits Cocoa, creates window, etc... + + // If not red, is blue. + // If not tfbit (twenty-four-bit), is 444. + // If swap, swap R and B before testing (see TODOs in skia_utils_mac.mm) + void ShapeHelper(int width, int height, bool isred, bool tfbit, bool swap); +}; + +void SkiaUtilsMacTest::ShapeHelper(int width, int height, + bool isred, bool tfbit, bool swap) { + SkBitmap thing; + + if (tfbit) + thing.setConfig(SkBitmap::kARGB_8888_Config, width, height); + else + thing.setConfig(SkBitmap::kARGB_4444_Config, width, height); + thing.allocPixels(); + + if (isred) + thing.eraseRGB(0xff, 0, 0); + else + thing.eraseRGB(0, 0, 0xff); + + // Confirm size + NSImage* image = gfx::SkBitmapToNSImage(thing); + EXPECT_DOUBLE_EQ([image size].width, (double)width); + EXPECT_DOUBLE_EQ([image size].height, (double)height); + + // Get the color of a pixel and make sure it looks fine + [image lockFocus]; + + int x = width > 17 ? 17 : 0; + int y = height > 17 ? 17 : 0; + NSColor* color = NSReadPixel(NSMakePoint(x, y)); + CGFloat red = 0, green = 0, blue = 0, alpha = 0; + [image unlockFocus]; + [color getRed:&red green:&green blue:&blue alpha:&alpha]; + + if (swap) { + CGFloat tmp = red; + red = blue; + blue = tmp; + } + + // Be tolerant of floating point rounding, gamma, etc. + if (isred) { + EXPECT_GT(red, 0.8); + EXPECT_LT(blue, 0.2); + } else { + EXPECT_LT(red, 0.2); + EXPECT_GT(blue, 0.8); + } + EXPECT_LT(green, 0.2); + EXPECT_GT(alpha, 0.9); +} + + +TEST_F(SkiaUtilsMacTest, BitmapToNSImage_RedSquare64x64) { + ShapeHelper(64, 64, true, true, true); +} + +TEST_F(SkiaUtilsMacTest, BitmapToNSImage_BlueRectangle199x19) { + ShapeHelper(199, 19, false, true, true); +} + +TEST_F(SkiaUtilsMacTest, BitmapToNSImage_BlueRectangle444) { + ShapeHelper(200, 200, false, false, false); +} + + +} // namespace diff --git a/webkit/tools/test_shell/test_shell.gyp b/webkit/tools/test_shell/test_shell.gyp index ec42d92..ec3b871e 100644 --- a/webkit/tools/test_shell/test_shell.gyp +++ b/webkit/tools/test_shell/test_shell.gyp @@ -521,7 +521,10 @@ '../webcore_unit_tests/XBMImageDecoder_unittest.cpp', 'image_decoder_unittest.cc', 'image_decoder_unittest.h', - ] + ], + 'sources': [ + '../../../skia/ext/skia_utils_mac_unittest.mm', + ], }], ['OS=="win"', { 'msvs_disabled_warnings': [ 4800 ], -- cgit v1.1