diff options
| author | Allan Sandfeld Jensen <allan.jensen@qt.io> | 2019-02-13 16:23:34 +0100 |
|---|---|---|
| committer | Allan Sandfeld Jensen <allan.jensen@qt.io> | 2019-02-14 10:37:21 +0000 |
| commit | 38a9a29f4f9436cace7f0e7abf9c586057df8a4e (patch) | |
| tree | c4e8c458dc595bc0ddb435708fa2229edfd00bd4 /chromium/components/history | |
| parent | e684a3455bcc29a6e3e66a004e352dea4e1141e7 (diff) | |
| download | qtwebengine-chromium-38a9a29f4f9436cace7f0e7abf9c586057df8a4e.tar.gz | |
BASELINE: Update Chromium to 73.0.3683.37
Change-Id: I08c9af2948b645f671e5d933aca1f7a90ea372f2
Reviewed-by: Michael Brüning <michael.bruning@qt.io>
Diffstat (limited to 'chromium/components/history')
53 files changed, 926 insertions, 2652 deletions
diff --git a/chromium/components/history/content/browser/content_history_backend_db_unittest.cc b/chromium/components/history/content/browser/content_history_backend_db_unittest.cc index d189ba01f35..59c6a0284dd 100644 --- a/chromium/components/history/content/browser/content_history_backend_db_unittest.cc +++ b/chromium/components/history/content/browser/content_history_backend_db_unittest.cc @@ -21,7 +21,7 @@ #include <stddef.h> -#include "base/macros.h" +#include "base/stl_util.h" #include "components/history/core/test/history_backend_db_base_test.h" namespace history { @@ -89,11 +89,11 @@ TEST_F(ContentHistoryBackendDBTest, ConfirmDownloadInterruptReasonBackwardsCompatible) { // Are there any cases in which a historical number has been repurposed // for an error other than it's original? - for (size_t i = 0; i < arraysize(current_reasons); i++) { + for (size_t i = 0; i < base::size(current_reasons); i++) { const InterruptReasonAssociation& cur_reason(current_reasons[i]); bool found = false; - for (size_t j = 0; j < arraysize(historical_reasons); ++j) { + for (size_t j = 0; j < base::size(historical_reasons); ++j) { const InterruptReasonAssociation& hist_reason(historical_reasons[j]); if (hist_reason.value == cur_reason.value) { diff --git a/chromium/components/history/content/browser/history_context_helper.cc b/chromium/components/history/content/browser/history_context_helper.cc index 95257b732b7..b43106cfd28 100644 --- a/chromium/components/history/content/browser/history_context_helper.cc +++ b/chromium/components/history/content/browser/history_context_helper.cc @@ -17,8 +17,11 @@ class WebContentsContext private: friend class content::WebContentsUserData<WebContentsContext>; explicit WebContentsContext(content::WebContents* web_contents) {} + WEB_CONTENTS_USER_DATA_KEY_DECL(); }; +WEB_CONTENTS_USER_DATA_KEY_IMPL(WebContentsContext) + } // namespace namespace history { 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 d26c7fac624..f0ffbdc858e 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 @@ -51,4 +51,6 @@ void WebContentsTopSitesObserver::NavigationEntryCommitted( } } +WEB_CONTENTS_USER_DATA_KEY_IMPL(WebContentsTopSitesObserver) + } // namespace history diff --git a/chromium/components/history/content/browser/web_contents_top_sites_observer.h b/chromium/components/history/content/browser/web_contents_top_sites_observer.h index 32d22be9f6d..4f9c8409c0e 100644 --- a/chromium/components/history/content/browser/web_contents_top_sites_observer.h +++ b/chromium/components/history/content/browser/web_contents_top_sites_observer.h @@ -37,6 +37,8 @@ class WebContentsTopSitesObserver // Underlying TopSites instance, may be null during testing. TopSites* top_sites_; + WEB_CONTENTS_USER_DATA_KEY_DECL(); + DISALLOW_COPY_AND_ASSIGN(WebContentsTopSitesObserver); }; diff --git a/chromium/components/history/core/browser/BUILD.gn b/chromium/components/history/core/browser/BUILD.gn index 1fa054df751..347fbd32ec8 100644 --- a/chromium/components/history/core/browser/BUILD.gn +++ b/chromium/components/history/core/browser/BUILD.gn @@ -168,6 +168,7 @@ bundle_data("unit_tests_bundle_data") { "//components/test/data/history/TopSites.v1.sql", "//components/test/data/history/TopSites.v2.sql", "//components/test/data/history/TopSites.v3.sql", + "//components/test/data/history/TopSites.v4.sql", "//components/test/data/history/history.22.sql", "//components/test/data/history/history.26.sql", "//components/test/data/history/history.27.sql", diff --git a/chromium/components/history/core/browser/android/android_history_types.cc b/chromium/components/history/core/browser/android/android_history_types.cc index 4f304469268..36d44f77480 100644 --- a/chromium/components/history/core/browser/android/android_history_types.cc +++ b/chromium/components/history/core/browser/android/android_history_types.cc @@ -6,7 +6,7 @@ #include <stddef.h> -#include "base/macros.h" +#include "base/stl_util.h" #include "sql/statement.h" namespace history { @@ -37,9 +37,9 @@ class BookmarkIDMapping public: BookmarkIDMapping() { static_assert( - arraysize(kAndroidBookmarkColumn) <= HistoryAndBookmarkRow::COLUMN_END, + base::size(kAndroidBookmarkColumn) <= HistoryAndBookmarkRow::COLUMN_END, "kAndroidBookmarkColumn should not have more than COLUMN_END elements"); - for (size_t i = 0; i < arraysize(kAndroidBookmarkColumn); ++i) { + for (size_t i = 0; i < base::size(kAndroidBookmarkColumn); ++i) { (*this)[kAndroidBookmarkColumn[i]] = static_cast<HistoryAndBookmarkRow::ColumnID>(i); } @@ -53,10 +53,10 @@ BookmarkIDMapping* g_bookmark_id_mapping = NULL; class SearchIDMapping : public std::map<std::string, SearchRow::ColumnID> { public: SearchIDMapping() { - static_assert(arraysize(kAndroidSearchColumn) <= SearchRow::COLUMN_END, + static_assert(base::size(kAndroidSearchColumn) <= SearchRow::COLUMN_END, "kAndroidSearchColumn should not have more than " "COLUMN_END elements"); - for (size_t i = 0; i < arraysize(kAndroidSearchColumn); ++i) { + for (size_t i = 0; i < base::size(kAndroidSearchColumn); ++i) { (*this)[kAndroidSearchColumn[i]] = static_cast<SearchRow::ColumnID>(i); } } diff --git a/chromium/components/history/core/browser/android/android_urls_sql_handler.cc b/chromium/components/history/core/browser/android/android_urls_sql_handler.cc index 31d22999f40..db1f0d53bfb 100644 --- a/chromium/components/history/core/browser/android/android_urls_sql_handler.cc +++ b/chromium/components/history/core/browser/android/android_urls_sql_handler.cc @@ -5,7 +5,7 @@ #include "components/history/core/browser/android/android_urls_sql_handler.h" #include "base/logging.h" -#include "base/macros.h" +#include "base/stl_util.h" #include "components/history/core/browser/android/android_urls_database.h" namespace history { @@ -20,9 +20,8 @@ const HistoryAndBookmarkRow::ColumnID kInterestingColumns[] = { AndroidURLsSQLHandler::AndroidURLsSQLHandler( AndroidURLsDatabase* android_urls_db) - : SQLHandler(kInterestingColumns, arraysize(kInterestingColumns)), - android_urls_db_(android_urls_db) { -} + : SQLHandler(kInterestingColumns, base::size(kInterestingColumns)), + android_urls_db_(android_urls_db) {} AndroidURLsSQLHandler::~AndroidURLsSQLHandler() { } diff --git a/chromium/components/history/core/browser/android/favicon_sql_handler.cc b/chromium/components/history/core/browser/android/favicon_sql_handler.cc index 28205cf07ba..08577647b5d 100644 --- a/chromium/components/history/core/browser/android/favicon_sql_handler.cc +++ b/chromium/components/history/core/browser/android/favicon_sql_handler.cc @@ -5,9 +5,9 @@ #include "components/history/core/browser/android/favicon_sql_handler.h" #include "base/logging.h" -#include "base/macros.h" #include "base/memory/ref_counted.h" #include "base/memory/ref_counted_memory.h" +#include "base/stl_util.h" #include "components/history/core/browser/thumbnail_database.h" using base::Time; @@ -23,9 +23,8 @@ const HistoryAndBookmarkRow::ColumnID kInterestingColumns[] = { } // namespace FaviconSQLHandler::FaviconSQLHandler(ThumbnailDatabase* thumbnail_db) - : SQLHandler(kInterestingColumns, arraysize(kInterestingColumns)), - thumbnail_db_(thumbnail_db) { -} + : SQLHandler(kInterestingColumns, base::size(kInterestingColumns)), + thumbnail_db_(thumbnail_db) {} FaviconSQLHandler::~FaviconSQLHandler() { } diff --git a/chromium/components/history/core/browser/android/urls_sql_handler.cc b/chromium/components/history/core/browser/android/urls_sql_handler.cc index 2dfd6c5dd41..292c13b6a8d 100644 --- a/chromium/components/history/core/browser/android/urls_sql_handler.cc +++ b/chromium/components/history/core/browser/android/urls_sql_handler.cc @@ -5,7 +5,7 @@ #include "components/history/core/browser/android/urls_sql_handler.h" #include "base/logging.h" -#include "base/macros.h" +#include "base/stl_util.h" #include "components/history/core/browser/url_database.h" using base::Time; @@ -21,9 +21,8 @@ const HistoryAndBookmarkRow::ColumnID kInterestingColumns[] = { } // namespace UrlsSQLHandler::UrlsSQLHandler(URLDatabase* url_db) - : SQLHandler(kInterestingColumns, arraysize(kInterestingColumns)), - url_db_(url_db) { -} + : SQLHandler(kInterestingColumns, base::size(kInterestingColumns)), + url_db_(url_db) {} UrlsSQLHandler:: ~UrlsSQLHandler() { } diff --git a/chromium/components/history/core/browser/android/visit_sql_handler.cc b/chromium/components/history/core/browser/android/visit_sql_handler.cc index 5af06e70487..21353449f26 100644 --- a/chromium/components/history/core/browser/android/visit_sql_handler.cc +++ b/chromium/components/history/core/browser/android/visit_sql_handler.cc @@ -7,7 +7,7 @@ #include <stdint.h> #include "base/logging.h" -#include "base/macros.h" +#include "base/stl_util.h" #include "components/history/core/browser/url_database.h" #include "components/history/core/browser/visit_database.h" @@ -25,10 +25,9 @@ const HistoryAndBookmarkRow::ColumnID kInterestingColumns[] = { } // namespace VisitSQLHandler::VisitSQLHandler(URLDatabase* url_db, VisitDatabase* visit_db) - : SQLHandler(kInterestingColumns, arraysize(kInterestingColumns)), + : SQLHandler(kInterestingColumns, base::size(kInterestingColumns)), url_db_(url_db), - visit_db_(visit_db) { -} + visit_db_(visit_db) {} VisitSQLHandler::~VisitSQLHandler() { } diff --git a/chromium/components/history/core/browser/expire_history_backend.cc b/chromium/components/history/core/browser/expire_history_backend.cc index 1f9575071cc..60c3f8a0775 100644 --- a/chromium/components/history/core/browser/expire_history_backend.cc +++ b/chromium/components/history/core/browser/expire_history_backend.cc @@ -9,6 +9,7 @@ #include <algorithm> #include <functional> #include <limits> +#include <utility> #include "base/bind.h" #include "base/compiler_specific.h" @@ -125,10 +126,10 @@ constexpr base::TimeDelta kExpirationSleepWakeupThreshold = // should be cleared. const int kClearOnDemandFaviconsIntervalHours = 24; -bool IsAnyURLBookmarked(HistoryBackendClient* backend_client, - const std::vector<GURL>& urls) { +bool IsAnyURLPinned(HistoryBackendClient* backend_client, + const std::vector<GURL>& urls) { for (const GURL& url : urls) { - if (backend_client->IsBookmarked(url)) + if (backend_client->IsPinnedURL(url)) return true; } return false; @@ -192,11 +193,11 @@ void ExpireHistoryBackend::DeleteURLs(const std::vector<GURL>& urls) { DeleteEffects effects; for (auto url = urls.begin(); url != urls.end(); ++url) { - const bool is_bookmarked = - backend_client_ && backend_client_->IsBookmarked(*url); + const bool is_pinned = + backend_client_ && backend_client_->IsPinnedURL(*url); URLRow url_row; - if (!main_db_->GetRowForURL(*url, &url_row) && !is_bookmarked) { - // If the URL isn't in the database and not bookmarked, we should still + if (!main_db_->GetRowForURL(*url, &url_row) && !is_pinned) { + // If the URL isn't in the database and not pinned, we should still // check to see if any favicons need to be deleted. DeleteIcons(*url, &effects); continue; @@ -214,7 +215,7 @@ void ExpireHistoryBackend::DeleteURLs(const std::vector<GURL>& urls) { // URL, and not starting with visits in a given time range). We // therefore need to call the deletion and favicon update // functions manually. - DeleteOneURL(url_row, is_bookmarked, &effects); + DeleteOneURL(url_row, is_pinned, &effects); } DeleteFaviconsIfPossible(&effects); @@ -226,7 +227,8 @@ void ExpireHistoryBackend::DeleteURLs(const std::vector<GURL>& urls) { void ExpireHistoryBackend::ExpireHistoryBetween( const std::set<GURL>& restrict_urls, base::Time begin_time, - base::Time end_time) { + base::Time end_time, + bool user_initiated) { if (!main_db_) return; @@ -245,7 +247,9 @@ void ExpireHistoryBackend::ExpireHistoryBetween( } } DeletionTimeRange time_range(begin_time, end_time); - ExpireVisitsInternal(visits, time_range, restrict_urls); + ExpireVisitsInternal( + visits, time_range, restrict_urls, + user_initiated ? DELETION_USER_INITIATED : DELETION_EXPIRED); } void ExpireHistoryBackend::ExpireHistoryForTimes( @@ -268,13 +272,15 @@ void ExpireHistoryBackend::ExpireHistoryForTimes( } void ExpireHistoryBackend::ExpireVisits(const VisitVector& visits) { - ExpireVisitsInternal(visits, DeletionTimeRange::Invalid(), {}); + ExpireVisitsInternal(visits, DeletionTimeRange::Invalid(), {}, + DELETION_USER_INITIATED); } void ExpireHistoryBackend::ExpireVisitsInternal( const VisitVector& visits, const DeletionTimeRange& time_range, - const std::set<GURL>& restrict_urls) { + const std::set<GURL>& restrict_urls, + DeletionType type) { if (visits.empty()) return; @@ -292,7 +298,7 @@ void ExpireHistoryBackend::ExpireVisitsInternal( ExpireURLsForVisits(visits_and_redirects, &effects); DeleteFaviconsIfPossible(&effects); BroadcastNotifications( - &effects, DELETION_USER_INITIATED, time_range, + &effects, type, time_range, restrict_urls.empty() ? base::Optional<std::set<GURL>>() : restrict_urls); // Pick up any bits possibly left over. @@ -403,6 +409,7 @@ VisitVector ExpireHistoryBackend::GetVisitsAndRedirectParents( visits_and_redirects.push_back(current_visit); } while (current_visit.referring_visit && + !(current_visit.transition & ui::PAGE_TRANSITION_CHAIN_START) && main_db_->GetRowForVisit(current_visit.referring_visit, ¤t_visit)); } @@ -425,14 +432,14 @@ void ExpireHistoryBackend::DeleteVisitRelatedInfo(const VisitVector& visits, } void ExpireHistoryBackend::DeleteOneURL(const URLRow& url_row, - bool is_bookmarked, + bool is_pinned, DeleteEffects* effects) { main_db_->DeleteSegmentForURL(url_row.id()); effects->deleted_urls.push_back(url_row); - // If the URL is bookmarked we should still keep its favicon around to show - // in bookmark-related UI. We'll delete this icon if the URL is unbookmarked. - // (See comments in DeleteURLs().) - if (!is_bookmarked) + // If the URL is pinned we should still keep its favicon around to show + // in the UI. We'll delete this icon if the URL is unpinned. (See comments in + // DeleteURLs().) + if (!is_pinned) DeleteIcons(url_row.url(), effects); main_db_->DeleteURLRow(url_row.id()); } @@ -493,12 +500,12 @@ void ExpireHistoryBackend::ExpireURLsForVisits(const VisitVector& visits, else url_row.set_last_visit(base::Time()); - // Don't delete URLs with visits still in the DB, or bookmarked. - bool is_bookmarked = - (backend_client_ && backend_client_->IsBookmarked(url_row.url())); - if (!is_bookmarked && url_row.last_visit().is_null()) { - // Not bookmarked and no more visits. Nuke the url. - DeleteOneURL(url_row, is_bookmarked, effects); + // Don't delete URLs with visits still in the DB, or pinned. + bool is_pinned = + (backend_client_ && backend_client_->IsPinnedURL(url_row.url())); + if (!is_pinned && url_row.last_visit().is_null()) { + // Not pinned and no more visits. Nuke the url. + DeleteOneURL(url_row, is_pinned, effects); } else { // NOTE: The calls to std::max() below are a backstop, but they should // never actually be needed unless the database is corrupt (I think). @@ -595,7 +602,7 @@ void ExpireHistoryBackend::ClearOldOnDemandFaviconsIfPossible( const IconMappingsForExpiry& mappings = id_and_mappings_pair.second; if (backend_client_ && - IsAnyURLBookmarked(backend_client_, mappings.page_urls)) { + IsAnyURLPinned(backend_client_, mappings.page_urls)) { continue; } diff --git a/chromium/components/history/core/browser/expire_history_backend.h b/chromium/components/history/core/browser/expire_history_backend.h index c936412effc..8c95f374da7 100644 --- a/chromium/components/history/core/browser/expire_history_backend.h +++ b/chromium/components/history/core/browser/expire_history_backend.h @@ -5,6 +5,7 @@ #ifndef COMPONENTS_HISTORY_CORE_BROWSER_EXPIRE_HISTORY_BACKEND_H_ #define COMPONENTS_HISTORY_CORE_BROWSER_EXPIRE_HISTORY_BACKEND_H_ +#include <map> #include <memory> #include <set> #include <vector> @@ -57,7 +58,7 @@ class ExpireHistoryBackend { // The delegate pointer must be non-null. We will NOT take ownership of it. // HistoryBackendClient may be null. The HistoryBackendClient is used when // expiring URLS so that we don't remove any URLs or favicons that are - // bookmarked (visits are removed though). + // bookmarked or have a credential saved on (visits are removed though). ExpireHistoryBackend(HistoryBackendNotifier* notifier, HistoryBackendClient* backend_client, scoped_refptr<base::SequencedTaskRunner> task_runner); @@ -80,7 +81,9 @@ class ExpireHistoryBackend { // Removes all visits to restrict_urls (or all URLs if empty) in the given // time range, updating the URLs accordingly. void ExpireHistoryBetween(const std::set<GURL>& restrict_urls, - base::Time begin_time, base::Time end_time); + base::Time begin_time, + base::Time end_time, + bool user_initiated); // Removes all visits to all URLs with the given times, updating the // URLs accordingly. |times| must be in reverse chronological order @@ -168,9 +171,9 @@ class ExpireHistoryBackend { // // Assumes the main_db_ is non-NULL. // - // NOTE: If the url is bookmarked, we keep the favicons and thumbnails. + // NOTE: If the url is pinned, we keep the favicons and thumbnails. void DeleteOneURL(const URLRow& url_row, - bool is_bookmarked, + bool is_pinned, DeleteEffects* effects); // Deletes all favicons associated with |gurl|. @@ -197,17 +200,6 @@ class ExpireHistoryBackend { // any now-unused favicons. void ExpireURLsForVisits(const VisitVector& visits, DeleteEffects* effects); - void ExpireVisitsInternal(const VisitVector& visits, - const DeletionTimeRange& time_range, - const std::set<GURL>& restrict_urls); - - // Deletes the favicons listed in |effects->affected_favicons| if they are - // unsued. Fails silently (we don't care about favicons so much, so don't want - // to stop everything if it fails). Fills |expired_favicons| with the set of - // favicon urls that no longer have associated visits and were therefore - // expired. - void DeleteFaviconsIfPossible(DeleteEffects* effects); - // Enum representing what type of action resulted in the history DB deletion. enum DeletionType { // User initiated the deletion from the History UI. @@ -217,6 +209,18 @@ class ExpireHistoryBackend { DELETION_EXPIRED }; + void ExpireVisitsInternal(const VisitVector& visits, + const DeletionTimeRange& time_range, + const std::set<GURL>& restrict_urls, + DeletionType type); + + // Deletes the favicons listed in |effects->affected_favicons| if they are + // unused. Fails silently (we don't care about favicons so much, so don't want + // to stop everything if it fails). Fills |expired_favicons| with the set of + // favicon urls that no longer have associated visits and were therefore + // expired. + void DeleteFaviconsIfPossible(DeleteEffects* effects); + // Broadcasts URL modified and deleted notifications. void BroadcastNotifications(DeleteEffects* effects, DeletionType type, 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 f3cc09ea21f..3a9b995e3fb 100644 --- a/chromium/components/history/core/browser/expire_history_backend_unittest.cc +++ b/chromium/components/history/core/browser/expire_history_backend_unittest.cc @@ -15,10 +15,10 @@ #include "base/files/file_path.h" #include "base/files/file_util.h" #include "base/files/scoped_temp_dir.h" -#include "base/macros.h" #include "base/message_loop/message_loop_current.h" #include "base/run_loop.h" #include "base/scoped_observer.h" +#include "base/stl_util.h" #include "base/strings/string16.h" #include "base/strings/utf_string_conversions.h" #include "base/test/scoped_feature_list.h" @@ -93,9 +93,8 @@ class ExpireHistoryTest : public testing::Test, public HistoryBackendNotifier { // Add visits with source information. void AddExampleSourceData(const GURL& url, URLID* id); - // Returns true if the given favicon/thumbnail has an entry in the DB. + // Returns true if the given favicon has an entry in the DB. bool HasFavicon(favicon_base::FaviconID favicon_id); - bool HasThumbnail(URLID url_id); favicon_base::FaviconID GetFavicon(const GURL& page_url, favicon_base::IconType icon_type); @@ -268,15 +267,6 @@ void ExpireHistoryTest::AddExampleData(URLID url_ids[3], url_ids[2] = main_db_->AddURL(url_row3); thumb_db_->AddIconMapping(url_row3.url(), favicon2); - // Thumbnails for each URL. - gfx::Image thumbnail = CreateGoogleThumbnailForTest(); - ThumbnailScore score(0.25, true, true, PretendNow()); - - GURL gurl; - top_sites_->SetPageThumbnail(url_row1.url(), thumbnail, score); - top_sites_->SetPageThumbnail(url_row2.url(), thumbnail, score); - top_sites_->SetPageThumbnail(url_row3.url(), thumbnail, score); - // Four visits. VisitRow visit_row1; visit_row1.url_id = url_ids[0]; @@ -348,17 +338,6 @@ favicon_base::FaviconID ExpireHistoryTest::GetFavicon( return 0; } -bool ExpireHistoryTest::HasThumbnail(URLID url_id) { - // TODO(sky): fix this. This test isn't really valid for TopSites. For - // TopSites we should be checking URL always, not the id. - URLRow info; - if (!main_db_->GetURLRow(url_id, &info)) - return false; - GURL url = info.url(); - scoped_refptr<base::RefCountedMemory> data; - return top_sites_->GetPageThumbnail(url, false, &data); -} - void ExpireHistoryTest::EnsureURLInfoGone(const URLRow& row, bool expired) { // The passed in |row| must originate from |main_db_| so that its ID will be // set to what had been in effect in |main_db_| before the deletion. @@ -373,10 +352,6 @@ void ExpireHistoryTest::EnsureURLInfoGone(const URLRow& row, bool expired) { main_db_->GetVisitsForURL(row.id(), &visits); EXPECT_EQ(0U, visits.size()); - // Thumbnail should be gone. - // TODO(sky): fix this, see comment in HasThumbnail. - // EXPECT_FALSE(HasThumbnail(row.id())); - bool found_delete_notification = false; for (const auto& info : urls_deleted_notifications_) { EXPECT_EQ(expired, info.is_from_expiration()); @@ -481,14 +456,12 @@ TEST_F(ExpireHistoryTest, DISABLED_DeleteURLAndFavicon) { base::Time visit_times[4]; AddExampleData(url_ids, visit_times); - // Verify things are the way we expect with a URL row, favicon, thumbnail. + // Verify things are the way we expect with a URL row, favicon. URLRow last_row; ASSERT_TRUE(main_db_->GetURLRow(url_ids[2], &last_row)); favicon_base::FaviconID favicon_id = GetFavicon(last_row.url(), favicon_base::IconType::kFavicon); EXPECT_TRUE(HasFavicon(favicon_id)); - // TODO(sky): fix this, see comment in HasThumbnail. - // EXPECT_TRUE(HasThumbnail(url_ids[2])); VisitVector visits; main_db_->GetVisitsForURL(url_ids[2], &visits); @@ -510,14 +483,12 @@ TEST_F(ExpireHistoryTest, DeleteURLWithoutFavicon) { base::Time visit_times[4]; AddExampleData(url_ids, visit_times); - // Verify things are the way we expect with a URL row, favicon, thumbnail. + // Verify things are the way we expect with a URL row, favicon. URLRow last_row; ASSERT_TRUE(main_db_->GetURLRow(url_ids[1], &last_row)); favicon_base::FaviconID favicon_id = GetFavicon(last_row.url(), favicon_base::IconType::kFavicon); EXPECT_TRUE(HasFavicon(favicon_id)); - // TODO(sky): fix this, see comment in HasThumbnail. - // EXPECT_TRUE(HasThumbnail(url_ids[1])); VisitVector visits; main_db_->GetVisitsForURL(url_ids[1], &visits); @@ -556,10 +527,6 @@ TEST_F(ExpireHistoryTest, DeleteStarredVisitedURL) { favicon_base::FaviconID favicon_id = GetFavicon(url, favicon_base::IconType::kFavicon); EXPECT_TRUE(HasFavicon(favicon_id)); - - // Should still have the thumbnail. - // TODO(sky): fix this, see comment in HasThumbnail. - // ASSERT_TRUE(HasThumbnail(url_row.id())); } // DeleteURL should not delete the favicon of bookmarked URLs. @@ -595,20 +562,17 @@ TEST_F(ExpireHistoryTest, DeleteURLs) { base::Time visit_times[4]; AddExampleData(url_ids, visit_times); - // Verify things are the way we expect with URL rows, favicons, - // thumbnails. + // Verify things are the way we expect with URL rows, favicons. URLRow rows[3]; favicon_base::FaviconID favicon_ids[3]; std::vector<GURL> urls; // Push back a bogus URL (which shouldn't change anything). urls.push_back(GURL()); - for (size_t i = 0; i < arraysize(rows); ++i) { + for (size_t i = 0; i < base::size(rows); ++i) { ASSERT_TRUE(main_db_->GetURLRow(url_ids[i], &rows[i])); favicon_ids[i] = GetFavicon(rows[i].url(), favicon_base::IconType::kFavicon); EXPECT_TRUE(HasFavicon(favicon_ids[i])); - // TODO(sky): fix this, see comment in HasThumbnail. - // EXPECT_TRUE(HasThumbnail(url_ids[i])); urls.push_back(rows[i].url()); } @@ -643,7 +607,8 @@ TEST_F(ExpireHistoryTest, FlushRecentURLsUnstarred) { // This should delete the last two visits. std::set<GURL> restrict_urls; - expirer_.ExpireHistoryBetween(restrict_urls, visit_times[2], base::Time()); + expirer_.ExpireHistoryBetween(restrict_urls, visit_times[2], base::Time(), + /*user_initiated*/ true); EXPECT_EQ(GetLastDeletionInfo()->time_range().begin(), visit_times[2]); EXPECT_EQ(GetLastDeletionInfo()->time_range().end(), base::Time()); @@ -663,12 +628,10 @@ TEST_F(ExpireHistoryTest, FlushRecentURLsUnstarred) { EXPECT_EQ(1, url_row1.typed_count()); EXPECT_EQ(0, temp_row.typed_count()); - // Verify that the middle URL's favicon and thumbnail is still there. + // Verify that the middle URL's favicon is still there. favicon_base::FaviconID favicon_id = GetFavicon(url_row1.url(), favicon_base::IconType::kFavicon); EXPECT_TRUE(HasFavicon(favicon_id)); - // TODO(sky): fix this, see comment in HasThumbnail. - // EXPECT_TRUE(HasThumbnail(url_row1.id())); // Verify that the last URL was deleted. favicon_base::FaviconID favicon_id2 = @@ -698,7 +661,8 @@ TEST_F(ExpireHistoryTest, FlushURLsUnstarredBetweenTwoTimestamps) { // This should delete the two visits of the url_ids[1]. std::set<GURL> restrict_urls; - expirer_.ExpireHistoryBetween(restrict_urls, visit_times[1], visit_times[3]); + expirer_.ExpireHistoryBetween(restrict_urls, visit_times[1], visit_times[3], + /*user_initiated*/ true); main_db_->GetVisitsForURL(url_ids[0], &visits); EXPECT_EQ(1U, visits.size()); @@ -713,19 +677,15 @@ TEST_F(ExpireHistoryTest, FlushURLsUnstarredBetweenTwoTimestamps) { EnsureURLInfoGone(url_row1, false); EXPECT_FALSE(HasFavicon(favicon_id1)); - // Verify that the url_ids[0]'s favicon and thumbnail are still there. + // Verify that the url_ids[0]'s favicon are still there. favicon_base::FaviconID favicon_id0 = GetFavicon(url_row0.url(), favicon_base::IconType::kFavicon); EXPECT_TRUE(HasFavicon(favicon_id0)); - // TODO(sky): fix this, see comment in HasThumbnail. - // EXPECT_TRUE(HasThumbnail(url_row0.id())); - // Verify that the url_ids[2]'s favicon and thumbnail are still there. + // Verify that the url_ids[2]'s favicon are still there. favicon_base::FaviconID favicon_id2 = GetFavicon(url_row2.url(), favicon_base::IconType::kFavicon); EXPECT_TRUE(HasFavicon(favicon_id2)); - // TODO(sky): fix this, see comment in HasThumbnail. - // EXPECT_TRUE(HasThumbnail(url_row2.id())); } // Expires all URLs more recent than a given time, with no starred items. @@ -747,7 +707,7 @@ TEST_F(ExpireHistoryTest, FlushRecentURLsUnstarredWithMaxTime) { // This should delete the last two visits. std::set<GURL> restrict_urls; expirer_.ExpireHistoryBetween(restrict_urls, visit_times[2], - base::Time::Max()); + base::Time::Max(), /*user_initiated*/ true); // Verify that the middle URL had its last visit deleted only. visits.clear(); @@ -765,12 +725,10 @@ TEST_F(ExpireHistoryTest, FlushRecentURLsUnstarredWithMaxTime) { EXPECT_EQ(1, url_row1.typed_count()); EXPECT_EQ(0, temp_row.typed_count()); - // Verify that the middle URL's favicon and thumbnail is still there. + // Verify that the middle URL's favicon is still there. favicon_base::FaviconID favicon_id = GetFavicon(url_row1.url(), favicon_base::IconType::kFavicon); EXPECT_TRUE(HasFavicon(favicon_id)); - // TODO(sky): fix this, see comment in HasThumbnail. - // EXPECT_TRUE(HasThumbnail(url_row1.id())); // Verify that the last URL was deleted. favicon_base::FaviconID favicon_id2 = @@ -795,7 +753,8 @@ TEST_F(ExpireHistoryTest, FlushAllURLsUnstarred) { // This should delete all URL visits. std::set<GURL> restrict_urls; - expirer_.ExpireHistoryBetween(restrict_urls, base::Time(), base::Time::Max()); + expirer_.ExpireHistoryBetween(restrict_urls, base::Time(), base::Time::Max(), + /*user_initiated*/ true); // Verify that all URL visits deleted. visits.clear(); @@ -853,12 +812,10 @@ TEST_F(ExpireHistoryTest, FlushURLsForTimes) { EXPECT_EQ(1, url_row1.typed_count()); EXPECT_EQ(0, temp_row.typed_count()); - // Verify that the middle URL's favicon and thumbnail is still there. + // Verify that the middle URL's favicon is still there. favicon_base::FaviconID favicon_id = GetFavicon(url_row1.url(), favicon_base::IconType::kFavicon); EXPECT_TRUE(HasFavicon(favicon_id)); - // TODO(sky): fix this, see comment in HasThumbnail. - // EXPECT_TRUE(HasThumbnail(url_row1.id())); // Verify that the last URL was deleted. favicon_base::FaviconID favicon_id2 = @@ -885,7 +842,8 @@ TEST_F(ExpireHistoryTest, FlushRecentURLsUnstarredRestricted) { // This should delete the last two visits. std::set<GURL> restrict_urls = {url_row1.url()}; - expirer_.ExpireHistoryBetween(restrict_urls, visit_times[2], base::Time()); + expirer_.ExpireHistoryBetween(restrict_urls, visit_times[2], base::Time(), + /*user_initiated*/ true); EXPECT_EQ(GetLastDeletionInfo()->time_range().begin(), visit_times[2]); EXPECT_EQ(GetLastDeletionInfo()->time_range().end(), base::Time()); EXPECT_EQ(GetLastDeletionInfo()->deleted_rows().size(), 0U); @@ -907,18 +865,14 @@ TEST_F(ExpireHistoryTest, FlushRecentURLsUnstarredRestricted) { EXPECT_EQ(1, url_row1.typed_count()); EXPECT_EQ(0, temp_row.typed_count()); - // Verify that the middle URL's favicon and thumbnail is still there. + // Verify that the middle URL's favicon is still there. favicon_base::FaviconID favicon_id = GetFavicon(url_row1.url(), favicon_base::IconType::kFavicon); EXPECT_TRUE(HasFavicon(favicon_id)); - // TODO(sky): fix this, see comment in HasThumbnail. - // EXPECT_TRUE(HasThumbnail(url_row1.id())); // Verify that the last URL was not touched. EXPECT_TRUE(main_db_->GetURLRow(url_ids[2], &temp_row)); EXPECT_TRUE(HasFavicon(favicon_id)); - // TODO(sky): fix this, see comment in HasThumbnail. - // EXPECT_TRUE(HasThumbnail(url_row2.id())); } // Expire a starred URL, it shouldn't get deleted @@ -937,7 +891,8 @@ TEST_F(ExpireHistoryTest, FlushRecentURLsStarred) { // This should delete the last two visits. std::set<GURL> restrict_urls; - expirer_.ExpireHistoryBetween(restrict_urls, visit_times[2], base::Time()); + expirer_.ExpireHistoryBetween(restrict_urls, visit_times[2], base::Time(), + /*user_initiated*/ true); // The URL rows should still exist. URLRow new_url_row1, new_url_row2; @@ -963,12 +918,23 @@ TEST_F(ExpireHistoryTest, FlushRecentURLsStarred) { favicon_base::FaviconID favicon_id = GetFavicon(url_row1.url(), favicon_base::IconType::kFavicon); EXPECT_TRUE(HasFavicon(favicon_id)); - // TODO(sky): fix this, see comment in HasThumbnail. - // EXPECT_TRUE(HasThumbnail(new_url_row1.id())); favicon_id = GetFavicon(url_row1.url(), favicon_base::IconType::kFavicon); EXPECT_TRUE(HasFavicon(favicon_id)); - // TODO(sky): fix this, see comment in HasThumbnail. - // EXPECT_TRUE(HasThumbnail(new_url_row2.id())); +} + +TEST_F(ExpireHistoryTest, ExpireHistoryBetweenPropagatesUserInitiated) { + URLID url_ids[3]; + base::Time visit_times[4]; + AddExampleData(url_ids, visit_times); + std::set<GURL> restrict_urls; + + expirer_.ExpireHistoryBetween(restrict_urls, visit_times[3], base::Time(), + /*user_initiated*/ true); + EXPECT_FALSE(GetLastDeletionInfo()->is_from_expiration()); + + expirer_.ExpireHistoryBetween(restrict_urls, visit_times[1], base::Time(), + /*user_initiated*/ false); + EXPECT_TRUE(GetLastDeletionInfo()->is_from_expiration()); } TEST_F(ExpireHistoryTest, ExpireHistoryBeforeUnstarred) { @@ -1253,12 +1219,15 @@ TEST_F(ExpireHistoryTest, DeleteVisitAndRedirects) { VisitRow visit_row1; visit_row1.url_id = url1; visit_row1.visit_time = now - base::TimeDelta::FromDays(1); + visit_row1.transition = ui::PAGE_TRANSITION_CHAIN_START; + main_db_->AddVisit(&visit_row1, SOURCE_BROWSED); VisitRow visit_row2; visit_row2.url_id = url2; visit_row2.visit_time = now; visit_row2.referring_visit = visit_row1.visit_id; + visit_row1.transition = ui::PAGE_TRANSITION_CHAIN_END; main_db_->AddVisit(&visit_row2, SOURCE_BROWSED); // Expiring visit_row2 should also expire visit_row1 which is its redirect @@ -1292,12 +1261,14 @@ TEST_F(ExpireHistoryTest, DeleteVisitAndRedirectsWithLoop) { VisitRow visit_row1; visit_row1.url_id = url1; visit_row1.visit_time = now - base::TimeDelta::FromDays(1); + visit_row1.transition = ui::PAGE_TRANSITION_CHAIN_START; main_db_->AddVisit(&visit_row1, SOURCE_BROWSED); VisitRow visit_row2; visit_row2.url_id = url2; visit_row2.visit_time = now; visit_row2.referring_visit = visit_row1.visit_id; + visit_row1.transition = ui::PAGE_TRANSITION_CHAIN_END; main_db_->AddVisit(&visit_row2, SOURCE_BROWSED); // Set the first visit to be redirect parented to the second visit. @@ -1316,6 +1287,50 @@ TEST_F(ExpireHistoryTest, DeleteVisitAndRedirectsWithLoop) { EXPECT_FALSE(main_db_->GetURLRow(url2, &u)); } +// Test that visits that are referers but not part of a redirect chain don't +// get deleted. See crbug.com/919488. +TEST_F(ExpireHistoryTest, DeleteVisitButNotActualReferers) { + // Set up the example data. + base::Time now = PretendNow(); + URLRow url_row1(GURL("http://google.com/1")); + url_row1.set_last_visit(now - base::TimeDelta::FromDays(1)); + url_row1.set_visit_count(1); + URLID url1 = main_db_->AddURL(url_row1); + + URLRow url_row2(GURL("http://www.google.com/1")); + url_row2.set_last_visit(now); + url_row2.set_visit_count(1); + URLID url2 = main_db_->AddURL(url_row2); + + // Add a visit to "http://google.com/1" that is a referer to + // "http://www.google.com/1". But both are separate redirect chains. + VisitRow visit_row1; + visit_row1.url_id = url1; + visit_row1.visit_time = now - base::TimeDelta::FromDays(1); + visit_row1.transition = ui::PageTransitionFromInt( + ui::PAGE_TRANSITION_CHAIN_START | ui::PAGE_TRANSITION_CHAIN_END); + main_db_->AddVisit(&visit_row1, SOURCE_BROWSED); + + VisitRow visit_row2; + visit_row2.url_id = url2; + visit_row2.visit_time = now; + visit_row2.referring_visit = visit_row1.visit_id; + visit_row2.transition = ui::PageTransitionFromInt( + ui::PAGE_TRANSITION_CHAIN_START | ui::PAGE_TRANSITION_CHAIN_END); + main_db_->AddVisit(&visit_row2, SOURCE_BROWSED); + + // Expiring visit_row2 should not expire visit_row1 which is its referer + // parent. + expirer_.ExpireVisits({visit_row2}); + + VisitRow v; + EXPECT_TRUE(main_db_->GetRowForVisit(visit_row1.visit_id, &v)); + EXPECT_FALSE(main_db_->GetRowForVisit(visit_row2.visit_id, &v)); + URLRow u; + EXPECT_TRUE(main_db_->GetURLRow(url1, &u)); + EXPECT_FALSE(main_db_->GetURLRow(url2, &u)); +} + // TODO(brettw) add some visits with no URL to make sure everything is updated // properly. Have the visits also refer to nonexistent FTS rows. // diff --git a/chromium/components/history/core/browser/history_backend.cc b/chromium/components/history/core/browser/history_backend.cc index 00cf677be8f..0ddac915671 100644 --- a/chromium/components/history/core/browser/history_backend.cc +++ b/chromium/components/history/core/browser/history_backend.cc @@ -7,7 +7,6 @@ #include <algorithm> #include <functional> #include <limits> -#include <list> #include <map> #include <memory> #include <set> @@ -773,13 +772,10 @@ void HistoryBackend::InitImpl( void HistoryBackend::OnMemoryPressure( base::MemoryPressureListener::MemoryPressureLevel memory_pressure_level) { - bool trim_aggressively = - memory_pressure_level == - base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL; if (db_) - db_->TrimMemory(trim_aggressively); + db_->TrimMemory(); if (thumbnail_db_) - thumbnail_db_->TrimMemory(trim_aggressively); + thumbnail_db_->TrimMemory(); } void HistoryBackend::CloseAllDatabases() { @@ -1669,7 +1665,7 @@ void HistoryBackend::MergeFavicon( // Sync calls MergeFavicon() for all of the favicons that it manages at // startup. Do not update the "last updated" time if the favicon bitmap // data matches that in the database. - // TODO: Pass in boolean to MergeFavicon() if any users of + // TODO(pkotwicz): Pass in boolean to MergeFavicon() if any users of // MergeFavicon() want the last_updated time to be updated when the new // bitmap data is identical to the old. bitmap_identical = true; @@ -1939,8 +1935,9 @@ void HistoryBackend::SetImportedFavicons( // save the favicon mapping. This will match with what history db does // for regular bookmarked URLs with favicons - when history db is // cleaned, we keep an entry in the db with 0 visits as long as that - // url is bookmarked. - if (backend_client_ && backend_client_->IsBookmarked(*url)) { + // url is bookmarked. The same is applicable to the saved credential's + // URLs. + if (backend_client_ && backend_client_->IsPinnedURL(*url)) { URLRow url_info(*url); url_info.set_visit_count(0); url_info.set_typed_count(0); @@ -2441,7 +2438,8 @@ void HistoryBackend::DeleteURL(const GURL& url) { void HistoryBackend::ExpireHistoryBetween(const std::set<GURL>& restrict_urls, Time begin_time, - Time end_time) { + Time end_time, + bool user_initiated) { if (!db_) return; @@ -2452,7 +2450,8 @@ void HistoryBackend::ExpireHistoryBetween(const std::set<GURL>& restrict_urls, DeleteAllHistory(); } else { // Clearing parts of history, have the expirer do the depend - expirer_.ExpireHistoryBetween(restrict_urls, begin_time, end_time); + expirer_.ExpireHistoryBetween(restrict_urls, begin_time, end_time, + user_initiated); // Force a commit, if the user is deleting something for privacy reasons, // we want to get it on disk ASAP. @@ -2517,7 +2516,8 @@ void HistoryBackend::ExpireHistory( bool update_first_recorded_time = false; for (auto it = expire_list.begin(); it != expire_list.end(); ++it) { - expirer_.ExpireHistoryBetween(it->urls, it->begin_time, it->end_time); + expirer_.ExpireHistoryBetween(it->urls, it->begin_time, it->end_time, + true); if (it->begin_time < first_recorded_time_) update_first_recorded_time = true; @@ -2672,39 +2672,38 @@ void HistoryBackend::NotifyURLsDeleted(DeletionInfo deletion_info) { void HistoryBackend::DeleteAllHistory() { // Our approach to deleting all history is: - // 1. Copy the bookmarks and their dependencies to new tables with temporary - // names. + // 1. Copy the pinned URLs and their dependencies to new tables with + // temporary names. // 2. Delete the original tables. Since tables can not share pages, we know // that any data we don't want to keep is now in an unused page. // 3. Renaming the temporary tables to match the original. // 4. Vacuuming the database to delete the unused pages. // - // Since we are likely to have very few bookmarks and their dependencies + // Since we are likely to have very few pinned URLs and their dependencies // compared to all history, this is also much faster than just deleting from // the original tables directly. - // Get the bookmarked URLs. - std::vector<URLAndTitle> starred_url_and_titles; + // Get the pinned URLs. + std::vector<URLAndTitle> pinned_url; if (backend_client_) - backend_client_->GetBookmarks(&starred_url_and_titles); + pinned_url = backend_client_->GetPinnedURLs(); URLRows kept_url_rows; std::vector<GURL> starred_urls; - for (const URLAndTitle& url_and_title : starred_url_and_titles) { - const GURL& url = url_and_title.url; - starred_urls.push_back(url); - + for (URLAndTitle& url_and_title : pinned_url) { URLRow row; - if (db_->GetRowForURL(url, &row)) { + if (db_->GetRowForURL(url_and_title.url, &row)) { // Clear the last visit time so when we write these rows they are "clean." row.set_last_visit(Time()); row.set_visit_count(0); row.set_typed_count(0); kept_url_rows.push_back(row); } + + starred_urls.push_back(std::move(url_and_title.url)); } - // Delete all cached favicons which are not used by bookmarks. + // Delete all cached favicons which are not used by the UI. if (!ClearAllThumbnailHistory(starred_urls)) { LOG(ERROR) << "Thumbnail history could not be cleared"; // We continue in this error case. If the user wants to delete their @@ -2749,7 +2748,7 @@ bool HistoryBackend::ClearAllThumbnailHistory( } #if defined(OS_ANDROID) - // TODO (michaelbai): Add the unit test once AndroidProviderBackend is + // TODO(michaelbai): Add the unit test once AndroidProviderBackend is // available in HistoryBackend. db_->ClearAndroidURLRows(); #endif diff --git a/chromium/components/history/core/browser/history_backend.h b/chromium/components/history/core/browser/history_backend.h index c64e3c247b5..2a125ad697b 100644 --- a/chromium/components/history/core/browser/history_backend.h +++ b/chromium/components/history/core/browser/history_backend.h @@ -8,6 +8,7 @@ #include <stddef.h> #include <stdint.h> +#include <list> #include <memory> #include <set> #include <string> @@ -421,7 +422,8 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>, // Calls ExpireHistoryBackend::ExpireHistoryBetween and commits the change. void ExpireHistoryBetween(const std::set<GURL>& restrict_urls, base::Time begin_time, - base::Time end_time); + base::Time end_time, + bool user_initiated); // Finds the URLs visited at |times| and expires all their visits within // [|begin_time|, |end_time|). All times in |times| should be in diff --git a/chromium/components/history/core/browser/history_backend_client.h b/chromium/components/history/core/browser/history_backend_client.h index 512170478d2..29da78a9120 100644 --- a/chromium/components/history/core/browser/history_backend_client.h +++ b/chromium/components/history/core/browser/history_backend_client.h @@ -32,14 +32,12 @@ class HistoryBackendClient { HistoryBackendClient() {} virtual ~HistoryBackendClient() {} - // Returns true if the specified URL is bookmarked. - virtual bool IsBookmarked(const GURL& url) = 0; - - // Returns, by reference in |bookmarks|, the set of bookmarked URLs and their - // title. This returns the unique set of URLs. For example, if two bookmarks - // reference the same URL only one entry is added even if the title are not - // the same. - virtual void GetBookmarks(std::vector<URLAndTitle>* bookmarks) = 0; + // Returns true if the specified URL is pinned due to being bookmarked or used + // by the password manager. + virtual bool IsPinnedURL(const GURL& url) = 0; + + // Returns the set of pinned URLs with their titles. + virtual std::vector<URLAndTitle> GetPinnedURLs() = 0; // Returns whether database errors should be reported to the crash server. virtual bool ShouldReportDatabaseError() = 0; diff --git a/chromium/components/history/core/browser/history_backend_unittest.cc b/chromium/components/history/core/browser/history_backend_unittest.cc index 4f735fa9c6f..f83a69392e6 100644 --- a/chromium/components/history/core/browser/history_backend_unittest.cc +++ b/chromium/components/history/core/browser/history_backend_unittest.cc @@ -19,13 +19,13 @@ #include "base/files/file_path.h" #include "base/files/file_util.h" #include "base/files/scoped_temp_dir.h" -#include "base/macros.h" #include "base/memory/ptr_util.h" #include "base/memory/ref_counted.h" #include "base/metrics/histogram_base.h" #include "base/metrics/histogram_samples.h" #include "base/metrics/statistics_recorder.h" #include "base/run_loop.h" +#include "base/stl_util.h" #include "base/strings/string16.h" #include "base/strings/string_number_conversions.h" #include "base/strings/utf_string_conversions.h" @@ -941,8 +941,8 @@ TEST_F(HistoryBackendTest, KeywordGenerated) { // Expire the visits. std::set<GURL> restrict_urls; - backend_->expire_backend()->ExpireHistoryBetween(restrict_urls, visit_time, - base::Time::Now()); + backend_->expire_backend()->ExpireHistoryBetween( + restrict_urls, visit_time, base::Time::Now(), /*user_initiated*/ true); // The visit should have been nuked. visits.clear(); @@ -1165,8 +1165,8 @@ TEST_F(HistoryBackendTest, ImportedFaviconsTest) { rows.push_back(row2); backend_->AddPagesWithDetails(rows, history::SOURCE_BROWSED); URLRow url_row1, url_row2; - EXPECT_FALSE(backend_->db_->GetRowForURL(row1.url(), &url_row1) == 0); - EXPECT_FALSE(backend_->db_->GetRowForURL(row2.url(), &url_row2) == 0); + EXPECT_NE(0, backend_->db_->GetRowForURL(row1.url(), &url_row1)); + EXPECT_NE(0, backend_->db_->GetRowForURL(row2.url(), &url_row2)); EXPECT_EQ(1u, NumIconMappingsForPageURL(row1.url(), IconType::kFavicon)); EXPECT_EQ(0u, NumIconMappingsForPageURL(row2.url(), IconType::kFavicon)); @@ -1181,8 +1181,8 @@ TEST_F(HistoryBackendTest, ImportedFaviconsTest) { favicon.urls.insert(row2.url()); favicons.push_back(favicon); backend_->SetImportedFavicons(favicons); - EXPECT_FALSE(backend_->db_->GetRowForURL(row1.url(), &url_row1) == 0); - EXPECT_FALSE(backend_->db_->GetRowForURL(row2.url(), &url_row2) == 0); + EXPECT_NE(0, backend_->db_->GetRowForURL(row1.url(), &url_row1)); + EXPECT_NE(0, backend_->db_->GetRowForURL(row2.url(), &url_row2)); std::vector<IconMapping> mappings; EXPECT_TRUE(backend_->thumbnail_db_->GetIconMappingsForPageURL( @@ -1207,13 +1207,13 @@ TEST_F(HistoryBackendTest, ImportedFaviconsTest) { favicons.push_back(favicon); backend_->SetImportedFavicons(favicons); URLRow url_row3; - EXPECT_TRUE(backend_->db_->GetRowForURL(url3, &url_row3) == 0); + EXPECT_EQ(0, backend_->db_->GetRowForURL(url3, &url_row3)); // If the URL is bookmarked, it should get added to history with 0 visits. history_client_.AddBookmark(url3); backend_->SetImportedFavicons(favicons); - EXPECT_FALSE(backend_->db_->GetRowForURL(url3, &url_row3) == 0); - EXPECT_TRUE(url_row3.visit_count() == 0); + EXPECT_NE(0, backend_->db_->GetRowForURL(url3, &url_row3)); + EXPECT_EQ(0, url_row3.visit_count()); } #endif // !defined(OS_ANDROID) @@ -3411,7 +3411,7 @@ TEST_F(HistoryBackendTest, ExpireHistoryForTimes) { ASSERT_TRUE(backend_.get()); HistoryAddPageArgs args[10]; - for (size_t i = 0; i < arraysize(args); ++i) { + for (size_t i = 0; i < base::size(args); ++i) { args[i].url = GURL("http://example" + std::string((i % 2 == 0 ? ".com" : ".net"))); args[i].time = base::Time::FromInternalValue(i); @@ -3420,7 +3420,7 @@ TEST_F(HistoryBackendTest, ExpireHistoryForTimes) { EXPECT_EQ(base::Time(), backend_->GetFirstRecordedTimeForTest()); URLRow row; - for (size_t i = 0; i < arraysize(args); ++i) { + for (size_t i = 0; i < base::size(args); ++i) { EXPECT_TRUE(backend_->GetURL(args[i].url, &row)); } @@ -3469,14 +3469,14 @@ TEST_F(HistoryBackendTest, ExpireHistory) { // Insert 4 entries into the database. HistoryAddPageArgs args[4]; - for (size_t i = 0; i < arraysize(args); ++i) { + for (size_t i = 0; i < base::size(args); ++i) { args[i].url = GURL("http://example" + base::NumberToString(i) + ".com"); args[i].time = reference_time + base::TimeDelta::FromDays(i); backend_->AddPage(args[i]); } URLRow url_rows[4]; - for (unsigned int i = 0; i < arraysize(args); ++i) + for (unsigned int i = 0; i < base::size(args); ++i) ASSERT_TRUE(backend_->GetURL(args[i].url, &url_rows[i])); std::vector<ExpireHistoryArgs> expire_list; @@ -3510,7 +3510,7 @@ TEST_F(HistoryBackendTest, ExpireHistory) { EXPECT_EQ(backend_->GetFirstRecordedTimeForTest(), args[1].time); // Now delete the rest of the visits in one call. - for (unsigned int i = 1; i < arraysize(args); ++i) { + for (unsigned int i = 1; i < base::size(args); ++i) { expire_list.resize(expire_list.size() + 1); expire_list[i].SetTimeRangeForOneDay(args[i].time); expire_list[i].urls.insert(args[i].url); @@ -3654,7 +3654,8 @@ TEST_F(HistoryBackendTest, DatabaseErrorSynchronouslyKillAndNotifyBridge) { // history. backend_->ExpireHistoryBetween(/*restrict_urls=*/std::set<GURL>(), /*begin_time=*/base::Time(), - /*end_time=*/base::Time::Max()); + /*end_time=*/base::Time::Max(), + /*user_initiated*/ true); // Run loop to let the posted task to kill the DB run. base::RunLoop().RunUntilIdle(); @@ -3662,7 +3663,8 @@ TEST_F(HistoryBackendTest, DatabaseErrorSynchronouslyKillAndNotifyBridge) { // effect but it should not crash). backend_->ExpireHistoryBetween(/*restrict_urls=*/std::set<GURL>(), /*begin_time=*/base::Time(), - /*end_time=*/base::Time::Max()); + /*end_time=*/base::Time::Max(), + /*user_initiated*/ true); } // Tests that a typed navigation which results in a redirect from HTTP to HTTPS diff --git a/chromium/components/history/core/browser/history_database.cc b/chromium/components/history/core/browser/history_database.cc index 43f1da992ad..39d547f129f 100644 --- a/chromium/components/history/core/browser/history_database.cc +++ b/chromium/components/history/core/browser/history_database.cc @@ -13,7 +13,6 @@ #include <vector> #include "base/command_line.h" -#include "base/containers/hash_tables.h" #include "base/files/file_util.h" #include "base/metrics/histogram_functions.h" #include "base/metrics/histogram_macros.h" @@ -306,8 +305,8 @@ void HistoryDatabase::Vacuum() { ignore_result(db_.Execute("VACUUM")); } -void HistoryDatabase::TrimMemory(bool aggressively) { - db_.TrimMemory(aggressively); +void HistoryDatabase::TrimMemory() { + db_.TrimMemory(); } bool HistoryDatabase::Raze() { diff --git a/chromium/components/history/core/browser/history_database.h b/chromium/components/history/core/browser/history_database.h index f457fe1451d..6c552c4547b 100644 --- a/chromium/components/history/core/browser/history_database.h +++ b/chromium/components/history/core/browser/history_database.h @@ -137,9 +137,8 @@ class HistoryDatabase : public DownloadDatabase, // unused space in the file. It can be VERY SLOW. void Vacuum(); - // Try to trim the cache memory used by the database. If |aggressively| is - // true try to trim all unused cache, otherwise trim by half. - void TrimMemory(bool aggressively); + // Release all non-essential memory associated with this database connection. + void TrimMemory(); // Razes the database. Returns true if successful. bool Raze(); diff --git a/chromium/components/history/core/browser/history_querying_unittest.cc b/chromium/components/history/core/browser/history_querying_unittest.cc index 3cdc1660cd6..6f1f9503d68 100644 --- a/chromium/components/history/core/browser/history_querying_unittest.cc +++ b/chromium/components/history/core/browser/history_querying_unittest.cc @@ -9,8 +9,8 @@ #include "base/files/file_path.h" #include "base/files/file_util.h" #include "base/files/scoped_temp_dir.h" -#include "base/macros.h" #include "base/run_loop.h" +#include "base/stl_util.h" #include "base/strings/utf_string_conversions.h" #include "base/task/cancelable_task_tracker.h" #include "base/test/scoped_task_environment.h" @@ -173,7 +173,7 @@ class HistoryQueryTest : public testing::Test { // Fill the test data. base::Time now = base::Time::Now().LocalMidnight(); - for (size_t i = 0; i < arraysize(test_entries); i++) { + for (size_t i = 0; i < base::size(test_entries); i++) { test_entries[i].time = now - (test_entries[i].days_ago * base::TimeDelta::FromDays(1)); AddEntryToHistory(test_entries[i]); @@ -410,7 +410,7 @@ TEST_F(HistoryQueryTest, TextSearchIDN) { L"\u0438\u0434\u0435\u043d\u0442.\u0440\u0444"), 1, }, }; - for (size_t i = 0; i < arraysize(queries); ++i) { + for (size_t i = 0; i < base::size(queries); ++i) { QueryHistory(queries[i].query, options, &results); EXPECT_EQ(queries[i].results_size, results.size()); } @@ -421,7 +421,7 @@ TEST_F(HistoryQueryTest, Paging) { // Since results are fetched 1 and 2 at a time, entry #0 and #6 will not // be de-duplicated. int expected_results[] = { 4, 2, 3, 1, 7, 6, 5, 0 }; - TestPaging(std::string(), expected_results, arraysize(expected_results)); + TestPaging(std::string(), expected_results, base::size(expected_results)); } TEST_F(HistoryQueryTest, TextSearchPaging) { @@ -429,7 +429,7 @@ TEST_F(HistoryQueryTest, TextSearchPaging) { // be de-duplicated. Entry #4 does not contain the text "title", so it // shouldn't appear. int expected_results[] = { 2, 3, 1, 7, 6, 5 }; - TestPaging("title", expected_results, arraysize(expected_results)); + TestPaging("title", expected_results, base::size(expected_results)); } } // namespace history diff --git a/chromium/components/history/core/browser/history_service.cc b/chromium/components/history/core/browser/history_service.cc index 513a89f79c8..a737d5dd176 100644 --- a/chromium/components/history/core/browser/history_service.cc +++ b/chromium/components/history/core/browser/history_service.cc @@ -914,14 +914,11 @@ void HistoryService::Cleanup() { // // TODO(ajwong): Cleanup HistoryBackend lifetime issues. // See http://crbug.com/99767. - history_backend_->AddRef(); base::Closure closing_task = base::Bind(&HistoryBackend::Closing, history_backend_); ScheduleTask(PRIORITY_NORMAL, closing_task); closing_task.Reset(); - HistoryBackend* raw_ptr = history_backend_.get(); - history_backend_ = nullptr; - backend_task_runner_->ReleaseSoon(FROM_HERE, raw_ptr); + backend_task_runner_->ReleaseSoon(FROM_HERE, std::move(history_backend_)); } // Clear |backend_task_runner_| to make sure it's not used after Cleanup(). @@ -1093,27 +1090,28 @@ void HistoryService::ExpireHistoryBetween( const std::set<GURL>& restrict_urls, Time begin_time, Time end_time, - const base::Closure& callback, + bool user_initiated, + base::OnceClosure callback, base::CancelableTaskTracker* tracker) { DCHECK(backend_task_runner_) << "History service being called after cleanup"; DCHECK(thread_checker_.CalledOnValidThread()); tracker->PostTaskAndReply( backend_task_runner_.get(), FROM_HERE, base::BindOnce(&HistoryBackend::ExpireHistoryBetween, history_backend_, - restrict_urls, begin_time, end_time), - callback); + restrict_urls, begin_time, end_time, user_initiated), + std::move(callback)); } void HistoryService::ExpireHistory( const std::vector<ExpireHistoryArgs>& expire_list, - const base::Closure& callback, + base::OnceClosure callback, base::CancelableTaskTracker* tracker) { DCHECK(backend_task_runner_) << "History service being called after cleanup"; DCHECK(thread_checker_.CalledOnValidThread()); tracker->PostTaskAndReply(backend_task_runner_.get(), FROM_HERE, base::BindOnce(&HistoryBackend::ExpireHistory, history_backend_, expire_list), - callback); + std::move(callback)); } void HistoryService::ExpireHistoryBeforeForTesting( @@ -1134,7 +1132,8 @@ void HistoryService::ExpireLocalAndRemoteHistoryBetween( const std::set<GURL>& restrict_urls, Time begin_time, Time end_time, - const base::Closure& callback, + bool user_initiated, + base::OnceClosure callback, base::CancelableTaskTracker* tracker) { // TODO(dubroy): This should be factored out into a separate class that // dispatches deletions to the proper places. @@ -1178,7 +1177,8 @@ void HistoryService::ExpireLocalAndRemoteHistoryBetween( base::Bind(&ExpireWebHistoryComplete), partial_traffic_annotation); } - ExpireHistoryBetween(restrict_urls, begin_time, end_time, callback, tracker); + ExpireHistoryBetween(restrict_urls, begin_time, end_time, user_initiated, + std::move(callback), tracker); } void HistoryService::OnDBLoaded() { diff --git a/chromium/components/history/core/browser/history_service.h b/chromium/components/history/core/browser/history_service.h index 7200ea0dae6..69c681257bb 100644 --- a/chromium/components/history/core/browser/history_service.h +++ b/chromium/components/history/core/browser/history_service.h @@ -339,7 +339,8 @@ class HistoryService : public syncer::SyncableService, public KeyedService { void ExpireHistoryBetween(const std::set<GURL>& restrict_urls, base::Time begin_time, base::Time end_time, - const base::Closure& callback, + bool user_initiated, + base::OnceClosure callback, base::CancelableTaskTracker* tracker); // Removes all visits to specified URLs in specific time ranges. @@ -347,7 +348,7 @@ class HistoryService : public syncer::SyncableService, public KeyedService { // vector. The fields of |ExpireHistoryArgs| map directly to the arguments of // of ExpireHistoryBetween(). void ExpireHistory(const std::vector<ExpireHistoryArgs>& expire_list, - const base::Closure& callback, + base::OnceClosure callback, base::CancelableTaskTracker* tracker); // Expires all visits before and including the given time, updating the URLs @@ -363,7 +364,8 @@ class HistoryService : public syncer::SyncableService, public KeyedService { const std::set<GURL>& restrict_urls, base::Time begin_time, base::Time end_time, - const base::Closure& callback, + bool user_initiated, + base::OnceClosure callback, base::CancelableTaskTracker* tracker); // Processes the given |delete_directive| and sends it to the diff --git a/chromium/components/history/core/browser/history_types.cc b/chromium/components/history/core/browser/history_types.cc index 2ba16c63f41..a7cc78b4daf 100644 --- a/chromium/components/history/core/browser/history_types.cc +++ b/chromium/components/history/core/browser/history_types.cc @@ -177,10 +177,8 @@ QueryURLResult::~QueryURLResult() { MostVisitedURL::MostVisitedURL() {} -MostVisitedURL::MostVisitedURL(const GURL& url, - const base::string16& title, - base::Time last_forced_time) - : url(url), title(title), last_forced_time(last_forced_time) {} +MostVisitedURL::MostVisitedURL(const GURL& url, const base::string16& title) + : url(url), title(title) {} MostVisitedURL::MostVisitedURL(const GURL& url, const base::string16& title, @@ -230,14 +228,6 @@ FilteredURL::~FilteredURL() {} FilteredURL::ExtendedInfo::ExtendedInfo() = default; -// Images --------------------------------------------------------------------- - -Images::Images() {} - -Images::Images(const Images& other) = default; - -Images::~Images() {} - // TopSitesDelta -------------------------------------------------------------- TopSitesDelta::TopSitesDelta() {} @@ -292,18 +282,6 @@ HistoryAddPageArgs::HistoryAddPageArgs(const HistoryAddPageArgs& other) = HistoryAddPageArgs::~HistoryAddPageArgs() {} -// ThumbnailMigration --------------------------------------------------------- - -ThumbnailMigration::ThumbnailMigration() {} - -ThumbnailMigration::~ThumbnailMigration() {} - -// MostVisitedThumbnails ------------------------------------------------------ - -MostVisitedThumbnails::MostVisitedThumbnails() {} - -MostVisitedThumbnails::~MostVisitedThumbnails() {} - // IconMapping ---------------------------------------------------------------- IconMapping::IconMapping() {} diff --git a/chromium/components/history/core/browser/history_types.h b/chromium/components/history/core/browser/history_types.h index 3a83b8bb696..325dee1bfff 100644 --- a/chromium/components/history/core/browser/history_types.h +++ b/chromium/components/history/core/browser/history_types.h @@ -299,9 +299,7 @@ struct VisibleVisitCountToHostResult { // Holds the per-URL information of the most visited query. struct MostVisitedURL { MostVisitedURL(); - MostVisitedURL(const GURL& url, - const base::string16& title, - base::Time last_forced_time = base::Time()); + MostVisitedURL(const GURL& url, const base::string16& title); MostVisitedURL(const GURL& url, const base::string16& title, const RedirectList& preceding_redirects); @@ -316,11 +314,6 @@ struct MostVisitedURL { GURL url; base::string16 title; - // If this is a URL for which we want to force a thumbnail, records the last - // time it was forced so we can evict it when more recent URLs are requested. - // If it's not a forced thumbnail, keep a time of 0. - base::Time last_forced_time; - RedirectList redirects; MostVisitedURL& operator=(const MostVisitedURL&); @@ -410,19 +403,6 @@ struct HistoryAddPageArgs { typedef std::vector<MostVisitedURL> MostVisitedURLList; typedef std::vector<FilteredURL> FilteredURLList; -// Used by TopSites to store the thumbnails. -struct Images { - Images(); - Images(const Images& other); - ~Images(); - - scoped_refptr<base::RefCountedMemory> thumbnail; - ThumbnailScore thumbnail_score; - - // TODO(brettw): this will eventually store the favicon. - // scoped_refptr<base::RefCountedBytes> favicon; -}; - struct MostVisitedURLWithRank { MostVisitedURL url; int rank; @@ -440,33 +420,7 @@ struct TopSitesDelta { MostVisitedURLWithRankList moved; }; -typedef std::map<GURL, scoped_refptr<base::RefCountedBytes>> URLToThumbnailMap; - -// Used when migrating most visited thumbnails out of history and into topsites. -struct ThumbnailMigration { - ThumbnailMigration(); - ~ThumbnailMigration(); - - MostVisitedURLList most_visited; - URLToThumbnailMap url_to_thumbnail_map; -}; - -typedef std::map<GURL, Images> URLToImagesMap; - -class MostVisitedThumbnails - : public base::RefCountedThreadSafe<MostVisitedThumbnails> { - public: - MostVisitedThumbnails(); - - MostVisitedURLList most_visited; - URLToImagesMap url_to_images_map; - - private: - friend class base::RefCountedThreadSafe<MostVisitedThumbnails>; - virtual ~MostVisitedThumbnails(); - - DISALLOW_COPY_AND_ASSIGN(MostVisitedThumbnails); -}; +typedef base::RefCountedData<MostVisitedURLList> MostVisitedThreadSafe; // Map from origins to a count of matching URLs and the last visited time to any // URL under that origin. diff --git a/chromium/components/history/core/browser/sync/delete_directive_handler.cc b/chromium/components/history/core/browser/sync/delete_directive_handler.cc index f7fa12a076f..f5544527f38 100644 --- a/chromium/components/history/core/browser/sync/delete_directive_handler.cc +++ b/chromium/components/history/core/browser/sync/delete_directive_handler.cc @@ -274,7 +274,8 @@ void DeleteDirectiveHandler::DeleteDirectiveTask:: // time range in directive is inclusive. history_backend->ExpireHistoryBetween( std::set<GURL>(), current_start_time, - current_end_time + base::TimeDelta::FromMicroseconds(1)); + current_end_time + base::TimeDelta::FromMicroseconds(1), + /*user_initiated*/ true); } current_start_time = directive_start_time; } @@ -285,7 +286,8 @@ void DeleteDirectiveHandler::DeleteDirectiveTask:: if (!current_start_time.is_null()) { history_backend->ExpireHistoryBetween( std::set<GURL>(), current_start_time, - current_end_time + base::TimeDelta::FromMicroseconds(1)); + current_end_time + base::TimeDelta::FromMicroseconds(1), + /*user_initiated*/ true); } } diff --git a/chromium/components/history/core/browser/sync/history_delete_directives_data_type_controller.cc b/chromium/components/history/core/browser/sync/history_delete_directives_data_type_controller.cc index f36863fb85b..7f86bc61ca1 100644 --- a/chromium/components/history/core/browser/sync/history_delete_directives_data_type_controller.cc +++ b/chromium/components/history/core/browser/sync/history_delete_directives_data_type_controller.cc @@ -7,39 +7,41 @@ #include "base/threading/thread_task_runner_handle.h" #include "components/sync/driver/sync_client.h" #include "components/sync/driver/sync_service.h" +#include "components/sync/driver/sync_user_settings.h" namespace browser_sync { HistoryDeleteDirectivesDataTypeController:: HistoryDeleteDirectivesDataTypeController(const base::Closure& dump_stack, + syncer::SyncService* sync_service, syncer::SyncClient* sync_client) - : syncer::AsyncDirectoryTypeController(syncer::HISTORY_DELETE_DIRECTIVES, - dump_stack, - sync_client, - syncer::GROUP_UI, - base::ThreadTaskRunnerHandle::Get()), - sync_client_(sync_client) {} + : syncer::AsyncDirectoryTypeController( + syncer::HISTORY_DELETE_DIRECTIVES, + dump_stack, + sync_service, + sync_client, + syncer::GROUP_UI, + base::ThreadTaskRunnerHandle::Get()) {} HistoryDeleteDirectivesDataTypeController:: ~HistoryDeleteDirectivesDataTypeController() {} bool HistoryDeleteDirectivesDataTypeController::ReadyForStart() const { DCHECK(CalledOnValidThread()); - return !sync_client_->GetSyncService()->IsEncryptEverythingEnabled(); + return !sync_service()->GetUserSettings()->IsEncryptEverythingEnabled(); } bool HistoryDeleteDirectivesDataTypeController::StartModels() { DCHECK(CalledOnValidThread()); if (DisableTypeIfNecessary()) return false; - sync_client_->GetSyncService()->AddObserver(this); + sync_service()->AddObserver(this); return true; } void HistoryDeleteDirectivesDataTypeController::StopModels() { DCHECK(CalledOnValidThread()); - if (sync_client_->GetSyncService()->HasObserver(this)) - sync_client_->GetSyncService()->RemoveObserver(this); + sync_service()->RemoveObserver(this); } void HistoryDeleteDirectivesDataTypeController::OnStateChanged( @@ -49,14 +51,13 @@ void HistoryDeleteDirectivesDataTypeController::OnStateChanged( bool HistoryDeleteDirectivesDataTypeController::DisableTypeIfNecessary() { DCHECK(CalledOnValidThread()); - if (!sync_client_->GetSyncService()->IsSyncFeatureActive()) + if (!sync_service()->IsSyncFeatureActive()) return false; if (ReadyForStart()) return false; - if (sync_client_->GetSyncService()->HasObserver(this)) - sync_client_->GetSyncService()->RemoveObserver(this); + sync_service()->RemoveObserver(this); syncer::SyncError error(FROM_HERE, syncer::SyncError::DATATYPE_POLICY_ERROR, "Delete directives not supported with encryption.", type()); diff --git a/chromium/components/history/core/browser/sync/history_delete_directives_data_type_controller.h b/chromium/components/history/core/browser/sync/history_delete_directives_data_type_controller.h index 30b8a78f0ba..c550a5e6f11 100644 --- a/chromium/components/history/core/browser/sync/history_delete_directives_data_type_controller.h +++ b/chromium/components/history/core/browser/sync/history_delete_directives_data_type_controller.h @@ -9,6 +9,11 @@ #include "components/sync/driver/async_directory_type_controller.h" #include "components/sync/driver/sync_service_observer.h" +namespace syncer { +class SyncClient; +class SyncService; +} // namespace syncer + namespace browser_sync { // A controller for delete directives, which cannot sync when full encryption @@ -19,6 +24,7 @@ class HistoryDeleteDirectivesDataTypeController public: // |dump_stack| is called when an unrecoverable error occurs. HistoryDeleteDirectivesDataTypeController(const base::Closure& dump_stack, + syncer::SyncService* sync_service, syncer::SyncClient* sync_client); ~HistoryDeleteDirectivesDataTypeController() override; @@ -35,8 +41,6 @@ class HistoryDeleteDirectivesDataTypeController // type is no longer ready, else does nothing and returns false. bool DisableTypeIfNecessary(); - syncer::SyncClient* sync_client_; - DISALLOW_COPY_AND_ASSIGN(HistoryDeleteDirectivesDataTypeController); }; diff --git a/chromium/components/history/core/browser/sync/history_delete_directives_model_type_controller.cc b/chromium/components/history/core/browser/sync/history_delete_directives_model_type_controller.cc index 07a537e62a8..faf0c238199 100644 --- a/chromium/components/history/core/browser/sync/history_delete_directives_model_type_controller.cc +++ b/chromium/components/history/core/browser/sync/history_delete_directives_model_type_controller.cc @@ -9,6 +9,7 @@ #include "base/bind.h" #include "components/sync/driver/sync_client.h" #include "components/sync/driver/sync_service.h" +#include "components/sync/driver/sync_user_settings.h" #include "components/sync/model/model_type_store_service.h" namespace browser_sync { @@ -16,6 +17,7 @@ namespace browser_sync { HistoryDeleteDirectivesModelTypeController:: HistoryDeleteDirectivesModelTypeController( const base::RepeatingClosure& dump_stack, + syncer::SyncService* sync_service, syncer::SyncClient* sync_client) : SyncableServiceBasedModelTypeController( syncer::HISTORY_DELETE_DIRECTIVES, @@ -24,14 +26,14 @@ HistoryDeleteDirectivesModelTypeController:: base::Unretained(sync_client), syncer::HISTORY_DELETE_DIRECTIVES), dump_stack), - sync_client_(sync_client) {} + sync_service_(sync_service) {} HistoryDeleteDirectivesModelTypeController:: ~HistoryDeleteDirectivesModelTypeController() {} bool HistoryDeleteDirectivesModelTypeController::ReadyForStart() const { DCHECK(CalledOnValidThread()); - return !sync_client_->GetSyncService()->IsEncryptEverythingEnabled(); + return !sync_service_->GetUserSettings()->IsEncryptEverythingEnabled(); } void HistoryDeleteDirectivesModelTypeController::LoadModels( @@ -44,7 +46,7 @@ void HistoryDeleteDirectivesModelTypeController::LoadModels( return; } - sync_client_->GetSyncService()->AddObserver(this); + sync_service_->AddObserver(this); SyncableServiceBasedModelTypeController::LoadModels(configure_context, model_load_callback); } @@ -54,9 +56,7 @@ void HistoryDeleteDirectivesModelTypeController::Stop( StopCallback callback) { DCHECK(CalledOnValidThread()); - if (sync_client_->GetSyncService()->HasObserver(this)) { - sync_client_->GetSyncService()->RemoveObserver(this); - } + sync_service_->RemoveObserver(this); SyncableServiceBasedModelTypeController::Stop(shutdown_reason, std::move(callback)); @@ -71,7 +71,7 @@ void HistoryDeleteDirectivesModelTypeController::OnStateChanged( bool HistoryDeleteDirectivesModelTypeController::DisableTypeIfNecessary() { DCHECK(CalledOnValidThread()); - if (!sync_client_->GetSyncService()->IsSyncFeatureActive()) { + if (!sync_service_->IsSyncFeatureActive()) { return false; } @@ -79,9 +79,7 @@ bool HistoryDeleteDirectivesModelTypeController::DisableTypeIfNecessary() { return false; } - if (sync_client_->GetSyncService()->HasObserver(this)) { - sync_client_->GetSyncService()->RemoveObserver(this); - } + sync_service_->RemoveObserver(this); ReportModelError( syncer::SyncError::DATATYPE_POLICY_ERROR, diff --git a/chromium/components/history/core/browser/sync/history_delete_directives_model_type_controller.h b/chromium/components/history/core/browser/sync/history_delete_directives_model_type_controller.h index 0e113f6c6ff..07910a19aed 100644 --- a/chromium/components/history/core/browser/sync/history_delete_directives_model_type_controller.h +++ b/chromium/components/history/core/browser/sync/history_delete_directives_model_type_controller.h @@ -12,6 +12,7 @@ namespace syncer { class SyncClient; +class SyncService; } // namespace syncer namespace browser_sync { @@ -22,9 +23,11 @@ class HistoryDeleteDirectivesModelTypeController : public syncer::SyncableServiceBasedModelTypeController, public syncer::SyncServiceObserver { public: - // |sync_client| must not be null and must outlive this object. + // |sync_service| and |sync_client| must not be null and must outlive this + // object. HistoryDeleteDirectivesModelTypeController( const base::RepeatingClosure& dump_stack, + syncer::SyncService* sync_service, syncer::SyncClient* sync_client); ~HistoryDeleteDirectivesModelTypeController() override; @@ -43,7 +46,7 @@ class HistoryDeleteDirectivesModelTypeController // type is no longer ready, else does nothing and returns false. bool DisableTypeIfNecessary(); - syncer::SyncClient* const sync_client_; + syncer::SyncService* const sync_service_; DISALLOW_COPY_AND_ASSIGN(HistoryDeleteDirectivesModelTypeController); }; diff --git a/chromium/components/history/core/browser/sync/typed_url_sync_bridge_unittest.cc b/chromium/components/history/core/browser/sync/typed_url_sync_bridge_unittest.cc index be842970287..96855200f4c 100644 --- a/chromium/components/history/core/browser/sync/typed_url_sync_bridge_unittest.cc +++ b/chromium/components/history/core/browser/sync/typed_url_sync_bridge_unittest.cc @@ -4,10 +4,12 @@ #include "components/history/core/browser/sync/typed_url_sync_bridge.h" +#include <algorithm> #include <memory> #include "base/big_endian.h" #include "base/files/scoped_temp_dir.h" +#include "base/stl_util.h" #include "base/strings/utf_string_conversions.h" #include "base/test/scoped_task_environment.h" #include "base/threading/thread_task_runner_handle.h" @@ -21,6 +23,8 @@ #include "components/sync/model/sync_metadata_store.h" #include "testing/gtest/include/gtest/gtest.h" +using base::Time; +using base::TimeDelta; using sync_pb::TypedUrlSpecifics; using syncer::DataBatch; using syncer::EntityChange; @@ -51,7 +55,12 @@ const char kTitle2[] = "cookie"; const char kURL[] = "http://pie.com/"; const char kURL2[] = "http://cookie.com/"; -bool URLsEqual(URLRow& row, const sync_pb::TypedUrlSpecifics& specifics) { +Time SinceEpoch(int64_t microseconds_since_epoch) { + return Time::FromDeltaSinceWindowsEpoch( + TimeDelta::FromMicroseconds(microseconds_since_epoch)); +} + +bool URLsEqual(const URLRow& row, const sync_pb::TypedUrlSpecifics& specifics) { return ((row.url().spec().compare(specifics.url()) == 0) && (base::UTF16ToUTF8(row.title()).compare(specifics.title()) == 0) && (row.hidden() == specifics.hidden())); @@ -69,7 +78,7 @@ void AddNewestVisit(ui::PageTransition transition, int64_t visit_time, URLRow* url, VisitVector* visits) { - base::Time time = base::Time::FromInternalValue(visit_time); + Time time = SinceEpoch(visit_time); visits->insert(visits->begin(), VisitRow(url->id(), time, 0, transition, 0, HistoryBackend::IsTypedIncrement(transition))); @@ -83,10 +92,10 @@ void AddNewestVisit(ui::PageTransition transition, } void AddOldestVisit(ui::PageTransition transition, - int64_t visit_time, + int visit_time, URLRow* url, VisitVector* visits) { - base::Time time = base::Time::FromInternalValue(visit_time); + Time time = SinceEpoch(visit_time); visits->push_back(VisitRow(url->id(), time, 0, transition, 0, HistoryBackend::IsTypedIncrement(transition))); @@ -112,15 +121,15 @@ URLRow MakeTypedUrlRow(const std::string& url, history_url.set_typed_count(typed_count); history_url.set_hidden(hidden); - base::Time last_visit_time = base::Time::FromInternalValue(last_visit); + Time last_visit_time = SinceEpoch(last_visit); history_url.set_last_visit(last_visit_time); if (typed_count > 0) { - // Add a typed visit for time |last_visit|. + // Add a typed visit for time |last_visit_time|. visits->push_back(VisitRow(history_url.id(), last_visit_time, 0, ui::PAGE_TRANSITION_TYPED, 0, true)); } else { - // Add a non-typed visit for time |last_visit|. + // Add a non-typed visit for time |last_visit_time|. visits->push_back(VisitRow(history_url.id(), last_visit_time, 0, ui::PAGE_TRANSITION_RELOAD, 0, false)); } @@ -145,8 +154,8 @@ URLRow MakeTypedUrlRowWithTwoVisits(const std::string& url, history_url.set_visit_count(2); history_url.set_hidden(hidden); - base::Time typed_visit_time = base::Time::FromInternalValue(typed_visit); - base::Time reload_visit_time = base::Time::FromInternalValue(reload_visit); + Time typed_visit_time = SinceEpoch(typed_visit); + Time reload_visit_time = SinceEpoch(reload_visit); history_url.set_last_visit(std::max(typed_visit_time, reload_visit_time)); @@ -208,7 +217,7 @@ class TestHistoryBackendDelegate : public HistoryBackend::Delegate { void NotifyURLVisited(ui::PageTransition transition, const URLRow& row, const RedirectList& redirects, - base::Time visit_time) override {} + Time visit_time) override {} void NotifyURLsModified(const URLRows& changed_urls) override {} void NotifyURLsDeleted(DeletionInfo deletion_info) override {} void NotifyKeywordSearchTermUpdated(const URLRow& row, @@ -228,18 +237,18 @@ class TestHistoryBackend : public HistoryBackend { nullptr, base::ThreadTaskRunnerHandle::Get()) {} - bool IsExpiredVisitTime(const base::Time& time) override { - return time.ToInternalValue() == kExpiredVisit; + bool IsExpiredVisitTime(const Time& time) override { + return time.ToDeltaSinceWindowsEpoch().InMicroseconds() == kExpiredVisit; } URLID GetIdByUrl(const GURL& gurl) { return db()->GetRowForURL(gurl, nullptr); } - void SetVisitsForUrl(URLRow& new_url, const VisitVector visits) { - if (!GetURL(new_url.url(), nullptr)) { + void SetVisitsForUrl(URLRow* new_url, const VisitVector visits) { + if (!GetURL(new_url->url(), nullptr)) { URLRows new_urls; - new_urls.push_back(new_url); + new_urls.push_back(*new_url); AddPagesWithDetails(new_urls, SOURCE_SYNCED); } @@ -247,8 +256,8 @@ class TestHistoryBackend : public HistoryBackend { for (const auto& visit : visits) { added_visits.push_back(VisitInfo(visit.visit_time, visit.transition)); } - AddVisits(new_url.url(), added_visits, SOURCE_SYNCED); - new_url.set_id(GetIdByUrl(new_url.url())); + AddVisits(new_url->url(), added_visits, SOURCE_SYNCED); + new_url->set_id(GetIdByUrl(new_url->url())); } private: @@ -315,7 +324,7 @@ class TypedURLSyncBridgeTest : public testing::Test { visit_vectors->push_back(visits); rows->push_back(MakeTypedUrlRow(urls[i], kTitle, typed, i + 3, false, &visit_vectors->back())); - fake_history_backend_->SetVisitsForUrl(rows->back(), + fake_history_backend_->SetVisitsForUrl(&rows->back(), visit_vectors->back()); changed_urls.push_back(rows->back()); } @@ -457,7 +466,7 @@ class TypedURLSyncBridgeTest : public testing::Test { } static VisitRow CreateVisit(ui::PageTransition type, int64_t timestamp) { - return VisitRow(0, base::Time::FromInternalValue(timestamp), 0, type, 0, + return VisitRow(0, SinceEpoch(timestamp), 0, type, 0, HistoryBackend::IsTypedIncrement(type)); } @@ -519,9 +528,9 @@ TEST_F(TypedURLSyncBridgeTest, GetAllData) { URLRow row2 = MakeTypedUrlRow(kURL2, kTitle2, 2, 4, false, &visits2); URLRow expired_row = MakeTypedUrlRow("http://expired.com/", kTitle, 1, kExpiredVisit, false, &visits3); - fake_history_backend_->SetVisitsForUrl(row1, visits1); - fake_history_backend_->SetVisitsForUrl(row2, visits2); - fake_history_backend_->SetVisitsForUrl(expired_row, visits3); + fake_history_backend_->SetVisitsForUrl(&row1, visits1); + fake_history_backend_->SetVisitsForUrl(&row2, visits2); + fake_history_backend_->SetVisitsForUrl(&expired_row, visits3); // Create the same data in sync. TypedUrlSpecifics typed_url1, typed_url2; @@ -538,8 +547,8 @@ TEST_F(TypedURLSyncBridgeTest, GetData) { VisitVector visits1, visits2; URLRow row1 = MakeTypedUrlRow(kURL, kTitle, 1, 3, false, &visits1); URLRow row2 = MakeTypedUrlRow(kURL2, kTitle2, 2, 4, false, &visits2); - fake_history_backend_->SetVisitsForUrl(row1, visits1); - fake_history_backend_->SetVisitsForUrl(row2, visits2); + fake_history_backend_->SetVisitsForUrl(&row1, visits1); + fake_history_backend_->SetVisitsForUrl(&row2, visits2); // Create the same data in sync. TypedUrlSpecifics typed_url1, typed_url2; @@ -557,7 +566,7 @@ TEST_F(TypedURLSyncBridgeTest, MergeUrlNoChange) { // Add a url to backend. VisitVector visits; URLRow row = MakeTypedUrlRow(kURL, kTitle, 1, 3, false, &visits); - fake_history_backend_->SetVisitsForUrl(row, visits); + fake_history_backend_->SetVisitsForUrl(&row, visits); // Create the same data in sync. sync_pb::EntitySpecifics entity_specifics; @@ -591,7 +600,7 @@ TEST_F(TypedURLSyncBridgeTest, MergeUrlNoTypedUrl) { // Mark typed_count to 1 even when there is no typed url visit. row.set_typed_count(1); - fake_history_backend_->SetVisitsForUrl(row, visits); + fake_history_backend_->SetVisitsForUrl(&row, visits); StartSyncing(std::vector<TypedUrlSpecifics>()); EXPECT_TRUE(processor().put_multimap().empty()); @@ -606,7 +615,7 @@ TEST_F(TypedURLSyncBridgeTest, MergeUrlEmptySync) { // Add a url to backend. VisitVector visits; URLRow row = MakeTypedUrlRow(kURL, kTitle, 1, 3, false, &visits); - fake_history_backend_->SetVisitsForUrl(row, visits); + fake_history_backend_->SetVisitsForUrl(&row, visits); StartSyncing(std::vector<TypedUrlSpecifics>()); @@ -654,7 +663,7 @@ TEST_F(TypedURLSyncBridgeTest, MergeUrlEmptyLocal) { // Check that the backend was updated correctly. VisitVector all_visits; - base::Time server_time = base::Time::FromInternalValue(3); + Time server_time = SinceEpoch(3); URLID url_id = fake_history_backend_->GetIdByUrl(GURL(kURL)); ASSERT_NE(0, url_id); fake_history_backend_->GetVisitsForURL(url_id, &all_visits); @@ -673,7 +682,7 @@ TEST_F(TypedURLSyncBridgeTest, MergeUrlOldLocal) { // Add a url to backend. VisitVector visits; URLRow local_row = MakeTypedUrlRow(kURL, kTitle, 1, 3, false, &visits); - fake_history_backend_->SetVisitsForUrl(local_row, visits); + fake_history_backend_->SetVisitsForUrl(&local_row, visits); // Create sync data for the same url with a more recent visit. VisitVector server_visits; @@ -687,7 +696,7 @@ TEST_F(TypedURLSyncBridgeTest, MergeUrlOldLocal) { // Check that the backend was updated correctly. VisitVector all_visits; - base::Time server_time = base::Time::FromInternalValue(6); + Time server_time = SinceEpoch(6); URLID url_id = fake_history_backend_->GetIdByUrl(GURL(kURL)); ASSERT_NE(0, url_id); fake_history_backend_->GetVisitsForURL(url_id, &all_visits); @@ -721,7 +730,7 @@ TEST_F(TypedURLSyncBridgeTest, MergeUrlOldSync) { // Add a url to backend. VisitVector visits; URLRow local_row = MakeTypedUrlRow(kURL, kTitle2, 1, 3, false, &visits); - fake_history_backend_->SetVisitsForUrl(local_row, visits); + fake_history_backend_->SetVisitsForUrl(&local_row, visits); // Create sync data for the same url with an older visit. VisitVector server_visits; @@ -734,7 +743,7 @@ TEST_F(TypedURLSyncBridgeTest, MergeUrlOldSync) { // Check that the backend was not updated. VisitVector all_visits; - base::Time local_visit_time = base::Time::FromInternalValue(3); + Time local_visit_time = SinceEpoch(3); URLID url_id = fake_history_backend_->GetIdByUrl(GURL(kURL)); ASSERT_NE(0, url_id); fake_history_backend_->GetVisitsForURL(url_id, &all_visits); @@ -765,7 +774,7 @@ TEST_F(TypedURLSyncBridgeTest, MergeUrlsWithUsernameAndPassword) { // Add a url to backend. VisitVector visits; URLRow local_row = MakeTypedUrlRow(kURL, kTitle2, 1, 3, false, &visits); - fake_history_backend_->SetVisitsForUrl(local_row, visits); + fake_history_backend_->SetVisitsForUrl(&local_row, visits); // Create sync data for the same url but contain username and password. VisitVector server_visits; @@ -780,8 +789,7 @@ TEST_F(TypedURLSyncBridgeTest, MergeUrlsWithUsernameAndPassword) { // Notify typed url sync service of the update. bridge()->OnURLVisited(fake_history_backend_.get(), ui::PAGE_TRANSITION_TYPED, - server_row, RedirectList(), - base::Time::FromInternalValue(7)); + server_row, RedirectList(), SinceEpoch(7)); // Check username/password url is not synced. ASSERT_EQ(1U, processor().put_multimap().size()); @@ -793,7 +801,7 @@ TEST_F(TypedURLSyncBridgeTest, SimpleMerge) { // Add a url to backend. VisitVector visits1; URLRow row1 = MakeTypedUrlRow(kURL, kTitle, 1, 3, false, &visits1); - fake_history_backend_->SetVisitsForUrl(row1, visits1); + fake_history_backend_->SetVisitsForUrl(&row1, visits1); // Create the sync data. VisitVector visits2; @@ -811,8 +819,8 @@ TEST_F(TypedURLSyncBridgeTest, SimpleMerge) { ASSERT_NE(0, url_id); fake_history_backend_->GetVisitsForURL(url_id, &all_visits); ASSERT_EQ(2U, all_visits.size()); - EXPECT_EQ(base::Time::FromInternalValue(3), all_visits[0].visit_time); - EXPECT_EQ(base::Time::FromInternalValue(4), all_visits[1].visit_time); + EXPECT_EQ(SinceEpoch(3), all_visits[0].visit_time); + EXPECT_EQ(SinceEpoch(4), all_visits[1].visit_time); EXPECT_TRUE(ui::PageTransitionTypeIncludingQualifiersIs( all_visits[0].transition, visits1[0].transition)); EXPECT_TRUE(ui::PageTransitionTypeIncludingQualifiersIs( @@ -867,7 +875,7 @@ TEST_F(TypedURLSyncBridgeTest, UpdateLocalTypedUrl) { AddNewestVisit(ui::PAGE_TRANSITION_TYPED, 7, &url_row, &visits); AddNewestVisit(ui::PAGE_TRANSITION_RELOAD, 8, &url_row, &visits); AddNewestVisit(ui::PAGE_TRANSITION_LINK, 9, &url_row, &visits); - fake_history_backend_->SetVisitsForUrl(url_row, visits); + fake_history_backend_->SetVisitsForUrl(&url_row, visits); changed_urls.push_back(url_row); // Notify typed url sync service of the update. @@ -913,13 +921,13 @@ TEST_F(TypedURLSyncBridgeTest, ReloadVisitLocalTypedUrl) { URLRows changed_urls; VisitVector new_visits; AddNewestVisit(ui::PAGE_TRANSITION_RELOAD, 7, &url_row, &new_visits); - fake_history_backend_->SetVisitsForUrl(url_row, new_visits); + fake_history_backend_->SetVisitsForUrl(&url_row, new_visits); changed_urls.push_back(url_row); // Notify typed url sync service of the update. bridge()->OnURLVisited(fake_history_backend_.get(), ui::PAGE_TRANSITION_RELOAD, url_row, RedirectList(), - base::Time::FromInternalValue(7)); + SinceEpoch(7)); // No change pass to processor ASSERT_EQ(1U, changes_multimap.size()); } @@ -941,12 +949,12 @@ TEST_F(TypedURLSyncBridgeTest, LinkVisitLocalTypedUrl) { URLRow url_row = url_rows.front(); VisitVector new_visits; AddNewestVisit(ui::PAGE_TRANSITION_LINK, 6, &url_row, &new_visits); - fake_history_backend_->SetVisitsForUrl(url_row, new_visits); + fake_history_backend_->SetVisitsForUrl(&url_row, new_visits); ui::PageTransition transition = ui::PAGE_TRANSITION_LINK; // Notify typed url sync service of non-typed visit, expect no change. bridge()->OnURLVisited(fake_history_backend_.get(), transition, url_row, - RedirectList(), base::Time::FromInternalValue(6)); + RedirectList(), SinceEpoch(6)); // No change pass to processor ASSERT_EQ(1U, changes_multimap.size()); } @@ -967,14 +975,14 @@ TEST_F(TypedURLSyncBridgeTest, TypedVisitLocalTypedUrl) { AddOldestVisit(ui::PAGE_TRANSITION_LINK, 1, &url_row, &visits); AddNewestVisit(ui::PAGE_TRANSITION_LINK, 6, &url_row, &visits); AddNewestVisit(ui::PAGE_TRANSITION_TYPED, 7, &url_row, &visits); - fake_history_backend_->SetVisitsForUrl(url_row, visits); + fake_history_backend_->SetVisitsForUrl(&url_row, visits); // Notify typed url sync service of typed visit. const auto& changes_multimap = processor().put_multimap(); ASSERT_EQ(0U, changes_multimap.size()); ui::PageTransition transition = ui::PAGE_TRANSITION_TYPED; bridge()->OnURLVisited(fake_history_backend_.get(), transition, url_row, - RedirectList(), base::Time::Now()); + RedirectList(), Time()); ASSERT_EQ(1U, changes_multimap.size()); sync_pb::TypedUrlSpecifics url_specifics = GetLastUpdateForURL(kURL); @@ -1044,16 +1052,17 @@ TEST_F(TypedURLSyncBridgeTest, DeleteLocalTypedUrlVisit) { /*hidden=*/false, &visits1); URLRow row2 = MakeTypedUrlRow(kURL2, kTitle2, /*typed_count=*/2, /*last_visit=*/10, false, &visits2); - fake_history_backend_->SetVisitsForUrl(row1, visits1); - fake_history_backend_->SetVisitsForUrl(row2, visits2); + fake_history_backend_->SetVisitsForUrl(&row1, visits1); + fake_history_backend_->SetVisitsForUrl(&row2, visits2); StartSyncing({}); // Simulate deletion of the last typed visit (e.g. by clearing browsing data), // the deletion must get synced up. - fake_history_backend_->ExpireHistoryBetween( - {}, /*begin_time=*/base::Time::FromInternalValue(1), - /*end_time=*/base::Time::FromInternalValue(3)); + fake_history_backend_->ExpireHistoryBetween({}, + /*begin_time=*/SinceEpoch(1), + /*end_time=*/SinceEpoch(3), + /*user_initiated*/ true); URLRow row1_updated; ASSERT_TRUE(fake_history_backend_->GetURL(GURL(kURL), &row1_updated)); URLRows changed_urls{row1_updated}; @@ -1154,12 +1163,12 @@ TEST_F(TypedURLSyncBridgeTest, MaxVisitLocalTypedUrl) { for (; i <= kMaxTypedUrlVisits + 10; ++i) AddNewestVisit(ui::PAGE_TRANSITION_TYPED, i, &url_row, &visits); - fake_history_backend_->SetVisitsForUrl(url_row, visits); + fake_history_backend_->SetVisitsForUrl(&url_row, visits); // Notify typed url sync service of typed visit. ui::PageTransition transition = ui::PAGE_TRANSITION_TYPED; bridge()->OnURLVisited(fake_history_backend_.get(), transition, url_row, - RedirectList(), base::Time::Now()); + RedirectList(), Time()); ASSERT_EQ(1U, changes_multimap.size()); sync_pb::TypedUrlSpecifics url_specifics = GetLastUpdateForURL(kURL); @@ -1205,12 +1214,12 @@ TEST_F(TypedURLSyncBridgeTest, ThrottleVisitLocalTypedUrl) { int i = 1; for (; i < kVisitThrottleThreshold + kVisitThrottleMultiple / 2; ++i) AddNewestVisit(ui::PAGE_TRANSITION_TYPED, i, &url_row, &visits); - fake_history_backend_->SetVisitsForUrl(url_row, visits); + fake_history_backend_->SetVisitsForUrl(&url_row, visits); // Notify typed url sync service of typed visit. ui::PageTransition transition = ui::PAGE_TRANSITION_TYPED; bridge()->OnURLVisited(fake_history_backend_.get(), transition, url_row, - RedirectList(), base::Time::Now()); + RedirectList(), Time()); // Should throttle, so sync and local cache should not update. ASSERT_EQ(0U, changes_multimap.size()); @@ -1219,11 +1228,11 @@ TEST_F(TypedURLSyncBridgeTest, ThrottleVisitLocalTypedUrl) { for (; i % kVisitThrottleMultiple != 1; ++i) AddNewestVisit(ui::PAGE_TRANSITION_TYPED, i, &url_row, &visits); --i; // Account for the increment before the condition ends. - fake_history_backend_->SetVisitsForUrl(url_row, visits); + fake_history_backend_->SetVisitsForUrl(&url_row, visits); // Notify typed url sync service of typed visit. bridge()->OnURLVisited(fake_history_backend_.get(), transition, url_row, - RedirectList(), base::Time::Now()); + RedirectList(), Time()); ASSERT_EQ(1U, changes_multimap.size()); sync_pb::TypedUrlSpecifics url_specifics = GetLastUpdateForURL(kURL); @@ -1250,7 +1259,7 @@ TEST_F(TypedURLSyncBridgeTest, AddUrlAndVisits) { EXPECT_EQ(it->second->specifics.typed_url().url(), kURL); EXPECT_EQ(it->second->specifics.typed_url().title(), kTitle); - base::Time visit_time = base::Time::FromInternalValue(3); + Time visit_time = SinceEpoch(3); VisitVector all_visits; URLID url_id = fake_history_backend_->GetIdByUrl(GURL(kURL)); ASSERT_NE(0, url_id); @@ -1291,7 +1300,7 @@ TEST_F(TypedURLSyncBridgeTest, UpdateUrlAndVisits) { kURL, kTitle, /*typed_count=*/1, /*last_visit=*/3, /*hidden=*/false, /*update_metadata=*/false, EntityChange::ACTION_ADD); - base::Time visit_time = base::Time::FromInternalValue(3); + Time visit_time = SinceEpoch(3); VisitVector all_visits; URLRow url_row; @@ -1312,7 +1321,7 @@ TEST_F(TypedURLSyncBridgeTest, UpdateUrlAndVisits) { /*hidden=*/false, /*update_metadata=*/false, EntityChange::ACTION_UPDATE); - base::Time new_visit_time = base::Time::FromInternalValue(6); + Time new_visit_time = SinceEpoch(6); url_id = fake_history_backend_->GetIdByUrl(GURL(kURL)); ASSERT_NE(0, url_id); fake_history_backend_->GetVisitsForURL(url_id, &all_visits); @@ -1338,7 +1347,7 @@ TEST_F(TypedURLSyncBridgeTest, DeleteUrlAndVisits) { const auto& changes_multimap = processor().put_multimap(); ASSERT_EQ(1U, changes_multimap.size()); - base::Time visit_time = base::Time::FromInternalValue(3); + Time visit_time = SinceEpoch(3); VisitVector all_visits; URLID url_id = fake_history_backend_->GetIdByUrl(GURL(kURL)); ASSERT_NE(0, url_id); @@ -1377,8 +1386,8 @@ TEST_F(TypedURLSyncBridgeTest, DiffVisitsSame) { const int64_t visits[] = {1024, 2065, 65534, 1237684}; for (int64_t visit : visits) { - old_visits.push_back(VisitRow(0, base::Time::FromInternalValue(visit), 0, - ui::PAGE_TRANSITION_TYPED, 0, true)); + old_visits.push_back( + VisitRow(0, SinceEpoch(visit), 0, ui::PAGE_TRANSITION_TYPED, 0, true)); new_url.add_visits(visit); new_url.add_visit_transitions(ui::PAGE_TRANSITION_TYPED); } @@ -1407,8 +1416,8 @@ TEST_F(TypedURLSyncBridgeTest, DiffVisitsRemove) { const int64_t visits_removed[] = {1500, 6000, 2237684}; for (int64_t visit : visits_left) { - old_visits.push_back(VisitRow(0, base::Time::FromInternalValue(visit), 0, - ui::PAGE_TRANSITION_TYPED, 0, true)); + old_visits.push_back( + VisitRow(0, SinceEpoch(visit), 0, ui::PAGE_TRANSITION_TYPED, 0, true)); } for (int64_t visit : visits_right) { @@ -1421,8 +1430,8 @@ TEST_F(TypedURLSyncBridgeTest, DiffVisitsRemove) { DiffVisits(old_visits, new_url, &new_visits, &removed_visits); EXPECT_TRUE(new_visits.empty()); - ASSERT_EQ(removed_visits.size(), arraysize(visits_removed)); - for (size_t i = 0; i < arraysize(visits_removed); ++i) { + ASSERT_EQ(removed_visits.size(), base::size(visits_removed)); + for (size_t i = 0; i < base::size(visits_removed); ++i) { EXPECT_EQ(removed_visits[i].visit_time.ToInternalValue(), visits_removed[i]); } @@ -1441,8 +1450,8 @@ TEST_F(TypedURLSyncBridgeTest, DiffVisitsAdd) { const int64_t visits_added[] = {1, 1500, 6000, 2237684}; for (int64_t visit : visits_left) { - old_visits.push_back(VisitRow(0, base::Time::FromInternalValue(visit), 0, - ui::PAGE_TRANSITION_TYPED, 0, true)); + old_visits.push_back( + VisitRow(0, SinceEpoch(visit), 0, ui::PAGE_TRANSITION_TYPED, 0, true)); } for (int64_t visit : visits_right) { @@ -1455,8 +1464,8 @@ TEST_F(TypedURLSyncBridgeTest, DiffVisitsAdd) { DiffVisits(old_visits, new_url, &new_visits, &removed_visits); EXPECT_TRUE(removed_visits.empty()); - ASSERT_TRUE(new_visits.size() == arraysize(visits_added)); - for (size_t i = 0; i < arraysize(visits_added); ++i) { + ASSERT_TRUE(new_visits.size() == base::size(visits_added)); + for (size_t i = 0; i < base::size(visits_added); ++i) { EXPECT_EQ(new_visits[i].first.ToInternalValue(), visits_added[i]); EXPECT_TRUE(ui::PageTransitionTypeIncludingQualifiersIs( new_visits[i].second, ui::PAGE_TRANSITION_TYPED)); @@ -1631,8 +1640,8 @@ TEST_F(TypedURLSyncBridgeTest, MergeUrls) { TEST_F(TypedURLSyncBridgeTest, MergeUrlsAfterExpiration) { // First, create a history row that has two visits, with timestamps 2 and 3. VisitVector(history_visits); - history_visits.push_back(VisitRow(0, base::Time::FromInternalValue(2), 0, - ui::PAGE_TRANSITION_TYPED, 0, true)); + history_visits.push_back( + VisitRow(0, SinceEpoch(2), 0, ui::PAGE_TRANSITION_TYPED, 0, true)); URLRow history_url( MakeTypedUrlRow(kURL, kTitle, 2, 3, false, &history_visits)); @@ -1674,7 +1683,7 @@ TEST_F(TypedURLSyncBridgeTest, LocalExpiredTypedUrlDoNotSync) { // Add an expired typed URL to local. row = MakeTypedUrlRow(kURL, kTitle, 1, kExpiredVisit, false, &visits); - fake_history_backend_->SetVisitsForUrl(row, visits); + fake_history_backend_->SetVisitsForUrl(&row, visits); StartSyncing(std::vector<TypedUrlSpecifics>()); @@ -1684,7 +1693,7 @@ TEST_F(TypedURLSyncBridgeTest, LocalExpiredTypedUrlDoNotSync) { // Add a non expired typed URL to local. row = MakeTypedUrlRow(kURL, kTitle, 2, 1, false, &visits); - fake_history_backend_->SetVisitsForUrl(row, visits); + fake_history_backend_->SetVisitsForUrl(&row, visits); changed_urls.push_back(row); // Notify typed url sync service of the update. diff --git a/chromium/components/history/core/browser/thumbnail_database.cc b/chromium/components/history/core/browser/thumbnail_database.cc index 1f296d7ad60..7748a797c2a 100644 --- a/chromium/components/history/core/browser/thumbnail_database.cc +++ b/chromium/components/history/core/browser/thumbnail_database.cc @@ -411,8 +411,8 @@ void ThumbnailDatabase::Vacuum() { ignore_result(db_.Execute("VACUUM")); } -void ThumbnailDatabase::TrimMemory(bool aggressively) { - db_.TrimMemory(aggressively); +void ThumbnailDatabase::TrimMemory() { + db_.TrimMemory(); } std::map<favicon_base::FaviconID, IconMappingsForExpiry> diff --git a/chromium/components/history/core/browser/thumbnail_database.h b/chromium/components/history/core/browser/thumbnail_database.h index d8530f40b6f..6b087e67b88 100644 --- a/chromium/components/history/core/browser/thumbnail_database.h +++ b/chromium/components/history/core/browser/thumbnail_database.h @@ -63,9 +63,8 @@ class ThumbnailDatabase { // unused space in the file. It can be VERY SLOW. void Vacuum(); - // Try to trim the cache memory used by the database. If |aggressively| is - // true try to trim all unused cache, otherwise trim by half. - void TrimMemory(bool aggressively); + // Release all non-essential memory associated with this database connection. + void TrimMemory(); // Get all on-demand favicon bitmaps that have been last requested prior to // |threshold|. diff --git a/chromium/components/history/core/browser/top_sites.cc b/chromium/components/history/core/browser/top_sites.cc index c24bacbf1ee..3f5ead1c4c4 100644 --- a/chromium/components/history/core/browser/top_sites.cc +++ b/chromium/components/history/core/browser/top_sites.cc @@ -8,17 +8,14 @@ namespace history { -PrepopulatedPage::PrepopulatedPage() - : favicon_id(-1), thumbnail_id(-1), color() {} +PrepopulatedPage::PrepopulatedPage() : favicon_id(-1), color() {} PrepopulatedPage::PrepopulatedPage(const GURL& url, const base::string16& title, int favicon_id, - int thumbnail_id, SkColor color) : most_visited(url, title), favicon_id(favicon_id), - thumbnail_id(thumbnail_id), color(color) { most_visited.redirects.push_back(url); } diff --git a/chromium/components/history/core/browser/top_sites.h b/chromium/components/history/core/browser/top_sites.h index 9937115322f..33d7c98d60c 100644 --- a/chromium/components/history/core/browser/top_sites.h +++ b/chromium/components/history/core/browser/top_sites.h @@ -12,32 +12,24 @@ #include "base/observer_list.h" #include "components/history/core/browser/history_types.h" #include "components/history/core/browser/top_sites_observer.h" -#include "components/history/core/common/thumbnail_score.h" #include "components/keyed_service/core/refcounted_keyed_service.h" #include "third_party/skia/include/core/SkColor.h" -#include "ui/gfx/image/image.h" class GURL; -namespace base { -class RefCountedMemory; -} - namespace history { -// PrepopulatedPage stores information for prepopulated page for the -// initial run. +// PrepopulatedPage stores information for prepopulated pages for the initial +// run. struct PrepopulatedPage { PrepopulatedPage(); PrepopulatedPage(const GURL& url, const base::string16& title, int favicon_id, - int thumbnail_id, SkColor color); MostVisitedURL most_visited; // The prepopulated page URL and title. int favicon_id; // The raw data resource for the favicon. - int thumbnail_id; // The raw data resource for the thumbnail. SkColor color; // The best color to highlight the page, should // roughly match the favicon. }; @@ -45,8 +37,7 @@ struct PrepopulatedPage { typedef std::vector<PrepopulatedPage> PrepopulatedPageList; // Interface for TopSites, which stores the data for the top "most visited" -// sites. This includes a cache of the most visited data from history, as well -// as the corresponding thumbnails of those sites. +// sites. This includes a cache of the most visited data from history. // // Some methods should only be called from the UI thread (see method // descriptions below). All others are assumed to be threadsafe. @@ -54,48 +45,15 @@ class TopSites : public RefcountedKeyedService { public: TopSites(); - // Sets the given thumbnail for the given URL. Returns true if the thumbnail - // was updated. False means either the URL wasn't known to us, or we felt - // that our current thumbnail was superior to the given one. Should be called - // from the UI thread. - virtual bool SetPageThumbnail(const GURL& url, - const gfx::Image& thumbnail, - const ThumbnailScore& score) = 0; - typedef base::Callback<void(const MostVisitedURLList&)> GetMostVisitedURLsCallback; - // Returns a list of most visited URLs via a callback, if - // |include_forced_urls| is false includes only non-forced URLs. This may be - // invoked on any thread. NOTE: the callback is called immediately if we have - // the data cached. If data is not available yet, callback will later be - // posted to the thread called this function. - virtual void GetMostVisitedURLs(const GetMostVisitedURLsCallback& callback, - bool include_forced_urls) = 0; - - // Gets a thumbnail for a given page. Returns true iff we have the thumbnail. - // This may be invoked on any thread. - // If an exact thumbnail URL match fails, |prefix_match| specifies whether or - // not to try harder by matching the query thumbnail URL as URL prefix (as - // defined by UrlIsPrefix()). - // As this method may be invoked on any thread the ref count needs to be - // incremented before this method returns, so this takes a scoped_refptr*. - virtual bool GetPageThumbnail( - const GURL& url, - bool prefix_match, - scoped_refptr<base::RefCountedMemory>* bytes) = 0; - - // Get a thumbnail score for a given page. Returns true iff we have the - // thumbnail score. This may be invoked on any thread. The score will - // be copied to |score|. - virtual bool GetPageThumbnailScore(const GURL& url, - ThumbnailScore* score) = 0; - - // Get a temporary thumbnail score for a given page. Returns true iff we - // have the thumbnail score. Useful when checking if we should update a - // thumbnail for a given page. The score will be copied to |score|. - virtual bool GetTemporaryPageThumbnailScore(const GURL& url, - ThumbnailScore* score) = 0; + // Returns a list of most visited URLs via a callback. This may be invoked on + // any thread. NOTE: The callback is called immediately if we have the data + // cached. If data is not available yet, callback will later be posted to the + // thread that called this function. + virtual void GetMostVisitedURLs( + const GetMostVisitedURLsCallback& callback) = 0; // Asks TopSites to refresh what it thinks the top sites are. This may do // nothing. Should be called from the UI thread. @@ -123,28 +81,16 @@ class TopSites : public RefcountedKeyedService { // This function also returns false if TopSites isn't loaded yet. virtual bool IsKnownURL(const GURL& url) = 0; - // Returns true if the top sites list of non-forced URLs is full (i.e. we - // already have the maximum number of non-forced top sites). This function - // also returns false if TopSites isn't loaded yet. - virtual bool IsNonForcedFull() = 0; - - // Returns true if the top sites list of forced URLs is full (i.e. we already - // have the maximum number of forced top sites). This function also returns - // false if TopSites isn't loaded yet. - virtual bool IsForcedFull() = 0; + // Returns true if the top sites list is full (i.e. we already have the + // maximum number of top sites). This function also returns false if TopSites + // isn't loaded yet. + virtual bool IsFull() = 0; virtual bool loaded() const = 0; - // Returns the set of prepopulate pages. + // Returns the set of prepopulated pages. virtual PrepopulatedPageList GetPrepopulatedPages() = 0; - // Adds or updates a |url| for which we should force the capture of a - // thumbnail next time it's visited. If there is already a non-forced URL - // matching this |url| this call has no effect. Indicate this URL was last - // forced at |time| so we can evict the older URLs when needed. Should be - // called from the UI thread. - virtual bool AddForcedURL(const GURL& url, const base::Time& time) = 0; - // Called when user has navigated to |url|. virtual void OnNavigationCommitted(const GURL& url) = 0; diff --git a/chromium/components/history/core/browser/top_sites_backend.cc b/chromium/components/history/core/browser/top_sites_backend.cc index aa7cfd627d2..25e2be47043 100644 --- a/chromium/components/history/core/browser/top_sites_backend.cc +++ b/chromium/components/history/core/browser/top_sites_backend.cc @@ -43,15 +43,15 @@ void TopSitesBackend::Shutdown() { FROM_HERE, base::BindOnce(&TopSitesBackend::ShutdownDBOnDBThread, this)); } -void TopSitesBackend::GetMostVisitedThumbnails( - const GetMostVisitedThumbnailsCallback& callback, +void TopSitesBackend::GetMostVisitedSites( + GetMostVisitedSitesCallback callback, base::CancelableTaskTracker* tracker) { - scoped_refptr<MostVisitedThumbnails> thumbnails = new MostVisitedThumbnails(); + scoped_refptr<MostVisitedThreadSafe> sites = new MostVisitedThreadSafe(); tracker->PostTaskAndReply( db_task_runner_.get(), FROM_HERE, - base::BindOnce(&TopSitesBackend::GetMostVisitedThumbnailsOnDBThread, this, - thumbnails), - base::BindOnce(callback, thumbnails)); + base::BindOnce(&TopSitesBackend::GetMostVisitedSitesOnDBThread, this, + sites), + base::BindOnce(std::move(callback), sites)); } void TopSitesBackend::UpdateTopSites(const TopSitesDelta& delta, @@ -61,26 +61,12 @@ void TopSitesBackend::UpdateTopSites(const TopSitesDelta& delta, this, delta, record_or_not)); } -void TopSitesBackend::SetPageThumbnail(const MostVisitedURL& url, - int url_rank, - const Images& thumbnail) { - db_task_runner_->PostTask( - FROM_HERE, base::BindOnce(&TopSitesBackend::SetPageThumbnailOnDBThread, - this, url, url_rank, thumbnail)); -} - void TopSitesBackend::ResetDatabase() { db_task_runner_->PostTask( FROM_HERE, base::BindOnce(&TopSitesBackend::ResetDatabaseOnDBThread, this, db_path_)); } -void TopSitesBackend::DoEmptyRequest(const base::Closure& reply, - base::CancelableTaskTracker* tracker) { - tracker->PostTaskAndReply(db_task_runner_.get(), FROM_HERE, base::DoNothing(), - reply); -} - TopSitesBackend::~TopSitesBackend() { DCHECK(!db_); // Shutdown should have happened first (which results in // nulling out db). @@ -99,13 +85,12 @@ void TopSitesBackend::ShutdownDBOnDBThread() { db_.reset(); } -void TopSitesBackend::GetMostVisitedThumbnailsOnDBThread( - scoped_refptr<MostVisitedThumbnails> thumbnails) { +void TopSitesBackend::GetMostVisitedSitesOnDBThread( + scoped_refptr<MostVisitedThreadSafe> sites) { DCHECK(db_task_runner_->RunsTasksInCurrentSequence()); if (db_) { - db_->GetPageThumbnails(&(thumbnails->most_visited), - &(thumbnails->url_to_images_map)); + db_->GetSites(&(sites->data)); } } @@ -126,15 +111,6 @@ void TopSitesBackend::UpdateTopSitesOnDBThread( } } -void TopSitesBackend::SetPageThumbnailOnDBThread(const MostVisitedURL& url, - int url_rank, - const Images& thumbnail) { - if (!db_) - return; - - db_->SetPageThumbnail(url, url_rank, thumbnail); -} - void TopSitesBackend::ResetDatabaseOnDBThread(const base::FilePath& file_path) { DCHECK(db_task_runner_->RunsTasksInCurrentSequence()); db_.reset(nullptr); diff --git a/chromium/components/history/core/browser/top_sites_backend.h b/chromium/components/history/core/browser/top_sites_backend.h index 620a76ac245..38745ab0e70 100644 --- a/chromium/components/history/core/browser/top_sites_backend.h +++ b/chromium/components/history/core/browser/top_sites_backend.h @@ -37,10 +37,8 @@ class TopSitesBackend : public base::RefCountedThreadSafe<TopSitesBackend> { RECORD_HISTOGRAM_NO }; - // The boolean parameter indicates if the DB existed on disk or needs to be - // migrated. - typedef base::Callback<void(const scoped_refptr<MostVisitedThumbnails>&)> - GetMostVisitedThumbnailsCallback; + using GetMostVisitedSitesCallback = + base::OnceCallback<void(const scoped_refptr<MostVisitedThreadSafe>&)>; TopSitesBackend(); @@ -49,29 +47,17 @@ class TopSitesBackend : public base::RefCountedThreadSafe<TopSitesBackend> { // Schedules the db to be shutdown. void Shutdown(); - // Fetches MostVisitedThumbnails. - void GetMostVisitedThumbnails( - const GetMostVisitedThumbnailsCallback& callback, - base::CancelableTaskTracker* tracker); + // Fetches MostVisitedThreadSafe. + void GetMostVisitedSites(GetMostVisitedSitesCallback callback, + base::CancelableTaskTracker* tracker); // Updates top sites database from the specified delta. void UpdateTopSites(const TopSitesDelta& delta, const RecordHistogram record_or_not); - // Sets the thumbnail. - void SetPageThumbnail(const MostVisitedURL& url, - int url_rank, - const Images& thumbnail); - // Deletes the database and recreates it. void ResetDatabase(); - // Schedules a request that does nothing on the DB thread, but then notifies - // the the calling thread with a reply. This is used to make sure the db has - // finished processing a request. - void DoEmptyRequest(const base::Closure& reply, - base::CancelableTaskTracker* tracker); - private: friend class base::RefCountedThreadSafe<TopSitesBackend>; @@ -83,19 +69,14 @@ class TopSitesBackend : public base::RefCountedThreadSafe<TopSitesBackend> { // Shuts down the db. void ShutdownDBOnDBThread(); - // Does the work of getting the most visted thumbnails. - void GetMostVisitedThumbnailsOnDBThread( - scoped_refptr<MostVisitedThumbnails> thumbnails); + // Does the work of getting the most visited sites. + void GetMostVisitedSitesOnDBThread( + scoped_refptr<MostVisitedThreadSafe> sites); // Updates top sites. void UpdateTopSitesOnDBThread(const TopSitesDelta& delta, const RecordHistogram record_or_not); - // Sets the thumbnail. - void SetPageThumbnailOnDBThread(const MostVisitedURL& url, - int url_rank, - const Images& thumbnail); - // Resets the database. void ResetDatabaseOnDBThread(const base::FilePath& file_path); diff --git a/chromium/components/history/core/browser/top_sites_cache.cc b/chromium/components/history/core/browser/top_sites_cache.cc index ec410481334..38198db5ec3 100644 --- a/chromium/components/history/core/browser/top_sites_cache.cc +++ b/chromium/components/history/core/browser/top_sites_cache.cc @@ -2,10 +2,8 @@ // 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/top_sites_cache.h" - #include "base/logging.h" -#include "base/memory/ref_counted_memory.h" +#include "components/history/core/browser/top_sites_cache.h" namespace history { @@ -18,12 +16,7 @@ TopSitesCache::CanonicalURLQuery::CanonicalURLQuery(const GURL& url) { TopSitesCache::CanonicalURLQuery::~CanonicalURLQuery() { } -TopSitesCache::TopSitesCache() : num_forced_urls_(0) { - clear_query_ref_.ClearQuery(); - clear_query_ref_.ClearRef(); - clear_path_query_ref_.ClearQuery(); - clear_path_query_ref_.ClearRef(); - clear_path_query_ref_.ClearPath(); +TopSitesCache::TopSitesCache() { } TopSitesCache::~TopSitesCache() { @@ -31,92 +24,14 @@ TopSitesCache::~TopSitesCache() { void TopSitesCache::SetTopSites(const MostVisitedURLList& top_sites) { top_sites_ = top_sites; - CountForcedURLs(); GenerateCanonicalURLs(); } -void TopSitesCache::SetThumbnails(const URLToImagesMap& images) { - images_ = images; -} - -void TopSitesCache::ClearUnreferencedThumbnails() { - URLToImagesMap images_to_keep; - for (const std::pair<GURL, Images>& entry : images_) { - if (IsKnownURL(entry.first)) { - images_to_keep.insert(entry); - } - } - images_ = std::move(images_to_keep); -} - -Images* TopSitesCache::GetImage(const GURL& url) { - return &images_[GetCanonicalURL(url)]; -} - -bool TopSitesCache::GetPageThumbnail( - const GURL& url, - scoped_refptr<base::RefCountedMemory>* bytes) const { - auto found = images_.find(GetCanonicalURL(url)); - if (found != images_.end()) { - base::RefCountedMemory* data = found->second.thumbnail.get(); - if (data) { - *bytes = data; - return true; - } - } - return false; -} - -bool TopSitesCache::GetPageThumbnailScore(const GURL& url, - ThumbnailScore* score) const { - auto found = images_.find(GetCanonicalURL(url)); - if (found != images_.end()) { - *score = found->second.thumbnail_score; - return true; - } - return false; -} - const GURL& TopSitesCache::GetCanonicalURL(const GURL& url) const { auto it = GetCanonicalURLsIterator(url); return it == canonical_urls_.end() ? url : it->first.first->url; } -GURL TopSitesCache::GetGeneralizedCanonicalURL(const GURL& url) const { - auto it_hi = canonical_urls_.lower_bound(CanonicalURLQuery(url).entry()); - if (it_hi != canonical_urls_.end()) { - // Test match ignoring "?query#ref". This also handles exact match. - if (url.ReplaceComponents(clear_query_ref_) == - GetURLFromIterator(it_hi).ReplaceComponents(clear_query_ref_)) { - return it_hi->first.first->url; - } - } - // Everything on or after |it_hi| is irrelevant. - - GURL base_url(url.ReplaceComponents(clear_path_query_ref_)); - auto it_lo = canonical_urls_.lower_bound(CanonicalURLQuery(base_url).entry()); - if (it_lo == canonical_urls_.end()) - return GURL::EmptyGURL(); - GURL compare_url_lo(GetURLFromIterator(it_lo)); - if (!HaveSameSchemeHostAndPort(base_url, compare_url_lo) || - !IsPathPrefix(base_url.path(), compare_url_lo.path())) { - return GURL::EmptyGURL(); - } - // Everything before |it_lo| is irrelevant. - - // Search in [|it_lo|, |it_hi|) in reversed order. The first URL found that's - // a prefix of |url| (ignoring "?query#ref") would be returned. - for (auto it = it_hi; it != it_lo;) { - --it; - GURL compare_url(GetURLFromIterator(it)); - DCHECK(HaveSameSchemeHostAndPort(compare_url, url)); - if (IsPathPrefix(compare_url.path(), url.path())) - return it->first.first->url; - } - - return GURL::EmptyGURL(); -} - bool TopSitesCache::IsKnownURL(const GURL& url) const { return GetCanonicalURLsIterator(url) != canonical_urls_.end(); } @@ -126,29 +41,8 @@ size_t TopSitesCache::GetURLIndex(const GURL& url) const { return GetCanonicalURLsIterator(url)->second; } -size_t TopSitesCache::GetNumNonForcedURLs() const { - return top_sites_.size() - num_forced_urls_; -} - -size_t TopSitesCache::GetNumForcedURLs() const { - return num_forced_urls_; -} - -void TopSitesCache::CountForcedURLs() { - num_forced_urls_ = 0; - while (num_forced_urls_ < top_sites_.size()) { - // Forced sites are all at the beginning. - if (top_sites_[num_forced_urls_].last_forced_time.is_null()) - break; - num_forced_urls_++; - } -#if DCHECK_IS_ON() - // In debug, ensure the cache user has no forced URLs pass that point. - for (size_t i = num_forced_urls_; i < top_sites_.size(); ++i) { - DCHECK(top_sites_[i].last_forced_time.is_null()) - << "All the forced URLs must appear before non-forced URLs."; - } -#endif +size_t TopSitesCache::GetNumURLs() const { + return top_sites_.size(); } void TopSitesCache::GenerateCanonicalURLs() { diff --git a/chromium/components/history/core/browser/top_sites_cache.h b/chromium/components/history/core/browser/top_sites_cache.h index 65f861e2124..aee043670e7 100644 --- a/chromium/components/history/core/browser/top_sites_cache.h +++ b/chromium/components/history/core/browser/top_sites_cache.h @@ -11,82 +11,33 @@ #include <utility> #include "base/macros.h" -#include "base/memory/ref_counted.h" #include "components/history/core/browser/history_types.h" #include "components/history/core/browser/url_utils.h" #include "url/gurl.h" namespace history { -// TopSitesCache caches the top sites and thumbnails for TopSites. -// -// Retrieving thumbnails from a given input URL is a two-stage process: -// -// input URL --(map 1)--> canonical URL --(map 2)--> image. -// -// (map 1) searches for an URL in |canonical_urls_| that "matches" (see below) -// input URL. If found, canonical URL assigned to the result. Otherwise the -// input URL is considered to already be a canonical URL. -// -// (map 2) simply looks up canonical URL in |images_|. -// -// The rule to "match" URL in |canonical_urls_| always favors exact match. -// - In GetCanonicalURL(), exact match is the only case examined. -// - In GetGeneralizedCanonicalURL(), we also perform "generalized" URL matches, -// i.e., stored URLs in |canonical_urls_| that are prefixes of input URL, -// ignoring "?query#ref". -// For the latter two "URL prefix matches", we prefer the match that is closest -// to input URL, w.r.t. path hierarchy. +// TopSitesCache caches the list of top sites for TopSites. class TopSitesCache { public: TopSitesCache(); ~TopSitesCache(); - // Set the top sites. In |top_sites| all forced URLs must appear before - // non-forced URLs. This is only checked in debug. + // Set the top sites. void SetTopSites(const MostVisitedURLList& top_sites); const MostVisitedURLList& top_sites() const { return top_sites_; } - // The thumbnails. - void SetThumbnails(const URLToImagesMap& images); - const URLToImagesMap& images() const { return images_; } - - void ClearUnreferencedThumbnails(); - - // Returns the thumbnail as an Image for the specified url. This adds an entry - // for |url| if one has not yet been added. - Images* GetImage(const GURL& url); - - // Fetches the thumbnail for the specified url. Returns true if there is a - // thumbnail for the specified url. It is possible for a URL to be in TopSites - // but not have an thumbnail. - bool GetPageThumbnail(const GURL& url, - scoped_refptr<base::RefCountedMemory>* bytes) const; - - // Fetches the thumbnail score for the specified url. Returns true if - // there is a thumbnail score for the specified url. - bool GetPageThumbnailScore(const GURL& url, ThumbnailScore* score) const; - - // Returns the canonical URL for |url|. + // Returns the canonical URL for |url|. If not found, returns |url|. const GURL& GetCanonicalURL(const GURL& url) const; - // Searches for a URL in |canonical_urls_| that is a URL prefix of |url|. - // Prefers an exact match if it exists, or the least generalized match while - // ignoring "?query#ref". Returns the resulting canonical URL if match is - // found, otherwise returns an empty GURL. - GURL GetGeneralizedCanonicalURL(const GURL& url) const; - // Returns true if |url| is known. bool IsKnownURL(const GURL& url) const; // Returns the index into |top_sites_| for |url|. size_t GetURLIndex(const GURL& url) const; - // Returns the number of non-forced URLs in the cache. - size_t GetNumNonForcedURLs() const; - - // Returns the number of forced URLs in the cache. - size_t GetNumForcedURLs() const; + // Returns the number of URLs in the cache. + size_t GetNumURLs() const; private: // The entries in CanonicalURLs, see CanonicalURLs for details. The second @@ -126,9 +77,6 @@ class TopSitesCache { typedef std::map<CanonicalURLEntry, size_t, CanonicalURLComparator> CanonicalURLs; - // Count the number of forced URLs. - void CountForcedURLs(); - // Generates the set of canonical urls from |top_sites_|. void GenerateCanonicalURLs(); @@ -141,29 +89,13 @@ class TopSitesCache { // Returns the GURL corresponding to an iterator in |canonical_urls_|. const GURL& GetURLFromIterator(CanonicalURLs::const_iterator it) const; - // The number of top sites with forced URLs. - size_t num_forced_urls_; - - // The top sites. This list must always contain the forced URLs first followed - // by the non-forced URLs. This is not strictly enforced but is checked in - // debug. + // The list of top sites. MostVisitedURLList top_sites_; - // The images. These map from canonical url to image. - URLToImagesMap images_; - // Generated from the redirects to and from the most visited pages. See // description above typedef for details. CanonicalURLs canonical_urls_; - // Helper to clear "?query#ref" from any GURL. This is set in the constructor - // and never modified after. - GURL::Replacements clear_query_ref_; - - // Helper to clear "/path?query#ref" from any GURL. This is set in the - // constructor and never modified after. - GURL::Replacements clear_path_query_ref_; - DISALLOW_COPY_AND_ASSIGN(TopSitesCache); }; diff --git a/chromium/components/history/core/browser/top_sites_cache_unittest.cc b/chromium/components/history/core/browser/top_sites_cache_unittest.cc index cbdafbe1aae..c4510c7b61c 100644 --- a/chromium/components/history/core/browser/top_sites_cache_unittest.cc +++ b/chromium/components/history/core/browser/top_sites_cache_unittest.cc @@ -11,8 +11,7 @@ #include <vector> #include "base/logging.h" -#include "base/macros.h" -#include "base/memory/ref_counted_memory.h" +#include "base/stl_util.h" #include "base/strings/string16.h" #include "base/strings/string_number_conversions.h" #include "base/strings/utf_string_conversions.h" @@ -38,11 +37,6 @@ class TopSitesCacheTest : public testing::Test { // Initializes |top_sites_| and |cache_| based on |spec|. void InitTopSiteCache(const char** spec, size_t size); - bool HasPageThumbnail(const GURL& url) { - scoped_refptr<base::RefCountedMemory> memory; - return cache_.GetPageThumbnail(url, &memory); - } - MostVisitedURLList top_sites_; TopSitesCache cache_; @@ -87,7 +81,7 @@ const char* kTopSitesSpecBasic[] = { }; TEST_F(TopSitesCacheTest, GetCanonicalURL) { - InitTopSiteCache(kTopSitesSpecBasic, arraysize(kTopSitesSpecBasic)); + InitTopSiteCache(kTopSitesSpecBasic, base::size(kTopSitesSpecBasic)); struct { const char* expected; const char* query; @@ -111,7 +105,7 @@ TEST_F(TopSitesCacheTest, GetCanonicalURL) { // Prefix should not work: as-is. {"http://www.youtube.com/a", "http://www.youtube.com/a"}, }; - for (size_t i = 0; i < arraysize(test_cases); ++i) { + for (size_t i = 0; i < base::size(test_cases); ++i) { std::string expected(test_cases[i].expected); std::string query(test_cases[i].query); EXPECT_EQ(expected, cache_.GetCanonicalURL(GURL(query)).spec()) @@ -120,7 +114,7 @@ TEST_F(TopSitesCacheTest, GetCanonicalURL) { } TEST_F(TopSitesCacheTest, IsKnownUrl) { - InitTopSiteCache(kTopSitesSpecBasic, arraysize(kTopSitesSpecBasic)); + InitTopSiteCache(kTopSitesSpecBasic, base::size(kTopSitesSpecBasic)); // Matches. EXPECT_TRUE(cache_.IsKnownURL(GURL("http://www.google.com"))); EXPECT_TRUE(cache_.IsKnownURL(GURL("http://www.gooogle.com"))); @@ -134,173 +128,6 @@ TEST_F(TopSitesCacheTest, IsKnownUrl) { EXPECT_FALSE(cache_.IsKnownURL(GURL("http://www.youtube.com/a"))); } -const char* kTopSitesSpecPrefix[] = { - "http://www.google.com/", - " http://www.google.com/test?q=3", // Redirects. - " http://www.google.com/test/y?d", // Redirects. - " http://www.chromium.org/a/b", // Redirects. - "http://www.google.com/2", - " http://www.google.com/test/q", // Redirects. - " http://www.google.com/test/y?b", // Redirects. - "http://www.google.com/3", - " http://www.google.com/testing", // Redirects. - "http://www.google.com/test-hyphen", - "http://www.google.com/sh", - " http://www.google.com/sh/1/2/3", // Redirects. - "http://www.google.com/sh/1", -}; - -TEST_F(TopSitesCacheTest, GetCanonicalURLExactMatch) { - InitTopSiteCache(kTopSitesSpecPrefix, arraysize(kTopSitesSpecPrefix)); - for (size_t i = 0; i < arraysize(kTopSitesSpecPrefix); ++i) { - // Go through each entry in kTopSitesSpecPrefix, trimming space. - const char* s = kTopSitesSpecPrefix[i]; - while (*s && *s == ' ') - ++s; - // Get the answer from direct lookup. - GURL stored_url(s); - GURL expected(cache_.GetCanonicalURL(stored_url)); - // Test generalization. - GURL result(cache_.GetGeneralizedCanonicalURL(stored_url)); - EXPECT_EQ(expected, result) << " for kTopSitesSpecPrefix[" << i << "]"; - } -} - -TEST_F(TopSitesCacheTest, GetGeneralizedCanonicalURL) { - InitTopSiteCache(kTopSitesSpecPrefix, arraysize(kTopSitesSpecPrefix)); - struct { - const char* expected; - const char* query; - } test_cases[] = { - // Exact match after trimming "?query": redirects. - {"http://www.google.com/", "http://www.google.com/test"}, - // Same, but different code path: redirects. - {"http://www.google.com/", "http://www.google.com/test/y?e"}, - {"http://www.google.com/", "http://www.google.com/test/y?c"}, - // Same, but code path leads to different result: redirects. - {"http://www.google.com/2", "http://www.google.com/test/y?a"}, - // Generalized match: redirects. - {"http://www.google.com/3", "http://www.google.com/3/1/4/1/5/9"}, - // Generalized match with trailing "/": redirects. - {"http://www.google.com/3", "http://www.google.com/3/1/4/1/5/9/"}, - // Unique generalization match: redirects. - {"http://www.google.com/", "http://www.chromium.org/a/b/c"}, - // Multiple exact matches after trimming: redirects to first. - {"http://www.google.com/2", "http://www.google.com/test/y"}, - // Multiple generalized matches: redirects to least general. - {"http://www.google.com/sh", "http://www.google.com/sh/1/2/3/4/"}, - // Multiple generalized matches: redirects to least general. - {"http://www.google.com/sh", "http://www.google.com/sh/1/2/3/4/"}, - // Competing generalized match: take the most specilized. - {"http://www.google.com/2", "http://www.google.com/test/q"}, - // No generalized match, early element: fails. - {"", "http://www.a.com/"}, - // No generalized match, intermediate element: fails. - {"", "http://www.e-is-between-chromium-and-google.com/"}, - // No generalized match, late element: fails. - {"", "http://www.zzzzzzz.com/"}, - // String prefix match but not URL-prefix match: fails. - {"", "http://www.chromium.org/a/beeswax"}, - // String prefix match and URL-prefix match: redirects. - {"http://www.google.com/", "http://www.google.com/shhhhhh"}, - // Different protocol: fails. - {"", "https://www.google.com/test"}, - // Smart enough to know that port 80 is HTTP: redirects. - {"http://www.google.com/", "http://www.google.com:80/test"}, - // Specialized match only: fails. - {"", "http://www.chromium.org/a"}, - }; - for (size_t i = 0; i < arraysize(test_cases); ++i) { - std::string expected(test_cases[i].expected); - std::string query(test_cases[i].query); - GURL result(cache_.GetGeneralizedCanonicalURL(GURL(query))); - EXPECT_EQ(expected, result.spec()) << " for test_case[" << i << "]"; - } -} - -// This tests a special case where there are 2 generalized matches, and both -// should be checked to find the correct match. -TEST_F(TopSitesCacheTest, GetPrefixCanonicalURLDiffByQuery) { - const char* top_sites_spec[] = { - "http://www.dest.com/1", - " http://www.source.com/a?m=5", // Redirects. - "http://www.dest.com/2", - " http://www.source.com/a/t?q=3", // Redirects. - }; - InitTopSiteCache(top_sites_spec, arraysize(top_sites_spec)); - - struct { - const char* expected; - const char* query; - } test_cases[] = { - // Slightly before "http://www.source.com/a?m=5". - {"http://www.dest.com/1", "http://www.source.com/a?l=5"}, - // Slightly after "http://www.source.com/a?m=5". - {"http://www.dest.com/1", "http://www.source.com/a?n=5"}, - // Slightly before "http://www.source.com/a/t?q=3". - {"http://www.dest.com/2", "http://www.source.com/a/t?q=2"}, - // Slightly after "http://www.source.com/a/t?q=3". - {"http://www.dest.com/2", "http://www.source.com/a/t?q=4"}, - }; - - for (size_t i = 0; i < arraysize(test_cases); ++i) { - std::string expected(test_cases[i].expected); - std::string query(test_cases[i].query); - GURL result(cache_.GetGeneralizedCanonicalURL(GURL(query))); - EXPECT_EQ(expected, result.spec()) << " for test_case[" << i << "]"; - } -} - -// This test ensures forced URLs behave in the expected way. -TEST_F(TopSitesCacheTest, CacheForcedURLs) { - // Forced URLs must always appear at the beginning of the list. - BuildTopSites(kTopSitesSpecBasic, arraysize(kTopSitesSpecBasic)); - top_sites_[0].last_forced_time = base::Time::FromJsTime(1000); - top_sites_[1].last_forced_time = base::Time::FromJsTime(2000); - cache_.SetTopSites(top_sites_); - - EXPECT_EQ(2u, cache_.GetNumForcedURLs()); - EXPECT_EQ(2u, cache_.GetNumNonForcedURLs()); -} - -TEST_F(TopSitesCacheTest, ClearUnreferencedThumbnails) { - InitTopSiteCache(kTopSitesSpecBasic, arraysize(kTopSitesSpecBasic)); - - // A "primary" URL. - const GURL url1("http://www.google.com"); - ASSERT_TRUE(cache_.IsKnownURL(url1)); - // A URL that's part of a redirect chain. - const GURL url2("https://www.gogle.com"); - ASSERT_TRUE(cache_.IsKnownURL(url2)); - - // Add thumbnails for these two URLs. - Images thumbnail1; - thumbnail1.thumbnail = - new base::RefCountedBytes(std::vector<unsigned char>()); - Images thumbnail2; - thumbnail2.thumbnail = - new base::RefCountedBytes(std::vector<unsigned char>()); - URLToImagesMap images; - images[cache_.GetCanonicalURL(url1)] = thumbnail1; - images[cache_.GetCanonicalURL(url2)] = thumbnail2; - cache_.SetThumbnails(images); - - ASSERT_TRUE(HasPageThumbnail(url1)); - ASSERT_TRUE(HasPageThumbnail(url2)); - - // Since both URLs are known, ClearUnreferencedThumbnails should do nothing. - cache_.ClearUnreferencedThumbnails(); - EXPECT_TRUE(HasPageThumbnail(url1)); - EXPECT_TRUE(HasPageThumbnail(url2)); - - // After the top sites themselves are cleared, ClearUnreferencedThumbnails - // should clear the corresponding thumbnails. - cache_.SetTopSites(MostVisitedURLList()); - cache_.ClearUnreferencedThumbnails(); - EXPECT_FALSE(HasPageThumbnail(url1)); - EXPECT_FALSE(HasPageThumbnail(url2)); -} - } // namespace } // namespace history diff --git a/chromium/components/history/core/browser/top_sites_database.cc b/chromium/components/history/core/browser/top_sites_database.cc index 2aae1cbd14d..4260727422a 100644 --- a/chromium/components/history/core/browser/top_sites_database.cc +++ b/chromium/components/history/core/browser/top_sites_database.cc @@ -10,14 +10,13 @@ #include "base/bind.h" #include "base/files/file_util.h" -#include "base/memory/ref_counted.h" #include "base/metrics/histogram_macros.h" #include "base/strings/string_piece.h" #include "base/strings/string_split.h" #include "base/strings/string_util.h" +#include "base/strings/stringprintf.h" #include "components/history/core/browser/history_types.h" #include "components/history/core/browser/top_sites.h" -#include "components/history/core/common/thumbnail_score.h" #include "sql/database.h" #include "sql/recovery.h" #include "sql/statement.h" @@ -27,24 +26,13 @@ namespace history { // Description of database table: // -// thumbnails -// url URL of the sites for which we have a thumbnail. -// url_rank Index of the URL in that thumbnail, 0-based. The thumbnail -// with the highest rank will be the next one evicted. Forced -// thumbnails have a rank of -1. -// title The title to display under that thumbnail. +// top_sites +// url URL of the top site. +// url_rank Index of the site, 0-based. The site with the highest rank +// will be the next one evicted. +// title The title to display under that site. // redirects A space separated list of URLs that are known to redirect // to this url. -// boring_score How "boring" that thumbnail is. See ThumbnailScore. -// good_clipping True if the thumbnail was clipped from the bottom, keeping -// the entire width of the window. See ThumbnailScore. -// at_top True if the thumbnail was captured at the top of the -// website. -// last_updated The time at which this thumbnail was last updated. -// load_completed True if the thumbnail was captured after the page load was -// completed. -// last_forced If this is a forced thumbnail, records the last time it -// was forced. If it's not a forced thumbnail, 0. namespace { @@ -56,30 +44,27 @@ namespace { // fatal (in fact, very old data may be expired immediately at startup // anyhow). +// Version 4: 95af34ec/r618360 kristipark@chromium.org on 2018-12-20 // Version 3: b6d6a783/r231648 by beaudoin@chromium.org on 2013-10-29 // Version 2: eb0b24e6/r87284 by satorux@chromium.org on 2011-05-31 (deprecated) // Version 1: 809cc4d8/r64072 by sky@chromium.org on 2010-10-27 (deprecated) // NOTE(shess): When changing the version, add a new golden file for // the new version and a test to verify that Init() works with it. -static const int kVersionNumber = 3; +static const int kVersionNumber = 4; static const int kDeprecatedVersionNumber = 2; // and earlier. +// Rank used to indicate that this is a newly added URL. +static const int kRankOfNewURL = -1; + bool InitTables(sql::Database* db) { - static const char kThumbnailsSql[] = - "CREATE TABLE IF NOT EXISTS thumbnails (" + static const char kTopSitesSql[] = + "CREATE TABLE IF NOT EXISTS top_sites (" "url LONGVARCHAR PRIMARY KEY," "url_rank INTEGER," "title LONGVARCHAR," - "thumbnail BLOB," - "redirects LONGVARCHAR," - "boring_score DOUBLE DEFAULT 1.0," - "good_clipping INTEGER DEFAULT 0," - "at_top INTEGER DEFAULT 0," - "last_updated INTEGER DEFAULT 0," - "load_completed INTEGER DEFAULT 0," - "last_forced INTEGER DEFAULT 0)"; - return db->Execute(kThumbnailsSql); + "redirects LONGVARCHAR)"; + return db->Execute(kTopSitesSql); } // Encodes redirects into a string. @@ -129,7 +114,7 @@ enum RecoveryEventType { OBSOLETE_RECOVERY_EVENT_FAILED_AUTORECOVER_THUMBNAILS, RECOVERY_EVENT_FAILED_COMMIT, - // Track invariants resolved by FixThumbnailsTable(). + // Track invariants resolved by FixTopSitesTable(). RECOVERY_EVENT_INVARIANT_RANK, RECOVERY_EVENT_INVARIANT_REDIRECT, RECOVERY_EVENT_INVARIANT_CONTIGUOUS, @@ -151,21 +136,28 @@ void RecordRecoveryEvent(RecoveryEventType recovery_event) { // depending on the operation broken. This table has large rows, which will use // overflow pages, so it is possible (though unlikely) that a chain could fit // together and yield a row with errors. -void FixThumbnailsTable(sql::Database* db) { - // Enforce invariant separating forced and non-forced thumbnails. - static const char kFixRankSql[] = - "DELETE FROM thumbnails " - "WHERE (url_rank = -1 AND last_forced = 0) " - "OR (url_rank <> -1 AND last_forced <> 0)"; - ignore_result(db->Execute(kFixRankSql)); - if (db->GetLastChangeCount() > 0) - RecordRecoveryEvent(RECOVERY_EVENT_INVARIANT_RANK); +void FixTopSitesTable(sql::Database* db, int version) { + // Forced sites are only present in version 3. + if (version == 3) { + // Enforce invariant separating forced and non-forced thumbnails. + static const char kFixRankSql[] = + "DELETE FROM thumbnails " + "WHERE (url_rank = -1 AND last_forced = 0) " + "OR (url_rank <> -1 AND last_forced <> 0)"; + ignore_result(db->Execute(kFixRankSql)); + if (db->GetLastChangeCount() > 0) + RecordRecoveryEvent(RECOVERY_EVENT_INVARIANT_RANK); + } + + // The table was renamed to "top_sites" in version 4. + const char* kTableName = (version == 3 ? "thumbnails" : "top_sites"); // Enforce invariant that url is in its own redirects. static const char kFixRedirectsSql[] = - "DELETE FROM thumbnails " + "DELETE FROM %s " "WHERE url <> substr(redirects, -length(url), length(url))"; - ignore_result(db->Execute(kFixRedirectsSql)); + ignore_result( + db->Execute(base::StringPrintf(kFixRedirectsSql, kTableName).c_str())); if (db->GetLastChangeCount() > 0) RecordRecoveryEvent(RECOVERY_EVENT_INVARIANT_REDIRECT); @@ -175,13 +167,15 @@ void FixThumbnailsTable(sql::Database* db) { // manually is easier to follow. Another option would be to somehow integrate // the renumbering into the table recovery code. static const char kByRankSql[] = - "SELECT url_rank, rowid FROM thumbnails WHERE url_rank <> -1 " + "SELECT url_rank, rowid FROM %s WHERE url_rank <> -1 " "ORDER BY url_rank"; - sql::Statement select_statement(db->GetUniqueStatement(kByRankSql)); + sql::Statement select_statement(db->GetUniqueStatement( + base::StringPrintf(kByRankSql, kTableName).c_str())); static const char kAdjustRankSql[] = - "UPDATE thumbnails SET url_rank = ? WHERE rowid = ?"; - sql::Statement update_statement(db->GetUniqueStatement(kAdjustRankSql)); + "UPDATE %s SET url_rank = ? WHERE rowid = ?"; + sql::Statement update_statement(db->GetUniqueStatement( + base::StringPrintf(kAdjustRankSql, kTableName).c_str())); // Update any rows where |next_rank| doesn't match |url_rank|. int next_rank = 0; @@ -205,7 +199,7 @@ void FixThumbnailsTable(sql::Database* db) { // constraints. void RecoverAndFixup(sql::Database* db, const base::FilePath& db_path) { // NOTE(shess): If the version changes, review this code. - DCHECK_EQ(3, kVersionNumber); + DCHECK_EQ(4, kVersionNumber); std::unique_ptr<sql::Recovery> recovery = sql::Recovery::BeginRecoverDatabase(db, db_path); @@ -244,7 +238,7 @@ void RecoverAndFixup(sql::Database* db, const base::FilePath& db_path) { } // TODO(shess): Inline this? - FixThumbnailsTable(recovery->db()); + FixTopSitesTable(recovery->db(), version); if (!sql::Recovery::Recovered(std::move(recovery))) { // TODO(shess): Very unclear what this failure would actually mean, and what @@ -304,9 +298,6 @@ void DatabaseErrorCallback(sql::Database* db, } // namespace // static -const int TopSitesDatabase::kRankOfForcedURL = -1; - -// static const int TopSitesDatabase::kRankOfNonExistingURL = -2; TopSitesDatabase::TopSitesDatabase() { @@ -370,6 +361,13 @@ bool TopSitesDatabase::InitImpl(const base::FilePath& db_name) { } } + if (meta_table_.GetVersionNumber() == 3) { + if (!UpgradeToVersion4()) { + LOG(WARNING) << "Unable to upgrade top sites database to version 4."; + return false; + } + } + // Version check. if (meta_table_.GetVersionNumber() != kVersionNumber) return false; @@ -390,13 +388,11 @@ void TopSitesDatabase::ApplyDelta(const TopSitesDelta& delta) { 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.added.size(); ++i) + SetSiteNoTransaction(delta.added[i].url, delta.added[i].rank); for (size_t i = 0; i < delta.moved.size(); ++i) - UpdatePageRankNoTransaction(delta.moved[i].url, delta.moved[i].rank); + UpdateSiteRankNoTransaction(delta.moved[i].url, delta.moved[i].rank); transaction.Commit(); } @@ -412,13 +408,30 @@ bool TopSitesDatabase::UpgradeToVersion3() { return true; } -void TopSitesDatabase::GetPageThumbnails(MostVisitedURLList* urls, - URLToImagesMap* thumbnails) { - sql::Statement statement(db_->GetCachedStatement( - SQL_FROM_HERE, - "SELECT url, url_rank, title, thumbnail, redirects, " - "boring_score, good_clipping, at_top, last_updated, load_completed, " - "last_forced FROM thumbnails ORDER BY url_rank, last_forced")); +bool TopSitesDatabase::UpgradeToVersion4() { + // Rename table to "top_sites" and retain only the url, url_rank, title, and + // redirects columns. Also, remove any remaining forced sites. + const char* statement = + // The top_sites table is created before the version upgrade. + "INSERT INTO top_sites SELECT " + "url,url_rank,title,redirects FROM thumbnails;" + "DROP TABLE thumbnails;" + // Remove any forced sites. + "DELETE FROM top_sites WHERE (url_rank = -1);"; + if (!db_->Execute(statement)) { + NOTREACHED(); + return false; + } + + meta_table_.SetVersionNumber(4); + return true; +} + +void TopSitesDatabase::GetSites(MostVisitedURLList* urls) { + sql::Statement statement( + db_->GetCachedStatement(SQL_FROM_HERE, + "SELECT url, url_rank, title, redirects " + "FROM top_sites ORDER BY url_rank")); if (!statement.is_valid()) { LOG(WARNING) << db_->GetErrorMessage(); @@ -426,140 +439,54 @@ void TopSitesDatabase::GetPageThumbnails(MostVisitedURLList* urls, } urls->clear(); - thumbnails->clear(); while (statement.Step()) { - // Results are sorted by url_rank. For forced thumbnails with url_rank = -1, - // thumbnails are sorted by last_forced. + // Results are sorted by url_rank. MostVisitedURL url; GURL gurl(statement.ColumnString(0)); url.url = gurl; url.title = statement.ColumnString16(2); - url.last_forced_time = base::Time() + base::TimeDelta::FromMicroseconds( - statement.ColumnInt64(10)); - std::string redirects = statement.ColumnString(4); + std::string redirects = statement.ColumnString(3); SetRedirects(redirects, &url); urls->push_back(url); - - std::vector<unsigned char> data; - statement.ColumnBlobAsVector(3, &data); - Images thumbnail; - if (!data.empty()) - thumbnail.thumbnail = base::RefCountedBytes::TakeVector(&data); - thumbnail.thumbnail_score.boring_score = statement.ColumnDouble(5); - thumbnail.thumbnail_score.good_clipping = statement.ColumnBool(6); - thumbnail.thumbnail_score.at_top = statement.ColumnBool(7); - thumbnail.thumbnail_score.time_at_snapshot = - 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) { +void TopSitesDatabase::SetSiteNoTransaction(const MostVisitedURL& url, + int new_rank) { int rank = GetURLRank(url); if (rank == kRankOfNonExistingURL) { - AddPageThumbnail(url, new_rank, thumbnail); + AddSite(url, new_rank); } else { - UpdatePageRankNoTransaction(url, new_rank); - UpdatePageThumbnail(url, thumbnail); + UpdateSiteRankNoTransaction(url, new_rank); + UpdateSite(url); } } -void TopSitesDatabase::AddPageThumbnail(const MostVisitedURL& url, - int new_rank, - const Images& thumbnail) { - sql::Statement statement(db_->GetCachedStatement( - SQL_FROM_HERE, - "INSERT OR REPLACE INTO thumbnails " - "(url, url_rank, title, thumbnail, redirects, " - "boring_score, good_clipping, at_top, last_updated, load_completed, " - "last_forced) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)")); +void TopSitesDatabase::AddSite(const MostVisitedURL& url, int new_rank) { + sql::Statement statement( + db_->GetCachedStatement(SQL_FROM_HERE, + "INSERT OR REPLACE INTO top_sites " + "(url, url_rank, title, redirects) " + "VALUES (?, ?, ?, ?)")); statement.BindString(0, url.url.spec()); - statement.BindInt(1, kRankOfForcedURL); // Fist make it a forced thumbnail. + statement.BindInt(1, kRankOfNewURL); statement.BindString16(2, url.title); - if (thumbnail.thumbnail.get() && thumbnail.thumbnail->front()) { - statement.BindBlob(3, thumbnail.thumbnail->front(), - static_cast<int>(thumbnail.thumbnail->size())); - } - statement.BindString(4, GetRedirects(url)); - const ThumbnailScore& score = thumbnail.thumbnail_score; - 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.since_origin().InMicroseconds()); - statement.BindBool(9, score.load_completed); - 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."; - statement.BindInt64(10, last_forced); + statement.BindString(3, GetRedirects(url)); if (!statement.Run()) return; - // Update rank if this is not a forced thumbnail. - if (new_rank != kRankOfForcedURL) - UpdatePageRankNoTransaction(url, new_rank); + // Update the new site's rank. + UpdateSiteRankNoTransaction(url, new_rank); } -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 = ? ")); +bool TopSitesDatabase::UpdateSite(const MostVisitedURL& url) { + sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE, + "UPDATE top_sites SET " + "title = ?, redirects = ?" + "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()); + statement.BindString(1, GetRedirects(url)); return statement.Run(); } @@ -568,7 +495,7 @@ int TopSitesDatabase::GetURLRank(const MostVisitedURL& url) { sql::Statement select_statement( db_->GetCachedStatement(SQL_FROM_HERE, "SELECT url_rank " - "FROM thumbnails WHERE url=?")); + "FROM top_sites WHERE url = ?")); select_statement.BindString(0, url.url.spec()); if (select_statement.Step()) return select_statement.ColumnInt(0); @@ -576,12 +503,9 @@ int TopSitesDatabase::GetURLRank(const MostVisitedURL& url) { return kRankOfNonExistingURL; } -void TopSitesDatabase::UpdatePageRankNoTransaction(const MostVisitedURL& url, +void TopSitesDatabase::UpdateSiteRankNoTransaction(const MostVisitedURL& url, int new_rank) { DCHECK_GT(db_->transaction_nesting(), 0); - DCHECK((url.last_forced_time.is_null()) == (new_rank != kRankOfForcedURL)) - << "Thumbnail without a forced time stamp has a forced rank, or the " - << "opposite."; int prev_rank = GetURLRank(url); if (prev_rank == kRankOfNonExistingURL) { @@ -590,69 +514,50 @@ void TopSitesDatabase::UpdatePageRankNoTransaction(const MostVisitedURL& url, } // Shift the ranks. - if (prev_rank > new_rank) { - if (new_rank == kRankOfForcedURL) { - // 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 > ?")); - 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 < ?")); - shift_statement.BindInt(0, new_rank); - shift_statement.BindInt(1, prev_rank); - shift_statement.Run(); - } + if (prev_rank == kRankOfNewURL) { + // Starting from new_rank, shift up. + // Example: -1 -> 2 + // [-1 -> 2], 0, 1, [2 -> 3], [3 -> 4], [4 -> 5] + sql::Statement shift_statement( + db_->GetCachedStatement(SQL_FROM_HERE, + "UPDATE top_sites " + "SET url_rank = url_rank + 1 " + "WHERE url_rank >= ?")); + shift_statement.BindInt(0, new_rank); + shift_statement.Run(); + } else if (prev_rank > new_rank) { + // From [new_rank, prev_rank), shift up. + // Example: 3 -> 1 + // 0, [1 -> 2], [2 -> 3], [3 -> 1], 4 + sql::Statement shift_statement( + db_->GetCachedStatement(SQL_FROM_HERE, + "UPDATE top_sites " + "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(); } else if (prev_rank < new_rank) { - if (prev_rank == kRankOfForcedURL) { - // 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 >= ?")); - 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 <= ?")); - shift_statement.BindInt(0, prev_rank); - shift_statement.BindInt(1, new_rank); - shift_statement.Run(); - } + // From (prev_rank, new_rank], shift down. + // Example: 1 -> 3. + // 0, [1 -> 3], [2 -> 1], [3 -> 2], 4 + sql::Statement shift_statement( + db_->GetCachedStatement(SQL_FROM_HERE, + "UPDATE top_sites " + "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(); } - // 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 == ?")); + // Set the url's new_rank. + sql::Statement set_statement(db_->GetCachedStatement(SQL_FROM_HERE, + "UPDATE top_sites " + "SET url_rank = ? " + "WHERE url == ?")); set_statement.BindInt(0, new_rank); - set_statement.BindInt64(1, - url.last_forced_time.since_origin().InMicroseconds()); - set_statement.BindString(2, url.url.spec()); + set_statement.BindString(1, url.url.spec()); set_statement.Run(); } @@ -661,21 +566,19 @@ bool TopSitesDatabase::RemoveURLNoTransaction(const MostVisitedURL& url) { if (old_rank == kRankOfNonExistingURL) return true; - 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 > ?")); - shift_statement.BindInt(0, old_rank); + // Decrement all following ranks. + sql::Statement shift_statement( + db_->GetCachedStatement(SQL_FROM_HERE, + "UPDATE top_sites " + "SET url_rank = url_rank - 1 " + "WHERE url_rank > ?")); + shift_statement.BindInt(0, old_rank); - if (!shift_statement.Run()) - return false; - } + if (!shift_statement.Run()) + return false; sql::Statement delete_statement(db_->GetCachedStatement( - SQL_FROM_HERE, "DELETE FROM thumbnails WHERE url = ?")); + SQL_FROM_HERE, "DELETE FROM top_sites WHERE url = ?")); delete_statement.BindString(0, url.url.spec()); return delete_statement.Run(); diff --git a/chromium/components/history/core/browser/top_sites_database.h b/chromium/components/history/core/browser/top_sites_database.h index 4342c370fb7..c479e110db1 100644 --- a/chromium/components/history/core/browser/top_sites_database.h +++ b/chromium/components/history/core/browser/top_sites_database.h @@ -38,29 +38,17 @@ class TopSitesDatabase { // Returns a list of all URLs currently in the table. // WARNING: clears both input arguments. - void GetPageThumbnails(MostVisitedURLList* urls, - std::map<GURL, Images>* thumbnails); - - // 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. - void SetPageThumbnail(const MostVisitedURL& url, - int new_rank, - const Images& thumbnail); + void GetSites(MostVisitedURLList* urls); private: FRIEND_TEST_ALL_PREFIXES(TopSitesDatabaseTest, Version1); FRIEND_TEST_ALL_PREFIXES(TopSitesDatabaseTest, Version2); FRIEND_TEST_ALL_PREFIXES(TopSitesDatabaseTest, Version3); + FRIEND_TEST_ALL_PREFIXES(TopSitesDatabaseTest, Version4); FRIEND_TEST_ALL_PREFIXES(TopSitesDatabaseTest, Recovery1); FRIEND_TEST_ALL_PREFIXES(TopSitesDatabaseTest, Recovery2); FRIEND_TEST_ALL_PREFIXES(TopSitesDatabaseTest, Recovery3); - FRIEND_TEST_ALL_PREFIXES(TopSitesDatabaseTest, AddRemoveEditThumbnails); - - // Rank of all URLs that are forced and therefore cannot be automatically - // evicted. - static const int kRankOfForcedURL; + FRIEND_TEST_ALL_PREFIXES(TopSitesDatabaseTest, Recovery4); // Rank used to indicate that a URL is not stored in the database. static const int kRankOfNonExistingURL; @@ -69,34 +57,30 @@ 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); + // Upgrades the top_sites table to version 4, returning true if the upgrade + // was successful. + bool UpgradeToVersion4(); - // Adds a new URL to the database. - void AddPageThumbnail(const MostVisitedURL& url, - int new_rank, - const Images& thumbnail); + // Sets a top site for the URL. |new_rank| is the position of the URL in the + // list of top sites, zero-based. + // If the URL is not in the table, adds it. If it is, updates its rank and + // shifts the ranks of other URLs if necessary. Should be called within an + // open transaction. + void SetSiteNoTransaction(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); + // Adds a new URL to the database. + void AddSite(const MostVisitedURL& url, int new_rank); - // Updates thumbnail of a URL that's already in the database. + // Updates title and redirects of a URL that's already in the database. // Returns true if the database query succeeds. - bool UpdatePageThumbnail(const MostVisitedURL& url, const Images& thumbnail); + bool UpdateSite(const MostVisitedURL& url); // 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); + // Sets the rank for a given URL. The URL must be in the database. Should be + // called within an open transaction. + void UpdateSiteRankNoTransaction(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. 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 bf217b3f4e5..f68c64c2732 100644 --- a/chromium/components/history/core/browser/top_sites_database_unittest.cc +++ b/chromium/components/history/core/browser/top_sites_database_unittest.cc @@ -38,27 +38,32 @@ const GURL kUrl2 = GURL("https://chrome.google.com/webstore?hl=en"); // present. Any extraneous items have the potential to interact // negatively with future schema changes. void VerifyTablesAndColumns(sql::Database* db) { - // [meta] and [thumbnails]. + // [meta] and [top_sites]. EXPECT_EQ(2u, sql::test::CountSQLTables(db)); - // Implicit index on [meta], index on [thumbnails]. + // Implicit index on [meta], index on [top_sites]. EXPECT_EQ(2u, sql::test::CountSQLIndices(db)); // [key] and [value]. EXPECT_EQ(2u, sql::test::CountTableColumns(db, "meta")); - // [url], [url_rank], [title], [thumbnail], [redirects], - // [boring_score], [good_clipping], [at_top], [last_updated], and - // [load_completed], [last_forced] - EXPECT_EQ(11u, sql::test::CountTableColumns(db, "thumbnails")); + // [url], [url_rank], [title], [redirects] + EXPECT_EQ(4u, sql::test::CountTableColumns(db, "top_sites")); } void VerifyDatabaseEmpty(sql::Database* db) { size_t rows = 0; - EXPECT_TRUE(sql::test::CountTableRows(db, "thumbnails", &rows)); + EXPECT_TRUE(sql::test::CountTableRows(db, "top_sites", &rows)); EXPECT_EQ(0u, rows); } +void VerifyURLsEqual(const std::vector<GURL>& expected, + const history::MostVisitedURLList& actual) { + EXPECT_EQ(expected.size(), actual.size()); + for (size_t i = 0; i < expected.size(); i++) + EXPECT_EQ(expected[i], actual[i].url) << " for i = " << i; +} + } // namespace namespace history { @@ -107,29 +112,44 @@ TEST_F(TopSitesDatabaseTest, Version3) { // Basic operational check. MostVisitedURLList urls; - std::map<GURL, Images> thumbnails; - db.GetPageThumbnails(&urls, &thumbnails); + db.GetSites(&urls); ASSERT_EQ(3u, urls.size()); - ASSERT_EQ(3u, thumbnails.size()); EXPECT_EQ(kUrl0, urls[0].url); // [0] because of url_rank. - // kGoogleThumbnail includes nul terminator. - ASSERT_EQ(sizeof(kGoogleThumbnail) - 1, - thumbnails[urls[0].url].thumbnail->size()); - EXPECT_TRUE(!memcmp(thumbnails[urls[0].url].thumbnail->front(), - kGoogleThumbnail, sizeof(kGoogleThumbnail) - 1)); sql::Transaction transaction(db.db_.get()); transaction.Begin(); ASSERT_TRUE(db.RemoveURLNoTransaction(urls[1])); transaction.Commit(); - db.GetPageThumbnails(&urls, &thumbnails); + db.GetSites(&urls); ASSERT_EQ(2u, urls.size()); - ASSERT_EQ(2u, thumbnails.size()); } -// Version 1 is deprecated, the resulting schema should be current, -// with no data. +TEST_F(TopSitesDatabaseTest, Version4) { + ASSERT_TRUE(CreateDatabaseFromSQL(file_name_, "TopSites.v4.sql")); + + TopSitesDatabase db; + ASSERT_TRUE(db.Init(file_name_)); + + VerifyTablesAndColumns(db.db_.get()); + + // Basic operational check. + MostVisitedURLList urls; + db.GetSites(&urls); + ASSERT_EQ(3u, urls.size()); + EXPECT_EQ(kUrl0, urls[0].url); // [0] because of url_rank. + + sql::Transaction transaction(db.db_.get()); + transaction.Begin(); + ASSERT_TRUE(db.RemoveURLNoTransaction(urls[1])); + transaction.Commit(); + + db.GetSites(&urls); + ASSERT_EQ(2u, urls.size()); +} + +// Version 1 is deprecated, the resulting schema should be current, with no +// data. TEST_F(TopSitesDatabaseTest, Recovery1) { // Create an example database. EXPECT_TRUE(CreateDatabaseFromSQL(file_name_, "TopSites.v1.sql")); @@ -161,6 +181,8 @@ TEST_F(TopSitesDatabaseTest, Recovery1) { } } +// Version 2 is deprecated, the resulting schema should be current, with no +// data. TEST_F(TopSitesDatabaseTest, Recovery2) { // Create an example database. EXPECT_TRUE(CreateDatabaseFromSQL(file_name_, "TopSites.v2.sql")); @@ -218,16 +240,9 @@ TEST_F(TopSitesDatabaseTest, Recovery3) { ASSERT_TRUE(db.Init(file_name_)); MostVisitedURLList urls; - std::map<GURL, Images> thumbnails; - db.GetPageThumbnails(&urls, &thumbnails); + db.GetSites(&urls); ASSERT_EQ(3u, urls.size()); - ASSERT_EQ(3u, thumbnails.size()); EXPECT_EQ(kUrl0, urls[0].url); // [0] because of url_rank. - // kGoogleThumbnail includes nul terminator. - ASSERT_EQ(sizeof(kGoogleThumbnail) - 1, - thumbnails[urls[0].url].thumbnail->size()); - EXPECT_TRUE(!memcmp(thumbnails[urls[0].url].thumbnail->front(), - kGoogleThumbnail, sizeof(kGoogleThumbnail) - 1)); ASSERT_TRUE(expecter.SawExpectedErrors()); } @@ -241,11 +256,11 @@ TEST_F(TopSitesDatabaseTest, Recovery3) { // Corrupt the thumnails.url auto-index by deleting an element from the table // but leaving it in the index. - static const char kIndexName[] = "sqlite_autoindex_thumbnails_1"; + static const char kIndexName[] = "sqlite_autoindex_top_sites_1"; // TODO(shess): Refactor CorruptTableOrIndex() to make parameterized // statements easy. static const char kDeleteSql[] = - "DELETE FROM thumbnails WHERE url = " + "DELETE FROM top_sites WHERE url = " "'http://www.google.com/chrome/intl/en/welcome.html'"; EXPECT_TRUE( sql::test::CorruptTableOrIndex(file_name_, kIndexName, kDeleteSql)); @@ -295,152 +310,184 @@ TEST_F(TopSitesDatabaseTest, Recovery3) { db.GetURLRank(MostVisitedURL(kUrl1, base::string16()))); MostVisitedURLList urls; - std::map<GURL, Images> thumbnails; - db.GetPageThumbnails(&urls, &thumbnails); + db.GetSites(&urls); ASSERT_EQ(2u, urls.size()); - ASSERT_EQ(2u, thumbnails.size()); EXPECT_EQ(kUrl0, urls[0].url); // [0] because of url_rank. EXPECT_EQ(kUrl2, urls[1].url); // [1] because of url_rank. } } -TEST_F(TopSitesDatabaseTest, AddRemoveEditThumbnails) { - ASSERT_TRUE(CreateDatabaseFromSQL(file_name_, "TopSites.v3.sql")); +TEST_F(TopSitesDatabaseTest, Recovery4) { + // Create an example database. + EXPECT_TRUE(CreateDatabaseFromSQL(file_name_, "TopSites.v4.sql")); + + // Corrupt the database by adjusting the header. + EXPECT_TRUE(sql::test::CorruptSizeInHeader(file_name_)); + + // Database is unusable at the SQLite level. + { + sql::test::ScopedErrorExpecter expecter; + expecter.ExpectError(SQLITE_CORRUPT); + sql::Database raw_db; + EXPECT_TRUE(raw_db.Open(file_name_)); + EXPECT_FALSE(raw_db.IsSQLValid("PRAGMA integrity_check")); + ASSERT_TRUE(expecter.SawExpectedErrors()); + } + + // Corruption should be detected and recovered during Init(). + { + sql::test::ScopedErrorExpecter expecter; + expecter.ExpectError(SQLITE_CORRUPT); + + TopSitesDatabase db; + ASSERT_TRUE(db.Init(file_name_)); + + MostVisitedURLList urls; + db.GetSites(&urls); + ASSERT_EQ(3u, urls.size()); + EXPECT_EQ(kUrl0, urls[0].url); // [0] because of url_rank. + + ASSERT_TRUE(expecter.SawExpectedErrors()); + } + + // Double-check database integrity. + { + sql::Database raw_db; + EXPECT_TRUE(raw_db.Open(file_name_)); + ASSERT_EQ("ok", sql::test::IntegrityCheck(&raw_db)); + } + + // Corrupt the tops_sites.url auto-index by deleting an element from the table + // but leaving it in the index. + static const char kIndexName[] = "sqlite_autoindex_top_sites_1"; + // TODO(shess): Refactor CorruptTableOrIndex() to make parameterized + // statements easy. + static const char kDeleteSql[] = + "DELETE FROM top_sites WHERE url = " + "'http://www.google.com/chrome/intl/en/welcome.html'"; + EXPECT_TRUE( + sql::test::CorruptTableOrIndex(file_name_, kIndexName, kDeleteSql)); + + // SQLite can operate on the database, but notices the corruption in integrity + // check. + { + sql::Database raw_db; + EXPECT_TRUE(raw_db.Open(file_name_)); + ASSERT_NE("ok", sql::test::IntegrityCheck(&raw_db)); + } + + // Open the database and access the corrupt index. + { + TopSitesDatabase db; + ASSERT_TRUE(db.Init(file_name_)); + + { + sql::test::ScopedErrorExpecter expecter; + expecter.ExpectError(SQLITE_CORRUPT); + + // Data for kUrl1 was deleted, but the index entry remains, this will + // throw SQLITE_CORRUPT. The corruption handler will recover the database + // and poison the handle, so the outer call fails. + EXPECT_EQ(TopSitesDatabase::kRankOfNonExistingURL, + db.GetURLRank(MostVisitedURL(kUrl1, base::string16()))); + + ASSERT_TRUE(expecter.SawExpectedErrors()); + } + } + + // Check that the database is recovered at the SQLite level. + { + sql::Database raw_db; + EXPECT_TRUE(raw_db.Open(file_name_)); + ASSERT_EQ("ok", sql::test::IntegrityCheck(&raw_db)); + } + + // After recovery, the database accesses won't throw errors. The top-ranked + // item is removed, but the ranking was revised in post-processing. + { + TopSitesDatabase db; + ASSERT_TRUE(db.Init(file_name_)); + VerifyTablesAndColumns(db.db_.get()); + + EXPECT_EQ(TopSitesDatabase::kRankOfNonExistingURL, + db.GetURLRank(MostVisitedURL(kUrl1, base::string16()))); + + MostVisitedURLList urls; + db.GetSites(&urls); + ASSERT_EQ(2u, urls.size()); + EXPECT_EQ(kUrl0, urls[0].url); // [0] because of url_rank. + EXPECT_EQ(kUrl2, urls[1].url); // [1] because of url_rank. + } +} + +TEST_F(TopSitesDatabaseTest, ApplyDelta_Delete) { + ASSERT_TRUE(CreateDatabaseFromSQL(file_name_, "TopSites.v4.sql")); TopSitesDatabase db; ASSERT_TRUE(db.Init(file_name_)); - // Add a new URL, not forced, rank = 1. - GURL mapsUrl = GURL("http://maps.google.com/"); - MostVisitedURL url1(mapsUrl, base::ASCIIToUTF16("Google Maps")); - db.SetPageThumbnail(url1, 1, Images()); + 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); + + // Update db. + db.ApplyDelta(delta); + // Read db and verify. MostVisitedURLList urls; - std::map<GURL, Images> thumbnails; - db.GetPageThumbnails(&urls, &thumbnails); - ASSERT_EQ(4u, urls.size()); - ASSERT_EQ(4u, thumbnails.size()); - EXPECT_EQ(kUrl0, urls[0].url); - EXPECT_EQ(mapsUrl, urls[1].url); - - // Add a new URL, forced. - GURL driveUrl = GURL("http://drive.google.com/"); - MostVisitedURL url2(driveUrl, base::ASCIIToUTF16("Google Drive")); - url2.last_forced_time = base::Time::FromJsTime(789714000000); // 10/1/1995 - db.SetPageThumbnail(url2, TopSitesDatabase::kRankOfForcedURL, Images()); - - db.GetPageThumbnails(&urls, &thumbnails); - ASSERT_EQ(5u, urls.size()); - ASSERT_EQ(5u, thumbnails.size()); - EXPECT_EQ(driveUrl, urls[0].url); // Forced URLs always appear first. - EXPECT_EQ(kUrl0, urls[1].url); - EXPECT_EQ(mapsUrl, urls[2].url); - - // Add a new URL, forced (earlier). - GURL plusUrl = GURL("http://plus.google.com/"); - MostVisitedURL url3(plusUrl, base::ASCIIToUTF16("Google Plus")); - url3.last_forced_time = base::Time::FromJsTime(787035600000); // 10/12/1994 - db.SetPageThumbnail(url3, TopSitesDatabase::kRankOfForcedURL, Images()); - - db.GetPageThumbnails(&urls, &thumbnails); - ASSERT_EQ(6u, urls.size()); - ASSERT_EQ(6u, thumbnails.size()); - EXPECT_EQ(plusUrl, urls[0].url); // New forced URL should appear first. - EXPECT_EQ(driveUrl, urls[1].url); - EXPECT_EQ(kUrl0, urls[2].url); - EXPECT_EQ(mapsUrl, urls[3].url); - - // Change the last_forced_time of a forced URL. - url3.last_forced_time = base::Time::FromJsTime(792392400000); // 10/2/1995 - db.SetPageThumbnail(url3, TopSitesDatabase::kRankOfForcedURL, Images()); - - db.GetPageThumbnails(&urls, &thumbnails); - ASSERT_EQ(6u, urls.size()); - ASSERT_EQ(6u, thumbnails.size()); - EXPECT_EQ(driveUrl, urls[0].url); - EXPECT_EQ(plusUrl, urls[1].url); // Forced URL should have moved second. - EXPECT_EQ(kUrl0, urls[2].url); - EXPECT_EQ(mapsUrl, urls[3].url); + db.GetSites(&urls); + VerifyURLsEqual(std::vector<GURL>({kUrl1, kUrl2}), urls); +} - sql::Transaction transaction(db.db_.get()); +TEST_F(TopSitesDatabaseTest, ApplyDelta_Add) { + ASSERT_TRUE(CreateDatabaseFromSQL(file_name_, "TopSites.v4.sql")); - // Change a non-forced URL to forced using UpdatePageRankNoTransaction. - url1.last_forced_time = base::Time::FromJsTime(792219600000); // 8/2/1995 - transaction.Begin(); - db.UpdatePageRankNoTransaction(url1, TopSitesDatabase::kRankOfForcedURL); - transaction.Commit(); + TopSitesDatabase db; + ASSERT_TRUE(db.Init(file_name_)); - db.GetPageThumbnails(&urls, &thumbnails); - ASSERT_EQ(6u, urls.size()); - ASSERT_EQ(6u, thumbnails.size()); - EXPECT_EQ(driveUrl, urls[0].url); - EXPECT_EQ(mapsUrl, urls[1].url); // Maps moves to second forced URL. - EXPECT_EQ(plusUrl, urls[2].url); - EXPECT_EQ(kUrl0, urls[3].url); - - // Change a forced URL to non-forced using SetPageThumbnail. - url3.last_forced_time = base::Time(); - db.SetPageThumbnail(url3, 1, Images()); - - db.GetPageThumbnails(&urls, &thumbnails); - ASSERT_EQ(6u, urls.size()); - ASSERT_EQ(6u, thumbnails.size()); - EXPECT_EQ(driveUrl, urls[0].url); - EXPECT_EQ(mapsUrl, urls[1].url); - 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 - // UpdatePageRankNoTransaction. - transaction.Begin(); - db.UpdatePageRankNoTransaction(url3, 0); - transaction.Commit(); + GURL mapsUrl = GURL("http://maps.google.com/"); - db.GetPageThumbnails(&urls, &thumbnails); - ASSERT_EQ(6u, urls.size()); - ASSERT_EQ(6u, thumbnails.size()); - EXPECT_EQ(driveUrl, urls[0].url); - EXPECT_EQ(mapsUrl, urls[1].url); - EXPECT_EQ(plusUrl, urls[2].url); // Plus moves to first non-forced URL. - EXPECT_EQ(kUrl0, urls[3].url); - - // Change a non-forced URL to later non-forced using SetPageThumbnail. - db.SetPageThumbnail(url3, 2, Images()); - - db.GetPageThumbnails(&urls, &thumbnails); - ASSERT_EQ(6u, urls.size()); - ASSERT_EQ(6u, thumbnails.size()); - EXPECT_EQ(driveUrl, urls[0].url); - EXPECT_EQ(mapsUrl, urls[1].url); - EXPECT_EQ(kUrl0, urls[2].url); - EXPECT_EQ(plusUrl, urls[4].url); // Plus moves to third non-forced URL. - - // Remove a non-forced URL. - transaction.Begin(); - ASSERT_TRUE(db.RemoveURLNoTransaction(url3)); - transaction.Commit(); + // Add a new URL, rank = 0. Now db has mapsUrl, kUrl0, kUrl1, and kUrl2. + TopSitesDelta delta; + 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); - db.GetPageThumbnails(&urls, &thumbnails); - ASSERT_EQ(5u, urls.size()); - ASSERT_EQ(5u, thumbnails.size()); - EXPECT_EQ(driveUrl, urls[0].url); - EXPECT_EQ(mapsUrl, urls[1].url); - EXPECT_EQ(kUrl0, urls[2].url); + // Update db. + db.ApplyDelta(delta); - // Remove a forced URL. - transaction.Begin(); - ASSERT_TRUE(db.RemoveURLNoTransaction(url2)); - transaction.Commit(); + // Read db and verify. + MostVisitedURLList urls; + db.GetSites(&urls); + VerifyURLsEqual(std::vector<GURL>({mapsUrl, kUrl0, kUrl1, kUrl2}), urls); +} + +TEST_F(TopSitesDatabaseTest, ApplyDelta_Move) { + ASSERT_TRUE(CreateDatabaseFromSQL(file_name_, "TopSites.v4.sql")); - db.GetPageThumbnails(&urls, &thumbnails); - ASSERT_EQ(4u, urls.size()); - ASSERT_EQ(4u, thumbnails.size()); - EXPECT_EQ(mapsUrl, urls[0].url); - EXPECT_EQ(kUrl0, urls[1].url); + TopSitesDatabase db; + ASSERT_TRUE(db.Init(file_name_)); + + // Move kUrl1 by updating its rank to 2. Now db has kUrl0, kUrl2, and kUrl1. + TopSitesDelta delta; + 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; + db.GetSites(&urls); + VerifyURLsEqual(std::vector<GURL>({kUrl0, kUrl2, kUrl1}), urls); } -TEST_F(TopSitesDatabaseTest, ApplyDelta) { - ASSERT_TRUE(CreateDatabaseFromSQL(file_name_, "TopSites.v3.sql")); +TEST_F(TopSitesDatabaseTest, ApplyDelta_All) { + ASSERT_TRUE(CreateDatabaseFromSQL(file_name_, "TopSites.v4.sql")); TopSitesDatabase db; ASSERT_TRUE(db.Init(file_name_)); @@ -469,13 +516,8 @@ TEST_F(TopSitesDatabaseTest, ApplyDelta) { // 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); + db.GetSites(&urls); + VerifyURLsEqual(std::vector<GURL>({mapsUrl, kUrl2, kUrl1}), urls); } } // 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 5a3e1a64b13..610bdb9af81 100644 --- a/chromium/components/history/core/browser/top_sites_impl.cc +++ b/chromium/components/history/core/browser/top_sites_impl.cc @@ -15,7 +15,6 @@ #include "base/location.h" #include "base/logging.h" #include "base/md5.h" -#include "base/memory/ref_counted_memory.h" #include "base/metrics/histogram_macros.h" #include "base/single_thread_task_runner.h" #include "base/strings/string_util.h" @@ -33,39 +32,23 @@ #include "components/history/core/browser/top_sites_observer.h" #include "components/history/core/browser/top_sites_provider.h" #include "components/history/core/browser/url_utils.h" -#include "components/history/core/common/thumbnail_score.h" #include "components/prefs/pref_registry_simple.h" #include "components/prefs/pref_service.h" #include "components/prefs/scoped_user_pref_update.h" -#include "ui/base/layout.h" -#include "ui/base/resource/resource_bundle.h" -#include "ui/gfx/image/image_util.h" namespace history { namespace { void RunOrPostGetMostVisitedURLsCallback( base::TaskRunner* task_runner, - bool include_forced_urls, const TopSitesImpl::GetMostVisitedURLsCallback& callback, - const MostVisitedURLList& all_urls, - const MostVisitedURLList& nonforced_urls) { - const MostVisitedURLList& urls = - include_forced_urls ? all_urls : nonforced_urls; + const MostVisitedURLList& urls) { if (task_runner->RunsTasksInCurrentSequence()) callback.Run(urls); else task_runner->PostTask(FROM_HERE, base::BindOnce(callback, urls)); } -// Compares two MostVisitedURL having a non-null |last_forced_time|. -bool ForcedURLComparator(const MostVisitedURL& first, - const MostVisitedURL& second) { - DCHECK(!first.last_forced_time.is_null() && - !second.last_forced_time.is_null()); - return first.last_forced_time < second.last_forced_time; -} - // Checks if the titles stored in |old_list| and |new_list| have changes. bool DoTitlesDiffer(const MostVisitedURLList& old_list, const MostVisitedURLList& new_list) { @@ -81,10 +64,6 @@ bool DoTitlesDiffer(const MostVisitedURLList& old_list, }); } -// Max number of temporary images we'll cache. See comment above -// temp_images_ for details. -const size_t kMaxTempTopImages = 8; - // The delay for the first HistoryService query at startup. constexpr base::TimeDelta kFirstDelayAtStartup = base::TimeDelta::FromSeconds(15); @@ -99,12 +78,8 @@ constexpr base::TimeDelta kDelayForUpdates = base::TimeDelta::FromMinutes(5); 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 -// artifacts for these small sized, highly detailed images. -const int kTopSitesImageQuality = 100; - -// Key for preference listing the URLs that should not be shown as most -// visited thumbnails. +// Key for preference listing the URLs that should not be shown as most visited +// tiles. const char kMostVisitedURLsBlacklist[] = "ntp.most_visited_blacklist"; } // namespace @@ -133,130 +108,35 @@ TopSitesImpl::TopSitesImpl(PrefService* pref_service, } void TopSitesImpl::Init(const base::FilePath& db_name) { - // Create the backend here, rather than in the constructor, so that - // unit tests that do not need the backend can run without a problem. + // Create the backend here, rather than in the constructor, so unit tests that + // do not need the backend can run without a problem. backend_ = new TopSitesBackend(); backend_->Init(db_name); - backend_->GetMostVisitedThumbnails( - base::Bind(&TopSitesImpl::OnGotMostVisitedThumbnails, - base::Unretained(this)), + backend_->GetMostVisitedSites( + base::BindOnce(&TopSitesImpl::OnGotMostVisitedURLs, + base::Unretained(this)), &cancelable_task_tracker_); } -bool TopSitesImpl::SetPageThumbnail(const GURL& url, - const gfx::Image& thumbnail, - const ThumbnailScore& score) { - ThumbnailEvent result = SetPageThumbnailImpl(url, thumbnail, score); - - UMA_HISTOGRAM_ENUMERATION("Thumbnails.AddedToTopSites", result, - THUMBNAIL_EVENT_COUNT); - - switch (result) { - case THUMBNAIL_FAILURE: - case THUMBNAIL_TOPSITES_FULL: - case THUMBNAIL_KEPT_EXISTING: - return false; - case THUMBNAIL_ADDED_TEMP: - case THUMBNAIL_ADDED_REGULAR: - return true; - case THUMBNAIL_PROMOTED_TEMP_TO_REGULAR: - case THUMBNAIL_EVENT_COUNT: - NOTREACHED(); - } - - return false; -} - // WARNING: this function may be invoked on any thread. void TopSitesImpl::GetMostVisitedURLs( - const GetMostVisitedURLsCallback& callback, - bool include_forced_urls) { + const GetMostVisitedURLsCallback& callback) { MostVisitedURLList filtered_urls; { base::AutoLock lock(lock_); if (!loaded_) { // A request came in before we finished loading. Store the callback and // we'll run it on current thread when we finish loading. - pending_callbacks_.push_back( - base::Bind(&RunOrPostGetMostVisitedURLsCallback, - base::RetainedRef(base::ThreadTaskRunnerHandle::Get()), - include_forced_urls, callback)); + pending_callbacks_.push_back(base::Bind( + &RunOrPostGetMostVisitedURLsCallback, + base::RetainedRef(base::ThreadTaskRunnerHandle::Get()), callback)); return; } - if (include_forced_urls) { - filtered_urls = thread_safe_cache_->top_sites(); - } else { - filtered_urls.assign(thread_safe_cache_->top_sites().begin() + - thread_safe_cache_->GetNumForcedURLs(), - thread_safe_cache_->top_sites().end()); - } + filtered_urls = thread_safe_cache_->top_sites(); } callback.Run(filtered_urls); } -bool TopSitesImpl::GetPageThumbnail( - const GURL& url, - bool prefix_match, - scoped_refptr<base::RefCountedMemory>* bytes) { - // WARNING: this may be invoked on any thread. - // Perform exact match. - { - base::AutoLock lock(lock_); - if (thread_safe_cache_->GetPageThumbnail(url, bytes)) - return true; - } - - // Resource bundle is thread safe. - for (const auto& prepopulated_page : prepopulated_pages_) { - if (url == prepopulated_page.most_visited.url) { - *bytes = - ui::ResourceBundle::GetSharedInstance().LoadDataResourceBytesForScale( - prepopulated_page.thumbnail_id, ui::SCALE_FACTOR_100P); - return true; - } - } - - if (prefix_match) { - // If http or https, search with |url| first, then try the other one. - std::vector<GURL> url_list; - url_list.push_back(url); - if (url.SchemeIsHTTPOrHTTPS()) - url_list.push_back(ToggleHTTPAndHTTPS(url)); - - for (const GURL& url : url_list) { - base::AutoLock lock(lock_); - - // Test whether any stored URL is a prefix of |url|. - GURL canonical_url = thread_safe_cache_->GetGeneralizedCanonicalURL(url); - if (!canonical_url.is_empty() && - thread_safe_cache_->GetPageThumbnail(canonical_url, bytes)) { - return true; - } - } - } - - return false; -} - -bool TopSitesImpl::GetPageThumbnailScore(const GURL& url, - ThumbnailScore* score) { - // WARNING: this may be invoked on any thread. - base::AutoLock lock(lock_); - return thread_safe_cache_->GetPageThumbnailScore(url, score); -} - -bool TopSitesImpl::GetTemporaryPageThumbnailScore(const GURL& url, - ThumbnailScore* score) { - for (const TempImage& temp_image : temp_images_) { - if (temp_image.first == url) { - *score = temp_image.second.thumbnail_score; - return true; - } - } - return false; -} - - // Returns the index of |url| in |urls|, or -1 if not found. static int IndexOf(const MostVisitedURLList& urls, const GURL& url) { for (size_t i = 0; i < urls.size(); i++) { @@ -325,12 +205,8 @@ bool TopSitesImpl::IsKnownURL(const GURL& url) { return loaded_ && cache_->IsKnownURL(url); } -bool TopSitesImpl::IsNonForcedFull() { - return loaded_ && cache_->GetNumNonForcedURLs() >= kNonForcedTopSitesNumber; -} - -bool TopSitesImpl::IsForcedFull() { - return loaded_ && cache_->GetNumForcedURLs() >= kForcedTopSitesNumber; +bool TopSitesImpl::IsFull() { + return loaded_ && cache_->GetNumURLs() >= kTopSitesNumber; } PrepopulatedPageList TopSitesImpl::GetPrepopulatedPages() { @@ -341,46 +217,6 @@ bool TopSitesImpl::loaded() const { return loaded_; } -bool TopSitesImpl::AddForcedURL(const GURL& url, const base::Time& time) { - DCHECK(thread_checker_.CalledOnValidThread()); - - if (!loaded_) { - // Optimally we could cache this and apply it after the load completes, but - // in practice it's not an issue since AddForcedURL will be called again - // next time the user hits the NTP. - return false; - } - - size_t num_forced = cache_->GetNumForcedURLs(); - MostVisitedURLList new_list(cache_->top_sites()); - MostVisitedURL new_url; - - if (cache_->IsKnownURL(url)) { - size_t index = cache_->GetURLIndex(url); - // Do nothing if we currently have that URL as non-forced. - if (new_list[index].last_forced_time.is_null()) - return false; - - // Update the |last_forced_time| of the already existing URL. Delete it and - // reinsert it at the right location. - new_url = new_list[index]; - new_list.erase(new_list.begin() + index); - num_forced--; - } else { - new_url.url = url; - new_url.redirects.push_back(url); - } - new_url.last_forced_time = time; - // Add forced URLs and sort. Added to the end of the list of forced URLs - // since this is almost always where it needs to go, unless the user's local - // clock is fiddled with. - auto mid = new_list.begin() + num_forced; - mid = new_list.insert(mid, new_url); - std::inplace_merge(new_list.begin(), mid, mid + 1, ForcedURLComparator); - SetTopSites(new_list, CALL_LOCATION_FROM_FORCED_URLS); - return true; -} - void TopSitesImpl::OnNavigationCommitted(const GURL& url) { DCHECK(thread_checker_.CalledOnValidThread()); if (!loaded_) @@ -426,32 +262,20 @@ void TopSitesImpl::StartQueryForMostVisited() { void TopSitesImpl::DiffMostVisited(const MostVisitedURLList& old_list, const MostVisitedURLList& new_list, TopSitesDelta* delta) { - // Add all the old URLs for quick lookup. This maps URLs to the corresponding // index in the input. std::map<GURL, size_t> all_old_urls; - size_t num_old_forced = 0; - for (size_t i = 0; i < old_list.size(); i++) { - if (!old_list[i].last_forced_time.is_null()) - num_old_forced++; - DCHECK(old_list[i].last_forced_time.is_null() || i < num_old_forced) - << "Forced URLs must all appear before non-forced URLs."; + for (size_t i = 0; i < old_list.size(); i++) all_old_urls[old_list[i].url] = i; - } // Check all the URLs in the new set to see which ones are new or just moved. // When we find a match in the old set, we'll reset its index to our special // marker. This allows us to quickly identify the deleted ones in a later // pass. const size_t kAlreadyFoundMarker = static_cast<size_t>(-1); - int rank = -1; // Forced URLs have a rank of -1. + int rank = -1; for (size_t i = 0; i < new_list.size(); i++) { - // Increase the rank if we're going through forced URLs. This works because - // non-forced URLs all come after forced URLs. - if (new_list[i].last_forced_time.is_null()) - rank++; - DCHECK(new_list[i].last_forced_time.is_null() == (rank != -1)) - << "Forced URLs must all appear before non-forced URLs."; + rank++; auto found = all_old_urls.find(new_list[i].url); if (found == all_old_urls.end()) { MostVisitedURLWithRank added; @@ -461,11 +285,8 @@ void TopSitesImpl::DiffMostVisited(const MostVisitedURLList& old_list, } else { DCHECK(found->second != kAlreadyFoundMarker) << "Same URL appears twice in the new list."; - int old_rank = found->second >= num_old_forced ? - found->second - num_old_forced : -1; - if (old_rank != rank || - old_list[found->second].last_forced_time != - new_list[i].last_forced_time) { + int old_rank = found->second; + if (old_rank != rank) { MostVisitedURLWithRank moved; moved.url = new_list[i]; moved.rank = rank; @@ -483,135 +304,6 @@ void TopSitesImpl::DiffMostVisited(const MostVisitedURLList& old_list, } } -TopSitesImpl::ThumbnailEvent TopSitesImpl::SetPageThumbnailImpl( - const GURL& url, - const gfx::Image& thumbnail, - const ThumbnailScore& score) { - DCHECK(thread_checker_.CalledOnValidThread()); - - if (!loaded_) { - // TODO(sky): I need to cache these and apply them after the load - // completes. - return THUMBNAIL_FAILURE; - } - - bool add_temp_thumbnail = false; - if (!IsKnownURL(url)) { - if (IsNonForcedFull()) { - // We're full, and this URL is not known to us. - return THUMBNAIL_TOPSITES_FULL; - } - - add_temp_thumbnail = true; - } - - if (!can_add_url_to_history_.Run(url)) - return THUMBNAIL_FAILURE; // It's not a real webpage. - - scoped_refptr<base::RefCountedBytes> thumbnail_data; - if (!EncodeBitmap(thumbnail, &thumbnail_data)) - return THUMBNAIL_FAILURE; - - if (add_temp_thumbnail) { - // Always remove the existing entry and then add it back. That way if we end - // up with too many temp thumbnails we'll prune the oldest first. - RemoveTemporaryThumbnailByURL(url); - AddTemporaryThumbnail(url, thumbnail_data.get(), score); - return THUMBNAIL_ADDED_TEMP; - } - - bool success = SetPageThumbnailEncoded(url, thumbnail_data.get(), score); - return success ? THUMBNAIL_ADDED_REGULAR : THUMBNAIL_KEPT_EXISTING; -} - -bool TopSitesImpl::SetPageThumbnailInCache( - const GURL& url, - const base::RefCountedMemory* thumbnail_data, - const ThumbnailScore& score) { - DCHECK(cache_->IsKnownURL(url)); - - const MostVisitedURL& most_visited = - cache_->top_sites()[cache_->GetURLIndex(url)]; - Images* image = cache_->GetImage(url); - - // When comparing the thumbnail scores, we need to take into account the - // redirect hops, which are not generated when the thumbnail is because the - // redirects weren't known. We fill that in here since we know the redirects. - ThumbnailScore new_score_with_redirects(score); - new_score_with_redirects.redirect_hops_from_dest = - GetRedirectDistanceForURL(most_visited, url); - - if (image->thumbnail.get() && - !ShouldReplaceThumbnailWith(image->thumbnail_score, - new_score_with_redirects)) { - return false; // The one we already have is better. - } - - image->thumbnail = const_cast<base::RefCountedMemory*>(thumbnail_data); - image->thumbnail_score = new_score_with_redirects; - - ResetThreadSafeImageCache(); - return true; -} - -bool TopSitesImpl::SetPageThumbnailEncoded( - const GURL& url, - const base::RefCountedMemory* thumbnail, - const ThumbnailScore& score) { - DCHECK(cache_->IsKnownURL(url)); - - if (!SetPageThumbnailInCache(url, thumbnail, score)) - return false; - - // Update the database. - size_t index = cache_->GetURLIndex(url); - int url_rank = index - cache_->GetNumForcedURLs(); - const MostVisitedURL& most_visited = cache_->top_sites()[index]; - backend_->SetPageThumbnail(most_visited, - url_rank < 0 ? -1 : url_rank, - *(cache_->GetImage(most_visited.url))); - return true; -} - -// static -bool TopSitesImpl::EncodeBitmap(const gfx::Image& bitmap, - scoped_refptr<base::RefCountedBytes>* bytes) { - if (bitmap.IsEmpty()) - return false; - *bytes = new base::RefCountedBytes(); - if (!gfx::JPEG1xEncodedDataFromImage(bitmap, kTopSitesImageQuality, - &(*bytes)->data())) { - return false; - } - - // As we're going to cache this data, make sure the vector is only as big as - // it needs to be, as JPEGCodec::Encode() over-allocates data.capacity(). - (*bytes)->data().shrink_to_fit(); - return true; -} - -void TopSitesImpl::RemoveTemporaryThumbnailByURL(const GURL& url) { - for (auto i = temp_images_.begin(); i != temp_images_.end(); ++i) { - if (i->first == url) { - temp_images_.erase(i); - return; - } - } -} - -void TopSitesImpl::AddTemporaryThumbnail(const GURL& url, - base::RefCountedMemory* thumbnail, - const ThumbnailScore& score) { - if (temp_images_.size() == kMaxTempTopImages) - temp_images_.pop_front(); - - TempImage image; - image.first = url; - image.second.thumbnail = thumbnail; - image.second.thumbnail_score = score; - temp_images_.push_back(image); -} - // static int TopSitesImpl::GetRedirectDistanceForURL(const MostVisitedURL& most_visited, const GURL& url) { @@ -623,11 +315,10 @@ int TopSitesImpl::GetRedirectDistanceForURL(const MostVisitedURL& most_visited, return 0; } -bool TopSitesImpl::AddPrepopulatedPages(MostVisitedURLList* urls, - size_t num_forced_urls) const { +bool TopSitesImpl::AddPrepopulatedPages(MostVisitedURLList* urls) const { bool added = false; for (const auto& prepopulated_page : prepopulated_pages_) { - if (urls->size() - num_forced_urls < kNonForcedTopSitesNumber && + if (urls->size() < kTopSitesNumber && IndexOf(*urls, prepopulated_page.most_visited.url) == -1) { urls->push_back(prepopulated_page.most_visited); added = true; @@ -636,46 +327,6 @@ bool TopSitesImpl::AddPrepopulatedPages(MostVisitedURLList* urls, return added; } -size_t TopSitesImpl::MergeCachedForcedURLs(MostVisitedURLList* new_list) const { - DCHECK(thread_checker_.CalledOnValidThread()); - // Add all the new URLs for quick lookup. Take that opportunity to count the - // number of forced URLs in |new_list|. - 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++) - all_new_urls.insert((*new_list)[i].redirects[j]); - - if (!(*new_list)[i].last_forced_time.is_null()) - ++num_forced; - } - - // Keep the forced URLs from |cache_| that are not found in |new_list|. - MostVisitedURLList filtered_forced_urls; - for (size_t i = 0; i < cache_->GetNumForcedURLs(); ++i) { - if (all_new_urls.find(cache_->top_sites()[i].url) == all_new_urls.end()) - filtered_forced_urls.push_back(cache_->top_sites()[i]); - } - num_forced += filtered_forced_urls.size(); - - // Prepend forced URLs and sort in order of ascending |last_forced_time|. - new_list->insert(new_list->begin(), filtered_forced_urls.begin(), - filtered_forced_urls.end()); - std::inplace_merge( - new_list->begin(), new_list->begin() + filtered_forced_urls.size(), - new_list->begin() + num_forced, ForcedURLComparator); - - // Drop older forced URLs if the list overflows. Since forced URLs are always - // sort in increasing order of |last_forced_time|, drop the first ones. - if (num_forced > kForcedTopSitesNumber) { - new_list->erase(new_list->begin(), - new_list->begin() + (num_forced - kForcedTopSitesNumber)); - num_forced = kForcedTopSitesNumber; - } - - return num_forced; -} - void TopSitesImpl::ApplyBlacklist(const MostVisitedURLList& urls, MostVisitedURLList* out) { // Log the number of times ApplyBlacklist is called so we can compute the @@ -685,21 +336,12 @@ void TopSitesImpl::ApplyBlacklist(const MostVisitedURLList& urls, UMA_HISTOGRAM_BOOLEAN("TopSites.NumberOfApplyBlacklist", true); UMA_HISTOGRAM_COUNTS_100("TopSites.NumberOfBlacklistedItems", (blacklist ? blacklist->size() : 0)); - size_t num_non_forced_urls = 0; - size_t num_forced_urls = 0; + size_t num_urls = 0; for (size_t i = 0; i < urls.size(); ++i) { if (!IsBlacklisted(urls[i].url)) { - if (urls[i].last_forced_time.is_null()) { - // Non-forced URL. - if (num_non_forced_urls >= kNonForcedTopSitesNumber) - continue; - num_non_forced_urls++; - } else { - // Forced URL. - if (num_forced_urls >= kForcedTopSitesNumber) - continue; - num_forced_urls++; - } + if (num_urls >= kTopSitesNumber) + break; + num_urls++; out->push_back(urls[i]); } } @@ -717,8 +359,7 @@ void TopSitesImpl::SetTopSites(const MostVisitedURLList& new_top_sites, DCHECK(thread_checker_.CalledOnValidThread()); MostVisitedURLList top_sites(new_top_sites); - size_t num_forced_urls = MergeCachedForcedURLs(&top_sites); - AddPrepopulatedPages(&top_sites, num_forced_urls); + AddPrepopulatedPages(&top_sites); TopSitesDelta delta; DiffMostVisited(cache_->top_sites(), top_sites, &delta); @@ -727,9 +368,9 @@ void TopSitesImpl::SetTopSites(const MostVisitedURLList& new_top_sites, TopSitesBackend::RECORD_HISTOGRAM_NO; // Record the delta size into a histogram if this function is called from - // function OnGotMostVisitedThumbnails and no histogram value has been - // recorded before. - if (location == CALL_LOCATION_FROM_ON_GOT_MOST_VISITED_THUMBNAILS && + // function OnGotMostVisitedURLs and no histogram value has been recorded + // before. + if (location == CALL_LOCATION_FROM_ON_GOT_MOST_VISITED_URLS && !histogram_recorded_) { size_t delta_size = delta.deleted.size() + delta.added.size() + delta.moved.size(); @@ -755,47 +396,11 @@ void TopSitesImpl::SetTopSites(const MostVisitedURLList& new_top_sites, // thread safe cache ...) as this method is invoked during startup at which // point the caches haven't been updated yet. cache_->SetTopSites(top_sites); - cache_->ClearUnreferencedThumbnails(); - - // See if we have any temp thumbnails for the new sites, and promote them to - // proper thumbnails. - if (!temp_images_.empty()) { - for (const MostVisitedURL& mv : top_sites) { - const GURL& canonical_url = cache_->GetCanonicalURL(mv.url); - // At the time we get the thumbnail redirects aren't known, so we have to - // iterate through all the images. - for (auto it = temp_images_.begin(); it != temp_images_.end(); ++it) { - if (canonical_url == cache_->GetCanonicalURL(it->first)) { - bool success = SetPageThumbnailEncoded( - mv.url, it->second.thumbnail.get(), it->second.thumbnail_score); - // TODO(treib): We shouldn't have a non-temp thumbnail yet at this - // point, so this should always succeed, but it doesn't - see - // crbug.com/735395. - if (success) { - UMA_HISTOGRAM_ENUMERATION("Thumbnails.AddedToTopSites", - THUMBNAIL_PROMOTED_TEMP_TO_REGULAR, - THUMBNAIL_EVENT_COUNT); - } - temp_images_.erase(it); - break; - } - } - } - } - - if (top_sites.size() - num_forced_urls >= kNonForcedTopSitesNumber) - temp_images_.clear(); ResetThreadSafeCache(); - ResetThreadSafeImageCache(); - - if (should_notify_observers) { - if (location == CALL_LOCATION_FROM_FORCED_URLS) - NotifyTopSitesChanged(TopSitesObserver::ChangeReason::FORCED_URL); - else - NotifyTopSitesChanged(TopSitesObserver::ChangeReason::MOST_VISITED); - } + if (should_notify_observers) + NotifyTopSitesChanged(TopSitesObserver::ChangeReason::MOST_VISITED); } int TopSitesImpl::num_results_to_request_from_history() const { @@ -803,14 +408,13 @@ int TopSitesImpl::num_results_to_request_from_history() const { const base::DictionaryValue* blacklist = pref_service_->GetDictionary(kMostVisitedURLsBlacklist); - return kNonForcedTopSitesNumber + (blacklist ? blacklist->size() : 0); + return kTopSitesNumber + (blacklist ? blacklist->size() : 0); } void TopSitesImpl::MoveStateToLoaded() { DCHECK(thread_checker_.CalledOnValidThread()); - MostVisitedURLList filtered_urls_all; - MostVisitedURLList filtered_urls_nonforced; + MostVisitedURLList urls; PendingCallbacks pending_callbacks; { base::AutoLock lock(lock_); @@ -822,18 +426,13 @@ void TopSitesImpl::MoveStateToLoaded() { // Now that we're loaded we can service the queued up callbacks. Copy them // here and service them outside the lock. if (!pending_callbacks_.empty()) { - // We always filter out forced URLs because callers of GetMostVisitedURLs - // are not interested in them. - filtered_urls_all = thread_safe_cache_->top_sites(); - filtered_urls_nonforced.assign(thread_safe_cache_->top_sites().begin() + - thread_safe_cache_->GetNumForcedURLs(), - thread_safe_cache_->top_sites().end()); + urls = thread_safe_cache_->top_sites(); pending_callbacks.swap(pending_callbacks_); } } for (size_t i = 0; i < pending_callbacks.size(); i++) - pending_callbacks[i].Run(filtered_urls_all, filtered_urls_nonforced); + pending_callbacks[i].Run(urls); if (history_service_) history_service_observer_.Add(history_service_); @@ -848,11 +447,6 @@ void TopSitesImpl::ResetThreadSafeCache() { thread_safe_cache_->SetTopSites(cached); } -void TopSitesImpl::ResetThreadSafeImageCache() { - base::AutoLock lock(lock_); - thread_safe_cache_->SetThumbnails(cache_->images()); -} - void TopSitesImpl::ScheduleUpdateTimer() { if (timer_.IsRunning()) return; @@ -861,18 +455,14 @@ void TopSitesImpl::ScheduleUpdateTimer() { &TopSitesImpl::StartQueryForMostVisited); } -void TopSitesImpl::OnGotMostVisitedThumbnails( - const scoped_refptr<MostVisitedThumbnails>& thumbnails) { +void TopSitesImpl::OnGotMostVisitedURLs( + const scoped_refptr<MostVisitedThreadSafe>& sites) { DCHECK(thread_checker_.CalledOnValidThread()); // Set the top sites directly in the cache so that SetTopSites diffs // correctly. - cache_->SetTopSites(thumbnails->most_visited); - SetTopSites(thumbnails->most_visited, - CALL_LOCATION_FROM_ON_GOT_MOST_VISITED_THUMBNAILS); - cache_->SetThumbnails(thumbnails->url_to_images_map); - - ResetThreadSafeImageCache(); + cache_->SetTopSites(sites->data); + SetTopSites(sites->data, CALL_LOCATION_FROM_ON_GOT_MOST_VISITED_URLS); MoveStateToLoaded(); diff --git a/chromium/components/history/core/browser/top_sites_impl.h b/chromium/components/history/core/browser/top_sites_impl.h index ea6751b3fb3..613df354ccc 100644 --- a/chromium/components/history/core/browser/top_sites_impl.h +++ b/chromium/components/history/core/browser/top_sites_impl.h @@ -17,7 +17,6 @@ #include "base/callback.h" #include "base/gtest_prod_util.h" #include "base/macros.h" -#include "base/memory/ref_counted.h" #include "base/scoped_observer.h" #include "base/synchronization/lock.h" #include "base/task/cancelable_task_tracker.h" @@ -29,9 +28,7 @@ #include "components/history/core/browser/top_sites.h" #include "components/history/core/browser/top_sites_backend.h" #include "components/history/core/browser/top_sites_provider.h" -#include "components/history/core/common/thumbnail_score.h" #include "third_party/skia/include/core/SkColor.h" -#include "ui/gfx/image/image.h" #include "url/gurl.h" class PrefRegistrySimple; @@ -39,8 +36,6 @@ class PrefService; namespace base { class FilePath; -class RefCountedBytes; -class RefCountedMemory; } namespace history { @@ -49,21 +44,18 @@ class HistoryService; class TopSitesCache; class TopSitesImplTest; -// This class allows requests for most visited urls and thumbnails on any -// thread. All other methods must be invoked on the UI thread. All mutations -// to internal state happen on the UI thread and are scheduled to update the -// db using TopSitesBackend. +// This class allows requests for most visited urls on any thread. All other +// methods must be invoked on the UI thread. All mutations to internal state +// happen on the UI thread and are scheduled to update the db using +// TopSitesBackend. class TopSitesImpl : public TopSites, public HistoryServiceObserver { public: // Called to check whether an URL can be added to the history. Must be // 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; + // How many top sites to store in the cache. + static constexpr size_t kTopSitesNumber = 10; TopSitesImpl(PrefService* pref_service, HistoryService* history_service, @@ -75,17 +67,7 @@ class TopSitesImpl : public TopSites, public HistoryServiceObserver { void Init(const base::FilePath& db_name); // TopSites implementation. - bool SetPageThumbnail(const GURL& url, - const gfx::Image& thumbnail, - const ThumbnailScore& score) override; - void GetMostVisitedURLs(const GetMostVisitedURLsCallback& callback, - bool include_forced_urls) override; - bool GetPageThumbnail(const GURL& url, - bool prefix_match, - scoped_refptr<base::RefCountedMemory>* bytes) override; - bool GetPageThumbnailScore(const GURL& url, ThumbnailScore* score) override; - bool GetTemporaryPageThumbnailScore(const GURL& url, - ThumbnailScore* score) override; + void GetMostVisitedURLs(const GetMostVisitedURLsCallback& callback) override; void SyncWithHistory() override; bool HasBlacklistedItems() const override; void AddBlacklistedURL(const GURL& url) override; @@ -93,11 +75,9 @@ class TopSitesImpl : public TopSites, public HistoryServiceObserver { bool IsBlacklisted(const GURL& url) override; void ClearBlacklistedURLs() override; bool IsKnownURL(const GURL& url) override; - bool IsNonForcedFull() override; - bool IsForcedFull() override; + bool IsFull() override; PrepopulatedPageList GetPrepopulatedPages() override; bool loaded() const override; - bool AddForcedURL(const GURL& url, const base::Time& time) override; void OnNavigationCommitted(const GURL& url) override; // RefcountedKeyedService: @@ -112,132 +92,62 @@ class TopSitesImpl : public TopSites, public HistoryServiceObserver { private: // TODO(yiyaoliu): Remove the enums and related code when crbug/223430 is // fixed. - // An enum representing different situations under which function - // SetTopSites can be initiated. + // An enum representing different situations under which function SetTopSites + // can be initiated. // This is needed because a histogram is used to record speed related metrics - // when SetTopSites are initiated from OnGotMostVisitedThumbnails, which - // usually happens early and might affect Chrome startup speed. + // when SetTopSites is initiated from OnGotMostVisitedURLs, which usually + // happens early and might affect Chrome startup speed. enum CallLocation { - // SetTopSites is called from function OnGotMostVisitedThumbnails. - CALL_LOCATION_FROM_ON_GOT_MOST_VISITED_THUMBNAILS, - // SetTopSites is called from AddForcedURLs. - CALL_LOCATION_FROM_FORCED_URLS, + // SetTopSites is called from function OnGotMostVisitedURLs. + CALL_LOCATION_FROM_ON_GOT_MOST_VISITED_URLS, // All other situations. CALL_LOCATION_FROM_OTHER_PLACES, }; - // An enum representing various outcomes of adding a page thumbnail, for use - // in UMA histograms. Most of these are recorded from SetPageThumbnail, - // except for PROMOTED_TEMP_TO_REGULAR which can happen during SetTopSites. - // Do not change existing entries, and only add new ones at the end! - enum ThumbnailEvent { - THUMBNAIL_FAILURE = 0, - THUMBNAIL_TOPSITES_FULL = 1, - THUMBNAIL_KEPT_EXISTING = 2, - THUMBNAIL_ADDED_TEMP = 3, - THUMBNAIL_ADDED_REGULAR = 4, - THUMBNAIL_PROMOTED_TEMP_TO_REGULAR = 5, - // Add new entries here. - THUMBNAIL_EVENT_COUNT = 6 - }; - friend class TopSitesImplTest; FRIEND_TEST_ALL_PREFIXES(TopSitesImplTest, DiffMostVisited); FRIEND_TEST_ALL_PREFIXES(TopSitesImplTest, DiffMostVisitedWithForced); - typedef base::Callback<void(const MostVisitedURLList&, - const MostVisitedURLList&)> PendingCallback; + typedef base::Callback<void(const MostVisitedURLList&)> PendingCallback; - typedef std::pair<GURL, Images> TempImage; - 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. + // Starts to query most visited URLs from history database instantly. Also + // cancels any pending queries requested in a delayed manner by canceling the + // timer. void StartQueryForMostVisited(); // Generates the diff of things that happened between "old" and "new." // - // This treats forced URLs separately than non-forced URLs. - // - // The URLs that are in "new" but not "old" will be have their index into - // "new" put in |added_urls|. The non-forced URLs that are in "old" but not - // "new" will have their index into "old" put into |deleted_urls|. + // The URLs that are in "new" but not "old" will be have their index from + // "new" placed in |added_urls|. The URLs that are in "old" but not "new" will + // have their index from "old" placed in |deleted_urls|. // - // URLs appearing in both old and new lists but having different indices will - // have their index into "new" be put into |moved_urls|. + // URLs that appear in both lists but have different indices will have their + // index from "new" placed in |moved_urls|. static void DiffMostVisited(const MostVisitedURLList& old_list, const MostVisitedURLList& new_list, TopSitesDelta* delta); - // The actual implementation of SetPageThumbnail. It returns a more detailed - // status code (for UMA) rather than just a bool. - ThumbnailEvent SetPageThumbnailImpl(const GURL& url, - const gfx::Image& thumbnail, - const ThumbnailScore& score); - - // Sets the thumbnail without writing to the database, i.e. updates the cache - // only. Must only be called for known URLs. Returns true if the thumbnail was - // set, false if the existing one (if any) is better. - bool SetPageThumbnailInCache(const GURL& url, - const base::RefCountedMemory* thumbnail_data, - const ThumbnailScore& score); - - // The major part of SetPageThumbnail, which sets the thumbnail both in the - // cache and in the database. Must only be called for known URLs. Returns true - // if the thumbnail was set, false if the existing one (if any) is better. - bool SetPageThumbnailEncoded(const GURL& url, - const base::RefCountedMemory* thumbnail, - const ThumbnailScore& score); - - // Encodes the bitmap to bytes for storage to the db. Returns true if the - // bitmap was successfully encoded. - static bool EncodeBitmap(const gfx::Image& bitmap, - scoped_refptr<base::RefCountedBytes>* bytes); - - // Removes the cached thumbnail for |url|. Does nothing if |url| is not cached - // in |temp_images_|. - void RemoveTemporaryThumbnailByURL(const GURL& url); - - // Adds a thumbnail for an unknown url. See |temp_images_|. - void AddTemporaryThumbnail(const GURL& url, - base::RefCountedMemory* thumbnail, - const ThumbnailScore& score); - // 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); - // 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; - - // 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 - // after the call. If the list of forced URLs overflows the older ones are - // dropped. Returns the number of forced URLs after the merge. - size_t MergeCachedForcedURLs(MostVisitedURLList* new_list) const; + // Adds prepopulated pages to TopSites. Returns true if any pages were added. + bool AddPrepopulatedPages(MostVisitedURLList* urls) const; // Takes |urls|, produces it's copy in |out| after removing blacklisted URLs. - // Also ensures we respect the maximum number of forced URLs and non-forced - // URLs. + // Also ensures we respect the maximum number TopSites URLs. void ApplyBlacklist(const MostVisitedURLList& urls, MostVisitedURLList* out); // Returns an MD5 hash of the URL. Hashing is required for blacklisted URLs. static std::string GetURLHash(const GURL& url); - // 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_|, - // if the list of forced URLs overflows, the oldest ones are dropped. - // All mutations to cache_ *must* go through this. Should - // be called from the UI thread. + // Updates URLs in |cache_| and the db (in the background). The URLs in + // |new_top_sites| replace those in |cache_|. All mutations to cache_ *must* + // go through this. Should be called from the UI thread. void SetTopSites(const MostVisitedURLList& new_top_sites, const CallLocation location); @@ -252,16 +162,13 @@ class TopSitesImpl : public TopSites, public HistoryServiceObserver { void ResetThreadSafeCache(); - void ResetThreadSafeImageCache(); - // 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. - void OnGotMostVisitedThumbnails( - const scoped_refptr<MostVisitedThumbnails>& thumbnails); + // Callback from TopSites with the list of top sites. Should be called from + // the UI thread. + void OnGotMostVisitedURLs(const scoped_refptr<MostVisitedThreadSafe>& sites); // Called when history service returns a list of top URLs. void OnTopSitesAvailableFromHistory(const MostVisitedURLList* data); @@ -298,12 +205,6 @@ class TopSitesImpl : public TopSites, public HistoryServiceObserver { // cached list and be able to run callbacks immediately. PendingCallbacks pending_callbacks_; - // Stores thumbnails for unknown pages. When SetPageThumbnail is - // called, if we don't know about that URL yet and we don't have - // enough Top Sites (new profile), we store it until the next - // SetTopSites call. - TempImages temp_images_; - // URL List of prepopulated page. const PrepopulatedPageList prepopulated_pages_; 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 a47746b5cbb..b991feecf8b 100644 --- a/chromium/components/history/core/browser/top_sites_impl_unittest.cc +++ b/chromium/components/history/core/browser/top_sites_impl_unittest.cc @@ -35,7 +35,6 @@ #include "components/prefs/testing_pref_service.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" -#include "third_party/skia/include/core/SkBitmap.h" #include "url/gurl.h" using testing::ContainerEq; @@ -64,20 +63,11 @@ class TopSitesQuerier { // Queries top sites. If |wait| is true a nested run loop is run until the // callback is notified. void QueryTopSites(TopSitesImpl* top_sites, bool wait) { - QueryAllTopSites(top_sites, wait, false); - } - - // Queries top sites, including potentially forced URLs if - // |include_forced_urls| is true. - void QueryAllTopSites(TopSitesImpl* top_sites, - bool wait, - bool include_forced_urls) { int start_number_of_callbacks = number_of_callbacks_; base::RunLoop run_loop; top_sites->GetMostVisitedURLs( base::Bind(&TopSitesQuerier::OnTopSitesAvailable, - weak_ptr_factory_.GetWeakPtr(), &run_loop), - include_forced_urls); + weak_ptr_factory_.GetWeakPtr(), &run_loop)); if (wait && start_number_of_callbacks == number_of_callbacks_) { waiting_ = true; run_loop.Run(); @@ -111,16 +101,6 @@ class TopSitesQuerier { DISALLOW_COPY_AND_ASSIGN(TopSitesQuerier); }; -// Returns true if t1 and t2 contain the same data. -bool ThumbnailsAreEqual(base::RefCountedMemory* t1, - base::RefCountedMemory* t2) { - if (!t1 || !t2) - return false; - if (t1->size() != t2->size()) - return false; - return !memcmp(t1->front(), t2->front(), t1->size()); -} - } // namespace class TopSitesImplTest : public HistoryUnitTestBase { @@ -146,14 +126,6 @@ class TopSitesImplTest : public HistoryUnitTestBase { pref_service_.reset(); } - // Creates a bitmap of the specified color. Caller takes ownership. - gfx::Image CreateBitmap(SkColor color) { - SkBitmap thumbnail; - thumbnail.allocN32Pixels(4, 4); - thumbnail.eraseColor(color); - return gfx::Image::CreateFrom1xBitmap(thumbnail); // adds ref. - } - // Forces top sites to load top sites from history, then recreates top sites. // Recreating top sites makes sure the changes from history are saved and // loaded from the db. @@ -224,14 +196,6 @@ class TopSitesImplTest : public HistoryUnitTestBase { // Delets a url. void DeleteURL(const GURL& url) { history_service()->DeleteURL(url); } - // Returns true if the thumbnail equals the specified bytes. - bool ThumbnailEqualsBytes(const gfx::Image& image, - base::RefCountedMemory* bytes) { - scoped_refptr<base::RefCountedBytes> encoded_image; - TopSitesImpl::EncodeBitmap(image, &encoded_image); - return ThumbnailsAreEqual(encoded_image.get(), bytes); - } - // Recreates top sites. This forces top sites to reread from the db. void RecreateTopSitesAndBlock() { // Recreate TopSites and wait for it to load. @@ -250,16 +214,12 @@ class TopSitesImplTest : public HistoryUnitTestBase { TopSitesImpl::CALL_LOCATION_FROM_OTHER_PLACES); } - bool AddForcedURL(const GURL& url, base::Time time) { - return top_sites()->AddForcedURL(url, time); - } - void StartQueryForMostVisited() { top_sites()->StartQueryForMostVisited(); } bool IsTopSitesLoaded() { return top_sites()->loaded_; } bool AddPrepopulatedPages(MostVisitedURLList* urls) { - return top_sites()->AddPrepopulatedPages(urls, 0u); + return top_sites()->AddPrepopulatedPages(urls); } void EmptyThreadSafeCache() { @@ -276,8 +236,8 @@ class TopSitesImplTest : public HistoryUnitTestBase { DestroyTopSites(); DCHECK(!top_sites_impl_); PrepopulatedPageList prepopulated_pages; - prepopulated_pages.push_back(PrepopulatedPage(GURL(kPrepopulatedPageURL), - base::string16(), -1, -1, 0)); + prepopulated_pages.push_back( + PrepopulatedPage(GURL(kPrepopulatedPageURL), base::string16(), -1, 0)); top_sites_impl_ = new TopSitesImpl( pref_service_.get(), history_service_.get(), std::make_unique<DefaultTopSitesProvider>(history_service_.get()), @@ -327,18 +287,6 @@ void AppendMostVisitedURL(const GURL& url, 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. -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); - mv.redirects.push_back(url); - list->push_back(mv); -} - // Same as AppendMostVisitedURL except that it adds a redirect from the first // URL to the second. void AppendMostVisitedURLWithRedirect(const GURL& redirect_source, @@ -494,188 +442,6 @@ TEST_F(TopSitesImplTest, DiffMostVisited) { EXPECT_EQ(3, delta.moved[0].rank); } -// Tests DiffMostVisited with forced URLs. -TEST_F(TopSitesImplTest, DiffMostVisitedWithForced) { - // Forced URLs. - GURL stays_the_same_1("http://staysthesame1/"); - GURL new_last_forced_time("http://newlastforcedtime/"); - GURL stays_the_same_2("http://staysthesame2/"); - GURL move_to_nonforced("http://movetononforced/"); - GURL gets_added_1("http://getsadded1/"); - GURL gets_deleted_1("http://getsdeleted1/"); - // Non-forced URLs. - GURL move_to_forced("http://movetoforced/"); - GURL stays_the_same_3("http://staysthesame3/"); - GURL gets_added_2("http://getsadded2/"); - GURL gets_deleted_2("http://getsdeleted2/"); - GURL gets_moved_1("http://getsmoved1/"); - - std::vector<MostVisitedURL> old_list; - 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(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); - EXPECT_EQ(-1, delta.added[0].rank); - EXPECT_TRUE(gets_added_2 == delta.added[1].url.url); - EXPECT_EQ(2, delta.added[1].rank); - - ASSERT_EQ(2u, delta.deleted.size()); - EXPECT_TRUE(gets_deleted_1 == delta.deleted[0].url); - EXPECT_TRUE(gets_deleted_2 == delta.deleted[1].url); - - ASSERT_EQ(3u, delta.moved.size()); - EXPECT_TRUE(new_last_forced_time == delta.moved[0].url.url); - EXPECT_EQ(-1, delta.moved[0].rank); - EXPECT_EQ(base::Time::FromJsTime(4000), delta.moved[0].url.last_forced_time); - EXPECT_TRUE(move_to_forced == delta.moved[1].url.url); - EXPECT_EQ(-1, delta.moved[1].rank); - EXPECT_EQ(base::Time::FromJsTime(6000), delta.moved[1].url.last_forced_time); - EXPECT_TRUE(move_to_nonforced == delta.moved[2].url.url); - EXPECT_EQ(0, delta.moved[2].rank); - EXPECT_TRUE(delta.moved[2].url.last_forced_time.is_null()); -} - -// Tests SetPageThumbnail. -TEST_F(TopSitesImplTest, SetPageThumbnail) { - GURL url1a("http://google.com/"); - GURL url1b("http://www.google.com/"); - GURL url2("http://images.google.com/"); - GURL invalid_url("application://favicon/http://google.com/"); - - std::vector<MostVisitedURL> list; - AppendMostVisitedURL(url2, &list); - - MostVisitedURL mv; - mv.url = url1b; - mv.redirects.push_back(url1a); - mv.redirects.push_back(url1b); - list.push_back(mv); - - // Save our most visited data containing that one site. - SetTopSites(list); - - // Create a dummy thumbnail. - gfx::Image thumbnail(CreateBitmap(SK_ColorWHITE)); - - base::Time now = base::Time::Now(); - ThumbnailScore low_score(1.0, true, true, now); - ThumbnailScore medium_score(0.5, true, true, now); - ThumbnailScore high_score(0.0, true, true, now); - - // Setting the thumbnail for invalid pages should fail. - EXPECT_FALSE( - top_sites()->SetPageThumbnail(invalid_url, thumbnail, medium_score)); - - // Setting the thumbnail for url2 should succeed, lower scores shouldn't - // replace it, higher scores should. - EXPECT_TRUE(top_sites()->SetPageThumbnail(url2, thumbnail, medium_score)); - EXPECT_FALSE(top_sites()->SetPageThumbnail(url2, thumbnail, low_score)); - EXPECT_TRUE(top_sites()->SetPageThumbnail(url2, thumbnail, high_score)); - - // Set on the redirect source should succeed. It should be replacable by - // the same score on the redirect destination, which in turn should not - // be replaced by the source again. - EXPECT_TRUE(top_sites()->SetPageThumbnail(url1a, thumbnail, medium_score)); - EXPECT_TRUE(top_sites()->SetPageThumbnail(url1b, thumbnail, medium_score)); - EXPECT_FALSE(top_sites()->SetPageThumbnail(url1a, thumbnail, medium_score)); -} - -// Makes sure a thumbnail is correctly removed when the page is removed. -TEST_F(TopSitesImplTest, ThumbnailRemoved) { - GURL url("http://google.com/"); - - // Configure top sites with 'google.com'. - std::vector<MostVisitedURL> list; - AppendMostVisitedURL(url, &list); - SetTopSites(list); - - // Create a dummy thumbnail. - gfx::Image thumbnail(CreateBitmap(SK_ColorRED)); - - base::Time now = base::Time::Now(); - ThumbnailScore low_score(1.0, true, true, now); - ThumbnailScore medium_score(0.5, true, true, now); - ThumbnailScore high_score(0.0, true, true, now); - - // Set the thumbnail. - EXPECT_TRUE(top_sites()->SetPageThumbnail(url, thumbnail, medium_score)); - - // Make sure the thumbnail was actually set. - scoped_refptr<base::RefCountedMemory> result; - EXPECT_TRUE(top_sites()->GetPageThumbnail(url, false, &result)); - EXPECT_TRUE(ThumbnailEqualsBytes(thumbnail, result.get())); - - // Reset the thumbnails and make sure we don't get it back. - SetTopSites(MostVisitedURLList()); - EXPECT_FALSE(top_sites()->GetPageThumbnail(url, false, &result)); - // Recreating the TopSites object should also not bring it back. - RefreshTopSitesAndRecreate(); - EXPECT_FALSE(top_sites()->GetPageThumbnail(url, false, &result)); -} - -// Tests GetPageThumbnail. -TEST_F(TopSitesImplTest, GetPageThumbnail) { - MostVisitedURLList url_list; - MostVisitedURL url1; - url1.url = GURL("http://asdf.com"); - url1.redirects.push_back(url1.url); - url_list.push_back(url1); - - MostVisitedURL url2; - url2.url = GURL("http://gmail.com"); - url2.redirects.push_back(url2.url); - url2.redirects.push_back(GURL("http://mail.google.com")); - url_list.push_back(url2); - - SetTopSites(url_list); - - // Create a dummy thumbnail. - gfx::Image thumbnail(CreateBitmap(SK_ColorWHITE)); - ThumbnailScore score(0.5, true, true, base::Time::Now()); - - scoped_refptr<base::RefCountedMemory> result; - EXPECT_TRUE(top_sites()->SetPageThumbnail(url1.url, thumbnail, score)); - EXPECT_TRUE(top_sites()->GetPageThumbnail(url1.url, false, &result)); - - EXPECT_TRUE(top_sites()->SetPageThumbnail(GURL("http://gmail.com"), - thumbnail, score)); - EXPECT_TRUE(top_sites()->GetPageThumbnail(GURL("http://gmail.com"), - false, - &result)); - // Get a thumbnail via a redirect. - EXPECT_TRUE(top_sites()->GetPageThumbnail(GURL("http://mail.google.com"), - false, - &result)); - - EXPECT_TRUE(top_sites()->SetPageThumbnail(GURL("http://mail.google.com"), - thumbnail, score)); - EXPECT_TRUE(top_sites()->GetPageThumbnail(url2.url, false, &result)); - - EXPECT_TRUE(ThumbnailEqualsBytes(thumbnail, result.get())); -} - // Tests GetMostVisitedURLs. TEST_F(TopSitesImplTest, GetMostVisited) { GURL news("http://news.google.com/"); @@ -720,9 +486,9 @@ TEST_F(TopSitesImplTest, GetMostVisitedWithRedirect) { // This behavior is not desirable: even though edition.cnn.com is in the list // of top sites, and the the bare URL cnn.com is just a redirect to it, we're - // returning both. Even worse, the NTP will show the same title, icon, and - // thumbnail for the site, so to the user it looks like we just have the same - // thing twice. (https://crbug.com/567132) + // returning both. Even worse, the NTP will show the same title, and icon for + // the site, so to the user it looks like we just have the same thing twice. + // (https://crbug.com/567132) std::vector<GURL> expected_urls = {bare, edition}; // should be {edition}. for (const auto& prepopulated : GetPrepopulatedPages()) { @@ -752,11 +518,6 @@ TEST_F(TopSitesImplTest, SaveToDB) { StartQueryForMostVisited(); WaitForHistory(); - // Add a thumbnail. - gfx::Image tmp_bitmap(CreateBitmap(SK_ColorBLUE)); - ASSERT_TRUE(top_sites()->SetPageThumbnail(asdf_url, tmp_bitmap, - ThumbnailScore())); - RecreateTopSitesAndBlock(); { @@ -766,10 +527,6 @@ TEST_F(TopSitesImplTest, SaveToDB) { EXPECT_EQ(asdf_url, querier.urls()[0].url); EXPECT_EQ(asdf_title, querier.urls()[0].title); ASSERT_NO_FATAL_FAILURE(ContainsPrepopulatePages(querier, 1)); - - scoped_refptr<base::RefCountedMemory> read_data; - EXPECT_TRUE(top_sites()->GetPageThumbnail(asdf_url, false, &read_data)); - EXPECT_TRUE(ThumbnailEqualsBytes(tmp_bitmap, read_data.get())); } MostVisitedURL url2; @@ -779,11 +536,6 @@ TEST_F(TopSitesImplTest, SaveToDB) { AddPageToHistory(url2.url, url2.title); - // Add new thumbnail at rank 0 and shift the other result to 1. - ASSERT_TRUE(top_sites()->SetPageThumbnail(google_url, - tmp_bitmap, - ThumbnailScore())); - // Make TopSites reread from the db. RefreshTopSitesAndRecreate(); @@ -799,65 +551,6 @@ TEST_F(TopSitesImplTest, SaveToDB) { } } -// Makes sure forced URLs in top sites get mirrored to the db. -TEST_F(TopSitesImplTest, SaveForcedToDB) { - MostVisitedURL url; - GURL asdf_url("http://asdf.com"); - base::string16 asdf_title(base::ASCIIToUTF16("ASDF")); - GURL google_url("http://google.com"); - base::string16 google_title(base::ASCIIToUTF16("Google")); - GURL news_url("http://news.google.com"); - base::string16 news_title(base::ASCIIToUTF16("Google News")); - - // Add a number of forced URLs. - std::vector<MostVisitedURL> list; - AppendForcedMostVisitedURL(GURL("http://forced1"), 1000, &list); - list[0].title = base::ASCIIToUTF16("forced1"); - AppendForcedMostVisitedURL(GURL("http://forced2"), 2000, &list); - AppendForcedMostVisitedURL(GURL("http://forced3"), 3000, &list); - AppendForcedMostVisitedURL(GURL("http://forced4"), 4000, &list); - SetTopSites(list); - - // Add a thumbnail. - gfx::Image red_thumbnail(CreateBitmap(SK_ColorRED)); - ASSERT_TRUE(top_sites()->SetPageThumbnail( - GURL("http://forced1"), red_thumbnail, ThumbnailScore())); - - // Get the original thumbnail for later comparison. Some compression can - // happen in |top_sites| and we don't want to depend on that. - scoped_refptr<base::RefCountedMemory> orig_thumbnail_data; - ASSERT_TRUE(top_sites()->GetPageThumbnail(GURL("http://forced1"), false, - &orig_thumbnail_data)); - - // Force-flush the cache to ensure we don't reread from it inadvertently. - EmptyThreadSafeCache(); - - // Make TopSites reread from the db. - StartQueryForMostVisited(); - WaitForHistory(); - - TopSitesQuerier querier; - querier.QueryAllTopSites(top_sites(), true, true); - - ASSERT_EQ(4u + GetPrepopulatedPages().size(), querier.urls().size()); - EXPECT_EQ(GURL("http://forced1"), querier.urls()[0].url); - EXPECT_EQ(base::ASCIIToUTF16("forced1"), querier.urls()[0].title); - scoped_refptr<base::RefCountedMemory> thumbnail_data; - ASSERT_TRUE(top_sites()->GetPageThumbnail(GURL("http://forced1"), false, - &thumbnail_data)); - ASSERT_TRUE( - ThumbnailsAreEqual(orig_thumbnail_data.get(), thumbnail_data.get())); - EXPECT_EQ(base::Time::FromJsTime(1000), querier.urls()[0].last_forced_time); - EXPECT_EQ(GURL("http://forced2"), querier.urls()[1].url); - EXPECT_EQ(base::Time::FromJsTime(2000), querier.urls()[1].last_forced_time); - EXPECT_EQ(GURL("http://forced3"), querier.urls()[2].url); - EXPECT_EQ(base::Time::FromJsTime(3000), querier.urls()[2].last_forced_time); - EXPECT_EQ(GURL("http://forced4"), querier.urls()[3].url); - EXPECT_EQ(base::Time::FromJsTime(4000), querier.urls()[3].last_forced_time); - - ASSERT_NO_FATAL_FAILURE(ContainsPrepopulatePages(querier, 4)); -} - // More permutations of saving to db. TEST_F(TopSitesImplTest, RealDatabase) { MostVisitedURL url; @@ -873,9 +566,6 @@ TEST_F(TopSitesImplTest, RealDatabase) { url.url = asdf_url; url.title = asdf_title; url.redirects.push_back(url.url); - gfx::Image asdf_thumbnail(CreateBitmap(SK_ColorRED)); - ASSERT_TRUE(top_sites()->SetPageThumbnail( - asdf_url, asdf_thumbnail, ThumbnailScore())); base::Time add_time(base::Time::Now()); AddPageToHistory(url.url, url.title, url.redirects, add_time); @@ -890,10 +580,6 @@ TEST_F(TopSitesImplTest, RealDatabase) { EXPECT_EQ(asdf_url, querier.urls()[0].url); EXPECT_EQ(asdf_title, querier.urls()[0].title); ASSERT_NO_FATAL_FAILURE(ContainsPrepopulatePages(querier, 1)); - - scoped_refptr<base::RefCountedMemory> read_data; - EXPECT_TRUE(top_sites()->GetPageThumbnail(asdf_url, false, &read_data)); - EXPECT_TRUE(ThumbnailEqualsBytes(asdf_thumbnail, read_data.get())); } MostVisitedURL url2; @@ -909,10 +595,6 @@ TEST_F(TopSitesImplTest, RealDatabase) { AddPageToHistory(google3_url, url2.title, url2.redirects, add_time - base::TimeDelta::FromMinutes(2)); - gfx::Image google_thumbnail(CreateBitmap(SK_ColorBLUE)); - ASSERT_TRUE(top_sites()->SetPageThumbnail( - url2.url, google_thumbnail, ThumbnailScore())); - RefreshTopSitesAndRecreate(); { @@ -924,52 +606,11 @@ TEST_F(TopSitesImplTest, RealDatabase) { EXPECT_EQ(google1_url, querier.urls()[0].url); EXPECT_EQ(google_title, querier.urls()[0].title); ASSERT_EQ(3u, querier.urls()[0].redirects.size()); - EXPECT_TRUE(top_sites()->GetPageThumbnail(google3_url, false, &read_data)); - EXPECT_TRUE(ThumbnailEqualsBytes(google_thumbnail, read_data.get())); EXPECT_EQ(asdf_url, querier.urls()[1].url); EXPECT_EQ(asdf_title, querier.urls()[1].title); ASSERT_NO_FATAL_FAILURE(ContainsPrepopulatePages(querier, 2)); } - - gfx::Image weewar_bitmap(CreateBitmap(SK_ColorYELLOW)); - - base::Time thumbnail_time(base::Time::Now()); - ThumbnailScore low_score(1.0, true, true, thumbnail_time); - ThumbnailScore medium_score(0.5, true, true, thumbnail_time); - ThumbnailScore high_score(0.0, true, true, thumbnail_time); - - // 1. Set to weewar. (Writes the thumbnail to the DB.) - EXPECT_TRUE(top_sites()->SetPageThumbnail(google3_url, - weewar_bitmap, - medium_score)); - RefreshTopSitesAndRecreate(); - { - scoped_refptr<base::RefCountedMemory> read_data; - EXPECT_TRUE(top_sites()->GetPageThumbnail(google3_url, false, &read_data)); - EXPECT_TRUE(ThumbnailEqualsBytes(weewar_bitmap, read_data.get())); - } - - gfx::Image green_bitmap(CreateBitmap(SK_ColorGREEN)); - - // 2. Set to google - low score. - EXPECT_FALSE(top_sites()->SetPageThumbnail(google3_url, - green_bitmap, - low_score)); - - // 3. Set to google - high score. - EXPECT_TRUE(top_sites()->SetPageThumbnail(google1_url, - green_bitmap, - high_score)); - - // Check that the thumbnail was updated. - RefreshTopSitesAndRecreate(); - { - scoped_refptr<base::RefCountedMemory> read_data; - EXPECT_TRUE(top_sites()->GetPageThumbnail(google3_url, false, &read_data)); - EXPECT_FALSE(ThumbnailEqualsBytes(weewar_bitmap, read_data.get())); - EXPECT_TRUE(ThumbnailEqualsBytes(green_bitmap, read_data.get())); - } } TEST_F(TopSitesImplTest, DeleteNotifications) { @@ -1161,53 +802,6 @@ TEST_F(TopSitesImplTest, CancelingRequestsForTopSites) { EXPECT_EQ(0, querier2.number_of_callbacks()); } -// Makes sure temporary thumbnails are copied over correctly. -TEST_F(TopSitesImplTest, AddTemporaryThumbnail) { - GURL unknown_url("http://news.google.com/"); - GURL invalid_url("application://thumb/http://google.com/"); - GURL url1a("http://google.com/"); - GURL url1b("http://www.google.com/"); - - // Create a dummy thumbnail. - gfx::Image thumbnail(CreateBitmap(SK_ColorRED)); - - ThumbnailScore medium_score(0.5, true, true, base::Time::Now()); - - // Don't store thumbnails for Javascript URLs. - EXPECT_FALSE(top_sites()->SetPageThumbnail(invalid_url, - thumbnail, - medium_score)); - // Store thumbnails for unknown (but valid) URLs temporarily - calls - // AddTemporaryThumbnail. - EXPECT_TRUE(top_sites()->SetPageThumbnail(unknown_url, - thumbnail, - medium_score)); - - // We shouldn't get the thumnail back though (the url isn't in to sites yet). - scoped_refptr<base::RefCountedMemory> out; - EXPECT_FALSE(top_sites()->GetPageThumbnail(unknown_url, false, &out)); - // But we should be able to get the temporary page thumbnail score. - ThumbnailScore out_score; - EXPECT_TRUE(top_sites()->GetTemporaryPageThumbnailScore(unknown_url, - &out_score)); - EXPECT_TRUE(medium_score.Equals(out_score)); - - std::vector<MostVisitedURL> list; - - MostVisitedURL mv; - mv.url = unknown_url; - mv.redirects.push_back(mv.url); - mv.redirects.push_back(url1a); - mv.redirects.push_back(url1b); - list.push_back(mv); - - // Update URLs. This should result in using thumbnail. - SetTopSites(list); - - ASSERT_TRUE(top_sites()->GetPageThumbnail(unknown_url, false, &out)); - EXPECT_TRUE(ThumbnailEqualsBytes(thumbnail, out.get())); -} - // Tests variations of blacklisting without testing prepopulated page // blacklisting. TEST_F(TopSitesImplTest, BlacklistingWithoutPrepopulated) { @@ -1380,307 +974,4 @@ TEST_F(TopSitesImplTest, AddPrepopulatedPages) { ASSERT_NO_FATAL_FAILURE(ContainsPrepopulatePages(q, 0)); } -// Ensure calling SetTopSites with forced sites already in the DB works. -// This test both eviction and -TEST_F(TopSitesImplTest, SetForcedTopSites) { - // Create forced elements in old URL list. - MostVisitedURLList old_url_list; - 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(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. - for (size_t i = 0; i < kNonForcedTopSitesCount; ++i) { - std::ostringstream url; - url << "http://oldnonforced/" << i; - AppendMostVisitedURL(GURL(url.str()), &old_url_list); - url.str(""); - url << "http://newnonforced/" << i; - AppendMostVisitedURL(GURL(url.str()), &new_url_list); - } - - // Set the initial list of URLs. - SetTopSites(old_url_list); - - TopSitesQuerier querier; - // Query only non-forced URLs first. - querier.QueryTopSites(top_sites(), false); - 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 + kNonForcedTopSitesCount, querier.urls().size()); - - // Check first URLs. - EXPECT_EQ("http://oldforced/0", querier.urls()[0].url.spec()); - EXPECT_EQ("http://oldnonforced/0", - querier.urls()[kNumOldForcedURLs].url.spec()); - - // Set the new list of URLs. - SetTopSites(new_url_list); - - // Query all URLs. - querier.QueryAllTopSites(top_sites(), false, true); - - // 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://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()[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(GURL("http://url/0"), 1000, &old_url_list); - // The following three will be evicted. - 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(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(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. - TopSitesQuerier querier; - querier.QueryAllTopSites(top_sites(), false, true); - - // Check URLs. When collision occurs, the incoming one is always preferred. - ASSERT_EQ(7u + GetPrepopulatedPages().size(), querier.urls().size()); - EXPECT_EQ("http://url/0", querier.urls()[0].url.spec()); - EXPECT_EQ(1000u, querier.urls()[0].last_forced_time.ToJsTime()); - EXPECT_EQ("http://collision/1", querier.urls()[1].url.spec()); - EXPECT_EQ(2000u, querier.urls()[1].last_forced_time.ToJsTime()); - EXPECT_EQ("http://url/2", querier.urls()[2].url.spec()); - EXPECT_EQ(3000u, querier.urls()[2].last_forced_time.ToJsTime()); - EXPECT_EQ("http://collision/0", querier.urls()[3].url.spec()); - EXPECT_EQ(5000u, querier.urls()[3].last_forced_time.ToJsTime()); - EXPECT_EQ("http://noncollision/0", querier.urls()[4].url.spec()); - EXPECT_EQ(9000u, querier.urls()[4].last_forced_time.ToJsTime()); - EXPECT_EQ("http://collision/2", querier.urls()[5].url.spec()); - EXPECT_TRUE(querier.urls()[5].last_forced_time.is_null()); - EXPECT_EQ("http://url/3", querier.urls()[6].url.spec()); - EXPECT_TRUE(querier.urls()[6].last_forced_time.is_null()); - ASSERT_NO_FATAL_FAILURE(ContainsPrepopulatePages(querier, 7)); -} - -TEST_F(TopSitesImplTest, SetTopSitesIdentical) { - // Set the initial list of URLs. - MostVisitedURLList url_list; - 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. - SetTopSites(MostVisitedURLList(url_list)); - - // Query all URLs. - TopSitesQuerier querier; - querier.QueryAllTopSites(top_sites(), false, true); - - // Check URLs. When collision occurs, the incoming one is always preferred. - ASSERT_EQ(3u + GetPrepopulatedPages().size(), querier.urls().size()); - EXPECT_EQ("http://url/0", querier.urls()[0].url.spec()); - EXPECT_EQ(1000u, querier.urls()[0].last_forced_time.ToJsTime()); - EXPECT_EQ("http://url/1", querier.urls()[1].url.spec()); - EXPECT_EQ("http://url/2", querier.urls()[2].url.spec()); - ASSERT_NO_FATAL_FAILURE(ContainsPrepopulatePages(querier, 3)); -} - -TEST_F(TopSitesImplTest, SetTopSitesWithAlreadyExistingForcedURLs) { - // Set the initial list of URLs. - MostVisitedURLList old_url_list; - 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(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. - TopSitesQuerier querier; - querier.QueryAllTopSites(top_sites(), false, true); - - // Check URLs. When collision occurs, the non-forced one is always preferred. - ASSERT_EQ(2u + GetPrepopulatedPages().size(), querier.urls().size()); - EXPECT_EQ("http://url/0", querier.urls()[0].url.spec()); - EXPECT_EQ("http://url/0/redir", querier.urls()[0].redirects[0].spec()); - EXPECT_TRUE(querier.urls()[0].last_forced_time.is_null()); - EXPECT_EQ("http://url/1", querier.urls()[1].url.spec()); - EXPECT_TRUE(querier.urls()[1].last_forced_time.is_null()); - ASSERT_NO_FATAL_FAILURE(ContainsPrepopulatePages(querier, 2)); -} - -TEST_F(TopSitesImplTest, AddForcedURL) { - // Set the initial list of URLs. - MostVisitedURLList url_list; - 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. - EXPECT_TRUE(AddForcedURL(GURL("http://forced/2"), - base::Time::FromJsTime(5000))); - EXPECT_TRUE(AddForcedURL(GURL("http://forced/3"), - base::Time::FromJsTime(1000))); - EXPECT_TRUE(AddForcedURL(GURL("http://forced/4"), - base::Time::FromJsTime(3000))); - - // Check URLs. - TopSitesQuerier querier; - querier.QueryAllTopSites(top_sites(), false, true); - ASSERT_EQ(8u + GetPrepopulatedPages().size(), querier.urls().size()); - EXPECT_EQ("http://forced/3", querier.urls()[0].url.spec()); - EXPECT_EQ(1000u, querier.urls()[0].last_forced_time.ToJsTime()); - EXPECT_EQ("http://forced/0", querier.urls()[1].url.spec()); - EXPECT_EQ(2000u, querier.urls()[1].last_forced_time.ToJsTime()); - EXPECT_EQ("http://forced/4", querier.urls()[2].url.spec()); - EXPECT_EQ(3000u, querier.urls()[2].last_forced_time.ToJsTime()); - EXPECT_EQ("http://forced/1", querier.urls()[3].url.spec()); - EXPECT_EQ(4000u, querier.urls()[3].last_forced_time.ToJsTime()); - EXPECT_EQ("http://forced/2", querier.urls()[4].url.spec()); - EXPECT_EQ(5000u, querier.urls()[4].last_forced_time.ToJsTime()); - EXPECT_EQ("http://nonforced/0", querier.urls()[5].url.spec()); - EXPECT_TRUE(querier.urls()[5].last_forced_time.is_null()); - EXPECT_EQ("http://nonforced/1", querier.urls()[6].url.spec()); - EXPECT_TRUE(querier.urls()[6].last_forced_time.is_null()); - EXPECT_EQ("http://nonforced/2", querier.urls()[7].url.spec()); - EXPECT_TRUE(querier.urls()[7].last_forced_time.is_null()); - ASSERT_NO_FATAL_FAILURE(ContainsPrepopulatePages(querier, 8)); - - // Add some collisions with forced and non-forced. Non-forced URLs are never - // expected to move. - EXPECT_TRUE(AddForcedURL(GURL("http://forced/3"), - base::Time::FromJsTime(4000))); - EXPECT_TRUE(AddForcedURL(GURL("http://forced/1"), - base::Time::FromJsTime(1000))); - EXPECT_FALSE(AddForcedURL(GURL("http://nonforced/0"), - base::Time::FromJsTime(6000))); - - // Check relevant URLs. - querier.QueryAllTopSites(top_sites(), false, true); - ASSERT_EQ(8u + GetPrepopulatedPages().size(), querier.urls().size()); - EXPECT_EQ("http://forced/1", querier.urls()[0].url.spec()); - EXPECT_EQ(1000u, querier.urls()[0].last_forced_time.ToJsTime()); - EXPECT_EQ("http://forced/3", querier.urls()[3].url.spec()); - EXPECT_EQ(4000u, querier.urls()[3].last_forced_time.ToJsTime()); - EXPECT_EQ("http://nonforced/0", querier.urls()[5].url.spec()); - EXPECT_TRUE(querier.urls()[5].last_forced_time.is_null()); - - // Add a timestamp collision and make sure things don't break. - EXPECT_TRUE(AddForcedURL(GURL("http://forced/5"), - base::Time::FromJsTime(4000))); - querier.QueryAllTopSites(top_sites(), false, true); - ASSERT_EQ(9u + GetPrepopulatedPages().size(), querier.urls().size()); - EXPECT_EQ(4000u, querier.urls()[3].last_forced_time.ToJsTime()); - EXPECT_EQ(4000u, querier.urls()[4].last_forced_time.ToJsTime()); - // We don't care which order they get sorted in. - if (querier.urls()[3].url.spec() == "http://forced/3") { - EXPECT_EQ("http://forced/3", querier.urls()[3].url.spec()); - EXPECT_EQ("http://forced/5", querier.urls()[4].url.spec()); - } else { - EXPECT_EQ("http://forced/5", querier.urls()[3].url.spec()); - EXPECT_EQ("http://forced/3", querier.urls()[4].url.spec()); - } - - // Make sure the thumbnail is not lost when the timestamp is updated. - gfx::Image red_thumbnail(CreateBitmap(SK_ColorRED)); - ASSERT_TRUE(top_sites()->SetPageThumbnail( - GURL("http://forced/5"), red_thumbnail, ThumbnailScore())); - - // Get the original thumbnail for later comparison. Some compression can - // happen in |top_sites| and we don't want to depend on that. - scoped_refptr<base::RefCountedMemory> orig_thumbnail_data; - ASSERT_TRUE(top_sites()->GetPageThumbnail(GURL("http://forced/5"), false, - &orig_thumbnail_data)); - - EXPECT_TRUE(AddForcedURL(GURL("http://forced/5"), - base::Time::FromJsTime(6000))); - - // Ensure the thumbnail is still there even if the timestamp changed. - querier.QueryAllTopSites(top_sites(), false, true); - EXPECT_EQ("http://forced/5", querier.urls()[5].url.spec()); - EXPECT_EQ(6000u, querier.urls()[5].last_forced_time.ToJsTime()); - scoped_refptr<base::RefCountedMemory> thumbnail_data; - ASSERT_TRUE(top_sites()->GetPageThumbnail(GURL("http://forced/5"), false, - &thumbnail_data)); - ASSERT_TRUE( - ThumbnailsAreEqual(orig_thumbnail_data.get(), thumbnail_data.get())); -} - } // namespace history diff --git a/chromium/components/history/core/browser/url_database.cc b/chromium/components/history/core/browser/url_database.cc index 9a34fcc3608..286fc9fc15b 100644 --- a/chromium/components/history/core/browser/url_database.cc +++ b/chromium/components/history/core/browser/url_database.cc @@ -9,7 +9,7 @@ #include <vector> #include "base/i18n/case_conversion.h" -#include "base/macros.h" +#include "base/stl_util.h" #include "base/strings/utf_string_conversions.h" #include "components/history/core/browser/keyword_search_term.h" #include "components/url_formatter/url_formatter.h" @@ -322,7 +322,7 @@ bool URLDatabase::IsTypedHost(const std::string& host, std::string* scheme) { url::kFtpScheme }; URLRows dummy; - for (size_t i = 0; i < arraysize(schemes); ++i) { + for (size_t i = 0; i < base::size(schemes); ++i) { std::string scheme_and_host(schemes[i]); scheme_and_host += url::kStandardSchemeSeparator + host; if (AutocompleteForPrefix(scheme_and_host + '/', 1, true, &dummy) || diff --git a/chromium/components/history/core/browser/url_utils_unittest.cc b/chromium/components/history/core/browser/url_utils_unittest.cc index 1b8a3e87a87..ad19bd8e854 100644 --- a/chromium/components/history/core/browser/url_utils_unittest.cc +++ b/chromium/components/history/core/browser/url_utils_unittest.cc @@ -6,7 +6,7 @@ #include <stddef.h> -#include "base/macros.h" +#include "base/stl_util.h" #include "testing/gtest/include/gtest/gtest.h" #include "url/gurl.h" @@ -36,11 +36,11 @@ TEST(HistoryUrlUtilsTest, CanonicalURLStringCompare) { "http://www.google.com:80/", "https://www.google.com", }; - for (size_t i = 0; i < arraysize(sorted_list); ++i) { + for (size_t i = 0; i < base::size(sorted_list); ++i) { EXPECT_FALSE(CanonicalURLStringCompare(sorted_list[i], sorted_list[i])) << " for \"" << sorted_list[i] << "\" < \"" << sorted_list[i] << "\""; // Every disjoint pair-wise comparison. - for (size_t j = i + 1; j < arraysize(sorted_list); ++j) { + for (size_t j = i + 1; j < base::size(sorted_list); ++j) { EXPECT_TRUE(CanonicalURLStringCompare(sorted_list[i], sorted_list[j])) << " for \"" << sorted_list[i] << "\" < \"" << sorted_list[j] << "\""; EXPECT_FALSE(CanonicalURLStringCompare(sorted_list[j], sorted_list[i])) @@ -64,7 +64,7 @@ TEST(HistoryUrlUtilsTest, HaveSameSchemeHostAndPort) { {"http://www.google.com/test", "http://www.google.com/test/with/dir/"}, {"http://www.google.com/test?", "http://www.google.com/test/with/dir/"}, }; - for (size_t i = 0; i < arraysize(true_cases); ++i) { + for (size_t i = 0; i < base::size(true_cases); ++i) { EXPECT_TRUE(HaveSameSchemeHostAndPort(GURL(true_cases[i].s1), GURL(true_cases[i].s2))) << " for true_cases[" << i << "]"; @@ -79,7 +79,7 @@ TEST(HistoryUrlUtilsTest, HaveSameSchemeHostAndPort) { {"http://www.google.com/path", "http://www.google.com:137/path"}, {"http://www.google.com/same/dir", "http://www.youtube.com/same/dir"}, }; - for (size_t i = 0; i < arraysize(false_cases); ++i) { + for (size_t i = 0; i < base::size(false_cases); ++i) { EXPECT_FALSE(HaveSameSchemeHostAndPort(GURL(false_cases[i].s1), GURL(false_cases[i].s2))) << " for false_cases[" << i << "]"; @@ -99,7 +99,7 @@ TEST(HistoryUrlUtilsTest, IsPathPrefix) { {"/test", "/test/with/dir/"}, {"/test/", "/test/with/dir"}, }; - for (size_t i = 0; i < arraysize(true_cases); ++i) { + for (size_t i = 0; i < base::size(true_cases); ++i) { EXPECT_TRUE(IsPathPrefix(true_cases[i].p1, true_cases[i].p2)) << " for true_cases[" << i << "]"; } @@ -114,7 +114,7 @@ TEST(HistoryUrlUtilsTest, IsPathPrefix) { {"/test", "/test-bed"}, {"/test-", "/test"}, }; - for (size_t i = 0; i < arraysize(false_cases); ++i) { + for (size_t i = 0; i < base::size(false_cases); ++i) { EXPECT_FALSE(IsPathPrefix(false_cases[i].p1, false_cases[i].p2)) << " for false_cases[" << i << "]"; } diff --git a/chromium/components/history/core/browser/visit_tracker_unittest.cc b/chromium/components/history/core/browser/visit_tracker_unittest.cc index 27dbb684db3..3c69e7cfeb9 100644 --- a/chromium/components/history/core/browser/visit_tracker_unittest.cc +++ b/chromium/components/history/core/browser/visit_tracker_unittest.cc @@ -4,7 +4,7 @@ #include "components/history/core/browser/visit_tracker.h" -#include "base/macros.h" +#include "base/stl_util.h" #include "testing/gtest/include/gtest/gtest.h" namespace history { @@ -58,7 +58,7 @@ TEST(VisitTracker, SimpleTransitions) { }; VisitTracker tracker; - RunTest(&tracker, test_simple, arraysize(test_simple)); + RunTest(&tracker, test_simple, base::size(test_simple)); } // Test that referrer is properly computed when there are different frame @@ -80,7 +80,7 @@ TEST(VisitTracker, Frames) { }; VisitTracker tracker; - RunTest(&tracker, test_frames, arraysize(test_frames)); + RunTest(&tracker, test_frames, base::size(test_frames)); } // Test frame navigation to make sure that the referrer is properly computed @@ -102,7 +102,7 @@ TEST(VisitTracker, MultiProcess) { }; VisitTracker tracker; - RunTest(&tracker, test_processes, arraysize(test_processes)); + RunTest(&tracker, test_processes, base::size(test_processes)); } // Test that processes get removed properly. @@ -114,7 +114,7 @@ TEST(VisitTracker, ProcessRemove) { }; VisitTracker tracker; - RunTest(&tracker, part1, arraysize(part1)); + RunTest(&tracker, part1, base::size(part1)); // Say that context has been invalidated. tracker.ClearCachedDataForContextID(reinterpret_cast<ContextID>(1)); @@ -124,7 +124,7 @@ TEST(VisitTracker, ProcessRemove) { VisitToTest part2[] = { {1, 1, "http://images.google.com/", 2, "http://www.google.com/", 0}, }; - RunTest(&tracker, part2, arraysize(part2)); + RunTest(&tracker, part2, base::size(part2)); } } // namespace history diff --git a/chromium/components/history/core/common/BUILD.gn b/chromium/components/history/core/common/BUILD.gn index b2f332aafe3..d2f7a71d403 100644 --- a/chromium/components/history/core/common/BUILD.gn +++ b/chromium/components/history/core/common/BUILD.gn @@ -4,6 +4,8 @@ static_library("common") { sources = [ + "pref_names.cc", + "pref_names.h", "thumbnail_score.cc", "thumbnail_score.h", ] diff --git a/chromium/components/history/core/common/pref_names.cc b/chromium/components/history/core/common/pref_names.cc new file mode 100644 index 00000000000..4e57e7762ef --- /dev/null +++ b/chromium/components/history/core/common/pref_names.cc @@ -0,0 +1,12 @@ +// Copyright 2018 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/common/pref_names.h" + +namespace prefs { + +// Boolean controlling whether history saving is disabled. +const char kSavingBrowserHistoryDisabled[] = "history.saving_disabled"; + +} // namespace prefs diff --git a/chromium/components/history/core/common/pref_names.h b/chromium/components/history/core/common/pref_names.h new file mode 100644 index 00000000000..0c7f7e9a810 --- /dev/null +++ b/chromium/components/history/core/common/pref_names.h @@ -0,0 +1,18 @@ +// Copyright 2018 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. + +// Constants for the names of various preferences, for easier changing. + +#ifndef COMPONENTS_HISTORY_CORE_COMMON_PREF_NAMES_H_ +#define COMPONENTS_HISTORY_CORE_COMMON_PREF_NAMES_H_ + +#include <stddef.h> + +namespace prefs { + +extern const char kSavingBrowserHistoryDisabled[]; + +} // namespace prefs + +#endif // COMPONENTS_HISTORY_CORE_COMMON_PREF_NAMES_H_ diff --git a/chromium/components/history/ios/browser/web_state_top_sites_observer.h b/chromium/components/history/ios/browser/web_state_top_sites_observer.h index 720af3cdf7b..87053133e17 100644 --- a/chromium/components/history/ios/browser/web_state_top_sites_observer.h +++ b/chromium/components/history/ios/browser/web_state_top_sites_observer.h @@ -29,9 +29,8 @@ class WebStateTopSitesObserver WebStateTopSitesObserver(web::WebState* web_state, TopSites* top_sites); // web::WebStateObserver implementation. - void NavigationItemCommitted( - web::WebState* web_state, - const web::LoadCommittedDetails& load_details) override; + void DidFinishNavigation(web::WebState* web_state, + web::NavigationContext* navigation_context) override; void WebStateDestroyed(web::WebState* web_state) override; // Underlying TopSites instance, may be null during testing. diff --git a/chromium/components/history/ios/browser/web_state_top_sites_observer.mm b/chromium/components/history/ios/browser/web_state_top_sites_observer.mm index 7db3652948c..966857fc5d9 100644 --- a/chromium/components/history/ios/browser/web_state_top_sites_observer.mm +++ b/chromium/components/history/ios/browser/web_state_top_sites_observer.mm @@ -7,15 +7,14 @@ #include "base/logging.h" #include "base/memory/ptr_util.h" #include "components/history/core/browser/top_sites.h" -#include "ios/web/public/load_committed_details.h" #include "ios/web/public/navigation_item.h" +#import "ios/web/public/navigation_manager.h" +#import "ios/web/public/web_state/navigation_context.h" #if !defined(__has_feature) || !__has_feature(objc_arc) #error "This file requires ARC support." #endif -DEFINE_WEB_STATE_USER_DATA_KEY(history::WebStateTopSitesObserver); - namespace history { // static @@ -38,12 +37,13 @@ WebStateTopSitesObserver::WebStateTopSitesObserver(web::WebState* web_state, WebStateTopSitesObserver::~WebStateTopSitesObserver() { } -void WebStateTopSitesObserver::NavigationItemCommitted( +void WebStateTopSitesObserver::DidFinishNavigation( web::WebState* web_state, - const web::LoadCommittedDetails& load_details) { - DCHECK(load_details.item); - if (top_sites_) - top_sites_->OnNavigationCommitted(load_details.item->GetURL()); + web::NavigationContext* navigation_context) { + if (top_sites_ && navigation_context->HasCommitted()) { + top_sites_->OnNavigationCommitted( + web_state->GetNavigationManager()->GetLastCommittedItem()->GetURL()); + } } void WebStateTopSitesObserver::WebStateDestroyed(web::WebState* web_state) { |