diff options
| author | Allan Sandfeld Jensen <allan.jensen@qt.io> | 2017-11-20 15:06:40 +0100 |
|---|---|---|
| committer | Allan Sandfeld Jensen <allan.jensen@qt.io> | 2017-11-22 11:48:58 +0000 |
| commit | daa093eea7c773db06799a13bd7e4e2e2a9f8f14 (patch) | |
| tree | 96cc5e7b9194c1b29eab927730bfa419e7111c25 /chromium/components/history | |
| parent | be59a35641616a4cf23c4a13fa0632624b021c1b (diff) | |
| download | qtwebengine-chromium-daa093eea7c773db06799a13bd7e4e2e2a9f8f14.tar.gz | |
BASELINE: Update Chromium to 63.0.3239.58
Change-Id: Ia93b322a00ba4dd4004f3bcf1254063ba90e1605
Reviewed-by: Alexandru Croitor <alexandru.croitor@qt.io>
Diffstat (limited to 'chromium/components/history')
37 files changed, 1064 insertions, 636 deletions
diff --git a/chromium/components/history/content/browser/web_contents_top_sites_observer.cc b/chromium/components/history/content/browser/web_contents_top_sites_observer.cc index 2e7f10bc91c..50546ecf828 100644 --- a/chromium/components/history/content/browser/web_contents_top_sites_observer.cc +++ b/chromium/components/history/content/browser/web_contents_top_sites_observer.cc @@ -38,8 +38,16 @@ WebContentsTopSitesObserver::~WebContentsTopSitesObserver() { void WebContentsTopSitesObserver::NavigationEntryCommitted( const content::LoadCommittedDetails& load_details) { DCHECK(load_details.entry); - if (top_sites_) + + // Frame-wise, we only care about navigating the main frame. + // Type-wise, we only care about navigating to a new page, or renavigating to + // an existing navigation entry. + if (top_sites_ && load_details.is_main_frame && + (load_details.type == content::NavigationType::NAVIGATION_TYPE_NEW_PAGE || + load_details.type == + content::NavigationType::NAVIGATION_TYPE_EXISTING_PAGE)) { top_sites_->OnNavigationCommitted(load_details.entry->GetURL()); + } } } // namespace history diff --git a/chromium/components/history/core/browser/BUILD.gn b/chromium/components/history/core/browser/BUILD.gn index f9c88c8c106..3e4bfbc7fb3 100644 --- a/chromium/components/history/core/browser/BUILD.gn +++ b/chromium/components/history/core/browser/BUILD.gn @@ -67,6 +67,8 @@ static_library("browser") { "top_sites_observer.h", "typed_url_data_type_controller.cc", "typed_url_data_type_controller.h", + "typed_url_model_type_controller.cc", + "typed_url_model_type_controller.h", "typed_url_sync_bridge.cc", "typed_url_sync_bridge.h", "typed_url_sync_metadata_database.cc", diff --git a/chromium/components/history/core/browser/expire_history_backend.cc b/chromium/components/history/core/browser/expire_history_backend.cc index 2e69c9286c2..b19fef9e160 100644 --- a/chromium/components/history/core/browser/expire_history_backend.cc +++ b/chromium/components/history/core/browser/expire_history_backend.cc @@ -135,6 +135,8 @@ namespace internal { const base::Feature kClearOldOnDemandFavicons{ "ClearOldOnDemandFavicons", base::FEATURE_DISABLED_BY_DEFAULT}; +const int kOnDemandFaviconIsOldAfterDays = 30; + } // namespace internal // ExpireHistoryBackend::DeleteEffects ---------------------------------------- @@ -499,7 +501,9 @@ void ExpireHistoryBackend::DoExpireIteration() { work_queue_.push(reader); } else { // Otherwise do a final clean-up - remove old favicons not bound to visits. - ClearOldOnDemandFavicons(GetCurrentExpirationTime()); + ClearOldOnDemandFavicons( + base::Time::Now() - + base::TimeDelta::FromDays(internal::kOnDemandFaviconIsOldAfterDays)); } ScheduleExpire(); diff --git a/chromium/components/history/core/browser/expire_history_backend.h b/chromium/components/history/core/browser/expire_history_backend.h index f47666e4dd6..d571e8cfdec 100644 --- a/chromium/components/history/core/browser/expire_history_backend.h +++ b/chromium/components/history/core/browser/expire_history_backend.h @@ -6,10 +6,10 @@ #define COMPONENTS_HISTORY_CORE_BROWSER_EXPIRE_HISTORY_BACKEND_H_ #include <memory> -#include <queue> #include <set> #include <vector> +#include "base/containers/queue.h" #include "base/gtest_prod_util.h" #include "base/macros.h" #include "base/memory/weak_ptr.h" @@ -46,6 +46,9 @@ typedef std::vector<const ExpiringVisitsReader*> ExpiringVisitsReaders; namespace internal { // Feature that enables clearing old on-demand favicons. extern const base::Feature kClearOldOnDemandFavicons; + +// The minimum number of days since last use for an icon to be considered old. +extern const int kOnDemandFaviconIsOldAfterDays; } // namespace internal // Helper component to HistoryBackend that manages expiration and deleting of @@ -269,7 +272,7 @@ class ExpireHistoryBackend { // Work queue for periodic expiration tasks, used by DoExpireIteration() to // determine what to do at an iteration, as well as populate it for future // iterations. - std::queue<const ExpiringVisitsReader*> work_queue_; + base::queue<const ExpiringVisitsReader*> work_queue_; // Readers for various types of visits. // TODO(dglazkov): If you are adding another one, please consider reorganizing diff --git a/chromium/components/history/core/browser/expire_history_backend_unittest.cc b/chromium/components/history/core/browser/expire_history_backend_unittest.cc index be6bc13ac33..cdc1a82ec4d 100644 --- a/chromium/components/history/core/browser/expire_history_backend_unittest.cc +++ b/chromium/components/history/core/browser/expire_history_backend_unittest.cc @@ -49,6 +49,12 @@ namespace { bool MockCanAddURLToHistory(const GURL& url) { return url.is_valid(); } + +base::Time GetOldFaviconThreshold() { + return base::Time::Now() - + base::TimeDelta::FromDays(internal::kOnDemandFaviconIsOldAfterDays); +} + } // namespace // ExpireHistoryTest ----------------------------------------------------------- @@ -908,13 +914,12 @@ TEST_F(ExpireHistoryTest, ClearOldOnDemandFaviconsDoesDeleteUnstarred) { GURL url("http://google.com/favicon.ico"); favicon_base::FaviconID icon_id = thumb_db_->AddFavicon( url, favicon_base::FAVICON, favicon, FaviconBitmapType::ON_DEMAND, - base::Time::Now() - base::TimeDelta::FromDays(100), gfx::Size()); + GetOldFaviconThreshold() - base::TimeDelta::FromSeconds(1), gfx::Size()); ASSERT_NE(0, icon_id); GURL page_url("http://google.com/"); ASSERT_NE(0, thumb_db_->AddIconMapping(page_url, icon_id)); - expirer_.ClearOldOnDemandFavicons(base::Time::Now() - - base::TimeDelta::FromDays(90)); + expirer_.ClearOldOnDemandFavicons(GetOldFaviconThreshold()); // The icon gets deleted. EXPECT_FALSE(thumb_db_->GetIconMappingsForPageURL(page_url, nullptr)); @@ -937,7 +942,7 @@ TEST_F(ExpireHistoryTest, ClearOldOnDemandFaviconsDoesNotDeleteStarred) { GURL url("http://google.com/favicon.ico"); favicon_base::FaviconID icon_id = thumb_db_->AddFavicon( url, favicon_base::FAVICON, favicon, FaviconBitmapType::ON_DEMAND, - base::Time::Now() - base::TimeDelta::FromDays(100), gfx::Size()); + GetOldFaviconThreshold() - base::TimeDelta::FromSeconds(1), gfx::Size()); ASSERT_NE(0, icon_id); GURL page_url1("http://google.com/1"); ASSERT_NE(0, thumb_db_->AddIconMapping(page_url1, icon_id)); @@ -945,8 +950,7 @@ TEST_F(ExpireHistoryTest, ClearOldOnDemandFaviconsDoesNotDeleteStarred) { GURL page_url2("http://google.com/2"); ASSERT_NE(0, thumb_db_->AddIconMapping(page_url2, icon_id)); - expirer_.ClearOldOnDemandFavicons(base::Time::Now() - - base::TimeDelta::FromDays(90)); + expirer_.ClearOldOnDemandFavicons(GetOldFaviconThreshold()); // Nothing gets deleted. EXPECT_TRUE(thumb_db_->GetFaviconHeader(icon_id, nullptr, nullptr)); @@ -968,8 +972,8 @@ TEST_F(ExpireHistoryTest, ClearOldOnDemandFaviconsDoesDeleteAfterLongDelay) { feature_list.InitAndEnableFeature(internal::kClearOldOnDemandFavicons); // Previous clearing (2 days ago). - expirer_.ClearOldOnDemandFavicons(base::Time::Now() - - base::TimeDelta::FromDays(92)); + expirer_.ClearOldOnDemandFavicons(GetOldFaviconThreshold() - + base::TimeDelta::FromDays(2)); // The blob does not encode any real bitmap, obviously. const unsigned char kBlob[] = "0"; @@ -980,13 +984,12 @@ TEST_F(ExpireHistoryTest, ClearOldOnDemandFaviconsDoesDeleteAfterLongDelay) { GURL url("http://google.com/favicon.ico"); favicon_base::FaviconID icon_id = thumb_db_->AddFavicon( url, favicon_base::FAVICON, favicon, FaviconBitmapType::ON_DEMAND, - base::Time::Now() - base::TimeDelta::FromDays(100), gfx::Size()); + GetOldFaviconThreshold() - base::TimeDelta::FromSeconds(1), gfx::Size()); ASSERT_NE(0, icon_id); GURL page_url("http://google.com/"); ASSERT_NE(0, thumb_db_->AddIconMapping(page_url, icon_id)); - expirer_.ClearOldOnDemandFavicons(base::Time::Now() - - base::TimeDelta::FromDays(90)); + expirer_.ClearOldOnDemandFavicons(GetOldFaviconThreshold()); // The icon gets deleted. EXPECT_FALSE(thumb_db_->GetIconMappingsForPageURL(page_url, nullptr)); @@ -1002,8 +1005,7 @@ TEST_F(ExpireHistoryTest, feature_list.InitAndEnableFeature(internal::kClearOldOnDemandFavicons); // Previous clearing (5 minutes ago). - expirer_.ClearOldOnDemandFavicons(base::Time::Now() - - base::TimeDelta::FromDays(90) - + expirer_.ClearOldOnDemandFavicons(GetOldFaviconThreshold() - base::TimeDelta::FromMinutes(5)); // The blob does not encode any real bitmap, obviously. @@ -1015,15 +1017,14 @@ TEST_F(ExpireHistoryTest, GURL url("http://google.com/favicon.ico"); favicon_base::FaviconID icon_id = thumb_db_->AddFavicon( url, favicon_base::FAVICON, favicon, FaviconBitmapType::ON_DEMAND, - base::Time::Now() - base::TimeDelta::FromDays(100), gfx::Size()); + GetOldFaviconThreshold() - base::TimeDelta::FromSeconds(1), gfx::Size()); ASSERT_NE(0, icon_id); GURL page_url1("http://google.com/1"); ASSERT_NE(0, thumb_db_->AddIconMapping(page_url1, icon_id)); GURL page_url2("http://google.com/2"); ASSERT_NE(0, thumb_db_->AddIconMapping(page_url2, icon_id)); - expirer_.ClearOldOnDemandFavicons(base::Time::Now() - - base::TimeDelta::FromDays(90)); + expirer_.ClearOldOnDemandFavicons(GetOldFaviconThreshold()); // Nothing gets deleted. EXPECT_TRUE(thumb_db_->GetFaviconHeader(icon_id, nullptr, nullptr)); diff --git a/chromium/components/history/core/browser/history_backend.cc b/chromium/components/history/core/browser/history_backend.cc index dec7e10fc08..6002fa2c56a 100644 --- a/chromium/components/history/core/browser/history_backend.cc +++ b/chromium/components/history/core/browser/history_backend.cc @@ -6,6 +6,7 @@ #include <algorithm> #include <functional> +#include <limits> #include <list> #include <map> #include <memory> @@ -15,8 +16,6 @@ #include "base/bind.h" #include "base/compiler_specific.h" -#include "base/debug/dump_without_crashing.h" -#include "base/feature_list.h" #include "base/files/file_enumerator.h" #include "base/memory/ptr_util.h" #include "base/metrics/histogram_macros.h" @@ -55,7 +54,6 @@ #include "base/ios/scoped_critical_action.h" #endif -using base::debug::DumpWithoutCrashing; using base::Time; using base::TimeDelta; using base::TimeTicks; @@ -74,6 +72,11 @@ using syncer::ModelTypeChangeProcessor; namespace history { +// TODO(crbug.com/746268): Clean up this toggle after impact is measured via +// Finch, which is expected to be neutral. +const base::Feature kAvoidStrippingRefFromFaviconPageUrls{ + "AvoidStrippingRefFromFaviconPageUrls", base::FEATURE_DISABLED_BY_DEFAULT}; + namespace { void RunUnlessCanceled( @@ -219,10 +222,8 @@ void HistoryBackend::Init( if (base::FeatureList::IsEnabled(switches::kSyncUSSTypedURL)) { typed_url_sync_bridge_ = base::MakeUnique<TypedURLSyncBridge>( this, db_.get(), - base::BindRepeating( - &ModelTypeChangeProcessor::Create, - // TODO(gangwu): use ReportUnrecoverableError before launch. - base::BindRepeating(base::IgnoreResult(&DumpWithoutCrashing)))); + base::BindRepeating(&ModelTypeChangeProcessor::Create, + base::RepeatingClosure())); typed_url_sync_bridge_->Init(); } else { typed_url_syncable_service_ = @@ -976,9 +977,20 @@ void HistoryBackend::AddPageNoVisitForBookmark(const GURL& url, } bool HistoryBackend::GetAllTypedURLs(URLRows* urls) { - if (db_) - return db_->GetAllTypedUrls(urls); - return false; + DCHECK(urls); + if (!db_) + return false; + std::vector<URLID> url_ids; + if (!db_->GetAllURLIDsForTransition(ui::PAGE_TRANSITION_TYPED, &url_ids)) + return false; + urls->reserve(url_ids.size()); + for (const auto& url_id : url_ids) { + URLRow url; + if (!db_->GetURLRow(url_id, &url)) + return false; + urls->push_back(url); + } + return true; } bool HistoryBackend::GetVisitsForURL(URLID id, VisitVector* visits) { @@ -1325,7 +1337,7 @@ void HistoryBackend::QueryHistoryText(const base::string16& text_query, } result->SetURLResults(std::move(matching_visits)); - if(!has_more_results && options.begin_time <= first_recorded_time_) + if (!has_more_results && options.begin_time <= first_recorded_time_) result->set_reached_beginning(true); } @@ -1372,6 +1384,8 @@ void HistoryBackend::QueryMostVisitedURLs(int result_count, if (!db_) return; + base::TimeTicks begin_time = base::TimeTicks::Now(); + auto url_filter = backend_client_ ? base::Bind(&HistoryBackendClient::IsWebSafe, base::Unretained(backend_client_.get())) @@ -1386,6 +1400,9 @@ void HistoryBackend::QueryMostVisitedURLs(int result_count, MostVisitedURL url = MakeMostVisitedURL(*current_data, redirects); result->push_back(url); } + + UMA_HISTOGRAM_TIMES("History.QueryMostVisitedURLsTime", + base::TimeTicks::Now() - begin_time); } void HistoryBackend::GetRedirectsFromSpecificVisit(VisitID cur_visit, @@ -1453,7 +1470,7 @@ void HistoryBackend::GetFavicon( favicon_base::IconType icon_type, const std::vector<int>& desired_sizes, std::vector<favicon_base::FaviconRawBitmapResult>* bitmap_results) { - UpdateFaviconMappingsAndFetchImpl(std::set<GURL>(), icon_url, icon_type, + UpdateFaviconMappingsAndFetchImpl(base::flat_set<GURL>(), icon_url, icon_type, desired_sizes, bitmap_results); } @@ -1533,18 +1550,19 @@ void HistoryBackend::GetLargestFaviconForURL( } base::Time last_updated; + base::Time last_requested; favicon_base::FaviconRawBitmapResult bitmap_result; bitmap_result.icon_url = icon_url; bitmap_result.icon_type = icon_type; - if (!thumbnail_db_->GetFaviconBitmap(largest_icon.bitmap_id, - &last_updated, nullptr, - &bitmap_result.bitmap_data, - &bitmap_result.pixel_size)) { + if (!thumbnail_db_->GetFaviconBitmap( + largest_icon.bitmap_id, &last_updated, &last_requested, + &bitmap_result.bitmap_data, &bitmap_result.pixel_size)) { return; } bitmap_result.expired = (Time::Now() - last_updated) > TimeDelta::FromDays(kFaviconRefetchDays); + bitmap_result.fetched_because_of_page_visit = last_requested.is_null(); if (bitmap_result.is_valid()) *favicon_bitmap_result = bitmap_result; @@ -1585,7 +1603,7 @@ void HistoryBackend::GetFaviconForID( } void HistoryBackend::UpdateFaviconMappingsAndFetch( - const std::set<GURL>& page_urls, + const base::flat_set<GURL>& page_urls, const GURL& icon_url, favicon_base::IconType icon_type, const std::vector<int>& desired_sizes, @@ -1754,11 +1772,11 @@ void HistoryBackend::MergeFavicon( ScheduleCommit(); } -void HistoryBackend::SetFavicons(const GURL& page_url, +void HistoryBackend::SetFavicons(const base::flat_set<GURL>& page_urls, favicon_base::IconType icon_type, const GURL& icon_url, const std::vector<SkBitmap>& bitmaps) { - SetFaviconsImpl(page_url, icon_type, icon_url, bitmaps, + SetFaviconsImpl(page_urls, icon_type, icon_url, bitmaps, FaviconBitmapType::ON_VISIT); } @@ -1775,7 +1793,7 @@ bool HistoryBackend::SetOnDemandFavicons(const GURL& page_url, return false; } - return SetFaviconsImpl(page_url, icon_type, icon_url, bitmaps, + return SetFaviconsImpl({page_url}, icon_type, icon_url, bitmaps, FaviconBitmapType::ON_DEMAND); } @@ -1862,11 +1880,13 @@ void HistoryBackend::SetImportedFavicons( } } -bool HistoryBackend::SetFaviconsImpl(const GURL& page_url, +bool HistoryBackend::SetFaviconsImpl(const base::flat_set<GURL>& page_urls, favicon_base::IconType icon_type, const GURL& icon_url, const std::vector<SkBitmap>& bitmaps, FaviconBitmapType type) { + DCHECK(!page_urls.empty()); + if (!thumbnail_db_ || !db_) return false; @@ -1887,12 +1907,14 @@ bool HistoryBackend::SetFaviconsImpl(const GURL& page_url, } std::vector<favicon_base::FaviconID> icon_ids(1u, icon_id); - bool mapping_changed = - SetFaviconMappingsForPageAndRedirects(page_url, icon_type, icon_ids); + for (const GURL& page_url : page_urls) { + bool mapping_changed = + SetFaviconMappingsForPageAndRedirects(page_url, icon_type, icon_ids); - if (mapping_changed) { - // Notify the UI that this function changed an icon mapping. - SendFaviconChangedNotificationForPageAndRedirects(page_url); + if (mapping_changed) { + // Notify the UI that this function changed an icon mapping. + SendFaviconChangedNotificationForPageAndRedirects(page_url); + } } if (favicon_data_modified && !favicon_created) { @@ -1906,7 +1928,7 @@ bool HistoryBackend::SetFaviconsImpl(const GURL& page_url, } void HistoryBackend::UpdateFaviconMappingsAndFetchImpl( - const std::set<GURL>& page_urls, + const base::flat_set<GURL>& page_urls, const GURL& icon_url, favicon_base::IconType icon_type, const std::vector<int>& desired_sizes, @@ -2095,17 +2117,19 @@ bool HistoryBackend::GetFaviconBitmapResultsForBestMatch( for (size_t i = 0; i < best_bitmap_ids.size(); ++i) { base::Time last_updated; + base::Time last_requested; favicon_base::FaviconRawBitmapResult bitmap_result; bitmap_result.icon_url = icon_url; bitmap_result.icon_type = icon_type; - if (!thumbnail_db_->GetFaviconBitmap(best_bitmap_ids[i], &last_updated, - nullptr, &bitmap_result.bitmap_data, - &bitmap_result.pixel_size)) { + if (!thumbnail_db_->GetFaviconBitmap( + best_bitmap_ids[i], &last_updated, &last_requested, + &bitmap_result.bitmap_data, &bitmap_result.pixel_size)) { return false; } bitmap_result.expired = (Time::Now() - last_updated) > TimeDelta::FromDays(kFaviconRefetchDays); + bitmap_result.fetched_because_of_page_visit = last_requested.is_null(); if (bitmap_result.is_valid()) favicon_bitmap_results->push_back(bitmap_result); } @@ -2125,7 +2149,8 @@ bool HistoryBackend::SetFaviconMappingsForPageAndRedirects( GetCachedRecentRedirects(page_url, &redirects); bool mappings_changed = SetFaviconMappingsForPages(redirects, icon_type, icon_ids); - if (page_url.has_ref()) { + if (page_url.has_ref() && + !base::FeatureList::IsEnabled(kAvoidStrippingRefFromFaviconPageUrls)) { // Refs often gets added by Javascript, but the redirect chain is keyed to // the URL without a ref. // TODO(crbug.com/746268): This can cause orphan favicons, i.e. without a diff --git a/chromium/components/history/core/browser/history_backend.h b/chromium/components/history/core/browser/history_backend.h index 1a698364b8c..7dc0043d7c1 100644 --- a/chromium/components/history/core/browser/history_backend.h +++ b/chromium/components/history/core/browser/history_backend.h @@ -15,8 +15,10 @@ #include <vector> #include "base/cancelable_callback.h" +#include "base/containers/flat_set.h" #include "base/containers/hash_tables.h" #include "base/containers/mru_cache.h" +#include "base/feature_list.h" #include "base/files/file_path.h" #include "base/gtest_prod_util.h" #include "base/macros.h" @@ -65,6 +67,8 @@ static const size_t kMaxFaviconsPerPage = 8; // the thumbnail database. static const size_t kMaxFaviconBitmapsPerIconURL = 8; +extern const base::Feature kAvoidStrippingRefFromFaviconPageUrls; + // Keeps track of a queued HistoryDBTask. This class lives solely on the // DB thread. class QueuedHistoryDBTask { @@ -309,7 +313,7 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>, std::vector<favicon_base::FaviconRawBitmapResult>* bitmap_results); void UpdateFaviconMappingsAndFetch( - const std::set<GURL>& page_urls, + const base::flat_set<GURL>& page_urls, const GURL& icon_url, favicon_base::IconType icon_type, const std::vector<int>& desired_sizes, @@ -321,7 +325,8 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>, scoped_refptr<base::RefCountedMemory> bitmap_data, const gfx::Size& pixel_size); - void SetFavicons(const GURL& page_url, + // |page_urls| must not be empty. + void SetFavicons(const base::flat_set<GURL>& page_urls, favicon_base::IconType icon_type, const GURL& icon_url, const std::vector<SkBitmap>& bitmaps); @@ -534,6 +539,7 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>, FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, SetFaviconsReplaceBitmapData); FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, SetFaviconsSameFaviconURLForTwoPages); + FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, SetFaviconsWithTwoPageURLs); FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, SetOnDemandFaviconsForEmptyDB); FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, SetOnDemandFaviconsForPageInDB); FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, SetOnDemandFaviconsForIconInDB); @@ -685,8 +691,8 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>, // If |bitmaps_are_expired| is true, the icon for |icon_url| will be modified // only if it's not present in the database. In that case, it will be // initially set as expired. Returns whether the new bitmaps were actually - // written. - bool SetFaviconsImpl(const GURL& page_url, + // written. |page_urls| must not be empty. + bool SetFaviconsImpl(const base::flat_set<GURL>& page_urls, favicon_base::IconType icon_type, const GURL& icon_url, const std::vector<SkBitmap>& bitmaps, @@ -697,7 +703,7 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>, // added to the database from each element in |page_urls| (and all redirects) // to |icon_url|. void UpdateFaviconMappingsAndFetchImpl( - const std::set<GURL>& page_urls, + const base::flat_set<GURL>& page_urls, const GURL& icon_url, favicon_base::IconType icon_type, const std::vector<int>& desired_sizes, diff --git a/chromium/components/history/core/browser/history_backend_unittest.cc b/chromium/components/history/core/browser/history_backend_unittest.cc index 79b79003ad1..dee14a1639e 100644 --- a/chromium/components/history/core/browser/history_backend_unittest.cc +++ b/chromium/components/history/core/browser/history_backend_unittest.cc @@ -30,6 +30,7 @@ #include "base/strings/string_number_conversions.h" #include "base/strings/utf_string_conversions.h" #include "base/test/histogram_tester.h" +#include "base/test/scoped_feature_list.h" #include "base/test/scoped_task_environment.h" #include "base/threading/thread_task_runner_handle.h" #include "build/build_config.h" @@ -1758,19 +1759,19 @@ TEST_F(HistoryBackendTest, SetFaviconMappingsForPageAndRedirects) { bitmaps.push_back(CreateBitmap(SK_ColorRED, kLargeEdgeSize)); // Add a favicon. - backend_->SetFavicons(url1, favicon_base::FAVICON, icon_url1, bitmaps); + backend_->SetFavicons({url1}, favicon_base::FAVICON, icon_url1, bitmaps); EXPECT_EQ(1u, NumIconMappingsForPageURL(url1, favicon_base::FAVICON)); EXPECT_EQ(1u, NumIconMappingsForPageURL(url2, favicon_base::FAVICON)); // Add one touch_icon - backend_->SetFavicons(url1, favicon_base::TOUCH_ICON, icon_url1, bitmaps); + backend_->SetFavicons({url1}, favicon_base::TOUCH_ICON, icon_url1, bitmaps); EXPECT_EQ(1u, NumIconMappingsForPageURL(url1, favicon_base::TOUCH_ICON)); EXPECT_EQ(1u, NumIconMappingsForPageURL(url2, favicon_base::TOUCH_ICON)); EXPECT_EQ(1u, NumIconMappingsForPageURL(url1, favicon_base::FAVICON)); // Add one TOUCH_PRECOMPOSED_ICON - backend_->SetFavicons( - url1, favicon_base::TOUCH_PRECOMPOSED_ICON, icon_url1, bitmaps); + backend_->SetFavicons({url1}, favicon_base::TOUCH_PRECOMPOSED_ICON, icon_url1, + bitmaps); // The touch_icon was replaced. EXPECT_EQ(0u, NumIconMappingsForPageURL(url1, favicon_base::TOUCH_ICON)); EXPECT_EQ(1u, NumIconMappingsForPageURL(url1, favicon_base::FAVICON)); @@ -1782,7 +1783,7 @@ TEST_F(HistoryBackendTest, SetFaviconMappingsForPageAndRedirects) { NumIconMappingsForPageURL(url2, favicon_base::TOUCH_PRECOMPOSED_ICON)); // Add a touch_icon. - backend_->SetFavicons(url1, favicon_base::TOUCH_ICON, icon_url1, bitmaps); + backend_->SetFavicons({url1}, favicon_base::TOUCH_ICON, icon_url1, bitmaps); EXPECT_EQ(1u, NumIconMappingsForPageURL(url1, favicon_base::TOUCH_ICON)); EXPECT_EQ(1u, NumIconMappingsForPageURL(url1, favicon_base::FAVICON)); // The TOUCH_PRECOMPOSED_ICON was replaced. @@ -1791,7 +1792,7 @@ TEST_F(HistoryBackendTest, SetFaviconMappingsForPageAndRedirects) { NumIconMappingsForPageURL(url1, favicon_base::TOUCH_PRECOMPOSED_ICON)); // Add a web manifest_icon. - backend_->SetFavicons(url1, favicon_base::WEB_MANIFEST_ICON, icon_url2, + backend_->SetFavicons({url1}, favicon_base::WEB_MANIFEST_ICON, icon_url2, bitmaps); EXPECT_EQ(1u, NumIconMappingsForPageURL(url1, favicon_base::WEB_MANIFEST_ICON)); @@ -1804,7 +1805,7 @@ TEST_F(HistoryBackendTest, SetFaviconMappingsForPageAndRedirects) { url1, favicon_base::TOUCH_PRECOMPOSED_ICON)); // Add a different favicon. - backend_->SetFavicons(url1, favicon_base::FAVICON, icon_url2, bitmaps); + backend_->SetFavicons({url1}, favicon_base::FAVICON, icon_url2, bitmaps); EXPECT_EQ(1u, NumIconMappingsForPageURL(url1, favicon_base::WEB_MANIFEST_ICON)); EXPECT_EQ(1u, NumIconMappingsForPageURL(url1, favicon_base::FAVICON)); @@ -1851,7 +1852,7 @@ TEST_F(HistoryBackendTest, SetFaviconMappingsForPageAndRedirectsWithFragment) { redirects.push_back(url2); backend_->recent_redirects_.Put(url2, redirects); - backend_->SetFavicons(url1, favicon_base::FAVICON, icon_url1, bitmaps); + backend_->SetFavicons({url1}, favicon_base::FAVICON, icon_url1, bitmaps); EXPECT_EQ(1u, NumIconMappingsForPageURL(url1, favicon_base::FAVICON)); EXPECT_EQ(1u, NumIconMappingsForPageURL(url2, favicon_base::FAVICON)); EXPECT_EQ(1u, NumIconMappingsForPageURL(url3, favicon_base::FAVICON)); @@ -1864,12 +1865,29 @@ TEST_F(HistoryBackendTest, SetFaviconMappingsForPageAndRedirectsWithFragment) { backend_->recent_redirects_.Clear(); backend_->recent_redirects_.Put(url1, redirects); - backend_->SetFavicons(url1, favicon_base::FAVICON, icon_url1, bitmaps); + backend_->SetFavicons({url1}, favicon_base::FAVICON, icon_url1, bitmaps); EXPECT_EQ(1u, NumIconMappingsForPageURL(url1, favicon_base::FAVICON)); EXPECT_EQ(1u, NumIconMappingsForPageURL(url2, favicon_base::FAVICON)); EXPECT_EQ(1u, NumIconMappingsForPageURL(url3, favicon_base::FAVICON)); } +TEST_F(HistoryBackendTest, + SetFaviconMappingsForPageAndRedirectsWithFragmentWithoutStripping) { + base::test::ScopedFeatureList override_features; + override_features.InitAndEnableFeature(kAvoidStrippingRefFromFaviconPageUrls); + + const GURL url("http://www.google.com#abc"); + const GURL url_without_ref("http://www.google.com"); + const GURL icon_url("http://www.google.com/icon"); + backend_->SetFavicons( + {url}, favicon_base::FAVICON, icon_url, + std::vector<SkBitmap>{CreateBitmap(SK_ColorBLUE, kSmallEdgeSize)}); + + EXPECT_EQ(1u, NumIconMappingsForPageURL(url, favicon_base::FAVICON)); + EXPECT_EQ(0u, + NumIconMappingsForPageURL(url_without_ref, favicon_base::FAVICON)); +} + // Test that |recent_redirects_| stores the full redirect chain in case of // client redirects. In this case, a server-side redirect is followed by a // client-side one. @@ -1905,7 +1923,7 @@ TEST_F(HistoryBackendTest, SetFaviconMappingsForPageDuplicates) { bitmaps.push_back(CreateBitmap(SK_ColorBLUE, kSmallEdgeSize)); bitmaps.push_back(CreateBitmap(SK_ColorRED, kLargeEdgeSize)); - backend_->SetFavicons(url, favicon_base::FAVICON, icon_url, bitmaps); + backend_->SetFavicons({url}, favicon_base::FAVICON, icon_url, bitmaps); std::vector<IconMapping> icon_mappings; EXPECT_TRUE(backend_->thumbnail_db_->GetIconMappingsForPageURL( @@ -1913,7 +1931,7 @@ TEST_F(HistoryBackendTest, SetFaviconMappingsForPageDuplicates) { EXPECT_EQ(1u, icon_mappings.size()); IconMappingID mapping_id = icon_mappings[0].mapping_id; - backend_->SetFavicons(url, favicon_base::FAVICON, icon_url, bitmaps); + backend_->SetFavicons({url}, favicon_base::FAVICON, icon_url, bitmaps); icon_mappings.clear(); EXPECT_TRUE(backend_->thumbnail_db_->GetIconMappingsForPageURL( @@ -1935,7 +1953,7 @@ TEST_F(HistoryBackendTest, SetFaviconsDeleteBitmaps) { std::vector<SkBitmap> bitmaps; bitmaps.push_back(CreateBitmap(SK_ColorBLUE, kSmallEdgeSize)); bitmaps.push_back(CreateBitmap(SK_ColorRED, kLargeEdgeSize)); - backend_->SetFavicons(page_url, favicon_base::FAVICON, icon_url, bitmaps); + backend_->SetFavicons({page_url}, favicon_base::FAVICON, icon_url, bitmaps); // Test initial state. std::vector<IconMapping> icon_mappings; @@ -1961,7 +1979,7 @@ TEST_F(HistoryBackendTest, SetFaviconsDeleteBitmaps) { // the small bitmap is in fact deleted. bitmaps.clear(); bitmaps.push_back(CreateBitmap(SK_ColorWHITE, kLargeEdgeSize)); - backend_->SetFavicons(page_url, favicon_base::FAVICON, icon_url, bitmaps); + backend_->SetFavicons({page_url}, favicon_base::FAVICON, icon_url, bitmaps); scoped_refptr<base::RefCountedMemory> bitmap_data_out; gfx::Size pixel_size_out; @@ -1987,7 +2005,7 @@ TEST_F(HistoryBackendTest, SetFaviconsReplaceBitmapData) { bitmaps.push_back(CreateBitmap(SK_ColorBLUE, kSmallEdgeSize)); // Add bitmap to the database. - backend_->SetFavicons(page_url, favicon_base::FAVICON, icon_url, bitmaps); + backend_->SetFavicons({page_url}, favicon_base::FAVICON, icon_url, bitmaps); favicon_base::FaviconID original_favicon_id = backend_->thumbnail_db_->GetFaviconIDForFaviconURL(icon_url, @@ -2002,7 +2020,7 @@ TEST_F(HistoryBackendTest, SetFaviconsReplaceBitmapData) { // Call SetFavicons() with completely identical data. bitmaps[0] = CreateBitmap(SK_ColorBLUE, kSmallEdgeSize); - backend_->SetFavicons(page_url, favicon_base::FAVICON, icon_url, bitmaps); + backend_->SetFavicons({page_url}, favicon_base::FAVICON, icon_url, bitmaps); favicon_base::FaviconID updated_favicon_id = backend_->thumbnail_db_->GetFaviconIDForFaviconURL(icon_url, @@ -2017,7 +2035,7 @@ TEST_F(HistoryBackendTest, SetFaviconsReplaceBitmapData) { // Call SetFavicons() with a different bitmap of the same size. bitmaps[0] = CreateBitmap(SK_ColorWHITE, kSmallEdgeSize); - backend_->SetFavicons(page_url, favicon_base::FAVICON, icon_url, bitmaps); + backend_->SetFavicons({page_url}, favicon_base::FAVICON, icon_url, bitmaps); updated_favicon_id = backend_->thumbnail_db_->GetFaviconIDForFaviconURL( icon_url, favicon_base::FAVICON); @@ -2045,12 +2063,12 @@ TEST_F(HistoryBackendTest, SetFaviconsSameFaviconURLForTwoPages) { bitmaps.push_back(CreateBitmap(SK_ColorBLUE, kSmallEdgeSize)); bitmaps.push_back(CreateBitmap(SK_ColorRED, kLargeEdgeSize)); - backend_->SetFavicons(page_url1, favicon_base::FAVICON, icon_url, bitmaps); + backend_->SetFavicons({page_url1}, favicon_base::FAVICON, icon_url, bitmaps); std::vector<favicon_base::FaviconRawBitmapResult> bitmap_results; backend_->UpdateFaviconMappingsAndFetch( - std::set<GURL>{page_url2}, icon_url, favicon_base::FAVICON, - GetEdgeSizesSmallAndLarge(), &bitmap_results); + {page_url2}, icon_url, favicon_base::FAVICON, GetEdgeSizesSmallAndLarge(), + &bitmap_results); // Check that the same FaviconID is mapped to both page URLs. std::vector<IconMapping> icon_mappings; @@ -2069,8 +2087,8 @@ TEST_F(HistoryBackendTest, SetFaviconsSameFaviconURLForTwoPages) { // Change the icon URL that |page_url1| is mapped to. bitmaps.clear(); bitmaps.push_back(CreateBitmap(SK_ColorWHITE, kSmallEdgeSize)); - backend_->SetFavicons( - page_url1, favicon_base::FAVICON, icon_url_new, bitmaps); + backend_->SetFavicons({page_url1}, favicon_base::FAVICON, icon_url_new, + bitmaps); // |page_url1| should map to a new FaviconID and have valid bitmap data. icon_mappings.clear(); @@ -2099,6 +2117,33 @@ TEST_F(HistoryBackendTest, SetFaviconsSameFaviconURLForTwoPages) { EXPECT_EQ(2u, favicon_bitmaps.size()); } +// Test that if two pages share the same favicon, reported via a single call to +// SetFavicons(), it gets associated to both page URLs. +TEST_F(HistoryBackendTest, SetFaviconsWithTwoPageURLs) { + GURL icon_url("http://www.google.com/favicon.ico"); + GURL page_url1("http://www.google.com"); + GURL page_url2("http://www.google.ca"); + std::vector<SkBitmap> bitmaps; + bitmaps.push_back(CreateBitmap(SK_ColorBLUE, kSmallEdgeSize)); + bitmaps.push_back(CreateBitmap(SK_ColorRED, kLargeEdgeSize)); + + backend_->SetFavicons({page_url1, page_url2}, favicon_base::FAVICON, icon_url, + bitmaps); + + std::vector<IconMapping> icon_mappings; + EXPECT_TRUE(backend_->thumbnail_db_->GetIconMappingsForPageURL( + page_url1, &icon_mappings)); + ASSERT_EQ(1u, icon_mappings.size()); + favicon_base::FaviconID favicon_id = icon_mappings[0].icon_id; + EXPECT_NE(0, favicon_id); + + icon_mappings.clear(); + EXPECT_TRUE(backend_->thumbnail_db_->GetIconMappingsForPageURL( + page_url2, &icon_mappings)); + ASSERT_EQ(1u, icon_mappings.size()); + EXPECT_EQ(favicon_id, icon_mappings[0].icon_id); +} + // Tests calling SetOnDemandFavicons(). Neither |page_url| nor |icon_url| are // known to the database. TEST_F(HistoryBackendTest, SetOnDemandFaviconsForEmptyDB) { @@ -2108,7 +2153,6 @@ TEST_F(HistoryBackendTest, SetOnDemandFaviconsForEmptyDB) { std::vector<SkBitmap> bitmaps; bitmaps.push_back(CreateBitmap(SK_ColorRED, kSmallEdgeSize)); - // Call SetOnDemandFavicons() with a different icon URL and bitmap data. EXPECT_TRUE(backend_->SetOnDemandFavicons(page_url, favicon_base::FAVICON, icon_url, bitmaps)); @@ -2119,10 +2163,17 @@ TEST_F(HistoryBackendTest, SetOnDemandFaviconsForEmptyDB) { FaviconBitmap favicon_bitmap; ASSERT_TRUE(GetOnlyFaviconBitmap(favicon_id, &favicon_bitmap)); - // The original bitmap should have been retrieved. + // The newly set bitmap should have been retrieved. EXPECT_TRUE(BitmapColorEqual(SK_ColorRED, favicon_bitmap.bitmap_data)); - // The favicon should not be marked as expired. + // The favicon should be marked as expired. EXPECT_EQ(base::Time(), favicon_bitmap.last_updated); + + // The raw bitmap result is marked as fetched on-demand. + favicon_base::FaviconRawBitmapResult result; + backend_->GetLargestFaviconForURL(page_url, + std::vector<int>{favicon_base::FAVICON}, + kSmallEdgeSize, &result); + EXPECT_FALSE(result.fetched_because_of_page_visit); } // Tests calling SetOnDemandFavicons(). |page_url| is known to the database @@ -2135,7 +2186,7 @@ TEST_F(HistoryBackendTest, SetOnDemandFaviconsForPageInDB) { bitmaps.push_back(CreateBitmap(SK_ColorBLUE, kSmallEdgeSize)); // Add bitmap to the database. - backend_->SetFavicons(page_url, favicon_base::FAVICON, icon_url1, bitmaps); + backend_->SetFavicons({page_url}, favicon_base::FAVICON, icon_url1, bitmaps); favicon_base::FaviconID original_favicon_id = backend_->thumbnail_db_->GetFaviconIDForFaviconURL(icon_url1, favicon_base::FAVICON); @@ -2154,6 +2205,13 @@ TEST_F(HistoryBackendTest, SetOnDemandFaviconsForPageInDB) { EXPECT_TRUE(BitmapColorEqual(SK_ColorBLUE, favicon_bitmap.bitmap_data)); // The favicon should not be marked as expired. EXPECT_NE(base::Time(), favicon_bitmap.last_updated); + + // The raw bitmap result is not marked as fetched on-demand. + favicon_base::FaviconRawBitmapResult result; + backend_->GetLargestFaviconForURL(page_url, + std::vector<int>{favicon_base::FAVICON}, + kSmallEdgeSize, &result); + EXPECT_TRUE(result.fetched_because_of_page_visit); } // Tests calling SetOnDemandFavicons(). |page_url| is not known to the @@ -2166,7 +2224,8 @@ TEST_F(HistoryBackendTest, SetOnDemandFaviconsForIconInDB) { bitmaps.push_back(CreateBitmap(SK_ColorBLUE, kSmallEdgeSize)); // Add bitmap to the database. - backend_->SetFavicons(old_page_url, favicon_base::FAVICON, icon_url, bitmaps); + backend_->SetFavicons({old_page_url}, favicon_base::FAVICON, icon_url, + bitmaps); favicon_base::FaviconID original_favicon_id = backend_->thumbnail_db_->GetFaviconIDForFaviconURL(icon_url, favicon_base::FAVICON); @@ -2187,6 +2246,13 @@ TEST_F(HistoryBackendTest, SetOnDemandFaviconsForIconInDB) { EXPECT_TRUE(BitmapColorEqual(SK_ColorBLUE, favicon_bitmap.bitmap_data)); // The favicon should not be marked as expired. EXPECT_NE(base::Time(), favicon_bitmap.last_updated); + + // The raw bitmap result is not marked as fetched on-demand. + favicon_base::FaviconRawBitmapResult result; + backend_->GetLargestFaviconForURL(page_url, + std::vector<int>{favicon_base::FAVICON}, + kSmallEdgeSize, &result); + EXPECT_TRUE(result.fetched_because_of_page_visit); } // Test repeatedly calling MergeFavicon(). |page_url| is initially not known @@ -2244,7 +2310,7 @@ TEST_F(HistoryBackendTest, MergeFaviconPageURLInDB) { std::vector<SkBitmap> bitmaps; bitmaps.push_back(CreateBitmap(SK_ColorBLUE, kSmallEdgeSize)); - backend_->SetFavicons(page_url, favicon_base::FAVICON, icon_url1, bitmaps); + backend_->SetFavicons({page_url}, favicon_base::FAVICON, icon_url1, bitmaps); // Test initial state. std::vector<IconMapping> icon_mappings; @@ -2362,7 +2428,7 @@ TEST_F(HistoryBackendTest, MergeFaviconIconURLMappedToDifferentPageURL) { std::vector<SkBitmap> bitmaps; bitmaps.push_back(CreateBitmap(SK_ColorBLUE, kSmallEdgeSize)); - backend_->SetFavicons(page_url1, favicon_base::FAVICON, icon_url, bitmaps); + backend_->SetFavicons({page_url1}, favicon_base::FAVICON, icon_url, bitmaps); // Test initial state. std::vector<IconMapping> icon_mappings; @@ -2481,7 +2547,7 @@ TEST_F(HistoryBackendTest, MergeFaviconShowsUpInGetFaviconsForURLResult) { bitmaps.push_back(CreateBitmap(SK_ColorRED, kLargeEdgeSize)); // Set some preexisting favicons for |page_url|. - backend_->SetFavicons(page_url, favicon_base::FAVICON, icon_url, bitmaps); + backend_->SetFavicons({page_url}, favicon_base::FAVICON, icon_url, bitmaps); // Merge small favicon. std::vector<unsigned char> data; @@ -2524,7 +2590,8 @@ TEST_F(HistoryBackendTest, FaviconChangedNotificationNewFavicon) { { std::vector<SkBitmap> bitmaps; bitmaps.push_back(CreateBitmap(SK_ColorBLUE, kSmallEdgeSize)); - backend_->SetFavicons(page_url1, favicon_base::FAVICON, icon_url1, bitmaps); + backend_->SetFavicons({page_url1}, favicon_base::FAVICON, icon_url1, + bitmaps); ASSERT_EQ(1u, favicon_changed_notifications_page_urls().size()); EXPECT_EQ(page_url1, favicon_changed_notifications_page_urls()[0]); EXPECT_EQ(0u, favicon_changed_notifications_icon_urls().size()); @@ -2557,7 +2624,7 @@ TEST_F(HistoryBackendTest, FaviconChangedNotificationBitmapDataChanged) { { std::vector<SkBitmap> bitmaps; bitmaps.push_back(CreateBitmap(SK_ColorBLUE, kSmallEdgeSize)); - backend_->SetFavicons(page_url, favicon_base::FAVICON, icon_url, bitmaps); + backend_->SetFavicons({page_url}, favicon_base::FAVICON, icon_url, bitmaps); ClearBroadcastedNotifications(); } @@ -2565,7 +2632,7 @@ TEST_F(HistoryBackendTest, FaviconChangedNotificationBitmapDataChanged) { { std::vector<SkBitmap> bitmaps; bitmaps.push_back(CreateBitmap(SK_ColorWHITE, kSmallEdgeSize)); - backend_->SetFavicons(page_url, favicon_base::FAVICON, icon_url, bitmaps); + backend_->SetFavicons({page_url}, favicon_base::FAVICON, icon_url, bitmaps); EXPECT_EQ(0u, favicon_changed_notifications_page_urls().size()); ASSERT_EQ(1u, favicon_changed_notifications_icon_urls().size()); EXPECT_EQ(icon_url, favicon_changed_notifications_icon_urls()[0]); @@ -2607,20 +2674,22 @@ TEST_F(HistoryBackendTest, FaviconChangedNotificationIconMappingChanged) { { std::vector<SkBitmap> bitmaps; bitmaps.push_back(CreateBitmap(SK_ColorBLUE, kSmallEdgeSize)); - backend_->SetFavicons(page_url1, favicon_base::FAVICON, icon_url1, bitmaps); - backend_->SetFavicons(page_url2, favicon_base::FAVICON, icon_url2, bitmaps); + backend_->SetFavicons({page_url1}, favicon_base::FAVICON, icon_url1, + bitmaps); + backend_->SetFavicons({page_url2}, favicon_base::FAVICON, icon_url2, + bitmaps); // Map |page_url3| to |icon_url1| so that the test does not delete the // favicon at |icon_url1|. std::vector<favicon_base::FaviconRawBitmapResult> bitmap_results; backend_->UpdateFaviconMappingsAndFetch( - std::set<GURL>{page_url3}, icon_url1, favicon_base::FAVICON, + {page_url3}, icon_url1, favicon_base::FAVICON, GetEdgeSizesSmallAndLarge(), &bitmap_results); ClearBroadcastedNotifications(); } // SetFavicons() - backend_->SetFavicons(page_url1, favicon_base::FAVICON, icon_url2, bitmaps); + backend_->SetFavicons({page_url1}, favicon_base::FAVICON, icon_url2, bitmaps); EXPECT_THAT(favicon_changed_notifications_page_urls(), ElementsAre(page_url1)); EXPECT_EQ(0u, favicon_changed_notifications_icon_urls().size()); @@ -2638,7 +2707,7 @@ TEST_F(HistoryBackendTest, FaviconChangedNotificationIconMappingChanged) { { std::vector<favicon_base::FaviconRawBitmapResult> bitmap_results; backend_->UpdateFaviconMappingsAndFetch( - std::set<GURL>{page_url1}, icon_url2, favicon_base::FAVICON, + {page_url1}, icon_url2, favicon_base::FAVICON, GetEdgeSizesSmallAndLarge(), &bitmap_results); EXPECT_THAT(favicon_changed_notifications_page_urls(), ElementsAre(page_url1)); @@ -2666,7 +2735,8 @@ TEST_F(HistoryBackendTest, { std::vector<SkBitmap> bitmaps; bitmaps.push_back(CreateBitmap(SK_ColorBLUE, kSmallEdgeSize)); - backend_->SetFavicons(page_url4, favicon_base::FAVICON, icon_url, bitmaps); + backend_->SetFavicons({page_url4}, favicon_base::FAVICON, icon_url, + bitmaps); ClearBroadcastedNotifications(); } @@ -2710,14 +2780,16 @@ TEST_F(HistoryBackendTest, { std::vector<SkBitmap> bitmaps; bitmaps.push_back(CreateBitmap(SK_ColorBLUE, kSmallEdgeSize)); - backend_->SetFavicons(page_url1, favicon_base::FAVICON, icon_url1, bitmaps); - backend_->SetFavicons(page_url2, favicon_base::FAVICON, icon_url2, bitmaps); + backend_->SetFavicons({page_url1}, favicon_base::FAVICON, icon_url1, + bitmaps); + backend_->SetFavicons({page_url2}, favicon_base::FAVICON, icon_url2, + bitmaps); // Map |page_url3| to |icon_url1| so that the test does not delete the // favicon at |icon_url1|. std::vector<favicon_base::FaviconRawBitmapResult> bitmap_results; backend_->UpdateFaviconMappingsAndFetch( - std::set<GURL>{page_url3}, icon_url1, favicon_base::FAVICON, + {page_url3}, icon_url1, favicon_base::FAVICON, GetEdgeSizesSmallAndLarge(), &bitmap_results); ClearBroadcastedNotifications(); } @@ -2726,7 +2798,8 @@ TEST_F(HistoryBackendTest, { std::vector<SkBitmap> bitmaps; bitmaps.push_back(CreateBitmap(SK_ColorWHITE, kSmallEdgeSize)); - backend_->SetFavicons(page_url1, favicon_base::FAVICON, icon_url2, bitmaps); + backend_->SetFavicons({page_url1}, favicon_base::FAVICON, icon_url2, + bitmaps); ASSERT_EQ(1u, favicon_changed_notifications_page_urls().size()); EXPECT_EQ(page_url1, favicon_changed_notifications_page_urls()[0]); ASSERT_EQ(1u, favicon_changed_notifications_icon_urls().size()); @@ -2850,11 +2923,11 @@ TEST_F(HistoryBackendTest, NoFaviconChangedNotifications) { ASSERT_TRUE(gfx::PNGCodec::EncodeBGRASkBitmap(bitmap, false, &png_bytes)); // Setup - backend_->SetFavicons(page_url, favicon_base::FAVICON, icon_url, bitmaps); + backend_->SetFavicons({page_url}, favicon_base::FAVICON, icon_url, bitmaps); ClearBroadcastedNotifications(); // SetFavicons() - backend_->SetFavicons(page_url, favicon_base::FAVICON, icon_url, bitmaps); + backend_->SetFavicons({page_url}, favicon_base::FAVICON, icon_url, bitmaps); // MergeFavicon() backend_->MergeFavicon(page_url, icon_url, favicon_base::FAVICON, @@ -2864,7 +2937,7 @@ TEST_F(HistoryBackendTest, NoFaviconChangedNotifications) { { std::vector<favicon_base::FaviconRawBitmapResult> bitmap_results; backend_->UpdateFaviconMappingsAndFetch( - std::set<GURL>{page_url}, icon_url, favicon_base::FAVICON, + {page_url}, icon_url, favicon_base::FAVICON, GetEdgeSizesSmallAndLarge(), &bitmap_results); } @@ -2886,10 +2959,10 @@ TEST_F(HistoryBackendTest, TestGetFaviconsForURLWithIconTypesPriority) { touch_bitmaps.push_back(CreateBitmap(SK_ColorWHITE, 64)); // Set some preexisting favicons for |page_url|. - backend_->SetFavicons( - page_url, favicon_base::FAVICON, icon_url, favicon_bitmaps); - backend_->SetFavicons( - page_url, favicon_base::TOUCH_ICON, touch_icon_url, touch_bitmaps); + backend_->SetFavicons({page_url}, favicon_base::FAVICON, icon_url, + favicon_bitmaps); + backend_->SetFavicons({page_url}, favicon_base::TOUCH_ICON, touch_icon_url, + touch_bitmaps); favicon_base::FaviconRawBitmapResult result; std::vector<int> icon_types; @@ -2923,10 +2996,10 @@ TEST_F(HistoryBackendTest, TestGetFaviconsForURLReturnFavicon) { touch_bitmaps.push_back(CreateBitmap(SK_ColorWHITE, 32)); // Set some preexisting favicons for |page_url|. - backend_->SetFavicons( - page_url, favicon_base::FAVICON, icon_url, favicon_bitmaps); - backend_->SetFavicons( - page_url, favicon_base::TOUCH_ICON, touch_icon_url, touch_bitmaps); + backend_->SetFavicons({page_url}, favicon_base::FAVICON, icon_url, + favicon_bitmaps); + backend_->SetFavicons({page_url}, favicon_base::TOUCH_ICON, touch_icon_url, + touch_bitmaps); favicon_base::FaviconRawBitmapResult result; std::vector<int> icon_types; @@ -2956,7 +3029,7 @@ TEST_F(HistoryBackendTest, TestGetFaviconsForURLReturnFaviconEvenItSmaller) { bitmaps.push_back(CreateBitmap(SK_ColorBLUE, 16)); // Set preexisting favicons for |page_url|. - backend_->SetFavicons(page_url, favicon_base::FAVICON, icon_url, bitmaps); + backend_->SetFavicons({page_url}, favicon_base::FAVICON, icon_url, bitmaps); favicon_base::FaviconRawBitmapResult result; std::vector<int> icon_types; @@ -3011,7 +3084,7 @@ TEST_F(HistoryBackendTest, GetFaviconsFromDBSelectClosestMatch) { bitmaps.push_back(CreateBitmap(SK_ColorBLUE, kSmallEdgeSize)); bitmaps.push_back(CreateBitmap(SK_ColorRED, kLargeEdgeSize)); - backend_->SetFavicons(page_url, favicon_base::FAVICON, icon_url, bitmaps); + backend_->SetFavicons({page_url}, favicon_base::FAVICON, icon_url, bitmaps); std::vector<favicon_base::FaviconRawBitmapResult> bitmap_results_out; EXPECT_TRUE(backend_->GetFaviconsFromDB(page_url, @@ -3053,8 +3126,9 @@ TEST_F(HistoryBackendTest, GetFaviconsFromDBIconType) { bitmaps.push_back(CreateBitmap(SK_ColorBLUE, kSmallEdgeSize)); std::vector<favicon_base::FaviconRawBitmapData> favicon_bitmap_data; - backend_->SetFavicons(page_url, favicon_base::FAVICON, icon_url1, bitmaps); - backend_->SetFavicons(page_url, favicon_base::TOUCH_ICON, icon_url2, bitmaps); + backend_->SetFavicons({page_url}, favicon_base::FAVICON, icon_url1, bitmaps); + backend_->SetFavicons({page_url}, favicon_base::TOUCH_ICON, icon_url2, + bitmaps); std::vector<favicon_base::FaviconRawBitmapResult> bitmap_results_out; EXPECT_TRUE(backend_->GetFaviconsFromDB(page_url, @@ -3085,9 +3159,9 @@ TEST_F(HistoryBackendTest, GetFaviconsFromDBMultipleIconTypes) { const GURL icon_url2("http://www.google.com/icon2.png"); std::vector<favicon_base::FaviconRawBitmapData> favicon_bitmap_data; - backend_->SetFavicons(page_url, favicon_base::FAVICON, icon_url1, + backend_->SetFavicons({page_url}, favicon_base::FAVICON, icon_url1, {CreateBitmap(SK_ColorBLUE, kSmallEdgeSize)}); - backend_->SetFavicons(page_url, favicon_base::TOUCH_ICON, icon_url2, + backend_->SetFavicons({page_url}, favicon_base::TOUCH_ICON, icon_url2, {CreateBitmap(SK_ColorBLUE, kLargeEdgeSize)}); struct TestCase { @@ -3142,8 +3216,8 @@ TEST_F(HistoryBackendTest, UpdateFaviconMappingsAndFetchNoDB) { std::vector<favicon_base::FaviconRawBitmapResult> bitmap_results; backend_->UpdateFaviconMappingsAndFetch( - std::set<GURL>{GURL()}, GURL(), favicon_base::FAVICON, - GetEdgeSizesSmallAndLarge(), &bitmap_results); + {GURL()}, GURL(), favicon_base::FAVICON, GetEdgeSizesSmallAndLarge(), + &bitmap_results); EXPECT_TRUE(bitmap_results.empty()); } diff --git a/chromium/components/history/core/browser/history_database.cc b/chromium/components/history/core/browser/history_database.cc index 59dcff235b9..c3db915b635 100644 --- a/chromium/components/history/core/browser/history_database.cc +++ b/chromium/components/history/core/browser/history_database.cc @@ -37,7 +37,7 @@ namespace { // Current version number. We write databases at the "current" version number, // but any previous version that can read the "compatible" one can make do with // our database without *too* many bad effects. -const int kCurrentVersionNumber = 36; +const int kCurrentVersionNumber = 37; const int kCompatibleVersionNumber = 16; const char kEarlyExpirationThresholdKey[] = "early_expiration_threshold"; const int kMaxHostsInMemory = 10000; @@ -563,23 +563,8 @@ sql::InitStatus HistoryDatabase::EnsureCurrentVersion() { } if (cur_version == 34) { - /* - This code is commented out because we suspect the additional disk storage - requirements of duplicating the URL table to update the schema cause - some devices to run out of storage. Errors during initialization are - very disruptive to the user experience. - - TODO(https://crbug.com/736136) figure out how to update users to use - AUTOINCREMENT. - - // AUTOINCREMENT is added to urls table PRIMARY KEY(id), need to recreate a - // new table and copy all contents over. favicon_id is removed from urls - // table since we never use it. Also typed_url_sync_metadata and - // autofill_model_type_state tables are introduced, no migration needed for - // those two tables. - if (!RecreateURLTableWithAllContents()) - return LogMigrationFailure(34); - */ + // This originally contained an autoincrement migration which was abandoned + // and added back in version 36. (see https://crbug.com/736136) cur_version++; meta_table_.SetVersionNumber(cur_version); } @@ -591,6 +576,20 @@ sql::InitStatus HistoryDatabase::EnsureCurrentVersion() { meta_table_.SetVersionNumber(cur_version); } + if (cur_version == 36) { + // Version 34 added AUTOINCREMENT but was reverted. Since some users will + // have been migrated and others not, explicitly check for the AUTOINCREMENT + // annotation rather than the version number. + if (!URLTableContainsAutoincrement()) { + if (!RecreateURLTableWithAllContents()) + return LogMigrationFailure(36); + } + + DCHECK(URLTableContainsAutoincrement()); + cur_version++; + meta_table_.SetVersionNumber(cur_version); + } + // ========================= ^^ new migration code goes here ^^ // ADDING NEW MIGRATION CODE // ========================= diff --git a/chromium/components/history/core/browser/history_service.cc b/chromium/components/history/core/browser/history_service.cc index 982a8b1f455..200e440b47e 100644 --- a/chromium/components/history/core/browser/history_service.cc +++ b/chromium/components/history/core/browser/history_service.cc @@ -574,7 +574,7 @@ base::CancelableTaskTracker::TaskId HistoryService::GetFaviconForID( base::CancelableTaskTracker::TaskId HistoryService::UpdateFaviconMappingsAndFetch( - const std::set<GURL>& page_urls, + const base::flat_set<GURL>& page_urls, const GURL& icon_url, favicon_base::IconType icon_type, const std::vector<int>& desired_sizes, @@ -611,18 +611,26 @@ void HistoryService::MergeFavicon( page_url, icon_url, icon_type, bitmap_data, pixel_size)); } -void HistoryService::SetFavicons(const GURL& page_url, +void HistoryService::SetFavicons(const base::flat_set<GURL>& page_urls, favicon_base::IconType icon_type, const GURL& icon_url, const std::vector<SkBitmap>& bitmaps) { DCHECK(backend_task_runner_) << "History service being called after cleanup"; DCHECK(thread_checker_.CalledOnValidThread()); - if (history_client_ && !history_client_->CanAddURL(page_url)) + + base::flat_set<GURL> page_urls_to_save; + page_urls_to_save.reserve(page_urls.capacity()); + for (const GURL& page_url : page_urls) { + if (!history_client_ || history_client_->CanAddURL(page_url)) + page_urls_to_save.insert(page_url); + } + + if (page_urls_to_save.empty()) return; ScheduleTask(PRIORITY_NORMAL, base::Bind(&HistoryBackend::SetFavicons, history_backend_, - page_url, icon_type, icon_url, bitmaps)); + page_urls_to_save, icon_type, icon_url, bitmaps)); } void HistoryService::SetOnDemandFavicons(const GURL& page_url, @@ -981,7 +989,7 @@ syncer::SyncDataList HistoryService::GetAllSyncData( } syncer::SyncError HistoryService::ProcessSyncChanges( - const tracked_objects::Location& from_here, + const base::Location& from_here, const syncer::SyncChangeList& change_list) { delete_directive_handler_.ProcessSyncChanges(this, change_list); return syncer::SyncError(); diff --git a/chromium/components/history/core/browser/history_service.h b/chromium/components/history/core/browser/history_service.h index 140bbd353af..9b79d259d4f 100644 --- a/chromium/components/history/core/browser/history_service.h +++ b/chromium/components/history/core/browser/history_service.h @@ -16,6 +16,7 @@ #include "base/bind.h" #include "base/callback.h" #include "base/callback_list.h" +#include "base/containers/flat_set.h" #include "base/files/file_path.h" #include "base/logging.h" #include "base/macros.h" @@ -535,7 +536,7 @@ class HistoryService : public syncer::SyncableService, public KeyedService { void StopSyncing(syncer::ModelType type) override; syncer::SyncDataList GetAllSyncData(syncer::ModelType type) const override; syncer::SyncError ProcessSyncChanges( - const tracked_objects::Location& from_here, + const base::Location& from_here, const syncer::SyncChangeList& change_list) override; protected: @@ -707,7 +708,7 @@ class HistoryService : public syncer::SyncableService, public KeyedService { // just mapped to |page_urls| are returned. If |desired_sizes| has a '0' // entry, the largest favicon bitmap is returned. base::CancelableTaskTracker::TaskId UpdateFaviconMappingsAndFetch( - const std::set<GURL>& page_urls, + const base::flat_set<GURL>& page_urls, const GURL& icon_url, favicon_base::IconType icon_type, const std::vector<int>& desired_sizes, @@ -740,13 +741,14 @@ class HistoryService : public syncer::SyncableService, public KeyedService { scoped_refptr<base::RefCountedMemory> bitmap_data, const gfx::Size& pixel_size); - // Used by the FaviconService to replace all of the favicon bitmaps mapped to - // |page_url| for |icon_type|. + // Used by the FaviconService to replace the favicon bitmaps mapped to all + // URLs in |page_urls| for |icon_type|. // Use MergeFavicon() if |bitmaps| is incomplete, and favicon bitmaps in the // database should be preserved if possible. For instance, favicon bitmaps // from sync are 1x only. MergeFavicon() is used to avoid deleting the 2x - // favicon bitmap if it is present in the history backend. - void SetFavicons(const GURL& page_url, + // favicon bitmap if it is present in the history backend. |page_urls| must + // not be empty. + void SetFavicons(const base::flat_set<GURL>& page_urls, favicon_base::IconType icon_type, const GURL& icon_url, const std::vector<SkBitmap>& bitmaps); diff --git a/chromium/components/history/core/browser/history_types.h b/chromium/components/history/core/browser/history_types.h index 00b6cb00aa8..fd6b86231a7 100644 --- a/chromium/components/history/core/browser/history_types.h +++ b/chromium/components/history/core/browser/history_types.h @@ -8,7 +8,6 @@ #include <stddef.h> #include <stdint.h> -#include <deque> #include <map> #include <set> #include <string> diff --git a/chromium/components/history/core/browser/thumbnail_database.cc b/chromium/components/history/core/browser/thumbnail_database.cc index d452bd5d744..01933fc464c 100644 --- a/chromium/components/history/core/browser/thumbnail_database.cc +++ b/chromium/components/history/core/browser/thumbnail_database.cc @@ -414,21 +414,16 @@ void ThumbnailDatabase::TrimMemory(bool aggressively) { std::map<favicon_base::FaviconID, IconMappingsForExpiry> ThumbnailDatabase::GetOldOnDemandFavicons(base::Time threshold) { - // Restrict to on-demand bitmaps (i.e. with last_requested != 0). This is - // called rarely during history expiration cleanup and hence not worth + // Restrict to on-demand bitmaps (i.e. with last_requested != 0). + // This is called rarely during history expiration cleanup and hence not worth // caching. - // TODO(jkrcal): In M63, remove the "(last_requested=0 AND last_updated=0)" - // clause which is only transitional - to clean up expired icons (previously, - // on-demand favicons were stored as expired on-visit favicons). sql::Statement old_icons(db_.GetUniqueStatement( "SELECT favicons.id, favicons.url, icon_mapping.page_url " "FROM favicons " "JOIN favicon_bitmaps ON (favicon_bitmaps.icon_id = favicons.id) " "JOIN icon_mapping ON (icon_mapping.icon_id = favicon_bitmaps.icon_id) " - "WHERE ((favicon_bitmaps.last_requested = 0 AND " - " favicon_bitmaps.last_updated = 0) OR " - " (favicon_bitmaps.last_requested > 0 AND " - " favicon_bitmaps.last_requested < ?))")); + "WHERE (favicon_bitmaps.last_requested > 0 AND " + " favicon_bitmaps.last_requested < ?)")); old_icons.BindInt64(0, threshold.ToInternalValue()); std::map<favicon_base::FaviconID, IconMappingsForExpiry> icon_mappings; diff --git a/chromium/components/history/core/browser/thumbnail_database.h b/chromium/components/history/core/browser/thumbnail_database.h index f9ba1c71de4..bd29eb4938b 100644 --- a/chromium/components/history/core/browser/thumbnail_database.h +++ b/chromium/components/history/core/browser/thumbnail_database.h @@ -28,7 +28,7 @@ class HistoryBackendClient; // The minimum number of days after which last_requested field gets updated. // All earlier updates are ignored. -static const int kFaviconUpdateLastRequestedAfterDays = 14; +static const int kFaviconUpdateLastRequestedAfterDays = 10; // This database interface is owned by the history backend and runs on the // history thread. It is a totally separate component from history partially diff --git a/chromium/components/history/core/browser/thumbnail_database_unittest.cc b/chromium/components/history/core/browser/thumbnail_database_unittest.cc index ffbf876080c..3bf1afb842b 100644 --- a/chromium/components/history/core/browser/thumbnail_database_unittest.cc +++ b/chromium/components/history/core/browser/thumbnail_database_unittest.cc @@ -413,7 +413,7 @@ TEST_F(ThumbnailDatabaseTest, GetOldOnDemandFaviconsReturnsOld) { // if the on-visit icons have expired. We need this behavior in order to delete // icons stored via HistoryService::SetOnDemandFavicons() prior to on-demand // icons setting the "last_requested" time. -TEST_F(ThumbnailDatabaseTest, GetOldOnDemandFaviconsReturnsExpired) { +TEST_F(ThumbnailDatabaseTest, GetOldOnDemandFaviconsDoesNotReturnExpired) { ThumbnailDatabase db(nullptr); ASSERT_EQ(sql::INIT_OK, db.Init(file_name_)); db.BeginTransaction(); @@ -432,14 +432,11 @@ TEST_F(ThumbnailDatabaseTest, GetOldOnDemandFaviconsReturnsExpired) { ASSERT_NE(0, db.AddIconMapping(page_url, icon)); ASSERT_TRUE(db.SetFaviconOutOfDate(icon)); - // The threshold is ignored for expired icons. - auto map = db.GetOldOnDemandFavicons(/*threshold=*/base::Time::Now()); + base::Time get_older_than = start + base::TimeDelta::FromSeconds(1); + auto map = db.GetOldOnDemandFavicons(get_older_than); - // The icon is returned. - EXPECT_THAT(map, ElementsAre(Pair( - icon, AllOf(Field(&IconMappingsForExpiry::icon_url, url), - Field(&IconMappingsForExpiry::page_urls, - ElementsAre(page_url)))))); + // No icon is returned. + EXPECT_TRUE(map.empty()); } // Test that ThumbnailDatabase::GetOldOnDemandFavicons() does not return diff --git a/chromium/components/history/core/browser/top_sites_backend.cc b/chromium/components/history/core/browser/top_sites_backend.cc index aa107e9fc85..a35248e43ff 100644 --- a/chromium/components/history/core/browser/top_sites_backend.cc +++ b/chromium/components/history/core/browser/top_sites_backend.cc @@ -118,14 +118,7 @@ void TopSitesBackend::UpdateTopSitesOnDBThread( base::TimeTicks begin_time = base::TimeTicks::Now(); - for (size_t i = 0; i < delta.deleted.size(); ++i) - db_->RemoveURL(delta.deleted[i]); - - for (size_t i = 0; i < delta.added.size(); ++i) - db_->SetPageThumbnail(delta.added[i].url, delta.added[i].rank, Images()); - - for (size_t i = 0; i < delta.moved.size(); ++i) - db_->UpdatePageRank(delta.moved[i].url, delta.moved[i].rank); + db_->ApplyDelta(delta); if (record_or_not == RECORD_HISTOGRAM_YES) { UMA_HISTOGRAM_TIMES("History.FirstUpdateTime", diff --git a/chromium/components/history/core/browser/top_sites_database.cc b/chromium/components/history/core/browser/top_sites_database.cc index 435930053a2..520a0d32ae5 100644 --- a/chromium/components/history/core/browser/top_sites_database.cc +++ b/chromium/components/history/core/browser/top_sites_database.cc @@ -21,7 +21,6 @@ #include "sql/connection.h" #include "sql/recovery.h" #include "sql/statement.h" -#include "sql/transaction.h" #include "third_party/sqlite/sqlite3.h" namespace history { @@ -382,6 +381,26 @@ bool TopSitesDatabase::InitImpl(const base::FilePath& db_name) { return true; } +void TopSitesDatabase::ApplyDelta(const TopSitesDelta& delta) { + sql::Transaction transaction(db_.get()); + transaction.Begin(); + + for (size_t i = 0; i < delta.deleted.size(); ++i) { + if (!RemoveURLNoTransaction(delta.deleted[i])) + return; + } + + for (size_t i = 0; i < delta.added.size(); ++i) { + SetPageThumbnailNoTransaction(delta.added[i].url, delta.added[i].rank, + Images()); + } + + for (size_t i = 0; i < delta.moved.size(); ++i) + UpdatePageRankNoTransaction(delta.moved[i].url, delta.moved[i].rank); + + transaction.Commit(); +} + bool TopSitesDatabase::UpgradeToVersion3() { // Add 'last_forced' column. if (!db_->Execute( @@ -416,8 +435,8 @@ void TopSitesDatabase::GetPageThumbnails(MostVisitedURLList* urls, GURL gurl(statement.ColumnString(0)); url.url = gurl; url.title = statement.ColumnString16(2); - url.last_forced_time = - base::Time::FromInternalValue(statement.ColumnInt64(10)); + url.last_forced_time = base::Time() + base::TimeDelta::FromMicroseconds( + statement.ColumnInt64(10)); std::string redirects = statement.ColumnString(4); SetRedirects(redirects, &url); urls->push_back(url); @@ -431,18 +450,46 @@ void TopSitesDatabase::GetPageThumbnails(MostVisitedURLList* urls, thumbnail.thumbnail_score.good_clipping = statement.ColumnBool(6); thumbnail.thumbnail_score.at_top = statement.ColumnBool(7); thumbnail.thumbnail_score.time_at_snapshot = - base::Time::FromInternalValue(statement.ColumnInt64(8)); + base::Time() + + base::TimeDelta::FromMicroseconds(statement.ColumnInt64(8)); thumbnail.thumbnail_score.load_completed = statement.ColumnBool(9); (*thumbnails)[gurl] = thumbnail; } } +bool TopSitesDatabase::GetPageThumbnail(const GURL& url, Images* thumbnail) { + sql::Statement statement(db_->GetCachedStatement( + SQL_FROM_HERE, + "SELECT thumbnail, boring_score, good_clipping, at_top, last_updated " + "FROM thumbnails WHERE url=?")); + statement.BindString(0, url.spec()); + if (!statement.Step()) + return false; + + std::vector<unsigned char> data; + statement.ColumnBlobAsVector(0, &data); + thumbnail->thumbnail = base::RefCountedBytes::TakeVector(&data); + thumbnail->thumbnail_score.boring_score = statement.ColumnDouble(1); + thumbnail->thumbnail_score.good_clipping = statement.ColumnBool(2); + thumbnail->thumbnail_score.at_top = statement.ColumnBool(3); + thumbnail->thumbnail_score.time_at_snapshot = + base::Time() + + base::TimeDelta::FromMicroseconds(statement.ColumnInt64(4)); + return true; +} + void TopSitesDatabase::SetPageThumbnail(const MostVisitedURL& url, int new_rank, const Images& thumbnail) { sql::Transaction transaction(db_.get()); transaction.Begin(); + SetPageThumbnailNoTransaction(url, new_rank, thumbnail); + transaction.Commit(); +} +void TopSitesDatabase::SetPageThumbnailNoTransaction(const MostVisitedURL& url, + int new_rank, + const Images& thumbnail) { int rank = GetURLRank(url); if (rank == kRankOfNonExistingURL) { AddPageThumbnail(url, new_rank, thumbnail); @@ -450,35 +497,6 @@ void TopSitesDatabase::SetPageThumbnail(const MostVisitedURL& url, UpdatePageRankNoTransaction(url, new_rank); UpdatePageThumbnail(url, thumbnail); } - - transaction.Commit(); -} - -bool TopSitesDatabase::UpdatePageThumbnail(const MostVisitedURL& url, - const Images& thumbnail) { - sql::Statement statement(db_->GetCachedStatement( - SQL_FROM_HERE, - "UPDATE thumbnails SET " - "title = ?, thumbnail = ?, redirects = ?, " - "boring_score = ?, good_clipping = ?, at_top = ?, last_updated = ?, " - "load_completed = ?, last_forced = ?" - "WHERE url = ? ")); - statement.BindString16(0, url.title); - if (thumbnail.thumbnail.get() && thumbnail.thumbnail->front()) { - statement.BindBlob(1, thumbnail.thumbnail->front(), - static_cast<int>(thumbnail.thumbnail->size())); - } - statement.BindString(2, GetRedirects(url)); - const ThumbnailScore& score = thumbnail.thumbnail_score; - statement.BindDouble(3, score.boring_score); - statement.BindBool(4, score.good_clipping); - statement.BindBool(5, score.at_top); - statement.BindInt64(6, score.time_at_snapshot.ToInternalValue()); - statement.BindBool(7, score.load_completed); - statement.BindInt64(8, url.last_forced_time.ToInternalValue()); - statement.BindString(9, url.url.spec()); - - return statement.Run(); } void TopSitesDatabase::AddPageThumbnail(const MostVisitedURL& url, @@ -502,9 +520,10 @@ void TopSitesDatabase::AddPageThumbnail(const MostVisitedURL& url, statement.BindDouble(5, score.boring_score); statement.BindBool(6, score.good_clipping); statement.BindBool(7, score.at_top); - statement.BindInt64(8, score.time_at_snapshot.ToInternalValue()); + statement.BindInt64(8, + score.time_at_snapshot.since_origin().InMicroseconds()); statement.BindBool(9, score.load_completed); - int64_t last_forced = url.last_forced_time.ToInternalValue(); + int64_t last_forced = url.last_forced_time.since_origin().InMicroseconds(); DCHECK((last_forced == 0) == (new_rank != kRankOfForcedURL)) << "Thumbnail without a forced time stamp has a forced rank, or the " << "opposite."; @@ -517,18 +536,46 @@ void TopSitesDatabase::AddPageThumbnail(const MostVisitedURL& url, UpdatePageRankNoTransaction(url, new_rank); } -void TopSitesDatabase::UpdatePageRank(const MostVisitedURL& url, int new_rank) { - DCHECK((url.last_forced_time.ToInternalValue() == 0) == - (new_rank != kRankOfForcedURL)) - << "Thumbnail without a forced time stamp has a forced rank, or the " - << "opposite."; - sql::Transaction transaction(db_.get()); - transaction.Begin(); - UpdatePageRankNoTransaction(url, new_rank); - transaction.Commit(); +bool TopSitesDatabase::UpdatePageThumbnail(const MostVisitedURL& url, + const Images& thumbnail) { + sql::Statement statement(db_->GetCachedStatement( + SQL_FROM_HERE, + "UPDATE thumbnails SET " + "title = ?, thumbnail = ?, redirects = ?, " + "boring_score = ?, good_clipping = ?, at_top = ?, last_updated = ?, " + "load_completed = ?, last_forced = ?" + "WHERE url = ? ")); + statement.BindString16(0, url.title); + if (thumbnail.thumbnail.get() && thumbnail.thumbnail->front()) { + statement.BindBlob(1, thumbnail.thumbnail->front(), + static_cast<int>(thumbnail.thumbnail->size())); + } + statement.BindString(2, GetRedirects(url)); + const ThumbnailScore& score = thumbnail.thumbnail_score; + statement.BindDouble(3, score.boring_score); + statement.BindBool(4, score.good_clipping); + statement.BindBool(5, score.at_top); + statement.BindInt64(6, + score.time_at_snapshot.since_origin().InMicroseconds()); + statement.BindBool(7, score.load_completed); + statement.BindInt64(8, url.last_forced_time.since_origin().InMicroseconds()); + statement.BindString(9, url.url.spec()); + + return statement.Run(); +} + +int TopSitesDatabase::GetURLRank(const MostVisitedURL& url) { + sql::Statement select_statement( + db_->GetCachedStatement(SQL_FROM_HERE, + "SELECT url_rank " + "FROM thumbnails WHERE url=?")); + select_statement.BindString(0, url.url.spec()); + if (select_statement.Step()) + return select_statement.ColumnInt(0); + + return kRankOfNonExistingURL; } -// Caller should have a transaction open. void TopSitesDatabase::UpdatePageRankNoTransaction(const MostVisitedURL& url, int new_rank) { DCHECK_GT(db_->transaction_nesting(), 0); @@ -548,22 +595,22 @@ void TopSitesDatabase::UpdatePageRankNoTransaction(const MostVisitedURL& url, // From non-forced to forced, shift down. // Example: 2 -> -1 // -1, -1, -1, 0, 1, [2 -> -1], [3 -> 2], [4 -> 3] - sql::Statement shift_statement(db_->GetCachedStatement( - SQL_FROM_HERE, - "UPDATE thumbnails " - "SET url_rank = url_rank - 1 " - "WHERE url_rank > ?")); + sql::Statement shift_statement( + db_->GetCachedStatement(SQL_FROM_HERE, + "UPDATE thumbnails " + "SET url_rank = url_rank - 1 " + "WHERE url_rank > ?")); shift_statement.BindInt(0, prev_rank); shift_statement.Run(); } else { // From non-forced to non-forced, shift up. // Example: 3 -> 1 // -1, -1, -1, 0, [1 -> 2], [2 -> 3], [3 -> 1], 4 - sql::Statement shift_statement(db_->GetCachedStatement( - SQL_FROM_HERE, - "UPDATE thumbnails " - "SET url_rank = url_rank + 1 " - "WHERE url_rank >= ? AND url_rank < ?")); + sql::Statement shift_statement( + db_->GetCachedStatement(SQL_FROM_HERE, + "UPDATE thumbnails " + "SET url_rank = url_rank + 1 " + "WHERE url_rank >= ? AND url_rank < ?")); shift_statement.BindInt(0, new_rank); shift_statement.BindInt(1, prev_rank); shift_statement.Run(); @@ -573,22 +620,22 @@ void TopSitesDatabase::UpdatePageRankNoTransaction(const MostVisitedURL& url, // From non-forced to forced, shift up. // Example: -1 -> 2 // -1, [-1 -> 2], -1, 0, 1, [2 -> 3], [3 -> 4], [4 -> 5] - sql::Statement shift_statement(db_->GetCachedStatement( - SQL_FROM_HERE, - "UPDATE thumbnails " - "SET url_rank = url_rank + 1 " - "WHERE url_rank >= ?")); + sql::Statement shift_statement( + db_->GetCachedStatement(SQL_FROM_HERE, + "UPDATE thumbnails " + "SET url_rank = url_rank + 1 " + "WHERE url_rank >= ?")); shift_statement.BindInt(0, new_rank); shift_statement.Run(); } else { // From non-forced to non-forced, shift down. // Example: 1 -> 3. // -1, -1, -1, 0, [1 -> 3], [2 -> 1], [3 -> 2], 4 - sql::Statement shift_statement(db_->GetCachedStatement( - SQL_FROM_HERE, - "UPDATE thumbnails " - "SET url_rank = url_rank - 1 " - "WHERE url_rank > ? AND url_rank <= ?")); + sql::Statement shift_statement( + db_->GetCachedStatement(SQL_FROM_HERE, + "UPDATE thumbnails " + "SET url_rank = url_rank - 1 " + "WHERE url_rank > ? AND url_rank <= ?")); shift_statement.BindInt(0, prev_rank); shift_statement.BindInt(1, new_rank); shift_statement.Run(); @@ -597,79 +644,41 @@ void TopSitesDatabase::UpdatePageRankNoTransaction(const MostVisitedURL& url, // Set the url's rank and last_forced, since the latter changes when a URL // goes from forced to non-forced and vice-versa. - sql::Statement set_statement(db_->GetCachedStatement( - SQL_FROM_HERE, - "UPDATE thumbnails " - "SET url_rank = ?, last_forced = ? " - "WHERE url == ?")); + sql::Statement set_statement( + db_->GetCachedStatement(SQL_FROM_HERE, + "UPDATE thumbnails " + "SET url_rank = ?, last_forced = ? " + "WHERE url == ?")); set_statement.BindInt(0, new_rank); - set_statement.BindInt64(1, url.last_forced_time.ToInternalValue()); + set_statement.BindInt64(1, + url.last_forced_time.since_origin().InMicroseconds()); set_statement.BindString(2, url.url.spec()); set_statement.Run(); } -bool TopSitesDatabase::GetPageThumbnail(const GURL& url, Images* thumbnail) { - sql::Statement statement(db_->GetCachedStatement( - SQL_FROM_HERE, - "SELECT thumbnail, boring_score, good_clipping, at_top, last_updated " - "FROM thumbnails WHERE url=?")); - statement.BindString(0, url.spec()); - if (!statement.Step()) - return false; - - std::vector<unsigned char> data; - statement.ColumnBlobAsVector(0, &data); - thumbnail->thumbnail = base::RefCountedBytes::TakeVector(&data); - thumbnail->thumbnail_score.boring_score = statement.ColumnDouble(1); - thumbnail->thumbnail_score.good_clipping = statement.ColumnBool(2); - thumbnail->thumbnail_score.at_top = statement.ColumnBool(3); - thumbnail->thumbnail_score.time_at_snapshot = - base::Time::FromInternalValue(statement.ColumnInt64(4)); - return true; -} - -int TopSitesDatabase::GetURLRank(const MostVisitedURL& url) { - sql::Statement select_statement(db_->GetCachedStatement( - SQL_FROM_HERE, - "SELECT url_rank " - "FROM thumbnails WHERE url=?")); - select_statement.BindString(0, url.url.spec()); - if (select_statement.Step()) - return select_statement.ColumnInt(0); - - return kRankOfNonExistingURL; -} - -// Remove the record for this URL. Returns true iff removed successfully. -bool TopSitesDatabase::RemoveURL(const MostVisitedURL& url) { +bool TopSitesDatabase::RemoveURLNoTransaction(const MostVisitedURL& url) { int old_rank = GetURLRank(url); if (old_rank == kRankOfNonExistingURL) - return false; + return true; - sql::Transaction transaction(db_.get()); - transaction.Begin(); if (old_rank != kRankOfForcedURL) { // Decrement all following ranks. - sql::Statement shift_statement(db_->GetCachedStatement( - SQL_FROM_HERE, - "UPDATE thumbnails " - "SET url_rank = url_rank - 1 " - "WHERE url_rank > ?")); + sql::Statement shift_statement( + db_->GetCachedStatement(SQL_FROM_HERE, + "UPDATE thumbnails " + "SET url_rank = url_rank - 1 " + "WHERE url_rank > ?")); shift_statement.BindInt(0, old_rank); if (!shift_statement.Run()) return false; } - sql::Statement delete_statement( - db_->GetCachedStatement(SQL_FROM_HERE, - "DELETE FROM thumbnails WHERE url = ?")); + sql::Statement delete_statement(db_->GetCachedStatement( + SQL_FROM_HERE, "DELETE FROM thumbnails WHERE url = ?")); delete_statement.BindString(0, url.url.spec()); - if (!delete_statement.Run()) - return false; - - return transaction.Commit(); + return delete_statement.Run(); } sql::Connection* TopSitesDatabase::CreateDB(const base::FilePath& db_name) { diff --git a/chromium/components/history/core/browser/top_sites_database.h b/chromium/components/history/core/browser/top_sites_database.h index 3556db1d20e..981d2d31b59 100644 --- a/chromium/components/history/core/browser/top_sites_database.h +++ b/chromium/components/history/core/browser/top_sites_database.h @@ -12,6 +12,7 @@ #include "base/macros.h" #include "components/history/core/browser/history_types.h" #include "sql/meta_table.h" +#include "sql/transaction.h" namespace base { class FilePath; @@ -32,31 +33,22 @@ class TopSitesDatabase { // Returns true on success. If false, no other functions should be called. bool Init(const base::FilePath& db_name); - // Thumbnails ---------------------------------------------------------------- + // Updates the database according to the changes recorded in |delta|. + void ApplyDelta(const TopSitesDelta& delta); // Returns a list of all URLs currently in the table. // WARNING: clears both input arguments. void GetPageThumbnails(MostVisitedURLList* urls, std::map<GURL, Images>* thumbnails); - // Set a thumbnail for a URL. |url_rank| is the position of the URL + // Sets a thumbnail for a URL. |new_rank| is the position of the URL // in the list of TopURLs, zero-based. - // If the URL is not in the table, add it. If it is, replace its - // thumbnail and rank. Shift the ranks of other URLs if necessary. + // If the URL is not in the table, add it. If it is, replaces its + // thumbnail and rank. Shifts the ranks of other URLs if necessary. void SetPageThumbnail(const MostVisitedURL& url, int new_rank, const Images& thumbnail); - // Sets the rank for a given URL. The URL must be in the database. - // Use SetPageThumbnail if it's not. - void UpdatePageRank(const MostVisitedURL& url, int new_rank); - - // Get a thumbnail for a given page. Returns true iff we have the thumbnail. - bool GetPageThumbnail(const GURL& url, Images* thumbnail); - - // Remove the record for this URL. Returns true iff removed successfully. - bool RemoveURL(const MostVisitedURL& url); - private: FRIEND_TEST_ALL_PREFIXES(TopSitesDatabaseTest, Version1); FRIEND_TEST_ALL_PREFIXES(TopSitesDatabaseTest, Version2); @@ -77,13 +69,22 @@ class TopSitesDatabase { // upgrade was successful. bool UpgradeToVersion3(); + // Sets a thumbnail for a URL. |new_rank| is the position of the URL + // in the list of TopURLs, zero-based. + // If the URL is not in the table, add it. If it is, replaces its + // thumbnail and rank. Shifts the ranks of other URLs if necessary. + // Should be called within an open transaction. + void SetPageThumbnailNoTransaction(const MostVisitedURL& url, + int new_rank, + const Images& thumbnail); + // Adds a new URL to the database. void AddPageThumbnail(const MostVisitedURL& url, int new_rank, const Images& thumbnail); - // Sets the page rank. Should be called within an open transaction. - void UpdatePageRankNoTransaction(const MostVisitedURL& url, int new_rank); + // Gets a thumbnail for a given page. Returns true iff we have the thumbnail. + bool GetPageThumbnail(const GURL& url, Images* thumbnail); // Updates thumbnail of a URL that's already in the database. // Returns true if the database query succeeds. @@ -92,6 +93,15 @@ class TopSitesDatabase { // Returns |url|'s current rank or kRankOfNonExistingURL if not present. int GetURLRank(const MostVisitedURL& url); + // Sets the rank for a given URL. The URL must be in the database. + // Uses SetPageThumbnail if it's not. Should be called within an open + // transaction. + void UpdatePageRankNoTransaction(const MostVisitedURL& url, int new_rank); + + // Removes the record for this URL. Returns false iff there is a failure in + // running the statement. Should be called within an open transaction. + bool RemoveURLNoTransaction(const MostVisitedURL& url); + // Helper function to implement internals of Init(). This allows // Init() to retry in case of failure, since some failures will // invoke recovery code. diff --git a/chromium/components/history/core/browser/top_sites_database_unittest.cc b/chromium/components/history/core/browser/top_sites_database_unittest.cc index 29683551742..4788bc197fd 100644 --- a/chromium/components/history/core/browser/top_sites_database_unittest.cc +++ b/chromium/components/history/core/browser/top_sites_database_unittest.cc @@ -118,7 +118,11 @@ TEST_F(TopSitesDatabaseTest, Version3) { EXPECT_TRUE(!memcmp(thumbnails[urls[0].url].thumbnail->front(), kGoogleThumbnail, sizeof(kGoogleThumbnail) - 1)); - ASSERT_TRUE(db.RemoveURL(urls[1])); + sql::Transaction transaction(db.db_.get()); + transaction.Begin(); + ASSERT_TRUE(db.RemoveURLNoTransaction(urls[1])); + transaction.Commit(); + db.GetPageThumbnails(&urls, &thumbnails); ASSERT_EQ(2u, urls.size()); ASSERT_EQ(2u, thumbnails.size()); @@ -370,9 +374,13 @@ TEST_F(TopSitesDatabaseTest, AddRemoveEditThumbnails) { EXPECT_EQ(kUrl0, urls[2].url); EXPECT_EQ(mapsUrl, urls[3].url); - // Change a non-forced URL to forced using UpdatePageRank. + sql::Transaction transaction(db.db_.get()); + + // Change a non-forced URL to forced using UpdatePageRankNoTransaction. url1.last_forced_time = base::Time::FromJsTime(792219600000); // 8/2/1995 - db.UpdatePageRank(url1, TopSitesDatabase::kRankOfForcedURL); + transaction.Begin(); + db.UpdatePageRankNoTransaction(url1, TopSitesDatabase::kRankOfForcedURL); + transaction.Commit(); db.GetPageThumbnails(&urls, &thumbnails); ASSERT_EQ(6u, urls.size()); @@ -394,8 +402,11 @@ TEST_F(TopSitesDatabaseTest, AddRemoveEditThumbnails) { EXPECT_EQ(kUrl0, urls[2].url); EXPECT_EQ(plusUrl, urls[3].url); // Plus moves to second non-forced URL. - // Change a non-forced URL to earlier non-forced using UpdatePageRank. - db.UpdatePageRank(url3, 0); + // Change a non-forced URL to earlier non-forced using + // UpdatePageRankNoTransaction. + transaction.Begin(); + db.UpdatePageRankNoTransaction(url3, 0); + transaction.Commit(); db.GetPageThumbnails(&urls, &thumbnails); ASSERT_EQ(6u, urls.size()); @@ -417,7 +428,9 @@ TEST_F(TopSitesDatabaseTest, AddRemoveEditThumbnails) { EXPECT_EQ(plusUrl, urls[4].url); // Plus moves to third non-forced URL. // Remove a non-forced URL. - db.RemoveURL(url3); + transaction.Begin(); + ASSERT_TRUE(db.RemoveURLNoTransaction(url3)); + transaction.Commit(); db.GetPageThumbnails(&urls, &thumbnails); ASSERT_EQ(5u, urls.size()); @@ -427,7 +440,9 @@ TEST_F(TopSitesDatabaseTest, AddRemoveEditThumbnails) { EXPECT_EQ(kUrl0, urls[2].url); // Remove a forced URL. - db.RemoveURL(url2); + transaction.Begin(); + ASSERT_TRUE(db.RemoveURLNoTransaction(url2)); + transaction.Commit(); db.GetPageThumbnails(&urls, &thumbnails); ASSERT_EQ(4u, urls.size()); @@ -436,4 +451,43 @@ TEST_F(TopSitesDatabaseTest, AddRemoveEditThumbnails) { EXPECT_EQ(kUrl0, urls[1].url); } +TEST_F(TopSitesDatabaseTest, ApplyDelta) { + ASSERT_TRUE(CreateDatabaseFromSQL(file_name_, "TopSites.v3.sql")); + + TopSitesDatabase db; + ASSERT_TRUE(db.Init(file_name_)); + + GURL mapsUrl = GURL("http://maps.google.com/"); + + TopSitesDelta delta; + // Delete kUrl0. Now db has kUrl1 and kUrl2. + MostVisitedURL url_to_delete(kUrl0, base::ASCIIToUTF16("Google")); + delta.deleted.push_back(url_to_delete); + + // Add a new URL, not forced, rank = 0. Now db has mapsUrl, kUrl1 and kUrl2. + MostVisitedURLWithRank url_to_add; + url_to_add.url = MostVisitedURL(mapsUrl, base::ASCIIToUTF16("Google Maps")); + url_to_add.rank = 0; + delta.added.push_back(url_to_add); + + // Move kUrl1 by updating its rank to 2. Now db has mapsUrl, kUrl2 and kUrl1. + MostVisitedURLWithRank url_to_move; + url_to_move.url = MostVisitedURL(kUrl1, base::ASCIIToUTF16("Google Chrome")); + url_to_move.rank = 2; + delta.moved.push_back(url_to_move); + + // Update db. + db.ApplyDelta(delta); + + // Read db and verify. + MostVisitedURLList urls; + std::map<GURL, Images> thumbnails; + db.GetPageThumbnails(&urls, &thumbnails); + ASSERT_EQ(3u, urls.size()); + ASSERT_EQ(3u, thumbnails.size()); + EXPECT_EQ(mapsUrl, urls[0].url); + EXPECT_EQ(kUrl2, urls[1].url); + EXPECT_EQ(kUrl1, urls[2].url); +} + } // namespace history diff --git a/chromium/components/history/core/browser/top_sites_impl.cc b/chromium/components/history/core/browser/top_sites_impl.cc index aa758327087..daf38913134 100644 --- a/chromium/components/history/core/browser/top_sites_impl.cc +++ b/chromium/components/history/core/browser/top_sites_impl.cc @@ -65,28 +65,39 @@ bool ForcedURLComparator(const MostVisitedURL& first, return first.last_forced_time < second.last_forced_time; } -// How many non-forced top sites to store in the cache. -const size_t kNonForcedTopSitesNumber = 20; +// Checks if the titles stored in |old_list| and |new_list| have changes. +bool DoTitlesDiffer(const MostVisitedURLList& old_list, + const MostVisitedURLList& new_list) { + // If the two lists have different sizes, the most visited titles are + // considered to have changes. + if (old_list.size() != new_list.size()) + return true; -// How many forced top sites to store in the cache. -const size_t kForcedTopSitesNumber = 20; + return !std::equal(std::begin(old_list), std::end(old_list), + std::begin(new_list), + [](const auto& old_item_ptr, const auto& new_item_ptr) { + return old_item_ptr.title == new_item_ptr.title; + }); +} // Max number of temporary images we'll cache. See comment above // temp_images_ for details. const size_t kMaxTempTopImages = 8; const int kDaysOfHistory = 90; -// Time from startup to first HistoryService query. -const int64_t kUpdateIntervalSecs = 15; -// Intervals between requests to HistoryService. -const int64_t kMinUpdateIntervalMinutes = 1; + +// The delay for the first HistoryService query at startup. +constexpr base::TimeDelta kFirstDelayAtStartup = + base::TimeDelta::FromSeconds(15); + +// The delay for the all HistoryService queries other than the first one. #if defined(OS_IOS) || defined(OS_ANDROID) // On mobile, having the max at 60 minutes results in the topsites database // being not updated often enough since the app isn't usually running for long // stretches of time. -const int64_t kMaxUpdateIntervalMinutes = 5; +constexpr base::TimeDelta kDelayForUpdates = base::TimeDelta::FromMinutes(5); #else -const int64_t kMaxUpdateIntervalMinutes = 60; +constexpr base::TimeDelta kDelayForUpdates = base::TimeDelta::FromMinutes(60); #endif // defined(OS_IOS) || defined(OS_ANDROID) // Use 100 quality (highest quality) because we're very sensitive to @@ -109,7 +120,6 @@ TopSitesImpl::TopSitesImpl(PrefService* pref_service, : backend_(nullptr), cache_(base::MakeUnique<TopSitesCache>()), thread_safe_cache_(base::MakeUnique<TopSitesCache>()), - last_num_urls_changed_(0), prepopulated_pages_(prepopulated_pages), pref_service_(pref_service), history_service_(history_service), @@ -198,7 +208,7 @@ bool TopSitesImpl::GetPageThumbnail( for (const auto& prepopulated_page : prepopulated_pages_) { if (url == prepopulated_page.most_visited.url) { *bytes = - ResourceBundle::GetSharedInstance().LoadDataResourceBytesForScale( + ui::ResourceBundle::GetSharedInstance().LoadDataResourceBytesForScale( prepopulated_page.thumbnail_id, ui::SCALE_FACTOR_100P); return true; } @@ -256,10 +266,8 @@ static int IndexOf(const MostVisitedURLList& urls, const GURL& url) { void TopSitesImpl::SyncWithHistory() { DCHECK(thread_checker_.CalledOnValidThread()); - if (loaded_) { - timer_.Stop(); + if (loaded_) StartQueryForMostVisited(); - } } bool TopSitesImpl::HasBlacklistedItems() const { @@ -373,14 +381,11 @@ bool TopSitesImpl::AddForcedURL(const GURL& url, const base::Time& time) { void TopSitesImpl::OnNavigationCommitted(const GURL& url) { DCHECK(thread_checker_.CalledOnValidThread()); - if (!loaded_ || IsNonForcedFull()) + if (!loaded_) return; - if (!cache_->IsKnownURL(url) && can_add_url_to_history_.Run(url)) { - // To avoid slamming history we throttle requests when the url updates. To - // do otherwise negatively impacts perf tests. - RestartQueryForTopSitesTimer(GetUpdateDelay()); - } + if (can_add_url_to_history_.Run(url)) + ScheduleUpdateTimer(); } void TopSitesImpl::ShutdownOnUIThread() { @@ -403,6 +408,8 @@ TopSitesImpl::~TopSitesImpl() = default; void TopSitesImpl::StartQueryForMostVisited() { DCHECK(loaded_); + timer_.Stop(); + if (!history_service_) return; @@ -604,10 +611,6 @@ void TopSitesImpl::AddTemporaryThumbnail(const GURL& url, temp_images_.push_back(image); } -void TopSitesImpl::TimerFired() { - StartQueryForMostVisited(); -} - // static int TopSitesImpl::GetRedirectDistanceForURL(const MostVisitedURL& most_visited, const GURL& url) { @@ -639,9 +642,9 @@ size_t TopSitesImpl::MergeCachedForcedURLs(MostVisitedURLList* new_list) const { std::set<GURL> all_new_urls; size_t num_forced = 0; for (size_t i = 0; i < new_list->size(); ++i) { - for (size_t j = 0; j < (*new_list)[i].redirects.size(); j++) { + for (size_t j = 0; j < (*new_list)[i].redirects.size(); j++) all_new_urls.insert((*new_list)[i].redirects[j]); - } + if (!(*new_list)[i].last_forced_time.is_null()) ++num_forced; } @@ -708,16 +711,6 @@ std::string TopSitesImpl::GetURLHash(const GURL& url) { return base::MD5String(url.spec()); } -base::TimeDelta TopSitesImpl::GetUpdateDelay() const { - if (cache_->top_sites().size() <= prepopulated_pages_.size()) - return base::TimeDelta::FromSeconds(30); - - int64_t range = kMaxUpdateIntervalMinutes - kMinUpdateIntervalMinutes; - int64_t minutes = kMaxUpdateIntervalMinutes - - last_num_urls_changed_ * range / cache_->top_sites().size(); - return base::TimeDelta::FromMinutes(minutes); -} - void TopSitesImpl::SetTopSites(const MostVisitedURLList& new_top_sites, const CallLocation location) { DCHECK(thread_checker_.CalledOnValidThread()); @@ -746,11 +739,16 @@ void TopSitesImpl::SetTopSites(const MostVisitedURLList& new_top_sites, histogram_recorded_ = true; } + bool should_notify_observers = false; + // If there is a change in urls, update the db and notify observers. if (!delta.deleted.empty() || !delta.added.empty() || !delta.moved.empty()) { backend_->UpdateTopSites(delta, record_or_not); + should_notify_observers = true; } - - last_num_urls_changed_ = delta.added.size() + delta.moved.size(); + // If there is no url change in top sites, check if the titles have changes. + // Notify observers if there's a change in titles. + if (!should_notify_observers) + should_notify_observers = DoTitlesDiffer(cache_->top_sites(), top_sites); // We always do the following steps (setting top sites in cache, and resetting // thread safe cache ...) as this method is invoked during startup at which @@ -789,14 +787,14 @@ void TopSitesImpl::SetTopSites(const MostVisitedURLList& new_top_sites, ResetThreadSafeCache(); ResetThreadSafeImageCache(); - if (location == CALL_LOCATION_FROM_FORCED_URLS) - NotifyTopSitesChanged(TopSitesObserver::ChangeReason::FORCED_URL); - else - NotifyTopSitesChanged(TopSitesObserver::ChangeReason::MOST_VISITED); - // Restart the timer that queries history for top sites. This is done to - // ensure we stay in sync with history. - RestartQueryForTopSitesTimer(GetUpdateDelay()); + if (should_notify_observers) { + if (location == CALL_LOCATION_FROM_FORCED_URLS) + NotifyTopSitesChanged(TopSitesObserver::ChangeReason::FORCED_URL); + else + NotifyTopSitesChanged(TopSitesObserver::ChangeReason::MOST_VISITED); + } + } int TopSitesImpl::num_results_to_request_from_history() const { @@ -854,15 +852,12 @@ void TopSitesImpl::ResetThreadSafeImageCache() { thread_safe_cache_->SetThumbnails(cache_->images()); } -void TopSitesImpl::RestartQueryForTopSitesTimer(base::TimeDelta delta) { - if (timer_.IsRunning() && ((timer_start_time_ + timer_.GetCurrentDelay()) < - (base::TimeTicks::Now() + delta))) { +void TopSitesImpl::ScheduleUpdateTimer() { + if (timer_.IsRunning()) return; - } - timer_start_time_ = base::TimeTicks::Now(); - timer_.Stop(); - timer_.Start(FROM_HERE, delta, this, &TopSitesImpl::TimerFired); + timer_.Start(FROM_HERE, kDelayForUpdates, this, + &TopSitesImpl::StartQueryForMostVisited); } void TopSitesImpl::OnGotMostVisitedThumbnails( @@ -881,8 +876,8 @@ void TopSitesImpl::OnGotMostVisitedThumbnails( MoveStateToLoaded(); // Start a timer that refreshes top sites from history. - RestartQueryForTopSitesTimer( - base::TimeDelta::FromSeconds(kUpdateIntervalSecs)); + timer_.Start(FROM_HERE, kFirstDelayAtStartup, this, + &TopSitesImpl::StartQueryForMostVisited); } void TopSitesImpl::OnTopSitesAvailableFromHistory( diff --git a/chromium/components/history/core/browser/top_sites_impl.h b/chromium/components/history/core/browser/top_sites_impl.h index e8d10234207..7e6cb3581c9 100644 --- a/chromium/components/history/core/browser/top_sites_impl.h +++ b/chromium/components/history/core/browser/top_sites_impl.h @@ -57,6 +57,12 @@ class TopSitesImpl : public TopSites, public HistoryServiceObserver { // callable multiple time and during the whole lifetime of TopSitesImpl. using CanAddURLToHistoryFn = base::Callback<bool(const GURL&)>; + // How many non-forced top sites to store in the cache. + static constexpr size_t kNonForcedTopSitesNumber = 10; + + // How many forced top sites to store in the cache. + static constexpr size_t kForcedTopSitesNumber = 10; + TopSitesImpl(PrefService* pref_service, HistoryService* history_service, const PrepopulatedPageList& prepopulated_pages, @@ -143,6 +149,9 @@ class TopSitesImpl : public TopSites, public HistoryServiceObserver { typedef std::list<TempImage> TempImages; typedef std::vector<PendingCallback> PendingCallbacks; + // Starts to query most visited URLs from history database instantly. + // Also cancels any pending queries requested in a delayed manner by + // cancelling the timer. void StartQueryForMostVisited(); // Generates the diff of things that happened between "old" and "new." @@ -188,26 +197,23 @@ class TopSitesImpl : public TopSites, public HistoryServiceObserver { // in |temp_images_|. void RemoveTemporaryThumbnailByURL(const GURL& url); - // Add a thumbnail for an unknown url. See |temp_images_|. + // Adds a thumbnail for an unknown url. See |temp_images_|. void AddTemporaryThumbnail(const GURL& url, base::RefCountedMemory* thumbnail, const ThumbnailScore& score); - // Called by our timer. Starts the query for the most visited sites. - void TimerFired(); - // Finds the given URL in the redirect chain for the given TopSite, and // returns the distance from the destination in hops that the given URL is. // The URL is assumed to be in the list. The destination is 0. static int GetRedirectDistanceForURL(const MostVisitedURL& most_visited, const GURL& url); - // Add prepopulated pages: 'welcome to Chrome' and themes gallery to |urls|. + // Adds prepopulated pages: 'welcome to Chrome' and themes gallery to |urls|. // Returns true if any pages were added. bool AddPrepopulatedPages(MostVisitedURLList* urls, size_t num_forced_urls) const; - // Add all the forced URLs from |cache_| into |new_list|, making sure not to + // Adds all the forced URLs from |cache_| into |new_list|, making sure not to // add any URL that's already in |new_list|'s non-forced URLs. The forced URLs // in |cache_| and |new_list| are assumed to appear at the front of the list // and be sorted in increasing |last_forced_time|. This will still be true @@ -223,10 +229,6 @@ class TopSitesImpl : public TopSites, public HistoryServiceObserver { // Returns an MD5 hash of the URL. Hashing is required for blacklisted URLs. static std::string GetURLHash(const GURL& url); - // Returns the delay until the next update of history is needed. - // Uses |last_num_urls_changed_|. - base::TimeDelta GetUpdateDelay() const; - // Updates URLs in |cache_| and the db (in the background). // The non-forced URLs in |new_top_sites| replace those in |cache_|. // The forced URLs of |new_top_sites| are merged with those in |cache_|, @@ -249,8 +251,9 @@ class TopSitesImpl : public TopSites, public HistoryServiceObserver { void ResetThreadSafeImageCache(); - // Stops and starts timer with a delay of |delta|. - void RestartQueryForTopSitesTimer(base::TimeDelta delta); + // Schedules a timer to update top sites with a delay. + // Does nothing if there is already a request queued. + void ScheduleUpdateTimer(); // Callback from TopSites with the top sites/thumbnails. Should be called // from the UI thread. @@ -286,16 +289,10 @@ class TopSitesImpl : public TopSites, public HistoryServiceObserver { // Task tracker for history and backend requests. base::CancelableTaskTracker cancelable_task_tracker_; - // Timer that asks history for the top sites. This is used to make sure our - // data stays in sync with history. + // Timer that asks history for the top sites. This is used to coalesce + // requests that are generated in quick succession. base::OneShotTimer timer_; - // The time we started |timer_| at. Only valid if |timer_| is running. - base::TimeTicks timer_start_time_; - - // The number of URLs changed on the last update. - size_t last_num_urls_changed_; - // The pending requests for the top sites list. Can only be non-empty at // startup. After we read the top sites from the DB, we'll always have a // cached list and be able to run callbacks immediately. diff --git a/chromium/components/history/core/browser/top_sites_impl_unittest.cc b/chromium/components/history/core/browser/top_sites_impl_unittest.cc index 00d1e10793b..682116d8c2c 100644 --- a/chromium/components/history/core/browser/top_sites_impl_unittest.cc +++ b/chromium/components/history/core/browser/top_sites_impl_unittest.cc @@ -255,14 +255,6 @@ class TopSitesImplTest : public HistoryUnitTestBase { void StartQueryForMostVisited() { top_sites()->StartQueryForMostVisited(); } - void SetLastNumUrlsChanged(size_t value) { - top_sites()->last_num_urls_changed_ = value; - } - - size_t last_num_urls_changed() { return top_sites()->last_num_urls_changed_; } - - base::TimeDelta GetUpdateDelay() { return top_sites()->GetUpdateDelay(); } - bool IsTopSitesLoaded() { return top_sites()->loaded_; } bool AddPrepopulatedPages(MostVisitedURLList* urls) { @@ -326,8 +318,7 @@ class TopSitesImplTest : public HistoryUnitTestBase { // Helper function for appending a URL to a vector of "most visited" URLs, // using the default values for everything but the URL. -static void AppendMostVisitedURL(std::vector<MostVisitedURL>* list, - const GURL& url) { +void AppendMostVisitedURL(const GURL& url, std::vector<MostVisitedURL>* list) { MostVisitedURL mv; mv.url = url; mv.redirects.push_back(url); @@ -336,9 +327,9 @@ static void AppendMostVisitedURL(std::vector<MostVisitedURL>* list, // Helper function for appending a URL to a vector of "most visited" URLs, // using the default values for everything but the URL. -static void AppendForcedMostVisitedURL(std::vector<MostVisitedURL>* list, - const GURL& url, - double last_forced_time) { +void AppendForcedMostVisitedURL(const GURL& url, + double last_forced_time, + std::vector<MostVisitedURL>* list) { MostVisitedURL mv; mv.url = url; mv.last_forced_time = base::Time::FromJsTime(last_forced_time); @@ -348,9 +339,9 @@ static void AppendForcedMostVisitedURL(std::vector<MostVisitedURL>* list, // Same as AppendMostVisitedURL except that it adds a redirect from the first // URL to the second. -static void AppendMostVisitedURLWithRedirect(std::vector<MostVisitedURL>* list, - const GURL& redirect_source, - const GURL& redirect_dest) { +void AppendMostVisitedURLWithRedirect(const GURL& redirect_source, + const GURL& redirect_dest, + std::vector<MostVisitedURL>* list) { MostVisitedURL mv; mv.url = redirect_dest; mv.redirects.push_back(redirect_source); @@ -358,6 +349,18 @@ static void AppendMostVisitedURLWithRedirect(std::vector<MostVisitedURL>* list, list->push_back(mv); } +// Helper function for appending a URL to a vector of "most visited" URLs, +// using the default values for everything but the URL and the title. +void AppendMostVisitedURLwithTitle(const GURL& url, + const base::string16& title, + std::vector<MostVisitedURL>* list) { + MostVisitedURL mv; + mv.url = url; + mv.title = title; + mv.redirects.push_back(url); + list->push_back(mv); +} + // Tests GetCanonicalURL. TEST_F(TopSitesImplTest, GetCanonicalURL) { // Have two chains: @@ -368,8 +371,8 @@ TEST_F(TopSitesImplTest, GetCanonicalURL) { GURL dest("http://www.google.com/"); std::vector<MostVisitedURL> most_visited; - AppendMostVisitedURLWithRedirect(&most_visited, source, dest); - AppendMostVisitedURL(&most_visited, news); + AppendMostVisitedURLWithRedirect(source, dest, &most_visited); + AppendMostVisitedURL(news, &most_visited); SetTopSites(most_visited); // Random URLs not in the database are returned unchanged. @@ -389,6 +392,70 @@ TEST_F(TopSitesImplTest, GetCanonicalURL) { EXPECT_EQ(dest, result); } +class MockTopSitesObserver : public TopSitesObserver { + public: + MockTopSitesObserver() {} + + // history::TopSitesObserver: + void TopSitesLoaded(TopSites* top_sites) override {} + void TopSitesChanged(TopSites* top_sites, + ChangeReason change_reason) override { + is_notified_ = true; + } + + void ResetIsNotifiedState() { is_notified_ = false; } + bool is_notified() const { return is_notified_; } + + private: + bool is_notified_ = false; + + DISALLOW_COPY_AND_ASSIGN(MockTopSitesObserver); +}; + +// Tests DoTitlesDiffer. +TEST_F(TopSitesImplTest, DoTitlesDiffer) { + GURL url_1("http://url1/"); + GURL url_2("http://url2/"); + base::string16 title_1(base::ASCIIToUTF16("title1")); + base::string16 title_2(base::ASCIIToUTF16("title2")); + + MockTopSitesObserver observer; + top_sites()->AddObserver(&observer); + + // TopSites has a new list of sites and should notify its observers. + std::vector<MostVisitedURL> list_1; + AppendMostVisitedURLwithTitle(url_1, title_1, &list_1); + SetTopSites(list_1); + EXPECT_TRUE(observer.is_notified()); + observer.ResetIsNotifiedState(); + EXPECT_FALSE(observer.is_notified()); + + // list_1 and list_2 have different sizes. TopSites should notify its + // observers. + std::vector<MostVisitedURL> list_2; + AppendMostVisitedURLwithTitle(url_1, title_1, &list_2); + AppendMostVisitedURLwithTitle(url_2, title_2, &list_2); + SetTopSites(list_2); + EXPECT_TRUE(observer.is_notified()); + observer.ResetIsNotifiedState(); + EXPECT_FALSE(observer.is_notified()); + + // list_1 and list_2 are exactly the same now. TopSites should not notify its + // observers. + AppendMostVisitedURLwithTitle(url_2, title_2, &list_1); + SetTopSites(list_1); + EXPECT_FALSE(observer.is_notified()); + + // Change |url_2|'s title to |title_1| in list_2. The two lists are different + // in titles now. TopSites should notify its observers. + list_2.pop_back(); + AppendMostVisitedURLwithTitle(url_2, title_1, &list_2); + SetTopSites(list_2); + EXPECT_TRUE(observer.is_notified()); + + top_sites()->RemoveObserver(&observer); +} + // Tests DiffMostVisited. TEST_F(TopSitesImplTest, DiffMostVisited) { GURL stays_the_same("http://staysthesame/"); @@ -398,18 +465,18 @@ TEST_F(TopSitesImplTest, DiffMostVisited) { GURL gets_moved_1("http://getsmoved1/"); std::vector<MostVisitedURL> old_list; - AppendMostVisitedURL(&old_list, stays_the_same); // 0 (unchanged) - AppendMostVisitedURL(&old_list, gets_deleted_1); // 1 (deleted) - AppendMostVisitedURL(&old_list, gets_moved_1); // 2 (moved to 3) + AppendMostVisitedURL(stays_the_same, &old_list); // 0 (unchanged) + AppendMostVisitedURL(gets_deleted_1, &old_list); // 1 (deleted) + AppendMostVisitedURL(gets_moved_1, &old_list); // 2 (moved to 3) std::vector<MostVisitedURL> new_list; - AppendMostVisitedURL(&new_list, stays_the_same); // 0 (unchanged) - AppendMostVisitedURL(&new_list, gets_added_1); // 1 (added) - AppendMostVisitedURL(&new_list, gets_added_2); // 2 (added) - AppendMostVisitedURL(&new_list, gets_moved_1); // 3 (moved from 2) + AppendMostVisitedURL(stays_the_same, &new_list); // 0 (unchanged) + AppendMostVisitedURL(gets_added_1, &new_list); // 1 (added) + AppendMostVisitedURL(gets_added_2, &new_list); // 2 (added) + AppendMostVisitedURL(gets_moved_1, &new_list); // 3 (moved from 2) history::TopSitesDelta delta; - history::TopSitesImpl::DiffMostVisited(old_list, new_list, &delta); + TopSitesImpl::DiffMostVisited(old_list, new_list, &delta); ASSERT_EQ(2u, delta.added.size()); EXPECT_TRUE(gets_added_1 == delta.added[0].url.url); @@ -442,29 +509,29 @@ TEST_F(TopSitesImplTest, DiffMostVisitedWithForced) { GURL gets_moved_1("http://getsmoved1/"); std::vector<MostVisitedURL> old_list; - AppendForcedMostVisitedURL(&old_list, stays_the_same_1, 1000); - AppendForcedMostVisitedURL(&old_list, new_last_forced_time, 2000); - AppendForcedMostVisitedURL(&old_list, stays_the_same_2, 3000); - AppendForcedMostVisitedURL(&old_list, move_to_nonforced, 4000); - AppendForcedMostVisitedURL(&old_list, gets_deleted_1, 5000); - AppendMostVisitedURL(&old_list, move_to_forced); - AppendMostVisitedURL(&old_list, stays_the_same_3); - AppendMostVisitedURL(&old_list, gets_deleted_2); - AppendMostVisitedURL(&old_list, gets_moved_1); + AppendForcedMostVisitedURL(stays_the_same_1, 1000, &old_list); + AppendForcedMostVisitedURL(new_last_forced_time, 2000, &old_list); + AppendForcedMostVisitedURL(stays_the_same_2, 3000, &old_list); + AppendForcedMostVisitedURL(move_to_nonforced, 4000, &old_list); + AppendForcedMostVisitedURL(gets_deleted_1, 5000, &old_list); + AppendMostVisitedURL(move_to_forced, &old_list); + AppendMostVisitedURL(stays_the_same_3, &old_list); + AppendMostVisitedURL(gets_deleted_2, &old_list); + AppendMostVisitedURL(gets_moved_1, &old_list); std::vector<MostVisitedURL> new_list; - AppendForcedMostVisitedURL(&new_list, stays_the_same_1, 1000); - AppendForcedMostVisitedURL(&new_list, stays_the_same_2, 3000); - AppendForcedMostVisitedURL(&new_list, new_last_forced_time, 4000); - AppendForcedMostVisitedURL(&new_list, gets_added_1, 5000); - AppendForcedMostVisitedURL(&new_list, move_to_forced, 6000); - AppendMostVisitedURL(&new_list, move_to_nonforced); - AppendMostVisitedURL(&new_list, stays_the_same_3); - AppendMostVisitedURL(&new_list, gets_added_2); - AppendMostVisitedURL(&new_list, gets_moved_1); - - history::TopSitesDelta delta; - history::TopSitesImpl::DiffMostVisited(old_list, new_list, &delta); + AppendForcedMostVisitedURL(stays_the_same_1, 1000, &new_list); + AppendForcedMostVisitedURL(stays_the_same_2, 3000, &new_list); + AppendForcedMostVisitedURL(new_last_forced_time, 4000, &new_list); + AppendForcedMostVisitedURL(gets_added_1, 5000, &new_list); + AppendForcedMostVisitedURL(move_to_forced, 6000, &new_list); + AppendMostVisitedURL(move_to_nonforced, &new_list); + AppendMostVisitedURL(stays_the_same_3, &new_list); + AppendMostVisitedURL(gets_added_2, &new_list); + AppendMostVisitedURL(gets_moved_1, &new_list); + + TopSitesDelta delta; + TopSitesImpl::DiffMostVisited(old_list, new_list, &delta); ASSERT_EQ(2u, delta.added.size()); EXPECT_TRUE(gets_added_1 == delta.added[0].url.url); @@ -496,7 +563,7 @@ TEST_F(TopSitesImplTest, SetPageThumbnail) { GURL invalid_url("application://favicon/http://google.com/"); std::vector<MostVisitedURL> list; - AppendMostVisitedURL(&list, url2); + AppendMostVisitedURL(url2, &list); MostVisitedURL mv; mv.url = url1b; @@ -539,7 +606,7 @@ TEST_F(TopSitesImplTest, ThumbnailRemoved) { // Configure top sites with 'google.com'. std::vector<MostVisitedURL> list; - AppendMostVisitedURL(&list, url); + AppendMostVisitedURL(url, &list); SetTopSites(list); // Create a dummy thumbnail. @@ -740,11 +807,11 @@ TEST_F(TopSitesImplTest, SaveForcedToDB) { // Add a number of forced URLs. std::vector<MostVisitedURL> list; - AppendForcedMostVisitedURL(&list, GURL("http://forced1"), 1000); + AppendForcedMostVisitedURL(GURL("http://forced1"), 1000, &list); list[0].title = base::ASCIIToUTF16("forced1"); - AppendForcedMostVisitedURL(&list, GURL("http://forced2"), 2000); - AppendForcedMostVisitedURL(&list, GURL("http://forced3"), 3000); - AppendForcedMostVisitedURL(&list, GURL("http://forced4"), 4000); + AppendForcedMostVisitedURL(GURL("http://forced2"), 2000, &list); + AppendForcedMostVisitedURL(GURL("http://forced3"), 3000, &list); + AppendForcedMostVisitedURL(GURL("http://forced4"), 4000, &list); SetTopSites(list); // Add a thumbnail. @@ -972,43 +1039,6 @@ TEST_F(TopSitesImplTest, DeleteNotifications) { } } -// Makes sure GetUpdateDelay is updated appropriately. -TEST_F(TopSitesImplTest, GetUpdateDelay) { -#if defined(OS_IOS) || defined(OS_ANDROID) - const int64_t kExpectedUpdateDelayInSecondEmpty = 30; - const int64_t kExpectedUpdateDelayInMinute0Changed = 5; - const int64_t kExpectedUpdateDelayInMinute3Changed = 5; - const int64_t kExpectedUpdateDelayInMinute20Changed = 1; -#else - const int64_t kExpectedUpdateDelayInSecondEmpty = 30; - const int64_t kExpectedUpdateDelayInMinute0Changed = 60; - const int64_t kExpectedUpdateDelayInMinute3Changed = 52; - const int64_t kExpectedUpdateDelayInMinute20Changed = 1; -#endif - - SetLastNumUrlsChanged(0); - EXPECT_EQ(kExpectedUpdateDelayInSecondEmpty, GetUpdateDelay().InSeconds()); - - MostVisitedURLList url_list; - url_list.resize(20); - GURL tmp_url(GURL("http://x")); - for (size_t i = 0; i < url_list.size(); ++i) { - url_list[i].url = tmp_url; - url_list[i].redirects.push_back(tmp_url); - } - SetTopSites(url_list); - EXPECT_EQ(20u, last_num_urls_changed()); - SetLastNumUrlsChanged(0); - EXPECT_EQ(kExpectedUpdateDelayInMinute0Changed, GetUpdateDelay().InMinutes()); - - SetLastNumUrlsChanged(3); - EXPECT_EQ(kExpectedUpdateDelayInMinute3Changed, GetUpdateDelay().InMinutes()); - - SetLastNumUrlsChanged(20); - EXPECT_EQ(kExpectedUpdateDelayInMinute20Changed, - GetUpdateDelay().InMinutes()); -} - // Verifies that callbacks are notified correctly if requested before top sites // has loaded. TEST_F(TopSitesImplTest, NotifyCallbacksWhenLoaded) { @@ -1351,59 +1381,55 @@ TEST_F(TopSitesImplTest, AddPrepopulatedPages) { TEST_F(TopSitesImplTest, SetForcedTopSites) { // Create forced elements in old URL list. MostVisitedURLList old_url_list; - AppendForcedMostVisitedURL(&old_url_list, GURL("http://oldforced/0"), 1000); - AppendForcedMostVisitedURL(&old_url_list, GURL("http://oldforced/1"), 4000); - AppendForcedMostVisitedURL(&old_url_list, GURL("http://oldforced/2"), 7000); - AppendForcedMostVisitedURL(&old_url_list, GURL("http://oldforced/3"), 10000); - AppendForcedMostVisitedURL(&old_url_list, GURL("http://oldforced/4"), 11000); - AppendForcedMostVisitedURL(&old_url_list, GURL("http://oldforced/5"), 12000); - AppendForcedMostVisitedURL(&old_url_list, GURL("http://oldforced/6"), 13000); - AppendForcedMostVisitedURL(&old_url_list, GURL("http://oldforced/7"), 18000); - AppendForcedMostVisitedURL(&old_url_list, GURL("http://oldforced/8"), 21000); - const size_t kNumOldForcedURLs = 9; + AppendForcedMostVisitedURL(GURL("http://oldforced/0"), 1000, &old_url_list); + AppendForcedMostVisitedURL(GURL("http://oldforced/1"), 4000, &old_url_list); + AppendForcedMostVisitedURL(GURL("http://oldforced/2"), 7000, &old_url_list); + AppendForcedMostVisitedURL(GURL("http://oldforced/3"), 10000, &old_url_list); + AppendForcedMostVisitedURL(GURL("http://oldforced/4"), 11000, &old_url_list); + AppendForcedMostVisitedURL(GURL("http://oldforced/5"), 12000, &old_url_list); + AppendForcedMostVisitedURL(GURL("http://oldforced/6"), 13000, &old_url_list); + + const size_t kNumOldForcedURLs = old_url_list.size(); // Create forced elements in new URL list. MostVisitedURLList new_url_list; - AppendForcedMostVisitedURL(&new_url_list, GURL("http://newforced/0"), 2000); - AppendForcedMostVisitedURL(&new_url_list, GURL("http://newforced/1"), 3000); - AppendForcedMostVisitedURL(&new_url_list, GURL("http://newforced/2"), 5000); - AppendForcedMostVisitedURL(&new_url_list, GURL("http://newforced/3"), 6000); - AppendForcedMostVisitedURL(&new_url_list, GURL("http://newforced/4"), 8000); - AppendForcedMostVisitedURL(&new_url_list, GURL("http://newforced/5"), 9000); - AppendForcedMostVisitedURL(&new_url_list, GURL("http://newforced/6"), 14000); - AppendForcedMostVisitedURL(&new_url_list, GURL("http://newforced/7"), 15000); - AppendForcedMostVisitedURL(&new_url_list, GURL("http://newforced/8"), 16000); - AppendForcedMostVisitedURL(&new_url_list, GURL("http://newforced/9"), 17000); - AppendForcedMostVisitedURL(&new_url_list, GURL("http://newforced/10"), 19000); - AppendForcedMostVisitedURL(&new_url_list, GURL("http://newforced/11"), 20000); - AppendForcedMostVisitedURL(&new_url_list, GURL("http://newforced/12"), 22000); + AppendForcedMostVisitedURL(GURL("http://newforced/0"), 2000, &new_url_list); + AppendForcedMostVisitedURL(GURL("http://newforced/1"), 3000, &new_url_list); + AppendForcedMostVisitedURL(GURL("http://newforced/2"), 5000, &new_url_list); + AppendForcedMostVisitedURL(GURL("http://newforced/3"), 6000, &new_url_list); + AppendForcedMostVisitedURL(GURL("http://newforced/4"), 8000, &new_url_list); + AppendForcedMostVisitedURL(GURL("http://newforced/5"), 9000, &new_url_list); + AppendForcedMostVisitedURL(GURL("http://newforced/6"), 14000, &new_url_list); + AppendForcedMostVisitedURL(GURL("http://newforced/7"), 15000, &new_url_list); + AppendForcedMostVisitedURL(GURL("http://newforced/8"), 16000, &new_url_list); + + const size_t kNonForcedTopSitesCount = TopSitesImpl::kNonForcedTopSitesNumber; + const size_t kForcedTopSitesCount = TopSitesImpl::kForcedTopSitesNumber; // Setup a number non-forced URLs in both old and new list. - const size_t kNumNonForcedURLs = 20; // Maximum number of non-forced URLs. - for (size_t i = 0; i < kNumNonForcedURLs; ++i) { + for (size_t i = 0; i < kNonForcedTopSitesCount; ++i) { std::ostringstream url; url << "http://oldnonforced/" << i; - AppendMostVisitedURL(&old_url_list, GURL(url.str())); + AppendMostVisitedURL(GURL(url.str()), &old_url_list); url.str(""); url << "http://newnonforced/" << i; - AppendMostVisitedURL(&new_url_list, GURL(url.str())); + AppendMostVisitedURL(GURL(url.str()), &new_url_list); } // Set the initial list of URLs. SetTopSites(old_url_list); - EXPECT_EQ(kNumOldForcedURLs + kNumNonForcedURLs, last_num_urls_changed()); TopSitesQuerier querier; // Query only non-forced URLs first. querier.QueryTopSites(top_sites(), false); - ASSERT_EQ(kNumNonForcedURLs, querier.urls().size()); + ASSERT_EQ(kNonForcedTopSitesCount, querier.urls().size()); // Check first URL. EXPECT_EQ("http://oldnonforced/0", querier.urls()[0].url.spec()); // Query all URLs. querier.QueryAllTopSites(top_sites(), false, true); - EXPECT_EQ(kNumOldForcedURLs + kNumNonForcedURLs, querier.urls().size()); + EXPECT_EQ(kNumOldForcedURLs + kNonForcedTopSitesCount, querier.urls().size()); // Check first URLs. EXPECT_EQ("http://oldforced/0", querier.urls()[0].url.spec()); @@ -1416,81 +1442,66 @@ TEST_F(TopSitesImplTest, SetForcedTopSites) { // Query all URLs. querier.QueryAllTopSites(top_sites(), false, true); - // We should have reached the maximum of 20 forced URLs. - ASSERT_EQ(20 + kNumNonForcedURLs, querier.urls().size()); + // We should have reached the maximum of forced URLs. + ASSERT_EQ(kForcedTopSitesCount + kNonForcedTopSitesCount, + querier.urls().size()); // Check forced URLs. They follow the order of timestamps above, smaller // timestamps since they were evicted. - EXPECT_EQ("http://newforced/1", querier.urls()[0].url.spec()); - EXPECT_EQ(3000, querier.urls()[0].last_forced_time.ToJsTime()); - EXPECT_EQ("http://oldforced/1", querier.urls()[1].url.spec()); - EXPECT_EQ(4000, querier.urls()[1].last_forced_time.ToJsTime()); - EXPECT_EQ("http://newforced/2", querier.urls()[2].url.spec()); - EXPECT_EQ(5000, querier.urls()[2].last_forced_time.ToJsTime()); - EXPECT_EQ("http://newforced/3", querier.urls()[3].url.spec()); - EXPECT_EQ(6000, querier.urls()[3].last_forced_time.ToJsTime()); - EXPECT_EQ("http://oldforced/2", querier.urls()[4].url.spec()); - EXPECT_EQ(7000, querier.urls()[4].last_forced_time.ToJsTime()); - EXPECT_EQ("http://newforced/4", querier.urls()[5].url.spec()); - EXPECT_EQ(8000, querier.urls()[5].last_forced_time.ToJsTime()); - EXPECT_EQ("http://newforced/5", querier.urls()[6].url.spec()); - EXPECT_EQ(9000, querier.urls()[6].last_forced_time.ToJsTime()); - EXPECT_EQ("http://oldforced/3", querier.urls()[7].url.spec()); - EXPECT_EQ(10000, querier.urls()[7].last_forced_time.ToJsTime()); - EXPECT_EQ("http://oldforced/4", querier.urls()[8].url.spec()); - EXPECT_EQ(11000, querier.urls()[8].last_forced_time.ToJsTime()); - EXPECT_EQ("http://oldforced/5", querier.urls()[9].url.spec()); - EXPECT_EQ(12000, querier.urls()[9].last_forced_time.ToJsTime()); - EXPECT_EQ("http://oldforced/6", querier.urls()[10].url.spec()); - EXPECT_EQ(13000, querier.urls()[10].last_forced_time.ToJsTime()); - EXPECT_EQ("http://newforced/6", querier.urls()[11].url.spec()); - EXPECT_EQ(14000, querier.urls()[11].last_forced_time.ToJsTime()); - EXPECT_EQ("http://newforced/7", querier.urls()[12].url.spec()); - EXPECT_EQ(15000, querier.urls()[12].last_forced_time.ToJsTime()); - EXPECT_EQ("http://newforced/8", querier.urls()[13].url.spec()); - EXPECT_EQ(16000, querier.urls()[13].last_forced_time.ToJsTime()); - EXPECT_EQ("http://newforced/9", querier.urls()[14].url.spec()); - EXPECT_EQ(17000, querier.urls()[14].last_forced_time.ToJsTime()); - EXPECT_EQ("http://oldforced/7", querier.urls()[15].url.spec()); - EXPECT_EQ(18000, querier.urls()[15].last_forced_time.ToJsTime()); - EXPECT_EQ("http://newforced/10", querier.urls()[16].url.spec()); - EXPECT_EQ(19000, querier.urls()[16].last_forced_time.ToJsTime()); - EXPECT_EQ("http://newforced/11", querier.urls()[17].url.spec()); - EXPECT_EQ(20000, querier.urls()[17].last_forced_time.ToJsTime()); - EXPECT_EQ("http://oldforced/8", querier.urls()[18].url.spec()); - EXPECT_EQ(21000, querier.urls()[18].last_forced_time.ToJsTime()); - EXPECT_EQ("http://newforced/12", querier.urls()[19].url.spec()); - EXPECT_EQ(22000, querier.urls()[19].last_forced_time.ToJsTime()); + EXPECT_EQ("http://oldforced/2", querier.urls()[0].url.spec()); + EXPECT_EQ(7000, querier.urls()[0].last_forced_time.ToJsTime()); + EXPECT_EQ("http://newforced/4", querier.urls()[1].url.spec()); + EXPECT_EQ(8000, querier.urls()[1].last_forced_time.ToJsTime()); + EXPECT_EQ("http://newforced/5", querier.urls()[2].url.spec()); + EXPECT_EQ(9000, querier.urls()[2].last_forced_time.ToJsTime()); + EXPECT_EQ("http://oldforced/3", querier.urls()[3].url.spec()); + EXPECT_EQ(10000, querier.urls()[3].last_forced_time.ToJsTime()); + EXPECT_EQ("http://oldforced/4", querier.urls()[4].url.spec()); + EXPECT_EQ(11000, querier.urls()[4].last_forced_time.ToJsTime()); + EXPECT_EQ("http://oldforced/5", querier.urls()[5].url.spec()); + EXPECT_EQ(12000, querier.urls()[5].last_forced_time.ToJsTime()); + EXPECT_EQ("http://oldforced/6", querier.urls()[6].url.spec()); + EXPECT_EQ(13000, querier.urls()[6].last_forced_time.ToJsTime()); + EXPECT_EQ("http://newforced/6", querier.urls()[7].url.spec()); + EXPECT_EQ(14000, querier.urls()[7].last_forced_time.ToJsTime()); + EXPECT_EQ("http://newforced/7", querier.urls()[8].url.spec()); + EXPECT_EQ(15000, querier.urls()[8].last_forced_time.ToJsTime()); + EXPECT_EQ("http://newforced/8", querier.urls()[9].url.spec()); + EXPECT_EQ(16000, querier.urls()[9].last_forced_time.ToJsTime()); // Check first and last non-forced URLs. - EXPECT_EQ("http://newnonforced/0", querier.urls()[20].url.spec()); - EXPECT_TRUE(querier.urls()[20].last_forced_time.is_null()); - EXPECT_EQ("http://newnonforced/19", querier.urls()[39].url.spec()); - EXPECT_TRUE(querier.urls()[39].last_forced_time.is_null()); + EXPECT_EQ("http://newnonforced/0", + querier.urls()[kForcedTopSitesCount].url.spec()); + EXPECT_TRUE(querier.urls()[kForcedTopSitesCount].last_forced_time.is_null()); + + size_t non_forced_end_index = querier.urls().size() - 1; + EXPECT_EQ("http://newnonforced/9", + querier.urls()[non_forced_end_index].url.spec()); + EXPECT_TRUE(querier.urls()[non_forced_end_index].last_forced_time.is_null()); } TEST_F(TopSitesImplTest, SetForcedTopSitesWithCollisions) { // Setup an old URL list in order to generate some collisions. MostVisitedURLList old_url_list; - AppendForcedMostVisitedURL(&old_url_list, GURL("http://url/0"), 1000); + AppendForcedMostVisitedURL(GURL("http://url/0"), 1000, &old_url_list); // The following three will be evicted. - AppendForcedMostVisitedURL(&old_url_list, GURL("http://collision/0"), 4000); - AppendForcedMostVisitedURL(&old_url_list, GURL("http://collision/1"), 6000); - AppendForcedMostVisitedURL(&old_url_list, GURL("http://collision/2"), 7000); + AppendForcedMostVisitedURL(GURL("http://collision/0"), 4000, &old_url_list); + AppendForcedMostVisitedURL(GURL("http://collision/1"), 6000, &old_url_list); + AppendForcedMostVisitedURL(GURL("http://collision/2"), 7000, &old_url_list); // The following is evicted since all non-forced URLs are, therefore it // doesn't cause a collision. - AppendMostVisitedURL(&old_url_list, GURL("http://noncollision/0")); + AppendMostVisitedURL(GURL("http://noncollision/0"), &old_url_list); SetTopSites(old_url_list); // Setup a new URL list that will cause collisions. MostVisitedURLList new_url_list; - AppendForcedMostVisitedURL(&new_url_list, GURL("http://collision/1"), 2000); - AppendForcedMostVisitedURL(&new_url_list, GURL("http://url/2"), 3000); - AppendForcedMostVisitedURL(&new_url_list, GURL("http://collision/0"), 5000); - AppendForcedMostVisitedURL(&new_url_list, GURL("http://noncollision/0"), - 9000); - AppendMostVisitedURL(&new_url_list, GURL("http://collision/2")); - AppendMostVisitedURL(&new_url_list, GURL("http://url/3")); + AppendForcedMostVisitedURL(GURL("http://collision/1"), 2000, &new_url_list); + AppendForcedMostVisitedURL(GURL("http://url/2"), 3000, &new_url_list); + AppendForcedMostVisitedURL(GURL("http://collision/0"), 5000, &new_url_list); + AppendForcedMostVisitedURL(GURL("http://noncollision/0"), 9000, + &new_url_list); + AppendMostVisitedURL(GURL("http://collision/2"), &new_url_list); + AppendMostVisitedURL(GURL("http://url/3"), &new_url_list); SetTopSites(new_url_list); // Query all URLs. @@ -1519,9 +1530,9 @@ TEST_F(TopSitesImplTest, SetForcedTopSitesWithCollisions) { TEST_F(TopSitesImplTest, SetTopSitesIdentical) { // Set the initial list of URLs. MostVisitedURLList url_list; - AppendForcedMostVisitedURL(&url_list, GURL("http://url/0"), 1000); - AppendMostVisitedURL(&url_list, GURL("http://url/1")); - AppendMostVisitedURL(&url_list, GURL("http://url/2")); + AppendForcedMostVisitedURL(GURL("http://url/0"), 1000, &url_list); + AppendMostVisitedURL(GURL("http://url/1"), &url_list); + AppendMostVisitedURL(GURL("http://url/2"), &url_list); SetTopSites(url_list); // Set the new list of URLs to be exactly the same. @@ -1543,15 +1554,15 @@ TEST_F(TopSitesImplTest, SetTopSitesIdentical) { TEST_F(TopSitesImplTest, SetTopSitesWithAlreadyExistingForcedURLs) { // Set the initial list of URLs. MostVisitedURLList old_url_list; - AppendForcedMostVisitedURL(&old_url_list, GURL("http://url/0/redir"), 1000); - AppendForcedMostVisitedURL(&old_url_list, GURL("http://url/1"), 2000); + AppendForcedMostVisitedURL(GURL("http://url/0/redir"), 1000, &old_url_list); + AppendForcedMostVisitedURL(GURL("http://url/1"), 2000, &old_url_list); SetTopSites(old_url_list); // Setup a new URL list that will cause collisions. MostVisitedURLList new_url_list; - AppendMostVisitedURLWithRedirect(&new_url_list, GURL("http://url/0/redir"), - GURL("http://url/0")); - AppendMostVisitedURL(&new_url_list, GURL("http://url/1")); + AppendMostVisitedURLWithRedirect(GURL("http://url/0/redir"), + GURL("http://url/0"), &new_url_list); + AppendMostVisitedURL(GURL("http://url/1"), &new_url_list); SetTopSites(new_url_list); // Query all URLs. @@ -1571,11 +1582,11 @@ TEST_F(TopSitesImplTest, SetTopSitesWithAlreadyExistingForcedURLs) { TEST_F(TopSitesImplTest, AddForcedURL) { // Set the initial list of URLs. MostVisitedURLList url_list; - AppendForcedMostVisitedURL(&url_list, GURL("http://forced/0"), 2000); - AppendForcedMostVisitedURL(&url_list, GURL("http://forced/1"), 4000); - AppendMostVisitedURL(&url_list, GURL("http://nonforced/0")); - AppendMostVisitedURL(&url_list, GURL("http://nonforced/1")); - AppendMostVisitedURL(&url_list, GURL("http://nonforced/2")); + AppendForcedMostVisitedURL(GURL("http://forced/0"), 2000, &url_list); + AppendForcedMostVisitedURL(GURL("http://forced/1"), 4000, &url_list); + AppendMostVisitedURL(GURL("http://nonforced/0"), &url_list); + AppendMostVisitedURL(GURL("http://nonforced/1"), &url_list); + AppendMostVisitedURL(GURL("http://nonforced/2"), &url_list); SetTopSites(url_list); // Add forced sites here and there to exercise a couple of cases. diff --git a/chromium/components/history/core/browser/typed_url_data_type_controller.cc b/chromium/components/history/core/browser/typed_url_data_type_controller.cc index d27a4f61a7e..2fc2c86ab21 100644 --- a/chromium/components/history/core/browser/typed_url_data_type_controller.cc +++ b/chromium/components/history/core/browser/typed_url_data_type_controller.cc @@ -91,7 +91,7 @@ void TypedUrlDataTypeController::OnSavingBrowserHistoryDisabledChanged() { } bool TypedUrlDataTypeController::PostTaskOnModelThread( - const tracked_objects::Location& from_here, + const base::Location& from_here, const base::Closure& task) { DCHECK(CalledOnValidThread()); history::HistoryService* history = sync_client_->GetHistoryService(); diff --git a/chromium/components/history/core/browser/typed_url_data_type_controller.h b/chromium/components/history/core/browser/typed_url_data_type_controller.h index 2f5e1d06bc7..1883652170d 100644 --- a/chromium/components/history/core/browser/typed_url_data_type_controller.h +++ b/chromium/components/history/core/browser/typed_url_data_type_controller.h @@ -30,7 +30,7 @@ class TypedUrlDataTypeController : public syncer::AsyncDirectoryTypeController { protected: // AsyncDirectoryTypeController implementation. - bool PostTaskOnModelThread(const tracked_objects::Location& from_here, + bool PostTaskOnModelThread(const base::Location& from_here, const base::Closure& task) override; private: diff --git a/chromium/components/history/core/browser/typed_url_model_type_controller.cc b/chromium/components/history/core/browser/typed_url_model_type_controller.cc new file mode 100644 index 00000000000..38d45165f95 --- /dev/null +++ b/chromium/components/history/core/browser/typed_url_model_type_controller.cc @@ -0,0 +1,108 @@ +// Copyright 2017 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 "components/history/core/browser/typed_url_model_type_controller.h" + +#include "base/bind.h" +#include "components/history/core/browser/history_db_task.h" +#include "components/history/core/browser/history_service.h" +#include "components/prefs/pref_service.h" +#include "components/sync/driver/sync_client.h" + +using syncer::ModelType; +using syncer::ModelTypeController; +using syncer::ModelTypeSyncBridge; +using syncer::SyncClient; + +namespace history { + +namespace { + +// The history service exposes a special non-standard task API which calls back +// once a task has been dispatched, so we have to build a special wrapper around +// the tasks we want to run. +class RunTaskOnHistoryThread : public HistoryDBTask { + public: + explicit RunTaskOnHistoryThread(const base::Closure& task) : task_(task) {} + + bool RunOnDBThread(HistoryBackend* backend, HistoryDatabase* db) override { + // Invoke the task, then free it immediately so we don't keep a reference + // around all the way until DoneRunOnMainThread() is invoked back on the + // main thread - we want to release references as soon as possible to avoid + // keeping them around too long during shutdown. + task_.Run(); + task_.Reset(); + return true; + } + + void DoneRunOnMainThread() override {} + + protected: + base::Closure task_; +}; + +} // namespace + +TypedURLModelTypeController::TypedURLModelTypeController( + SyncClient* sync_client, + const char* history_disabled_pref_name) + : ModelTypeController(syncer::TYPED_URLS, sync_client, nullptr), + history_disabled_pref_name_(history_disabled_pref_name) { + pref_registrar_.Init(sync_client->GetPrefService()); + pref_registrar_.Add( + history_disabled_pref_name_, + base::Bind( + &TypedURLModelTypeController::OnSavingBrowserHistoryDisabledChanged, + base::AsWeakPtr(this))); +} + +TypedURLModelTypeController::~TypedURLModelTypeController() {} + +bool TypedURLModelTypeController::ReadyForStart() const { + return !sync_client()->GetPrefService()->GetBoolean( + history_disabled_pref_name_); +} + +void TypedURLModelTypeController::PostBridgeTask(const base::Location& location, + const BridgeTask& task) { + history::HistoryService* history = sync_client()->GetHistoryService(); + if (!history) { + // History must be disabled - don't start. + LOG(WARNING) << "Cannot access history service."; + return; + } + + TypedURLSyncBridge* bridge = history->GetTypedURLSyncBridge(); + PostTaskOnHistoryThread(base::Bind(task, bridge)); +} + +void TypedURLModelTypeController::OnSavingBrowserHistoryDisabledChanged() { + if (sync_client()->GetPrefService()->GetBoolean( + history_disabled_pref_name_)) { + // We've turned off history persistence, so if we are running, + // generate an unrecoverable error. This can be fixed by restarting + // Chrome (on restart, typed urls will not be a registered type). + if (state() != NOT_RUNNING && state() != STOPPING) { + PostTaskOnHistoryThread(base::Bind( + &TypedURLModelTypeController::ReportModelError, base::AsWeakPtr(this), + syncer::ModelError(FROM_HERE, + "History saving is now disabled by policy."))); + } + } +} + +void TypedURLModelTypeController::PostTaskOnHistoryThread( + const base::Closure& task) { + history::HistoryService* history = sync_client()->GetHistoryService(); + if (!history) { + // History must be disabled - don't start. + LOG(WARNING) << "Cannot access history service."; + return; + } + + history->ScheduleDBTask(std::make_unique<RunTaskOnHistoryThread>(task), + &task_tracker_); +} + +} // namespace history diff --git a/chromium/components/history/core/browser/typed_url_model_type_controller.h b/chromium/components/history/core/browser/typed_url_model_type_controller.h new file mode 100644 index 00000000000..77e4f460c6e --- /dev/null +++ b/chromium/components/history/core/browser/typed_url_model_type_controller.h @@ -0,0 +1,47 @@ +// Copyright 2017 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_HISTORY_CORE_BROWSER_TYPED_URL_MODEL_TYPE_CONTROLLER_H__ +#define COMPONENTS_HISTORY_CORE_BROWSER_TYPED_URL_MODEL_TYPE_CONTROLLER_H__ + +#include "base/task/cancelable_task_tracker.h" +#include "components/prefs/pref_change_registrar.h" +#include "components/sync/driver/model_type_controller.h" + +namespace history { + +class TypedURLModelTypeController : public syncer::ModelTypeController { + public: + TypedURLModelTypeController(syncer::SyncClient* sync_client, + const char* history_disabled_pref_name); + + ~TypedURLModelTypeController() override; + + // syncer::DataTypeController implementation. + bool ReadyForStart() const override; + + private: + // syncer::ModelTypeController implementation. + void PostBridgeTask(const base::Location& location, + const BridgeTask& task) override; + + void OnSavingBrowserHistoryDisabledChanged(); + + void PostTaskOnHistoryThread(const base::Closure& task); + + // Name of the pref that indicates whether saving history is disabled. + const char* history_disabled_pref_name_; + + PrefChangeRegistrar pref_registrar_; + + // Helper object to make sure we don't leave tasks running on the history + // thread. + base::CancelableTaskTracker task_tracker_; + + DISALLOW_COPY_AND_ASSIGN(TypedURLModelTypeController); +}; + +} // namespace history + +#endif // COMPONENTS_HISTORY_CORE_BROWSER_TYPED_URL_MODEL_TYPE_CONTROLLER_H__ diff --git a/chromium/components/history/core/browser/typed_url_sync_bridge.cc b/chromium/components/history/core/browser/typed_url_sync_bridge.cc index 9a437f71686..c81a656f92d 100644 --- a/chromium/components/history/core/browser/typed_url_sync_bridge.cc +++ b/chromium/components/history/core/browser/typed_url_sync_bridge.cc @@ -4,6 +4,7 @@ #include "components/history/core/browser/typed_url_sync_bridge.h" +#include "base/auto_reset.h" #include "base/big_endian.h" #include "base/memory/ptr_util.h" #include "base/metrics/histogram_macros.h" @@ -82,6 +83,7 @@ TypedURLSyncBridge::TypedURLSyncBridge( const ChangeProcessorFactory& change_processor_factory) : ModelTypeSyncBridge(change_processor_factory, syncer::TYPED_URLS), history_backend_(history_backend), + processing_syncer_changes_(false), sync_metadata_database_(sync_metadata_database), num_db_accesses_(0), num_db_errors_(0), @@ -136,13 +138,12 @@ base::Optional<ModelError> TypedURLSyncBridge::MergeSyncData( // Ignore old sync urls that don't have any transition data stored with // them, or transition data that does not match the visit data (will be - // deleted below). + // deleted later by GarbageCollectionDirective). if (specifics.visit_transitions_size() == 0 || specifics.visit_transitions_size() != specifics.visits_size()) { - // Generate a debug assertion to help track down http://crbug.com/91473, - // even though we gracefully handle this case by overwriting this node. DCHECK_EQ(specifics.visits_size(), specifics.visit_transitions_size()); - DVLOG(1) << "Ignoring obsolete sync url with no visit transition info."; + DLOG(WARNING) + << "Ignoring obsolete sync url with no visit transition info."; continue; } @@ -151,17 +152,13 @@ base::Optional<ModelError> TypedURLSyncBridge::MergeSyncData( &updated_synced_urls); } - for (const auto& kv : new_db_urls) { - SendTypedURLToProcessor(kv.second, local_visit_vectors[kv.first], - metadata_change_list.get()); - } - base::Optional<ModelError> error = WriteToHistoryBackend( &new_synced_urls, &updated_synced_urls, NULL, &new_synced_visits, NULL); - if (error) return error; + // Update storage key here first, and then send updated typed URL to sync + // below, otherwise processor will have duplicate entries. for (const EntityChange& entity_change : entity_data) { DCHECK(entity_change.data().specifics.has_typed_url()); std::string storage_key = @@ -175,6 +172,12 @@ base::Optional<ModelError> TypedURLSyncBridge::MergeSyncData( } } + // Send new/updated typed URL to sync. + for (const auto& kv : new_db_urls) { + SendTypedURLToProcessor(kv.second, local_visit_vectors[kv.first], + metadata_change_list.get()); + } + UMA_HISTOGRAM_PERCENTAGE("Sync.TypedUrlMergeAndStartSyncingErrors", GetErrorPercentage()); ClearErrorStats(); @@ -229,9 +232,11 @@ base::Optional<ModelError> TypedURLSyncBridge::ApplySyncChanges( &updated_synced_urls, &new_synced_urls); } - WriteToHistoryBackend(&new_synced_urls, &updated_synced_urls, - &pending_deleted_urls, &new_synced_visits, - &deleted_visits); + base::Optional<ModelError> error = WriteToHistoryBackend( + &new_synced_urls, &updated_synced_urls, &pending_deleted_urls, + &new_synced_visits, &deleted_visits); + if (error) + return error; // New entities were either ignored or written to history DB and assigned a // storage key. Notify processor about updated storage keys. @@ -249,7 +254,9 @@ base::Optional<ModelError> TypedURLSyncBridge::ApplySyncChanges( } } - return {}; + return static_cast<syncer::SyncMetadataStoreChangeList*>( + metadata_change_list.get()) + ->TakeError(); } void TypedURLSyncBridge::GetData(StorageKeyList storage_keys, @@ -343,6 +350,9 @@ void TypedURLSyncBridge::OnURLVisited(HistoryBackend* history_backend, base::Time visit_time) { DCHECK(sequence_checker_.CalledOnValidSequence()); + if (processing_syncer_changes_) + return; // These are changes originating from us, ignore. + if (!change_processor()->IsTrackingMetadata()) return; // Sync processor not yet ready, don't sync. if (!ShouldSyncVisit(row.typed_count(), transition)) @@ -358,6 +368,9 @@ void TypedURLSyncBridge::OnURLsModified(HistoryBackend* history_backend, const URLRows& changed_urls) { DCHECK(sequence_checker_.CalledOnValidSequence()); + if (processing_syncer_changes_) + return; // These are changes originating from us, ignore. + if (!change_processor()->IsTrackingMetadata()) return; // Sync processor not yet ready, don't sync. @@ -380,6 +393,10 @@ void TypedURLSyncBridge::OnURLsDeleted(HistoryBackend* history_backend, const URLRows& deleted_rows, const std::set<GURL>& favicon_urls) { DCHECK(sequence_checker_.CalledOnValidSequence()); + + if (processing_syncer_changes_) + return; // These are changes originating from us, ignore. + if (!change_processor()->IsTrackingMetadata()) return; // Sync processor not yet ready, don't sync. @@ -912,6 +929,10 @@ base::Optional<ModelError> TypedURLSyncBridge::WriteToHistoryBackend( const std::vector<GURL>* deleted_urls, const TypedURLVisitVector* new_visits, const VisitVector* deleted_visits) { + DCHECK_EQ(processing_syncer_changes_, false); + // Set flag to stop accepting history change notifications from backend. + base::AutoReset<bool> processing_changes(&processing_syncer_changes_, true); + if (deleted_urls && !deleted_urls->empty()) history_backend_->DeleteURLs(*deleted_urls); diff --git a/chromium/components/history/core/browser/typed_url_sync_bridge.h b/chromium/components/history/core/browser/typed_url_sync_bridge.h index 01f25ee19b3..cb92a96ff0f 100644 --- a/chromium/components/history/core/browser/typed_url_sync_bridge.h +++ b/chromium/components/history/core/browser/typed_url_sync_bridge.h @@ -217,6 +217,10 @@ class TypedURLSyncBridge : public syncer::ModelTypeSyncBridge, // and sync changes to. HistoryBackend* const history_backend_; + // Whether we're currently processing changes from the syncer. While this is + // true, we ignore any local url changes, since we triggered them. + bool processing_syncer_changes_; + // A non-owning pointer to the database, which is for storing typed urls sync // metadata and state. TypedURLSyncMetadataDatabase* const sync_metadata_database_; diff --git a/chromium/components/history/core/browser/typed_url_syncable_service.cc b/chromium/components/history/core/browser/typed_url_syncable_service.cc index 3f357843574..98bf4b9ac3a 100644 --- a/chromium/components/history/core/browser/typed_url_syncable_service.cc +++ b/chromium/components/history/core/browser/typed_url_syncable_service.cc @@ -233,7 +233,7 @@ syncer::SyncDataList TypedUrlSyncableService::GetAllSyncData( } syncer::SyncError TypedUrlSyncableService::ProcessSyncChanges( - const tracked_objects::Location& from_here, + const base::Location& from_here, const syncer::SyncChangeList& change_list) { DCHECK(sequence_checker_.CalledOnValidSequence()); diff --git a/chromium/components/history/core/browser/typed_url_syncable_service.h b/chromium/components/history/core/browser/typed_url_syncable_service.h index 39f2fd53da9..9cad7f2acd5 100644 --- a/chromium/components/history/core/browser/typed_url_syncable_service.h +++ b/chromium/components/history/core/browser/typed_url_syncable_service.h @@ -51,7 +51,7 @@ class TypedUrlSyncableService : public syncer::SyncableService, void StopSyncing(syncer::ModelType type) override; syncer::SyncDataList GetAllSyncData(syncer::ModelType type) const override; syncer::SyncError ProcessSyncChanges( - const tracked_objects::Location& from_here, + const base::Location& from_here, const syncer::SyncChangeList& change_list) override; // history::HistoryBackendObserver: diff --git a/chromium/components/history/core/browser/url_database.cc b/chromium/components/history/core/browser/url_database.cc index 4d8c5df4172..5fb9762943c 100644 --- a/chromium/components/history/core/browser/url_database.cc +++ b/chromium/components/history/core/browser/url_database.cc @@ -60,7 +60,7 @@ std::string URLDatabase::GURLToDatabaseURL(const GURL& gurl) { // Convenience to fill a URLRow. Must be in sync with the fields in // kURLRowFields. -void URLDatabase::FillURLRow(sql::Statement& s, URLRow* i) { +void URLDatabase::FillURLRow(const sql::Statement& s, URLRow* i) { DCHECK(i); i->set_id(s.ColumnInt64(0)); i->set_url(GURL(s.ColumnString(1))); @@ -87,18 +87,6 @@ bool URLDatabase::GetURLRow(URLID url_id, URLRow* info) { return false; } -bool URLDatabase::GetAllTypedUrls(URLRows* urls) { - sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE, - "SELECT" HISTORY_URL_ROW_FIELDS "FROM urls WHERE typed_count > 0")); - - while (statement.Step()) { - URLRow info; - FillURLRow(statement, &info); - urls->push_back(info); - } - return true; -} - URLID URLDatabase::GetRowForURL(const GURL& url, URLRow* info) { sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE, "SELECT" HISTORY_URL_ROW_FIELDS "FROM urls WHERE url=?")); @@ -165,6 +153,27 @@ URLID URLDatabase::AddURLInternal(const URLRow& info, bool is_temporary) { return GetDB().GetLastInsertRowId(); } +bool URLDatabase::URLTableContainsAutoincrement() { + // sqlite_master has columns: + // type - "index" or "table". + // name - name of created element. + // tbl_name - name of element, or target table in case of index. + // rootpage - root page of the element in database file. + // sql - SQL to create the element. + sql::Statement statement(GetDB().GetUniqueStatement( + "SELECT sql FROM sqlite_master WHERE type = 'table' AND name = 'urls'")); + + // urls table does not exist. + if (!statement.Step()) + return false; + + std::string urls_schema = statement.ColumnString(0); + // We check if the whole schema contains "AUTOINCREMENT", since + // "AUTOINCREMENT" only can be used for "INTEGER PRIMARY KEY", so we assume no + // other columns could cantain "AUTOINCREMENT". + return urls_schema.find("AUTOINCREMENT") != std::string::npos; +} + bool URLDatabase::InsertOrUpdateURLRowByID(const URLRow& info) { // SQLite does not support INSERT OR UPDATE, however, it does have INSERT OR // REPLACE, which is feasible to use, because of the following. @@ -227,9 +236,7 @@ bool URLDatabase::CommitTemporaryURLTable() { // Re-create the index over the now permanent URLs table -- this was not there // for the temporary table. - CreateMainURLIndex(); - - return true; + return CreateMainURLIndex(); } bool URLDatabase::InitURLEnumeratorForEverything(URLEnumerator* enumerator) { @@ -639,9 +646,7 @@ bool URLDatabase::RecreateURLTableWithAllContents() { } // Rename/commit the tmp table. - CommitTemporaryURLTable(); - - return true; + return CommitTemporaryURLTable(); } const int kLowQualityMatchTypedLimit = 1; diff --git a/chromium/components/history/core/browser/url_database.h b/chromium/components/history/core/browser/url_database.h index 087e7864d68..dd8f2feff7a 100644 --- a/chromium/components/history/core/browser/url_database.h +++ b/chromium/components/history/core/browser/url_database.h @@ -7,6 +7,9 @@ #include <stddef.h> +#include <string> +#include <vector> + #include "base/macros.h" #include "components/history/core/browser/keyword_id.h" #include "components/history/core/browser/url_row.h" @@ -59,10 +62,6 @@ class URLDatabase { // success and false otherwise. bool GetURLRow(URLID url_id, URLRow* info); - // Looks up all urls that were typed in manually. Fills info with the data. - // Returns true on success and false otherwise. - bool GetAllTypedUrls(URLRows* urls); - // Looks up the given URL and if it exists, fills the given pointers with the // associated info and returns the ID of that URL. If the info pointer is // NULL, no information about the URL will be filled in, only the ID will be @@ -286,9 +285,13 @@ class URLDatabase { // be used in between CreateTemporaryURLTable() and CommitTemporaryURLTable(). URLID AddURLInternal(const URLRow& info, bool is_temporary); + // Return ture if the urls table's schema contains "AUTOINCREMENT". + // false if table do not contain AUTOINCREMENT, or the table is not created. + bool URLTableContainsAutoincrement(); + // Convenience to fill a URLRow. Must be in sync with the fields in // kHistoryURLRowFields. - static void FillURLRow(sql::Statement& s, URLRow* i); + static void FillURLRow(const sql::Statement& s, URLRow* i); // Returns the database for the functions in this interface. The decendent of // this class implements these functions to return its objects. diff --git a/chromium/components/history/core/browser/url_database_unittest.cc b/chromium/components/history/core/browser/url_database_unittest.cc index 5b5c33c1a63..76692a116a4 100644 --- a/chromium/components/history/core/browser/url_database_unittest.cc +++ b/chromium/components/history/core/browser/url_database_unittest.cc @@ -459,4 +459,13 @@ TEST_F(URLDatabaseTest, MigrationURLTableForAddingAUTOINCREMENT) { EXPECT_NE(info4.id(), info5.id()); } +TEST_F(URLDatabaseTest, URLTableContainsAUTOINCREMENTTest) { + CreateVersion33URLTable(); + EXPECT_FALSE(URLTableContainsAutoincrement()); + + // Upgrade urls table. + RecreateURLTableWithAllContents(); + EXPECT_TRUE(URLTableContainsAutoincrement()); +} + } // namespace history diff --git a/chromium/components/history/core/browser/url_row.h b/chromium/components/history/core/browser/url_row.h index 64f00dabec0..907ad2a1bf8 100644 --- a/chromium/components/history/core/browser/url_row.h +++ b/chromium/components/history/core/browser/url_row.h @@ -7,6 +7,8 @@ #include <stdint.h> +#include <vector> + #include "base/strings/string16.h" #include "base/time/time.h" #include "components/query_parser/snippet.h" diff --git a/chromium/components/history/core/browser/visit_database.cc b/chromium/components/history/core/browser/visit_database.cc index 3d998852472..0ef84afb008 100644 --- a/chromium/components/history/core/browser/visit_database.cc +++ b/chromium/components/history/core/browser/visit_database.cc @@ -11,6 +11,8 @@ #include <limits> #include <map> #include <set> +#include <string> +#include <utility> #include "base/logging.h" #include "base/strings/string_number_conversions.h" @@ -30,16 +32,17 @@ VisitDatabase::~VisitDatabase() { bool VisitDatabase::InitVisitTable() { if (!GetDB().DoesTableExist("visits")) { - if (!GetDB().Execute("CREATE TABLE visits(" - "id INTEGER PRIMARY KEY," - "url INTEGER NOT NULL," // key of the URL this corresponds to - "visit_time INTEGER NOT NULL," - "from_visit INTEGER," - "transition INTEGER DEFAULT 0 NOT NULL," - "segment_id INTEGER," - // Some old DBs may have an "is_indexed" field here, but this is no - // longer used and should NOT be read or written from any longer. - "visit_duration INTEGER DEFAULT 0 NOT NULL)")) + if (!GetDB().Execute( + "CREATE TABLE visits(" + "id INTEGER PRIMARY KEY," + "url INTEGER NOT NULL," // key of the URL this corresponds to + "visit_time INTEGER NOT NULL," + "from_visit INTEGER," + "transition INTEGER DEFAULT 0 NOT NULL," + "segment_id INTEGER," + // Some old DBs may have an "is_indexed" field here, but this is no + // longer used and should NOT be read or written from any longer. + "visit_duration INTEGER DEFAULT 0 NOT NULL)")) return false; } @@ -85,7 +88,8 @@ bool VisitDatabase::DropVisitTable() { // Must be in sync with HISTORY_VISIT_ROW_FIELDS. // static -void VisitDatabase::FillVisitRow(sql::Statement& statement, VisitRow* visit) { +void VisitDatabase::FillVisitRow(const sql::Statement& statement, + VisitRow* visit) { visit->visit_id = statement.ColumnInt64(0); visit->url_id = statement.ColumnInt64(1); visit->visit_time = base::Time::FromInternalValue(statement.ColumnInt64(2)); @@ -349,6 +353,22 @@ bool VisitDatabase::GetVisitsInRangeForTransition( return FillVisitVector(statement, visits); } +bool VisitDatabase::GetAllURLIDsForTransition(ui::PageTransition transition, + std::vector<URLID>* urls) { + DCHECK(urls); + urls->clear(); + sql::Statement statement( + GetDB().GetUniqueStatement("SELECT DISTINCT url FROM visits " + "WHERE (transition & ?) == ?")); + statement.BindInt(0, ui::PAGE_TRANSITION_CORE_MASK); + statement.BindInt(1, transition); + + while (statement.Step()) { + urls->push_back(statement.ColumnInt64(0)); + } + return statement.Succeeded(); +} + bool VisitDatabase::GetVisibleVisitsInRange(const QueryOptions& options, VisitVector* visits) { visits->clear(); diff --git a/chromium/components/history/core/browser/visit_database.h b/chromium/components/history/core/browser/visit_database.h index 1bb743a8a02..ee950544f97 100644 --- a/chromium/components/history/core/browser/visit_database.h +++ b/chromium/components/history/core/browser/visit_database.h @@ -102,6 +102,11 @@ class VisitDatabase { ui::PageTransition transition, VisitVector* visits); + // Looks up URLIDs for all visits with specified transition. Returns true on + // success and false otherwise. + bool GetAllURLIDsForTransition(ui::PageTransition transition, + std::vector<URLID>* urls); + // Fills all visits in the given time range into the given vector that should // be user-visible, which excludes things like redirects and subframes. The // begin time is inclusive, the end time is exclusive. Either time can be @@ -193,7 +198,7 @@ class VisitDatabase { // Convenience to fill a VisitRow. Assumes the visit values are bound starting // at index 0. - static void FillVisitRow(sql::Statement& statement, VisitRow* visit); + static void FillVisitRow(const sql::Statement& statement, VisitRow* visit); // Convenience to fill a VisitVector. Assumes that statement.step() // hasn't happened yet. diff --git a/chromium/components/history/core/browser/visit_database_unittest.cc b/chromium/components/history/core/browser/visit_database_unittest.cc index 77218eff842..77be2f6388d 100644 --- a/chromium/components/history/core/browser/visit_database_unittest.cc +++ b/chromium/components/history/core/browser/visit_database_unittest.cc @@ -353,6 +353,18 @@ TEST_F(VisitDatabaseTest, GetVisibleVisitsInRange) { EXPECT_TRUE(IsVisitInfoEqual(results[0], test_visit_rows[1])); } +TEST_F(VisitDatabaseTest, GetAllURLIDsForTransition) { + std::vector<VisitRow> test_visit_rows = GetTestVisitRows(); + + for (size_t i = 0; i < test_visit_rows.size(); ++i) { + EXPECT_TRUE(AddVisit(&test_visit_rows[i], SOURCE_BROWSED)); + } + std::vector<URLID> url_ids; + GetAllURLIDsForTransition(ui::PAGE_TRANSITION_TYPED, &url_ids); + EXPECT_EQ(1U, url_ids.size()); + EXPECT_EQ(test_visit_rows[0].url_id, url_ids[0]); +} + TEST_F(VisitDatabaseTest, VisitSource) { // Add visits. VisitRow visit_info1(111, Time::Now(), 0, ui::PAGE_TRANSITION_LINK, 0); |