diff options
8 files changed, 181 insertions, 61 deletions
diff --git a/chrome/browser/favicon/favicon_util.cc b/chrome/browser/favicon/favicon_util.cc index 0924483..c700d98 100644 --- a/chrome/browser/favicon/favicon_util.cc +++ b/chrome/browser/favicon/favicon_util.cc @@ -14,22 +14,31 @@ namespace { -// Attempts to create a resulting image of DIP size |favicon_size| with -// image reps of |scale_factors| without resizing or decoding the bitmap -// data. -// Returns an empty gfx::Image if the bitmap data must be resized. -gfx::Image SelectFaviconFramesFromPNGsWithoutResizing( +// Creates image reps of DIP size |favicon_size| for the subset of +// |scale_factors| for which the image reps can be created without resizing +// or decoding the bitmap data. +std::vector<gfx::ImagePNGRep> SelectFaviconFramesFromPNGsWithoutResizing( const std::vector<history::FaviconBitmapResult>& png_data, - const std::vector<ui::ScaleFactor> scale_factors, + const std::vector<ui::ScaleFactor>& scale_factors, int favicon_size) { - if (png_data.size() != scale_factors.size()) - return gfx::Image(); + std::vector<gfx::ImagePNGRep> png_reps; + if (png_data.empty()) + return png_reps; // A |favicon_size| of 0 indicates that the largest frame is desired. - if (favicon_size == 0 && png_data.size() == 1) { - scoped_refptr<base::RefCountedMemory> bitmap_data = png_data[0].bitmap_data; - return gfx::Image::CreateFrom1xPNGBytes(bitmap_data->front(), - bitmap_data->size()); + if (favicon_size == 0 && scale_factors.size() == 1) { + int maximum_area = 0; + scoped_refptr<base::RefCountedMemory> best_candidate; + for (size_t i = 0; i < png_data.size(); ++i) { + int area = png_data[i].pixel_size.GetArea(); + if (area > maximum_area) { + maximum_area = area; + best_candidate = png_data[i].bitmap_data; + } + } + png_reps.push_back(gfx::ImagePNGRep(best_candidate, + scale_factors[0])); + return png_reps; } // Cache the scale factor for each pixel size as |scale_factors| may contain @@ -42,24 +51,23 @@ gfx::Image SelectFaviconFramesFromPNGsWithoutResizing( desired_pixel_sizes[pixel_size] = scale_factors[i]; } - std::vector<gfx::ImagePNGRep> png_reps; for (size_t i = 0; i < png_data.size(); ++i) { if (!png_data[i].is_valid()) - return gfx::Image(); + continue; const gfx::Size& pixel_size = png_data[i].pixel_size; if (pixel_size.width() != pixel_size.height()) - return gfx::Image(); + continue; std::map<int, ui::ScaleFactor>::iterator it = desired_pixel_sizes.find( pixel_size.width()); if (it == desired_pixel_sizes.end()) - return gfx::Image(); + continue; png_reps.push_back(gfx::ImagePNGRep(png_data[i].bitmap_data, it->second)); } - return gfx::Image(png_reps); + return png_reps; } } // namespace @@ -97,17 +105,34 @@ gfx::Image FaviconUtil::SelectFaviconFramesFromPNGs( const std::vector<history::FaviconBitmapResult>& png_data, const std::vector<ui::ScaleFactor> scale_factors, int favicon_size) { - // Attempt to create a result image without resizing the bitmap data or - // decoding it. FaviconHandler stores already resized favicons into history - // so no additional resizing should be needed in the common case. Skipping - // decoding to ImageSkia is useful because the ImageSkia representation may - // not be needed and because the decoding is done on the UI thread and the - // decoding can be a significant performance hit if a user has many bookmarks. + // Create image reps for as many scale factors as possible without resizing + // the bitmap data or decoding it. FaviconHandler stores already resized + // favicons into history so no additional resizing should be needed in the + // common case. + // Creating the gfx::Image from |png_data| without resizing or decoding if + // possible is important because: + // - Sync does a byte-to-byte comparison of gfx::Image::As1xPNGBytes() to + // the data it put into the database in order to determine whether any + // updates should be pushed to sync. + // - The decoding occurs on the UI thread and the decoding can be a + // significant performance hit if a user has many bookmarks. // TODO(pkotwicz): Move the decoding off the UI thread. - gfx::Image without_resizing = SelectFaviconFramesFromPNGsWithoutResizing( - png_data, scale_factors, favicon_size); - if (!without_resizing.IsEmpty()) - return without_resizing; + std::vector<gfx::ImagePNGRep> png_reps = + SelectFaviconFramesFromPNGsWithoutResizing(png_data, scale_factors, + favicon_size); + + std::vector<ui::ScaleFactor> scale_factors_to_generate = scale_factors; + for (size_t i = 0; i < png_reps.size(); ++i) { + std::vector<ui::ScaleFactor>::iterator it = std::find( + scale_factors_to_generate.begin(), + scale_factors_to_generate.end(), + png_reps[i].scale_factor); + CHECK(it != scale_factors_to_generate.end()); + scale_factors_to_generate.erase(it); + } + + if (scale_factors_to_generate.empty()) + return gfx::Image(png_reps); std::vector<SkBitmap> bitmaps; for (size_t i = 0; i < png_data.size(); ++i) { @@ -126,8 +151,23 @@ gfx::Image FaviconUtil::SelectFaviconFramesFromPNGs( return gfx::Image(); gfx::ImageSkia resized_image_skia = SelectFaviconFrames(bitmaps, - scale_factors, favicon_size, NULL); - return gfx::Image(resized_image_skia); + scale_factors_to_generate, favicon_size, NULL); + + if (png_reps.empty()) + return gfx::Image(resized_image_skia); + + std::vector<gfx::ImageSkiaRep> resized_image_skia_reps = + resized_image_skia.image_reps(); + for (size_t i = 0; i < resized_image_skia_reps.size(); ++i) { + scoped_refptr<base::RefCountedBytes> png_bytes(new base::RefCountedBytes()); + if (gfx::PNGCodec::EncodeBGRASkBitmap( + resized_image_skia_reps[i].sk_bitmap(), false, &png_bytes->data())) { + png_reps.push_back(gfx::ImagePNGRep(png_bytes, + resized_image_skia_reps[i].scale_factor())); + } + } + + return gfx::Image(png_reps); } // static diff --git a/chrome/browser/history/history_backend.cc b/chrome/browser/history/history_backend.cc index 63b8b5b..b543692 100644 --- a/chrome/browser/history/history_backend.cc +++ b/chrome/browser/history/history_backend.cc @@ -1990,7 +1990,6 @@ void HistoryBackend::MergeFavicon( // Copy the favicon bitmaps mapped to |page_url| to the favicon at |icon_url| // till the limit of |kMaxFaviconBitmapsPerIconURL| is reached. - bool migrated_bitmaps = false; for (size_t i = 0; i < icon_mappings.size(); ++i) { if (favicon_sizes.size() >= kMaxFaviconBitmapsPerIconURL) break; @@ -2011,8 +2010,6 @@ void HistoryBackend::MergeFavicon( if (it != favicon_sizes.end()) continue; - migrated_bitmaps = true; - // Add the favicon bitmap as expired as it is not consistent with the // merged in data. thumbnail_db_->AddFaviconBitmap(favicon_id, @@ -2033,13 +2030,9 @@ void HistoryBackend::MergeFavicon( SetFaviconMappingsForPageAndRedirects(page_url, icon_type, favicon_ids); } - // Sync currently does not properly deal with notifications as a result of - // replacing a favicon bitmap. For M25, do not send any notifications if a - // bitmap was replaced and no bitmaps were added or deleted. This is a - // temporary fix for http://crbug.com/169460. - if (migrated_bitmaps || !replaced_bitmap) - SendFaviconChangedNotificationForPageAndRedirects(page_url); - + // Send notification to the UI as at least a favicon bitmap was added or + // replaced. + SendFaviconChangedNotificationForPageAndRedirects(page_url); ScheduleCommit(); } diff --git a/chrome/browser/history/history_backend_unittest.cc b/chrome/browser/history/history_backend_unittest.cc index 42d5e87..e59215e 100644 --- a/chrome/browser/history/history_backend_unittest.cc +++ b/chrome/browser/history/history_backend_unittest.cc @@ -1708,8 +1708,7 @@ TEST_F(HistoryBackendTest, MergeFaviconPageURLInDB) { bitmap_data = new base::RefCountedBytes(data); backend_->MergeFavicon(page_url, icon_url1, FAVICON, bitmap_data, kSmallSize); - // The small favicon bitmap at |icon_url1| should be overwritten and favicon - // sizes should remain unchanged. No notification should be sent. + // The small favicon bitmap at |icon_url1| should be overwritten. icon_mappings.clear(); EXPECT_TRUE(backend_->thumbnail_db_->GetIconMappingsForPageURL(page_url, &icon_mappings)); @@ -1721,8 +1720,6 @@ TEST_F(HistoryBackendTest, MergeFaviconPageURLInDB) { EXPECT_TRUE(BitmapDataEqual('b', favicon_bitmap.bitmap_data)); EXPECT_EQ(kSmallSize, favicon_bitmap.pixel_size); - EXPECT_EQ(1, num_broadcasted_notifications()); - // 3) Merge favicon for the same icon URL, but a pixel size for which there is // no favicon bitmap. data[0] = 'c'; @@ -1775,7 +1772,7 @@ TEST_F(HistoryBackendTest, MergeFaviconPageURLInDB) { // A notification should have been broadcast for each call to SetFavicons() // and MergeFavicon(). - EXPECT_EQ(3, num_broadcasted_notifications()); + EXPECT_EQ(4, num_broadcasted_notifications()); } // Test calling MergeFavicon() when |icon_url| is known to the database but not diff --git a/chrome/browser/sync/test/integration/bookmarks_helper.cc b/chrome/browser/sync/test/integration/bookmarks_helper.cc index 608f107..c6bc419 100644 --- a/chrome/browser/sync/test/integration/bookmarks_helper.cc +++ b/chrome/browser/sync/test/integration/bookmarks_helper.cc @@ -5,8 +5,11 @@ #include "chrome/browser/sync/test/integration/bookmarks_helper.h" #include "base/compiler_specific.h" +#include "base/file_util.h" +#include "base/path_service.h" #include "base/rand_util.h" #include "base/string_number_conversions.h" +#include "base/string_util.h" #include "base/stringprintf.h" #include "base/synchronization/waitable_event.h" #include "base/utf_string_conversions.h" @@ -19,9 +22,11 @@ #include "chrome/browser/history/history_service_factory.h" #include "chrome/browser/history/history_types.h" #include "chrome/browser/profiles/profile.h" +#include "chrome/browser/sync/glue/bookmark_change_processor.h" #include "chrome/browser/sync/profile_sync_service_harness.h" #include "chrome/browser/sync/test/integration/sync_test.h" #include "chrome/browser/sync/test/integration/sync_datatype_helper.h" +#include "chrome/common/chrome_paths.h" #include "chrome/test/base/ui_test_utils.h" #include "testing/gtest/include/gtest/gtest.h" #include "third_party/skia/include/core/SkBitmap.h" @@ -208,17 +213,23 @@ FaviconData GetFaviconData(BookmarkModel* model, void SetFaviconImpl(Profile* profile, const BookmarkNode* node, const GURL& icon_url, - const gfx::Image& image) { + const gfx::Image& image, + bookmarks_helper::FaviconSource favicon_source) { BookmarkModel* model = BookmarkModelFactory::GetForProfile(profile); FaviconChangeObserver observer(model, node); FaviconService* favicon_service = FaviconServiceFactory::GetForProfile(profile, Profile::EXPLICIT_ACCESS); - favicon_service->SetFavicons(node->url(), - icon_url, - history::FAVICON, - image); + if (favicon_source == bookmarks_helper::FROM_UI) { + favicon_service->SetFavicons(node->url(), + icon_url, + history::FAVICON, + image); + } else { + browser_sync::BookmarkChangeProcessor::ApplyBookmarkFavicon( + node, profile, icon_url, image.As1xPNGBytes()); + } // Wait for the favicon for |node| to be invalidated. observer.WaitForSetFavicon(); @@ -493,7 +504,8 @@ void SetTitle(int profile, void SetFavicon(int profile, const BookmarkNode* node, const GURL& icon_url, - const gfx::Image& image) { + const gfx::Image& image, + FaviconSource favicon_source) { ASSERT_EQ(GetBookmarkModel(profile)->GetNodeByID(node->id()), node) << "Node " << node->GetTitle() << " does not belong to " << "Profile " << profile; @@ -505,9 +517,10 @@ void SetFavicon(int profile, if (test()->use_verifier()) { const BookmarkNode* v_node = NULL; FindNodeInVerifier(GetBookmarkModel(profile), node, &v_node); - SetFaviconImpl(test()->verifier(), v_node, icon_url, image); + SetFaviconImpl(test()->verifier(), v_node, icon_url, image, favicon_source); } - SetFaviconImpl(test()->GetProfile(profile), node, icon_url, image); + SetFaviconImpl(test()->GetProfile(profile), node, icon_url, image, + favicon_source); } const BookmarkNode* SetURL(int profile, @@ -705,6 +718,22 @@ gfx::Image CreateFavicon(SkColor color) { return gfx::Image(favicon); } +gfx::Image Create1xFaviconFromPNGFile(const std::string& path) { + const char* kPNGExtension = ".png"; + if (!EndsWith(path, kPNGExtension, false)) + return gfx::Image(); + + FilePath full_path; + if (!PathService::Get(chrome::DIR_TEST_DATA, &full_path)) + return gfx::Image(); + + full_path = full_path.AppendASCII("sync").AppendASCII(path); + std::string contents; + file_util::ReadFileToString(full_path, &contents); + return gfx::Image::CreateFrom1xPNGBytes( + reinterpret_cast<const unsigned char*>(contents.data()), contents.size()); +} + std::string IndexedURL(int i) { return StringPrintf("http://www.host.ext:1234/path/filename/%d", i); } diff --git a/chrome/browser/sync/test/integration/bookmarks_helper.h b/chrome/browser/sync/test/integration/bookmarks_helper.h index f69310b..ddc60ef 100644 --- a/chrome/browser/sync/test/integration/bookmarks_helper.h +++ b/chrome/browser/sync/test/integration/bookmarks_helper.h @@ -88,12 +88,19 @@ void SetTitle(int profile, const BookmarkNode* node, const std::wstring& new_title); +// The source of the favicon. +enum FaviconSource { + FROM_UI, + FROM_SYNC +}; + // Sets the |icon_url| and |image| data for the favicon for |node| in the // bookmark model for |profile|. void SetFavicon(int profile, const BookmarkNode* node, const GURL& icon_url, - const gfx::Image& image); + const gfx::Image& image, + FaviconSource source); // Changes the url of the node |node| in the bookmark model of profile // |profile| to |new_url|. Returns a pointer to the node with the changed url. @@ -169,6 +176,9 @@ int CountFoldersWithTitlesMatching( // scale factors (eg MacOS) in addition to 1x. gfx::Image CreateFavicon(SkColor color); +// Creates a 1x only favicon from the PNG file at |path|. +gfx::Image Create1xFaviconFromPNGFile(const std::string& path); + // Returns a URL identifiable by |i|. std::string IndexedURL(int i); diff --git a/chrome/browser/sync/test/integration/single_client_bookmarks_sync_test.cc b/chrome/browser/sync/test/integration/single_client_bookmarks_sync_test.cc index a6e6ed2..fdc39bd 100644 --- a/chrome/browser/sync/test/integration/single_client_bookmarks_sync_test.cc +++ b/chrome/browser/sync/test/integration/single_client_bookmarks_sync_test.cc @@ -6,14 +6,18 @@ #include "chrome/browser/sync/profile_sync_service_harness.h" #include "chrome/browser/sync/test/integration/bookmarks_helper.h" #include "chrome/browser/sync/test/integration/sync_test.h" +#include "ui/base/layout.h" using bookmarks_helper::AddFolder; using bookmarks_helper::AddURL; +using bookmarks_helper::Create1xFaviconFromPNGFile; using bookmarks_helper::GetBookmarkBarNode; +using bookmarks_helper::GetBookmarkModel; using bookmarks_helper::GetOtherNode; using bookmarks_helper::ModelMatchesVerifier; using bookmarks_helper::Move; using bookmarks_helper::Remove; +using bookmarks_helper::SetFavicon; using bookmarks_helper::SetTitle; class SingleClientBookmarksSyncTest : public SyncTest { @@ -160,3 +164,47 @@ IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest, ASSERT_TRUE(GetClient(0)->AwaitFullSyncCompletion("Restarted sync.")); ASSERT_TRUE(ModelMatchesVerifier(0)); } + +// Test that a client doesn't mutate the favicon data in the process +// of storing the favicon data from sync to the database or in the process +// of requesting data from the database for sync. +IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest, + SetFaviconHiDPIDifferentCodec) { + // Set the supported scale factors to 1x and 2x such that + // BookmarkModel::GetFavicon() requests both 1x and 2x. + // 1x -> for sync, 2x -> for the UI. + std::vector<ui::ScaleFactor> supported_scale_factors; + supported_scale_factors.push_back(ui::SCALE_FACTOR_100P); + supported_scale_factors.push_back(ui::SCALE_FACTOR_200P); + ui::test::SetSupportedScaleFactors(supported_scale_factors); + + ASSERT_TRUE(SetupSync()) << "SetupSync() failed."; + ASSERT_TRUE(ModelMatchesVerifier(0)); + + const GURL page_url("http://www.google.com"); + const GURL icon_url("http://www.google.com/favicon.ico"); + const BookmarkNode* bookmark = AddURL(0, L"title", page_url); + + // Simulate receiving a favicon from sync encoded by a different PNG encoder + // than the one native to the OS. This tests the PNG data is not decoded to + // SkBitmap (or any other image format) then encoded back to PNG on the path + // between sync and the database. + gfx::Image original_favicon = Create1xFaviconFromPNGFile( + "favicon_cocoa_png_codec.png"); + ASSERT_FALSE(original_favicon.IsEmpty()); + SetFavicon(0, bookmark, icon_url, original_favicon, + bookmarks_helper::FROM_SYNC); + + ASSERT_TRUE(GetClient(0)->AwaitFullSyncCompletion("Added bookmark")); + ASSERT_TRUE(ModelMatchesVerifier(0)); + + scoped_refptr<base::RefCountedMemory> original_favicon_bytes = + original_favicon.As1xPNGBytes(); + gfx::Image final_favicon = GetBookmarkModel(0)->GetFavicon(bookmark); + scoped_refptr<base::RefCountedMemory> final_favicon_bytes = + final_favicon.As1xPNGBytes(); + + // Check that the data was not mutated from the original. + EXPECT_TRUE(original_favicon_bytes.get()); + EXPECT_TRUE(original_favicon_bytes->Equals(final_favicon_bytes)); +} diff --git a/chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc b/chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc index 0582614..85e80573 100644 --- a/chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc +++ b/chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc @@ -146,7 +146,8 @@ IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest, SC_AddFirstBMWithFavicon) { ASSERT_TRUE(bookmark != NULL); ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1))); - SetFavicon(0, bookmark, icon_url, CreateFavicon(SK_ColorWHITE)); + SetFavicon(0, bookmark, icon_url, CreateFavicon(SK_ColorWHITE), + bookmarks_helper::FROM_UI); ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1))); ASSERT_TRUE(AllModelsMatchVerifier()); } @@ -156,9 +157,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest, SC_AddFirstBMWithFavicon) { // In particular, the synced 16x16 favicon bitmap should overwrite 16x16 // favicon bitmaps on all clients. (Though non-16x16 favicon bitmaps // are unchanged). -// TODO(pkotwicz): Reenable test. http://crbug.com/171465 -IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest, - DISABLED_SC_SetFaviconHiDPI) { +IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest, SC_SetFaviconHiDPI) { // Set the supported scale factors to include 2x such that CreateFavicon() // creates a favicon with hidpi representations and that methods in the // FaviconService request hidpi favicons. @@ -177,16 +176,19 @@ IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest, const BookmarkNode* bookmark0 = AddURL(0, kGenericURLTitle, page_url); ASSERT_TRUE(bookmark0 != NULL); ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1))); - SetFavicon(0, bookmark0, icon_url1, CreateFavicon(SK_ColorWHITE)); + SetFavicon(0, bookmark0, icon_url1, CreateFavicon(SK_ColorWHITE), + bookmarks_helper::FROM_UI); ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1))); ASSERT_TRUE(AllModelsMatchVerifier()); const BookmarkNode* bookmark1 = GetUniqueNodeByURL(1, page_url); - SetFavicon(1, bookmark1, icon_url1, CreateFavicon(SK_ColorBLUE)); + SetFavicon(1, bookmark1, icon_url1, CreateFavicon(SK_ColorBLUE), + bookmarks_helper::FROM_UI); ASSERT_TRUE(GetClient(1)->AwaitMutualSyncCycleCompletion(GetClient(0))); ASSERT_TRUE(AllModelsMatchVerifier()); - SetFavicon(0, bookmark0, icon_url2, CreateFavicon(SK_ColorGREEN)); + SetFavicon(0, bookmark0, icon_url2, CreateFavicon(SK_ColorGREEN), + bookmarks_helper::FROM_UI); ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1))); ASSERT_TRUE(AllModelsMatchVerifier()); } diff --git a/chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc b/chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc index 2c4ef16..e7113e2 100644 --- a/chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc +++ b/chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc @@ -449,7 +449,8 @@ IN_PROC_BROWSER_TEST_F(TwoClientTypedUrlsSyncTest, BookmarksWithTypedVisit) { const BookmarkNode* node = bookmarks_helper::AddURL( 0, bookmarks_helper::IndexedURLTitle(0), bookmark_url); bookmarks_helper::SetFavicon(0, node, bookmark_icon_url, - bookmarks_helper::CreateFavicon(SK_ColorWHITE)); + bookmarks_helper::CreateFavicon(SK_ColorWHITE), + bookmarks_helper::FROM_UI); ASSERT_TRUE(SetupSync()) << "SetupSync() failed."; ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1))); |