From 89ba3866f85a50efec9405376e9ecbd0d155bbab Mon Sep 17 00:00:00 2001 From: "pkotwicz@chromium.org" Date: Wed, 1 May 2013 16:10:09 +0000 Subject: [PATCH] Add support for the legacy chrome://favicon/size/16 format. Refactored FaviconSource to add unittests BUG=225235 TEST=FaviconSourceTest.* Review URL: https://chromiumcodereview.appspot.com/14348040 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@197628 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/ui/webui/favicon_source.cc | 191 ++++++++------- chrome/browser/ui/webui/favicon_source.h | 19 +- .../ui/webui/favicon_source_unittest.cc | 221 ++++++++++++++++++ .../ui/webui/session_favicon_source.cc | 2 +- chrome/chrome_tests_unit.gypi | 1 + 5 files changed, 346 insertions(+), 88 deletions(-) create mode 100644 chrome/browser/ui/webui/favicon_source_unittest.cc diff --git a/chrome/browser/ui/webui/favicon_source.cc b/chrome/browser/ui/webui/favicon_source.cc index 275b60bd9c9d..2f75cadcd0ed 100644 --- a/chrome/browser/ui/webui/favicon_source.cc +++ b/chrome/browser/ui/webui/favicon_source.cc @@ -54,7 +54,7 @@ FaviconSource::IconRequest::IconRequest() FaviconSource::IconRequest::IconRequest( const content::URLDataSource::GotDataCallback& cb, - const std::string& path, + const GURL& path, int size, ui::ScaleFactor scale) : callback(cb), @@ -88,110 +88,42 @@ void FaviconSource::StartDataRequest( const content::URLDataSource::GotDataCallback& callback) { FaviconService* favicon_service = FaviconServiceFactory::GetForProfile(profile_, Profile::EXPLICIT_ACCESS); - if (!favicon_service || raw_path.empty()) { + if (!favicon_service) { SendDefaultResponse(callback); return; } - // Translate to regular path if |raw_path| is of the form - // chrome-search://favicon/, where - // "most_visited_item_id" is a uint64. - std::string path = InstantService::MaybeTranslateInstantPathOnUI(profile_, - raw_path); - - DCHECK_EQ(16, gfx::kFaviconSize); + bool is_icon_url = false; + GURL url; int size_in_dip = 16; ui::ScaleFactor scale_factor = ui::SCALE_FACTOR_100P; + bool success = ParsePath(raw_path, &is_icon_url, &url, &size_in_dip, + &scale_factor); - size_t parsed_index = 0; - if (HasSubstringAt(path, parsed_index, kLargestParameter)) { - parsed_index += strlen(kLargestParameter); - size_in_dip = 0; - } else if (HasSubstringAt(path, parsed_index, kSizeParameter)) { - parsed_index += strlen(kSizeParameter); - - size_t scale_delimiter = path.find("@", parsed_index); - if (scale_delimiter == std::string::npos) { - SendDefaultResponse(callback); - return; - } - - std::string size = path.substr(parsed_index, - scale_delimiter - parsed_index); - if (!base::StringToInt(size, &size_in_dip)) { - SendDefaultResponse(callback); - return; - } - - if (size_in_dip != 64 && size_in_dip != 32) { - // Only 64x64, 32x32 and 16x16 icons are supported. - size_in_dip = 16; - } - - size_t slash = path.find("/", scale_delimiter); - if (slash == std::string::npos) { - SendDefaultResponse(callback); - return; - } - - std::string scale_str = path.substr(scale_delimiter + 1, - slash - scale_delimiter - 1); - webui::ParseScaleFactor(scale_str, &scale_factor); - - // Return the default favicon (as opposed to a resized favicon) for - // favicon sizes which are not cached by the favicon service. - // Currently the favicon service caches: - // - favicons of sizes "16 * scale factor" px of type FAVICON - // where scale factor is one of FaviconUtil::GetFaviconScaleFactors(). - // - the largest TOUCH_ICON / TOUCH_PRECOMPOSED_ICON - if (size_in_dip != 16 && icon_types_ == history::FAVICON) { - SendDefaultResponse(callback); - return; - } - - parsed_index = slash + 1; + if (!success) { + SendDefaultResponse(callback); + return; } - if (HasSubstringAt(path, parsed_index, kIconURLParameter)) { - parsed_index += strlen(kIconURLParameter); + if (is_icon_url) { // TODO(michaelbai): Change GetRawFavicon to support combination of // IconType. favicon_service->GetRawFavicon( - GURL(path.substr(parsed_index)), + url, history::FAVICON, size_in_dip, scale_factor, base::Bind(&FaviconSource::OnFaviconDataAvailable, base::Unretained(this), IconRequest(callback, - path.substr(parsed_index), + url, size_in_dip, scale_factor)), &cancelable_task_tracker_); } else { - std::string url; - - // URL requests prefixed with "origin/" are converted to a form with an - // empty path and a valid scheme. (e.g., example.com --> - // http://example.com/ or http://example.com/a --> http://example.com/) - if (HasSubstringAt(path, parsed_index, kOriginParameter)) { - parsed_index += strlen(kOriginParameter); - url = path.substr(parsed_index); - - // If the URL does not specify a scheme (e.g., example.com instead of - // http://example.com), add "http://" as a default. - if (!GURL(url).has_scheme()) - url = "http://" + url; - - // Strip the path beyond the top-level domain. - url = GURL(url).GetOrigin().spec(); - } else { - url = path.substr(parsed_index); - } - // Intercept requests for prepopulated pages. for (size_t i = 0; i < arraysize(history::kPrepopulatedPages); i++) { - if (url == + if (url.spec() == l10n_util::GetStringUTF8(history::kPrepopulatedPages[i].url_id)) { callback.Run( ResourceBundle::GetSharedInstance().LoadDataResourceBytesForScale( @@ -203,7 +135,7 @@ void FaviconSource::StartDataRequest( favicon_service->GetRawFaviconForURL( FaviconService::FaviconForURLParams( - profile_, GURL(url), icon_types_, size_in_dip), + profile_, url, icon_types_, size_in_dip), scale_factor, base::Bind(&FaviconSource::OnFaviconDataAvailable, base::Unretained(this), @@ -241,6 +173,99 @@ bool FaviconSource::HandleMissingResource(const IconRequest& request) { return false; } +bool FaviconSource::ParsePath(const std::string& raw_path, + bool* is_icon_url, + GURL* url, + int* size_in_dip, + ui::ScaleFactor* scale_factor) const { + DCHECK_EQ(16, gfx::kFaviconSize); + + *is_icon_url = false; + *url = GURL(); + *size_in_dip = 16; + *scale_factor = ui::SCALE_FACTOR_100P; + + if (raw_path.empty()) + return false; + + // Translate to regular path if |raw_path| is of the form + // chrome-search://favicon/, where + // "most_visited_item_id" is a uint64. + std::string path = InstantService::MaybeTranslateInstantPathOnUI(profile_, + raw_path); + size_t parsed_index = 0; + if (HasSubstringAt(path, parsed_index, kLargestParameter)) { + parsed_index += strlen(kLargestParameter); + *size_in_dip = 0; + } else if (HasSubstringAt(path, parsed_index, kSizeParameter)) { + parsed_index += strlen(kSizeParameter); + + size_t slash = path.find("/", parsed_index); + if (slash == std::string::npos) + return false; + + size_t scale_delimiter = path.find("@", parsed_index); + std::string size_str; + std::string scale_str; + if (scale_delimiter == std::string::npos) { + // Support the legacy size format of 'size/aa/' where 'aa' is the desired + // size in DIP for the sake of not regressing the extensions which use it. + size_str = path.substr(parsed_index, slash - parsed_index); + } else { + size_str = path.substr(parsed_index, scale_delimiter - parsed_index); + scale_str = path.substr(scale_delimiter + 1, + slash - scale_delimiter - 1); + } + + if (!base::StringToInt(size_str, size_in_dip)) + return false; + + if (*size_in_dip != 64 && *size_in_dip != 32) { + // Only 64x64, 32x32 and 16x16 icons are supported. + *size_in_dip = 16; + } + + if (!scale_str.empty()) + webui::ParseScaleFactor(scale_str, scale_factor); + + // Return the default favicon (as opposed to a resized favicon) for + // favicon sizes which are not cached by the favicon service. + // Currently the favicon service caches: + // - favicons of sizes "16 * scale factor" px of type FAVICON + // where scale factor is one of FaviconUtil::GetFaviconScaleFactors(). + // - the largest TOUCH_ICON / TOUCH_PRECOMPOSED_ICON + if (*size_in_dip != 16 && icon_types_ == history::FAVICON) + return false; + + parsed_index = slash + 1; + } + + if (HasSubstringAt(path, parsed_index, kIconURLParameter)) { + parsed_index += strlen(kIconURLParameter); + *is_icon_url = true; + *url = GURL(path.substr(parsed_index)); + } else { + // URL requests prefixed with "origin/" are converted to a form with an + // empty path and a valid scheme. (e.g., example.com --> + // http://example.com/ or http://example.com/a --> http://example.com/) + if (HasSubstringAt(path, parsed_index, kOriginParameter)) { + parsed_index += strlen(kOriginParameter); + std::string possibly_invalid_url = path.substr(parsed_index); + + // If the URL does not specify a scheme (e.g., example.com instead of + // http://example.com), add "http://" as a default. + if (!GURL(possibly_invalid_url).has_scheme()) + possibly_invalid_url = "http://" + possibly_invalid_url; + + // Strip the path beyond the top-level domain. + *url = GURL(possibly_invalid_url).GetOrigin(); + } else { + *url = GURL(path.substr(parsed_index)); + } + } + return true; +} + void FaviconSource::OnFaviconDataAvailable( const IconRequest& request, const history::FaviconBitmapResult& bitmap_result) { @@ -255,7 +280,7 @@ void FaviconSource::OnFaviconDataAvailable( void FaviconSource::SendDefaultResponse( const content::URLDataSource::GotDataCallback& callback) { SendDefaultResponse( - IconRequest(callback, std::string(), 16, ui::SCALE_FACTOR_100P)); + IconRequest(callback, GURL(), 16, ui::SCALE_FACTOR_100P)); } void FaviconSource::SendDefaultResponse(const IconRequest& icon_request) { diff --git a/chrome/browser/ui/webui/favicon_source.h b/chrome/browser/ui/webui/favicon_source.h index 32c048e883c8..8921594143b2 100644 --- a/chrome/browser/ui/webui/favicon_source.h +++ b/chrome/browser/ui/webui/favicon_source.h @@ -68,6 +68,8 @@ class FaviconSource : public content::URLDataSource { // |type| is the type of icon this FaviconSource will provide. FaviconSource(Profile* profile, IconType type); + virtual ~FaviconSource(); + // content::URLDataSource implementation. virtual std::string GetSource() const OVERRIDE; virtual void StartDataRequest( @@ -84,19 +86,17 @@ class FaviconSource : public content::URLDataSource { struct IconRequest { IconRequest(); IconRequest(const content::URLDataSource::GotDataCallback& cb, - const std::string& path, + const GURL& path, int size, ui::ScaleFactor scale); ~IconRequest(); content::URLDataSource::GotDataCallback callback; - std::string request_path; + GURL request_path; int size_in_dip; ui::ScaleFactor scale_factor; }; - virtual ~FaviconSource(); - // Called when the favicon data is missing to perform additional checks to // locate the resource. // |request| contains information for the failed request. @@ -106,6 +106,9 @@ class FaviconSource : public content::URLDataSource { Profile* profile_; private: + FRIEND_TEST_ALL_PREFIXES(FaviconSourceTest, InstantParsing); + FRIEND_TEST_ALL_PREFIXES(FaviconSourceTest, Parsing); + // Defines the allowed pixel sizes for requested favicons. enum IconSize { SIZE_16, @@ -114,6 +117,14 @@ class FaviconSource : public content::URLDataSource { NUM_SIZES }; + // Parses |raw_path|, which should be in the format described at the top of + // the file. Returns true if |raw_path| could be parsed. + bool ParsePath(const std::string& raw_path, + bool* is_icon_url, + GURL* url, + int* size_in_dip, + ui::ScaleFactor* scale_factor) const; + // Called when favicon data is available from the history backend. void OnFaviconDataAvailable( const IconRequest& request, diff --git a/chrome/browser/ui/webui/favicon_source_unittest.cc b/chrome/browser/ui/webui/favicon_source_unittest.cc new file mode 100644 index 000000000000..e47861ea7de9 --- /dev/null +++ b/chrome/browser/ui/webui/favicon_source_unittest.cc @@ -0,0 +1,221 @@ +// Copyright (c) 2013 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 "chrome/browser/ui/webui/favicon_source.h" + +#include "base/message_loop.h" +#include "base/strings/string_number_conversions.h" +#include "chrome/browser/search/instant_service.h" +#include "chrome/browser/search/instant_service_factory.h" +#include "chrome/test/base/testing_profile.h" +#include "content/public/test/test_browser_thread.h" +#include "testing/gtest/include/gtest/gtest.h" +#include "ui/base/layout.h" + +class FaviconSourceTest : public testing::Test { + public: + FaviconSourceTest() + : loop_(MessageLoop::TYPE_UI), + ui_thread_(content::BrowserThread::UI, MessageLoop::current()), + io_thread_(content::BrowserThread::IO, MessageLoop::current()), + profile_(new TestingProfile()), + favicon_source_( + new FaviconSource(profile_.get(), FaviconSource::ANY)) { + + // Set the supported scale factors because the supported scale factors + // affect the result of ParsePathAndScale(). + std::vector supported_scale_factors; + supported_scale_factors.push_back(ui::SCALE_FACTOR_100P); + supported_scale_factors.push_back(ui::SCALE_FACTOR_140P); + scoped_set_supported_scale_factors_.reset( + new ui::test::ScopedSetSupportedScaleFactors(supported_scale_factors)); + } + + virtual ~FaviconSourceTest() { + } + + // Adds a most visited item with |url| and an arbitrary title to the instant + // service and returns the assigned id. + int AddInstantMostVisitedUrlAndGetId(GURL url) { + InstantMostVisitedItem item; + item.url = GURL(url); + std::vector items(1, item); + + InstantService* instant_service = + InstantServiceFactory::GetForProfile(profile_.get()); + instant_service->AddMostVisitedItems(items); + + std::vector items_with_ids; + instant_service->GetCurrentMostVisitedItems(&items_with_ids); + CHECK_EQ(1u, items_with_ids.size()); + return items_with_ids[0].first; + } + + FaviconSource* favicon_source() const { return favicon_source_.get(); } + + private: + MessageLoop loop_; + content::TestBrowserThread ui_thread_; + content::TestBrowserThread io_thread_; + scoped_ptr profile_; + + typedef scoped_ptr + ScopedSetSupportedScaleFactors; + ScopedSetSupportedScaleFactors scoped_set_supported_scale_factors_; + + scoped_ptr favicon_source_; + + DISALLOW_COPY_AND_ASSIGN(FaviconSourceTest); +}; + +// Test parsing the chrome-search://favicon/ URLs +TEST_F(FaviconSourceTest, InstantParsing) { + GURL kUrl1("http://www.google.com/"); + GURL kUrl2("http://maps.google.com/"); + int instant_id1 = AddInstantMostVisitedUrlAndGetId(kUrl1); + int instant_id2 = AddInstantMostVisitedUrlAndGetId(kUrl2); + + bool is_icon_url; + GURL url; + int size_in_dip; + ui::ScaleFactor scale_factor; + + const std::string path1 = base::IntToString(instant_id1); + EXPECT_TRUE(favicon_source()->ParsePath(path1, &is_icon_url, &url, + &size_in_dip, &scale_factor)); + EXPECT_FALSE(is_icon_url); + EXPECT_EQ(kUrl1, url); + EXPECT_EQ(16, size_in_dip); + EXPECT_EQ(ui::SCALE_FACTOR_100P, scale_factor); + + const std::string path2 = base::IntToString(instant_id2); + EXPECT_TRUE(favicon_source()->ParsePath(path2, &is_icon_url, &url, + &size_in_dip, &scale_factor)); + EXPECT_FALSE(is_icon_url); + EXPECT_EQ(kUrl2, url); + EXPECT_EQ(16, size_in_dip); + EXPECT_EQ(ui::SCALE_FACTOR_100P, scale_factor); +} + +// Test parsing the chrome://favicon URLs +TEST_F(FaviconSourceTest, Parsing) { + const GURL kUrl("https://www.google.ca/imghp?hl=en&tab=wi"); + + bool is_icon_url; + GURL url; + int size_in_dip; + ui::ScaleFactor scale_factor; + + // 1) Test parsing path with no extra parameters. + const std::string path1 = kUrl.spec(); + EXPECT_TRUE(favicon_source()->ParsePath(path1, &is_icon_url, &url, + &size_in_dip, &scale_factor)); + EXPECT_FALSE(is_icon_url); + EXPECT_EQ(kUrl, url); + EXPECT_EQ(16, size_in_dip); + EXPECT_EQ(ui::SCALE_FACTOR_100P, scale_factor); + + // 2) Test parsing path with a 'size' parameter. + // + // Test that we can still parse the legacy 'size' parameter format. + const std::string path2 = "size/32/" + kUrl.spec(); + EXPECT_TRUE(favicon_source()->ParsePath(path2, &is_icon_url, &url, + &size_in_dip, &scale_factor)); + EXPECT_FALSE(is_icon_url); + EXPECT_EQ(kUrl, url); + EXPECT_EQ(32, size_in_dip); + EXPECT_EQ(ui::SCALE_FACTOR_100P, scale_factor); + + // Test parsing current 'size' parameter format. + const std::string path3 = "size/32@1.4x/" + kUrl.spec(); + EXPECT_TRUE(favicon_source()->ParsePath(path3, &is_icon_url, &url, + &size_in_dip, &scale_factor)); + EXPECT_FALSE(is_icon_url); + EXPECT_EQ(kUrl, url); + EXPECT_EQ(32, size_in_dip); + EXPECT_EQ(ui::SCALE_FACTOR_140P, scale_factor); + + // Test that we pick the ui::ScaleFactor which is closest to the passed in + // scale factor. + const std::string path4 = "size/16@1.41x/" + kUrl.spec(); + EXPECT_TRUE(favicon_source()->ParsePath(path4, &is_icon_url, &url, + &size_in_dip, &scale_factor)); + EXPECT_FALSE(is_icon_url); + EXPECT_EQ(kUrl, url); + EXPECT_EQ(16, size_in_dip); + EXPECT_EQ(ui::SCALE_FACTOR_140P, scale_factor); + + // Invalid cases. + const std::string path5 = "size/" + kUrl.spec(); + EXPECT_FALSE(favicon_source()->ParsePath(path5, &is_icon_url, &url, + &size_in_dip, &scale_factor)); + const std::string path6 = "size/@1x/" + kUrl.spec(); + EXPECT_FALSE(favicon_source()->ParsePath(path6, &is_icon_url, &url, + &size_in_dip, &scale_factor)); + const std::string path7 = "size/abc@1x/" + kUrl.spec(); + EXPECT_FALSE(favicon_source()->ParsePath(path7, &is_icon_url, &url, + &size_in_dip, &scale_factor)); + + // Part of url looks like 'size' parameter. + const std::string path8 = "http://www.google.com/size/32@1.4x"; + EXPECT_TRUE(favicon_source()->ParsePath(path8, &is_icon_url, &url, + &size_in_dip, &scale_factor)); + EXPECT_FALSE(is_icon_url); + EXPECT_EQ(path8, url.spec()); + EXPECT_EQ(16, size_in_dip); + EXPECT_EQ(ui::SCALE_FACTOR_100P, scale_factor); + + // 3) Test parsing path with the 'largest' parameter. + const std::string path9 = "largest/" + kUrl.spec(); + EXPECT_TRUE(favicon_source()->ParsePath(path9, &is_icon_url, &url, + &size_in_dip, &scale_factor)); + EXPECT_FALSE(is_icon_url); + EXPECT_EQ(kUrl, url); + EXPECT_EQ(0, size_in_dip); + // The scale factor is meaningless when requesting the largest favicon. + + // 4) Test parsing path with 'iconurl' parameter. + const std::string path10 = "iconurl/http://www.google.com/favicon.ico"; + EXPECT_TRUE(favicon_source()->ParsePath(path10, &is_icon_url, &url, + &size_in_dip, &scale_factor)); + EXPECT_TRUE(is_icon_url); + EXPECT_EQ("http://www.google.com/favicon.ico", url.spec()); + EXPECT_EQ(16, size_in_dip); + EXPECT_EQ(ui::SCALE_FACTOR_100P, scale_factor); + + // 5) Test parsing path with 'origin' parameter. + const std::string path11 = "origin/" + kUrl.spec(); + EXPECT_TRUE(favicon_source()->ParsePath(path11, &is_icon_url, &url, + &size_in_dip, &scale_factor)); + EXPECT_FALSE(is_icon_url); + EXPECT_EQ("https://www.google.ca/", url.spec()); + EXPECT_EQ(16, size_in_dip); + EXPECT_EQ(ui::SCALE_FACTOR_100P, scale_factor); + + const std::string path12 = "origin/google.com"; + EXPECT_TRUE(favicon_source()->ParsePath(path12, &is_icon_url, &url, + &size_in_dip, &scale_factor)); + EXPECT_FALSE(is_icon_url); + EXPECT_EQ("http://google.com/", url.spec()); + EXPECT_EQ(16, size_in_dip); + EXPECT_EQ(ui::SCALE_FACTOR_100P, scale_factor); + + // 6) Test parsing paths with both a 'size' parameter and a 'url modifier' + // parameter. + const std::string path13 = "size/32@1.4x/origin/" + kUrl.spec(); + EXPECT_TRUE(favicon_source()->ParsePath(path13, &is_icon_url, &url, + &size_in_dip, &scale_factor)); + EXPECT_FALSE(is_icon_url); + EXPECT_EQ("https://www.google.ca/", url.spec()); + EXPECT_EQ(32, size_in_dip); + EXPECT_EQ(ui::SCALE_FACTOR_140P, scale_factor); + + const std::string path14 = + "largest/iconurl/http://www.google.com/favicon.ico"; + EXPECT_TRUE(favicon_source()->ParsePath(path14, &is_icon_url, &url, + &size_in_dip, &scale_factor)); + EXPECT_TRUE(is_icon_url); + EXPECT_EQ("http://www.google.com/favicon.ico", url.spec()); + EXPECT_EQ(0, size_in_dip); +} diff --git a/chrome/browser/ui/webui/session_favicon_source.cc b/chrome/browser/ui/webui/session_favicon_source.cc index 273b2a18a712..6891cd05ae37 100644 --- a/chrome/browser/ui/webui/session_favicon_source.cc +++ b/chrome/browser/ui/webui/session_favicon_source.cc @@ -46,7 +46,7 @@ bool SessionFaviconSource::HandleMissingResource(const IconRequest& request) { scoped_refptr response; if (associator && - associator->GetSyncedFaviconForPageURL(request.request_path, + associator->GetSyncedFaviconForPageURL(request.request_path.spec(), &response)) { request.callback.Run(response); return true; diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi index 0b9d15a66633..fc54c81f80d8 100644 --- a/chrome/chrome_tests_unit.gypi +++ b/chrome/chrome_tests_unit.gypi @@ -1504,6 +1504,7 @@ 'browser/ui/views/tabs/tab_strip_unittest.cc', 'browser/ui/web_contents_modal_dialog_manager_unittest.cc', 'browser/ui/website_settings/website_settings_unittest.cc', + 'browser/ui/webui/favicon_source_unittest.cc', 'browser/ui/webui/fileicon_source_unittest.cc', 'browser/ui/webui/history_ui_unittest.cc', 'browser/ui/webui/ntp/android/partner_bookmarks_shim_unittest.cc',