diff options
Diffstat (limited to 'chromium/components/history')
32 files changed, 2147 insertions, 539 deletions
diff --git a/chromium/components/history/core/browser/BUILD.gn b/chromium/components/history/core/browser/BUILD.gn index e60183b62e9..a8cd984f57e 100644 --- a/chromium/components/history/core/browser/BUILD.gn +++ b/chromium/components/history/core/browser/BUILD.gn @@ -33,8 +33,6 @@ static_library("browser") { "history_db_task.h", "history_delete_directives_data_type_controller.cc", "history_delete_directives_data_type_controller.h", - "history_match.cc", - "history_match.h", "history_model_worker.cc", "history_model_worker.h", "history_service.cc", @@ -200,6 +198,7 @@ source_set("unit_tests") { "top_sites_cache_unittest.cc", "top_sites_database_unittest.cc", "top_sites_impl_unittest.cc", + "typed_url_sync_bridge_unittest.cc", "typed_url_sync_metadata_database_unittest.cc", "typed_url_syncable_service_unittest.cc", "url_database_unittest.cc", 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 dd7a094ea60..b630bae4ea3 100644 --- a/chromium/components/history/core/browser/android/favicon_sql_handler.cc +++ b/chromium/components/history/core/browser/android/favicon_sql_handler.cc @@ -39,7 +39,8 @@ bool FaviconSQLHandler::Update(const HistoryAndBookmarkRow& row, // icon is already in database, just create a new favicon. // TODO(pkotwicz): Pass in real pixel size. favicon_base::FaviconID favicon_id = thumbnail_db_->AddFavicon( - GURL(), favicon_base::FAVICON, row.favicon(), Time::Now(), gfx::Size()); + GURL(), favicon_base::FAVICON, row.favicon(), FaviconBitmapType::ON_VISIT, + Time::Now(), gfx::Size()); if (!favicon_id) return false; @@ -104,7 +105,8 @@ bool FaviconSQLHandler::Insert(HistoryAndBookmarkRow* row) { // Is it a problem to give a empty URL? // TODO(pkotwicz): Pass in real pixel size. favicon_base::FaviconID id = thumbnail_db_->AddFavicon( - GURL(), favicon_base::FAVICON, row->favicon(), Time::Now(), gfx::Size()); + GURL(), favicon_base::FAVICON, row->favicon(), + FaviconBitmapType::ON_VISIT, Time::Now(), gfx::Size()); if (!id) return false; return thumbnail_db_->AddIconMapping(row->url(), id); 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 11cd46f7578..0a554fa8895 100644 --- a/chromium/components/history/core/browser/expire_history_backend_unittest.cc +++ b/chromium/components/history/core/browser/expire_history_backend_unittest.cc @@ -16,11 +16,11 @@ #include "base/files/file_util.h" #include "base/files/scoped_temp_dir.h" #include "base/macros.h" -#include "base/message_loop/message_loop.h" #include "base/run_loop.h" #include "base/scoped_observer.h" #include "base/strings/string16.h" #include "base/strings/utf_string_conversions.h" +#include "base/test/scoped_task_environment.h" #include "components/history/core/browser/history_backend_client.h" #include "components/history/core/browser/history_backend_notifier.h" #include "components/history/core/browser/history_constants.h" @@ -55,7 +55,9 @@ class ExpireHistoryTest : public testing::Test, public HistoryBackendNotifier { public: ExpireHistoryTest() : backend_client_(history_client_.CreateBackendClient()), - expirer_(this, backend_client_.get(), message_loop_.task_runner()), + expirer_(this, + backend_client_.get(), + scoped_task_environment_.GetMainThreadTaskRunner()), now_(base::Time::Now()) {} protected: @@ -96,11 +98,11 @@ class ExpireHistoryTest : public testing::Test, public HistoryBackendNotifier { // This must be destroyed last. base::ScopedTempDir tmp_dir_; + base::test::ScopedTaskEnvironment scoped_task_environment_; + HistoryClientFakeBookmarks history_client_; std::unique_ptr<HistoryBackendClient> backend_client_; - base::MessageLoopForUI message_loop_; - ExpireHistoryBackend expirer_; std::unique_ptr<TestingPrefServiceSimple> pref_service_; @@ -139,8 +141,7 @@ class ExpireHistoryTest : public testing::Test, public HistoryBackendNotifier { PrepopulatedPageList(), base::Bind(MockCanAddURLToHistory)); WaitTopSitesLoadedObserver wait_top_sites_observer(top_sites_); - top_sites_->Init(path().Append(kTopSitesFilename), - message_loop_.task_runner()); + top_sites_->Init(path().Append(kTopSitesFilename)); wait_top_sites_observer.Run(); } diff --git a/chromium/components/history/core/browser/history_backend.cc b/chromium/components/history/core/browser/history_backend.cc index 14855781aac..bd7d01dc097 100644 --- a/chromium/components/history/core/browser/history_backend.cc +++ b/chromium/components/history/core/browser/history_backend.cc @@ -41,7 +41,6 @@ #include "components/history/core/browser/in_memory_history_backend.h" #include "components/history/core/browser/keyword_search_term.h" #include "components/history/core/browser/page_usage_data.h" -#include "components/history/core/browser/typed_url_sync_bridge.h" #include "components/history/core/browser/typed_url_syncable_service.h" #include "components/history/core/browser/url_utils.h" #include "components/sync/driver/sync_driver_switches.h" @@ -224,6 +223,7 @@ void HistoryBackend::Init( &ModelTypeChangeProcessor::Create, // TODO(gangwu): use ReportUnrecoverableError before launch. base::BindRepeating(base::IgnoreResult(&DumpWithoutCrashing)))); + typed_url_sync_bridge_->Init(); } else { typed_url_syncable_service_ = base::MakeUnique<TypedUrlSyncableService>(this); @@ -1236,6 +1236,7 @@ void HistoryBackend::QueryHistoryBasic(const QueryOptions& options, DCHECK_LE(static_cast<int>(visits.size()), options.EffectiveMaxCount()); // Now add them and the URL rows to the results. + std::vector<URLResult> matching_results; URLResult url_result; for (size_t i = 0; i < visits.size(); i++) { const VisitRow visit = visits[i]; @@ -1261,8 +1262,9 @@ void HistoryBackend::QueryHistoryBasic(const QueryOptions& options, // We don't set any of the query-specific parts of the URLResult, since // snippets and stuff don't apply to basic querying. - result->AppendURLBySwapping(&url_result); + matching_results.push_back(std::move(url_result)); } + result->SetURLResults(std::move(matching_results)); if (!has_more_results && options.begin_time <= first_recorded_time_) result->set_reached_beginning(true); @@ -1295,13 +1297,14 @@ void HistoryBackend::QueryHistoryText(const base::string16& text_query, size_t max_results = options.max_count == 0 ? std::numeric_limits<size_t>::max() : static_cast<int>(options.max_count); - for (std::vector<URLResult>::iterator it = matching_visits.begin(); - it != matching_visits.end() && result->size() < max_results; ++it) { - result->AppendURLBySwapping(&(*it)); + bool has_more_results = false; + if (matching_visits.size() > max_results) { + has_more_results = true; + matching_visits.resize(max_results); } + result->SetURLResults(std::move(matching_visits)); - if (matching_visits.size() == result->size() && - options.begin_time <= first_recorded_time_) + if(!has_more_results && options.begin_time <= first_recorded_time_) result->set_reached_beginning(true); } @@ -1636,8 +1639,9 @@ void HistoryBackend::MergeFavicon( thumbnail_db_->DeleteFaviconBitmap(bitmap_id_sizes[0].bitmap_id); favicon_sizes.erase(favicon_sizes.begin()); } - thumbnail_db_->AddFaviconBitmap(favicon_id, bitmap_data, base::Time::Now(), - pixel_size); + thumbnail_db_->AddFaviconBitmap(favicon_id, bitmap_data, + FaviconBitmapType::ON_VISIT, + base::Time::Now(), pixel_size); favicon_sizes.push_back(pixel_size); } @@ -1698,9 +1702,10 @@ void HistoryBackend::MergeFavicon( // Add the favicon bitmap as expired as it is not consistent with the // merged in data. - thumbnail_db_->AddFaviconBitmap( - favicon_id, bitmaps_to_copy[j].bitmap_data, base::Time(), - bitmaps_to_copy[j].pixel_size); + thumbnail_db_->AddFaviconBitmap(favicon_id, + bitmaps_to_copy[j].bitmap_data, + FaviconBitmapType::ON_VISIT, base::Time(), + bitmaps_to_copy[j].pixel_size); favicon_sizes.push_back(bitmaps_to_copy[j].pixel_size); favicon_bitmaps_copied = true; @@ -1733,14 +1738,13 @@ void HistoryBackend::SetFavicons(const GURL& page_url, const GURL& icon_url, const std::vector<SkBitmap>& bitmaps) { SetFaviconsImpl(page_url, icon_type, icon_url, bitmaps, - /*bitmaps_are_expired=*/false); + FaviconBitmapType::ON_VISIT); } -bool HistoryBackend::SetLastResortFavicons( - const GURL& page_url, - favicon_base::IconType icon_type, - const GURL& icon_url, - const std::vector<SkBitmap>& bitmaps) { +bool HistoryBackend::SetOnDemandFavicons(const GURL& page_url, + favicon_base::IconType icon_type, + const GURL& icon_url, + const std::vector<SkBitmap>& bitmaps) { if (!thumbnail_db_ || !db_) return false; @@ -1751,7 +1755,7 @@ bool HistoryBackend::SetLastResortFavicons( } return SetFaviconsImpl(page_url, icon_type, icon_url, bitmaps, - /*bitmaps_are_expired=*/true); + FaviconBitmapType::ON_DEMAND); } void HistoryBackend::SetFaviconsOutOfDateForPage(const GURL& page_url) { @@ -1768,6 +1772,14 @@ void HistoryBackend::SetFaviconsOutOfDateForPage(const GURL& page_url) { ScheduleCommit(); } +void HistoryBackend::TouchOnDemandFavicon(const GURL& icon_url) { + if (!thumbnail_db_) + return; + + thumbnail_db_->TouchOnDemandFavicon(icon_url, Time::Now()); + ScheduleCommit(); +} + void HistoryBackend::SetImportedFavicons( const favicon_base::FaviconUsageDataList& favicon_usage) { if (!db_ || !thumbnail_db_) @@ -1787,8 +1799,8 @@ void HistoryBackend::SetImportedFavicons( // TODO(pkotwicz): Pass in real pixel size. favicon_id = thumbnail_db_->AddFavicon( favicon_usage[i].favicon_url, favicon_base::FAVICON, - new base::RefCountedBytes(favicon_usage[i].png_data), now, - gfx::Size()); + new base::RefCountedBytes(favicon_usage[i].png_data), + FaviconBitmapType::ON_VISIT, now, gfx::Size()); } // Save the mapping from all the URLs to the favicon. @@ -1833,7 +1845,7 @@ bool HistoryBackend::SetFaviconsImpl(const GURL& page_url, favicon_base::IconType icon_type, const GURL& icon_url, const std::vector<SkBitmap>& bitmaps, - bool bitmaps_are_expired) { + FaviconBitmapType type) { if (!thumbnail_db_ || !db_) return false; @@ -1849,11 +1861,9 @@ bool HistoryBackend::SetFaviconsImpl(const GURL& page_url, } bool favicon_data_modified = false; - if (favicon_created || !bitmaps_are_expired) - favicon_data_modified = SetFaviconBitmaps(icon_id, bitmaps); - - if (favicon_created && bitmaps_are_expired) - thumbnail_db_->SetFaviconOutOfDate(icon_id); + if (favicon_created || type == FaviconBitmapType::ON_VISIT) { + favicon_data_modified = SetFaviconBitmaps(icon_id, bitmaps, type); + } std::vector<favicon_base::FaviconID> icon_ids(1u, icon_id); bool mapping_changed = @@ -1907,7 +1917,8 @@ void HistoryBackend::UpdateFaviconMappingsAndFetchImpl( } bool HistoryBackend::SetFaviconBitmaps(favicon_base::FaviconID icon_id, - const std::vector<SkBitmap>& bitmaps) { + const std::vector<SkBitmap>& bitmaps, + FaviconBitmapType type) { std::vector<FaviconBitmapIDSize> bitmap_id_sizes; thumbnail_db_->GetFaviconBitmapIDSizes(icon_id, &bitmap_id_sizes); @@ -1944,11 +1955,12 @@ bool HistoryBackend::SetFaviconBitmaps(favicon_base::FaviconID icon_id, } else { if (!favicon_bitmaps_changed && IsFaviconBitmapDataEqual(bitmap_id, match_it->first)) { - thumbnail_db_->SetFaviconBitmapLastUpdateTime(bitmap_id, - base::Time::Now()); + thumbnail_db_->SetFaviconBitmapLastUpdateTime( + bitmap_id, base::Time::Now() /* new last updated time */); } else { - thumbnail_db_->SetFaviconBitmap(bitmap_id, match_it->first, - base::Time::Now()); + thumbnail_db_->SetFaviconBitmap( + bitmap_id, match_it->first, + base::Time::Now() /* new last updated time */); favicon_bitmaps_changed = true; } to_add.erase(match_it); @@ -1956,8 +1968,10 @@ bool HistoryBackend::SetFaviconBitmaps(favicon_base::FaviconID icon_id, } for (size_t i = 0; i < to_add.size(); ++i) { - thumbnail_db_->AddFaviconBitmap(icon_id, to_add[i].first, base::Time::Now(), - to_add[i].second); + thumbnail_db_->AddFaviconBitmap( + icon_id, to_add[i].first, type, + base::Time::Now() /* new last updated / last requested time */, + to_add[i].second); favicon_bitmaps_changed = true; } diff --git a/chromium/components/history/core/browser/history_backend.h b/chromium/components/history/core/browser/history_backend.h index 59c58389c12..85c5fb83f05 100644 --- a/chromium/components/history/core/browser/history_backend.h +++ b/chromium/components/history/core/browser/history_backend.h @@ -32,6 +32,7 @@ #include "components/history/core/browser/history_types.h" #include "components/history/core/browser/keyword_id.h" #include "components/history/core/browser/thumbnail_database.h" +#include "components/history/core/browser/typed_url_sync_bridge.h" #include "components/history/core/browser/visit_tracker.h" #include "sql/init_status.h" @@ -53,7 +54,6 @@ struct HistoryDatabaseParams; class HistoryDBTask; class InMemoryHistoryBackend; class TypedUrlSyncableService; -class TypedURLSyncBridge; class HistoryBackendHelper; class URLDatabase; @@ -326,13 +326,15 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>, const GURL& icon_url, const std::vector<SkBitmap>& bitmaps); - bool SetLastResortFavicons(const GURL& page_url, - favicon_base::IconType icon_type, - const GURL& icon_url, - const std::vector<SkBitmap>& bitmaps); + bool SetOnDemandFavicons(const GURL& page_url, + favicon_base::IconType icon_type, + const GURL& icon_url, + const std::vector<SkBitmap>& bitmaps); void SetFaviconsOutOfDateForPage(const GURL& page_url); + void TouchOnDemandFavicon(const GURL& icon_url); + void SetImportedFavicons( const favicon_base::FaviconUsageDataList& favicon_usage); @@ -476,6 +478,11 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>, HistoryDatabase* db() const { return db_.get(); } ExpireHistoryBackend* expire_backend() { return &expirer_; } + + void SetTypedURLSyncBridgeForTest( + std::unique_ptr<TypedURLSyncBridge> bridge) { + typed_url_sync_bridge_ = std::move(bridge); + } #endif // Returns true if the passed visit time is already expired (used by the sync @@ -523,11 +530,9 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>, FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, SetFaviconsReplaceBitmapData); FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, SetFaviconsSameFaviconURLForTwoPages); - FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, SetLastResortFaviconsForEmptyDB); - FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, - SetLastResortFaviconsForPageInDB); - FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, - SetLastResortFaviconsForIconInDB); + FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, SetOnDemandFaviconsForEmptyDB); + FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, SetOnDemandFaviconsForPageInDB); + FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, SetOnDemandFaviconsForIconInDB); FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, UpdateFaviconMappingsAndFetchNoChange); FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, MergeFaviconPageURLNotInDB); @@ -681,7 +686,7 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>, favicon_base::IconType icon_type, const GURL& icon_url, const std::vector<SkBitmap>& bitmaps, - bool bitmaps_are_expired); + FaviconBitmapType type); // Used by both UpdateFaviconMappingsAndFetch() and GetFavicon(). // If |page_url| is non-null and there is a favicon stored in the database @@ -694,16 +699,19 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>, const std::vector<int>& desired_sizes, std::vector<favicon_base::FaviconRawBitmapResult>* results); - // Set the favicon bitmaps for |icon_id|. + // Set the favicon bitmaps of |type| for |icon_id|. // For each entry in |bitmaps|, if a favicon bitmap already exists at the // entry's pixel size, replace the favicon bitmap's data with the entry's // bitmap data. Otherwise add a new favicon bitmap. // Any favicon bitmaps already mapped to |icon_id| whose pixel size does not // match the pixel size of one of |bitmaps| is deleted. + // For bitmap type FaviconBitmapType::ON_DEMAND, this is legal to call only + // for a newly created |icon_id| (that has no bitmaps yet). // Returns true if any of the bitmap data at |icon_id| is changed as a result // of calling this method. bool SetFaviconBitmaps(favicon_base::FaviconID icon_id, - const std::vector<SkBitmap>& bitmaps); + const std::vector<SkBitmap>& bitmaps, + FaviconBitmapType type); // Returns true if the bitmap data at |bitmap_id| equals |new_bitmap_data|. bool IsFaviconBitmapDataEqual( diff --git a/chromium/components/history/core/browser/history_backend_unittest.cc b/chromium/components/history/core/browser/history_backend_unittest.cc index 63dfe4b7309..c92e97e4d24 100644 --- a/chromium/components/history/core/browser/history_backend_unittest.cc +++ b/chromium/components/history/core/browser/history_backend_unittest.cc @@ -593,18 +593,22 @@ TEST_F(HistoryBackendTest, DeleteAll) { std::vector<unsigned char> data; data.push_back('a'); - EXPECT_TRUE(backend_->thumbnail_db_->AddFaviconBitmap(favicon1, - new base::RefCountedBytes(data), base::Time::Now(), kSmallSize)); + EXPECT_TRUE(backend_->thumbnail_db_->AddFaviconBitmap( + favicon1, new base::RefCountedBytes(data), FaviconBitmapType::ON_VISIT, + base::Time::Now(), kSmallSize)); data[0] = 'b'; - EXPECT_TRUE(backend_->thumbnail_db_->AddFaviconBitmap(favicon1, - new base::RefCountedBytes(data), base::Time::Now(), kLargeSize)); + EXPECT_TRUE(backend_->thumbnail_db_->AddFaviconBitmap( + favicon1, new base::RefCountedBytes(data), FaviconBitmapType::ON_VISIT, + base::Time::Now(), kLargeSize)); data[0] = 'c'; - EXPECT_TRUE(backend_->thumbnail_db_->AddFaviconBitmap(favicon2, - new base::RefCountedBytes(data), base::Time::Now(), kSmallSize)); + EXPECT_TRUE(backend_->thumbnail_db_->AddFaviconBitmap( + favicon2, new base::RefCountedBytes(data), FaviconBitmapType::ON_VISIT, + base::Time::Now(), kSmallSize)); data[0] = 'd'; - EXPECT_TRUE(backend_->thumbnail_db_->AddFaviconBitmap(favicon2, - new base::RefCountedBytes(data), base::Time::Now(), kLargeSize)); + EXPECT_TRUE(backend_->thumbnail_db_->AddFaviconBitmap( + favicon2, new base::RefCountedBytes(data), FaviconBitmapType::ON_VISIT, + base::Time::Now(), kLargeSize)); // First visit two URLs. URLRow row1(GURL("http://www.google.com/")); @@ -737,7 +741,7 @@ TEST_F(HistoryBackendTest, DeleteAllURLPreviouslyDeleted) { data.push_back('a'); favicon_base::FaviconID favicon = backend_->thumbnail_db_->AddFavicon( kFaviconURL, favicon_base::FAVICON, new base::RefCountedBytes(data), - base::Time::Now(), kSmallSize); + FaviconBitmapType::ON_VISIT, base::Time::Now(), kSmallSize); backend_->thumbnail_db_->AddIconMapping(row.url(), favicon); history_client_.AddBookmark(kPageURL); @@ -821,20 +825,14 @@ TEST_F(HistoryBackendTest, URLsNoLongerBookmarked) { std::vector<unsigned char> data; data.push_back('1'); - favicon_base::FaviconID favicon1 = - backend_->thumbnail_db_->AddFavicon(favicon_url1, - favicon_base::FAVICON, - new base::RefCountedBytes(data), - base::Time::Now(), - gfx::Size()); + favicon_base::FaviconID favicon1 = backend_->thumbnail_db_->AddFavicon( + favicon_url1, favicon_base::FAVICON, new base::RefCountedBytes(data), + FaviconBitmapType::ON_VISIT, base::Time::Now(), gfx::Size()); data[0] = '2'; - favicon_base::FaviconID favicon2 = - backend_->thumbnail_db_->AddFavicon(favicon_url2, - favicon_base::FAVICON, - new base::RefCountedBytes(data), - base::Time::Now(), - gfx::Size()); + favicon_base::FaviconID favicon2 = backend_->thumbnail_db_->AddFavicon( + favicon_url2, favicon_base::FAVICON, new base::RefCountedBytes(data), + FaviconBitmapType::ON_VISIT, base::Time::Now(), gfx::Size()); // First visit two URLs. URLRow row1(GURL("http://www.google.com/")); @@ -1171,11 +1169,9 @@ TEST_F(HistoryBackendTest, ImportedFaviconsTest) { std::vector<unsigned char> data; data.push_back('1'); favicon_base::FaviconID favicon1 = backend_->thumbnail_db_->AddFavicon( - favicon_url1, - favicon_base::FAVICON, - base::RefCountedBytes::TakeVector(&data), - base::Time::Now(), - gfx::Size()); + favicon_url1, favicon_base::FAVICON, + base::RefCountedBytes::TakeVector(&data), FaviconBitmapType::ON_VISIT, + base::Time::Now(), gfx::Size()); URLRow row1(GURL("http://www.google.com/")); row1.set_visit_count(1); row1.set_last_visit(base::Time::Now()); @@ -2076,18 +2072,18 @@ TEST_F(HistoryBackendTest, SetFaviconsSameFaviconURLForTwoPages) { EXPECT_EQ(2u, favicon_bitmaps.size()); } -// Tests calling SetLastResortFavicons(). Neither |page_url| nor |icon_url| are +// Tests calling SetOnDemandFavicons(). Neither |page_url| nor |icon_url| are // known to the database. -TEST_F(HistoryBackendTest, SetLastResortFaviconsForEmptyDB) { +TEST_F(HistoryBackendTest, SetOnDemandFaviconsForEmptyDB) { GURL page_url("http://www.google.com"); GURL icon_url("http:/www.google.com/favicon.ico"); std::vector<SkBitmap> bitmaps; bitmaps.push_back(CreateBitmap(SK_ColorRED, kSmallEdgeSize)); - // Call SetLastResortFavicons() with a different icon URL and bitmap data. - EXPECT_TRUE(backend_->SetLastResortFavicons(page_url, favicon_base::FAVICON, - icon_url, bitmaps)); + // Call SetOnDemandFavicons() with a different icon URL and bitmap data. + EXPECT_TRUE(backend_->SetOnDemandFavicons(page_url, favicon_base::FAVICON, + icon_url, bitmaps)); favicon_base::FaviconID favicon_id = backend_->thumbnail_db_->GetFaviconIDForFaviconURL(icon_url, @@ -2102,9 +2098,9 @@ TEST_F(HistoryBackendTest, SetLastResortFaviconsForEmptyDB) { EXPECT_EQ(base::Time(), favicon_bitmap.last_updated); } -// Tests calling SetLastResortFavicons(). |page_url| is known to the database +// Tests calling SetOnDemandFavicons(). |page_url| is known to the database // but |icon_url| is not (the second should be irrelevant though). -TEST_F(HistoryBackendTest, SetLastResortFaviconsForPageInDB) { +TEST_F(HistoryBackendTest, SetOnDemandFaviconsForPageInDB) { GURL page_url("http://www.google.com"); GURL icon_url1("http:/www.google.com/favicon1.ico"); GURL icon_url2("http:/www.google.com/favicon2.ico"); @@ -2118,10 +2114,10 @@ TEST_F(HistoryBackendTest, SetLastResortFaviconsForPageInDB) { favicon_base::FAVICON); ASSERT_NE(0, original_favicon_id); - // Call SetLastResortFavicons() with a different icon URL and bitmap data. + // Call SetOnDemandFavicons() with a different icon URL and bitmap data. bitmaps[0] = CreateBitmap(SK_ColorWHITE, kSmallEdgeSize); - EXPECT_FALSE(backend_->SetLastResortFavicons(page_url, favicon_base::FAVICON, - icon_url2, bitmaps)); + EXPECT_FALSE(backend_->SetOnDemandFavicons(page_url, favicon_base::FAVICON, + icon_url2, bitmaps)); EXPECT_EQ(0, backend_->thumbnail_db_->GetFaviconIDForFaviconURL( icon_url2, favicon_base::FAVICON)); @@ -2133,9 +2129,9 @@ TEST_F(HistoryBackendTest, SetLastResortFaviconsForPageInDB) { EXPECT_NE(base::Time(), favicon_bitmap.last_updated); } -// Tests calling SetLastResortFavicons(). |page_url| is not known to the +// Tests calling SetOnDemandFavicons(). |page_url| is not known to the // database but |icon_url| is. -TEST_F(HistoryBackendTest, SetLastResortFaviconsForIconInDB) { +TEST_F(HistoryBackendTest, SetOnDemandFaviconsForIconInDB) { const GURL old_page_url("http://www.google.com/old"); const GURL page_url("http://www.google.com/"); const GURL icon_url("http://www.google.com/icon"); @@ -2149,10 +2145,10 @@ TEST_F(HistoryBackendTest, SetLastResortFaviconsForIconInDB) { favicon_base::FAVICON); ASSERT_NE(0, original_favicon_id); - // Call SetLastResortFavicons() with a different bitmap. + // Call SetOnDemandFavicons() with a different bitmap. bitmaps[0] = CreateBitmap(SK_ColorWHITE, kSmallEdgeSize); - EXPECT_FALSE(backend_->SetLastResortFavicons(page_url, favicon_base::FAVICON, - icon_url, bitmaps)); + EXPECT_FALSE(backend_->SetOnDemandFavicons(page_url, favicon_base::FAVICON, + icon_url, bitmaps)); EXPECT_EQ(original_favicon_id, backend_->thumbnail_db_->GetFaviconIDForFaviconURL( @@ -3048,7 +3044,8 @@ TEST_F(HistoryBackendTest, GetFaviconsFromDBExpired) { base::RefCountedBytes::TakeVector(&data)); base::Time last_updated = base::Time::FromTimeT(0); favicon_base::FaviconID icon_id = backend_->thumbnail_db_->AddFavicon( - icon_url, favicon_base::FAVICON, bitmap_data, last_updated, kSmallSize); + icon_url, favicon_base::FAVICON, bitmap_data, FaviconBitmapType::ON_VISIT, + last_updated, kSmallSize); EXPECT_NE(0, icon_id); EXPECT_NE(0, backend_->thumbnail_db_->AddIconMapping(page_url, icon_id)); diff --git a/chromium/components/history/core/browser/history_match.cc b/chromium/components/history/core/browser/history_match.cc deleted file mode 100644 index ae9787bc21d..00000000000 --- a/chromium/components/history/core/browser/history_match.cc +++ /dev/null @@ -1,39 +0,0 @@ -// Copyright 2014 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "components/history/core/browser/history_match.h" - -#include "base/logging.h" - -namespace history { - -HistoryMatch::HistoryMatch() - : url_info(), - input_location(base::string16::npos), - match_in_scheme(false), - innermost_match(true) { -} - -HistoryMatch::HistoryMatch(const URLRow& url_info, - size_t input_location, - bool match_in_scheme, - bool innermost_match) - : url_info(url_info), - input_location(input_location), - match_in_scheme(match_in_scheme), - innermost_match(innermost_match) { -} - -bool HistoryMatch::EqualsGURL(const HistoryMatch& h, const GURL& url) { - return h.url_info.url() == url; -} - -bool HistoryMatch::IsHostOnly() const { - const GURL& gurl = url_info.url(); - DCHECK(gurl.is_valid()); - return (!gurl.has_path() || (gurl.path_piece() == "/")) && - !gurl.has_query() && !gurl.has_ref(); -} - -} // namespace history diff --git a/chromium/components/history/core/browser/history_match.h b/chromium/components/history/core/browser/history_match.h deleted file mode 100644 index 971db0316f7..00000000000 --- a/chromium/components/history/core/browser/history_match.h +++ /dev/null @@ -1,59 +0,0 @@ -// Copyright 2014 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef COMPONENTS_HISTORY_CORE_BROWSER_HISTORY_MATCH_H_ -#define COMPONENTS_HISTORY_CORE_BROWSER_HISTORY_MATCH_H_ - -#include <stddef.h> - -#include <deque> - -#include "components/history/core/browser/url_row.h" - -namespace history { - -// Used for intermediate history result operations. -struct HistoryMatch { - // Required for STL, we don't use this directly. - HistoryMatch(); - - HistoryMatch(const URLRow& url_info, - size_t input_location, - bool match_in_scheme, - bool innermost_match); - - static bool EqualsGURL(const HistoryMatch& h, const GURL& url); - - // Returns true if url in this HistoryMatch is just a host - // (e.g. "http://www.google.com/") and not some other subpage - // (e.g. "http://www.google.com/foo.html"). - bool IsHostOnly() const; - - URLRow url_info; - - // The offset of the user's input within the URL. - size_t input_location; - - // Whether this is a match in the scheme. This determines whether we'll go - // ahead and show a scheme on the URL even if the user didn't type one. - // If our best match was in the scheme, not showing the scheme is both - // confusing and, for inline autocomplete of the fill_into_edit, dangerous. - // (If the user types "h" and we match "http://foo/", we need to inline - // autocomplete that, not "foo/", which won't show anything at all, and - // will mislead the user into thinking the What You Typed match is what's - // selected.) - bool match_in_scheme; - - // A match after any scheme/"www.", if the user input could match at both - // locations. If the user types "w", an innermost match ("website.com") is - // better than a non-innermost match ("www.google.com"). If the user types - // "x", no scheme in our prefix list (or "www.") begins with x, so all - // matches are, vacuously, "innermost matches". - bool innermost_match; -}; -typedef std::deque<HistoryMatch> HistoryMatches; - -} // namespace history - -#endif // COMPONENTS_HISTORY_CORE_BROWSER_HISTORY_MATCH_H_ diff --git a/chromium/components/history/core/browser/history_model_worker.cc b/chromium/components/history/core/browser/history_model_worker.cc index ad3f9911e7a..7ae6d603873 100644 --- a/chromium/components/history/core/browser/history_model_worker.cc +++ b/chromium/components/history/core/browser/history_model_worker.cc @@ -63,7 +63,7 @@ syncer::ModelSafeGroup HistoryModelWorker::GetModelSafeGroup() { return syncer::GROUP_HISTORY; } -bool HistoryModelWorker::IsOnModelThread() { +bool HistoryModelWorker::IsOnModelSequence() { // Ideally HistoryService would expose a way to check whether this is the // history DB thread. Since it doesn't, just return true to bypass a CHECK in // the sync code. diff --git a/chromium/components/history/core/browser/history_model_worker.h b/chromium/components/history/core/browser/history_model_worker.h index f72b00afbc5..9a669e4ec70 100644 --- a/chromium/components/history/core/browser/history_model_worker.h +++ b/chromium/components/history/core/browser/history_model_worker.h @@ -30,7 +30,7 @@ class HistoryModelWorker : public syncer::ModelSafeWorker { // syncer::ModelSafeWorker implementation. syncer::ModelSafeGroup GetModelSafeGroup() override; - bool IsOnModelThread() override; + bool IsOnModelSequence() override; private: ~HistoryModelWorker() override; diff --git a/chromium/components/history/core/browser/history_service.cc b/chromium/components/history/core/browser/history_service.cc index 788c90da089..6e8b789018b 100644 --- a/chromium/components/history/core/browser/history_service.cc +++ b/chromium/components/history/core/browser/history_service.cc @@ -78,9 +78,9 @@ void RunWithFaviconResult( callback.Run(*bitmap_result); } -void RunWithQueryURLResult(const HistoryService::QueryURLCallback& callback, +void RunWithQueryURLResult(HistoryService::QueryURLCallback callback, const QueryURLResult* result) { - callback.Run(result->success, result->row, result->visits); + std::move(callback).Run(result->success, result->row, result->visits); } void RunWithVisibleVisitCountToHostResult( @@ -625,12 +625,11 @@ void HistoryService::SetFavicons(const GURL& page_url, page_url, icon_type, icon_url, bitmaps)); } -void HistoryService::SetLastResortFavicons( - const GURL& page_url, - favicon_base::IconType icon_type, - const GURL& icon_url, - const std::vector<SkBitmap>& bitmaps, - base::Callback<void(bool)> callback) { +void HistoryService::SetOnDemandFavicons(const GURL& page_url, + favicon_base::IconType icon_type, + const GURL& icon_url, + const std::vector<SkBitmap>& bitmaps, + base::Callback<void(bool)> callback) { DCHECK(backend_task_runner_) << "History service being called after cleanup"; DCHECK(thread_checker_.CalledOnValidThread()); if (history_client_ && !history_client_->CanAddURL(page_url)) @@ -638,7 +637,7 @@ void HistoryService::SetLastResortFavicons( PostTaskAndReplyWithResult( backend_task_runner_.get(), FROM_HERE, - base::Bind(&HistoryBackend::SetLastResortFavicons, history_backend_, + base::Bind(&HistoryBackend::SetOnDemandFavicons, history_backend_, page_url, icon_type, icon_url, bitmaps), callback); } @@ -651,6 +650,14 @@ void HistoryService::SetFaviconsOutOfDateForPage(const GURL& page_url) { history_backend_, page_url)); } +void HistoryService::TouchOnDemandFavicon(const GURL& icon_url) { + DCHECK(backend_task_runner_) << "History service being called after cleanup"; + DCHECK(thread_checker_.CalledOnValidThread()); + ScheduleTask(PRIORITY_NORMAL, + base::Bind(&HistoryBackend::TouchOnDemandFavicon, + history_backend_, icon_url)); +} + void HistoryService::SetImportedFavicons( const favicon_base::FaviconUsageDataList& favicon_usage) { DCHECK(backend_task_runner_) << "History service being called after cleanup"; @@ -663,7 +670,7 @@ void HistoryService::SetImportedFavicons( base::CancelableTaskTracker::TaskId HistoryService::QueryURL( const GURL& url, bool want_visits, - const QueryURLCallback& callback, + QueryURLCallback callback, base::CancelableTaskTracker* tracker) { DCHECK(backend_task_runner_) << "History service being called after cleanup"; DCHECK(thread_checker_.CalledOnValidThread()); @@ -672,8 +679,8 @@ base::CancelableTaskTracker::TaskId HistoryService::QueryURL( backend_task_runner_.get(), FROM_HERE, base::Bind(&HistoryBackend::QueryURL, history_backend_, url, want_visits, base::Unretained(query_url_result)), - base::Bind(&RunWithQueryURLResult, callback, - base::Owned(query_url_result))); + base::BindOnce(&RunWithQueryURLResult, std::move(callback), + base::Owned(query_url_result))); } // Statistics ------------------------------------------------------------------ @@ -1069,8 +1076,33 @@ void HistoryService::ExpireLocalAndRemoteHistoryBetween( // // TODO(davidben): |callback| should not run until this operation completes // too. + net::PartialNetworkTrafficAnnotationTag partial_traffic_annotation = + net::DefinePartialNetworkTrafficAnnotation( + "web_history_expire_between_dates", "web_history_service", R"( + semantics { + description: + "If a user who syncs their browsing history deletes history " + "items for a time range, Chrome sends a request to a google.com " + "host to execute the corresponding deletion serverside." + trigger: + "Deleting browsing history for a given time range, e.g. from the " + "Clear Browsing Data dialog, by an extension, or the " + "Clear-Site-Data header." + data: + "The begin and end timestamps of the selected time range, a " + "version info token to resolve transaction conflicts, and an " + "OAuth2 token authenticating the user." + } + policy { + chrome_policy { + AllowDeletingBrowserHistory { + AllowDeletingBrowserHistory: false + } + } + })"); web_history->ExpireHistoryBetween(restrict_urls, begin_time, end_time, - base::Bind(&ExpireWebHistoryComplete)); + base::Bind(&ExpireWebHistoryComplete), + partial_traffic_annotation); } ExpireHistoryBetween(restrict_urls, begin_time, end_time, callback, tracker); } diff --git a/chromium/components/history/core/browser/history_service.h b/chromium/components/history/core/browser/history_service.h index b1aedcdf655..710ac708a86 100644 --- a/chromium/components/history/core/browser/history_service.h +++ b/chromium/components/history/core/browser/history_service.h @@ -245,10 +245,11 @@ class HistoryService : public syncer::SyncableService, public KeyedService { // empty. // // If success is false, neither the row nor the vector will be valid. - typedef base::Callback<void( + typedef base::OnceCallback<void( bool, // Success flag, when false, nothing else is valid. const URLRow&, - const VisitVector&)> QueryURLCallback; + const VisitVector&)> + QueryURLCallback; // Queries the basic information about the URL in the history database. If // the caller is interested in the visits (each time the URL is visited), @@ -257,7 +258,7 @@ class HistoryService : public syncer::SyncableService, public KeyedService { base::CancelableTaskTracker::TaskId QueryURL( const GURL& url, bool want_visits, - const QueryURLCallback& callback, + QueryURLCallback callback, base::CancelableTaskTracker* tracker); // Provides the result of a query. See QueryResults in history_types.h. @@ -756,18 +757,37 @@ class HistoryService : public syncer::SyncableService, public KeyedService { // 2) If |icon_url| is known to the database, |bitmaps| will be ignored (i.e. // the icon won't be overwritten) but the mappings from |page_url| to // |icon_url| will be stored (conditioned to point 1 above). - // 3) If |icon_url| is stored, it will be marked as expired. + // 3) If |icon_url| is stored, it will be marked as "on-demand". + // + // On-demand favicons are those that are fetched without visiting their page. + // For this reason, their life-time cannot be bound to the life-time of the + // corresponding visit in history. + // - These bitmaps are evicted from the database based on the last time they + // get requested. The last requested time is initially set to Now() and is + // further updated by calling TouchOnDemandFavicon(). + // - Furthermore, on-demand bitmaps are immediately marked as expired. Hence, + // they are always replaced by standard favicons whenever their page gets + // visited. // The callback will receive whether the write actually happened. - void SetLastResortFavicons(const GURL& page_url, - favicon_base::IconType icon_type, - const GURL& icon_url, - const std::vector<SkBitmap>& bitmaps, - base::Callback<void(bool)> callback); + void SetOnDemandFavicons(const GURL& page_url, + favicon_base::IconType icon_type, + const GURL& icon_url, + const std::vector<SkBitmap>& bitmaps, + base::Callback<void(bool)> callback); // Used by the FaviconService to mark the favicon for the page as being out // of date. void SetFaviconsOutOfDateForPage(const GURL& page_url); + // Mark that the on-demand favicon at |icon_url| was requested now. This + // postpones the automatic eviction of the favicon from the database. Not all + // calls end up in a write into the DB: + // - it is no-op if the bitmaps are not stored using SetOnDemandFavicons(); + // - the updates of the "last requested time" have limited frequency for each + // particular favicon (e.g. once per week). This limits the overhead of + // cache management for on-demand favicons. + void TouchOnDemandFavicon(const GURL& icon_url); + // Used by the FaviconService for importing many favicons for many pages at // once. The pages must exist, any favicon sets for unknown pages will be // discarded. Existing favicons will not be overwritten. diff --git a/chromium/components/history/core/browser/history_types.cc b/chromium/components/history/core/browser/history_types.cc index 93a242a2f0c..1624e636b2a 100644 --- a/chromium/components/history/core/browser/history_types.cc +++ b/chromium/components/history/core/browser/history_types.cc @@ -55,18 +55,18 @@ const size_t* QueryResults::MatchesForURL(const GURL& url, } void QueryResults::Swap(QueryResults* other) { - std::swap(first_time_searched_, other->first_time_searched_); std::swap(reached_beginning_, other->reached_beginning_); results_.swap(other->results_); url_to_results_.swap(other->url_to_results_); } -void QueryResults::AppendURLBySwapping(URLResult* result) { - URLResult* new_result = new URLResult; - new_result->SwapResult(result); +void QueryResults::SetURLResults(std::vector<URLResult>&& results) { + results_ = std::move(results); - results_.push_back(new_result); - AddURLUsageAtIndex(new_result->url(), results_.size() - 1); + // Recreate the map for the results_ has been replaced. + url_to_results_.clear(); + for(size_t i = 0; i < results_.size(); ++i) + AddURLUsageAtIndex(results_[i].url(), i); } void QueryResults::DeleteURL(const GURL& url) { @@ -83,7 +83,7 @@ void QueryResults::DeleteRange(size_t begin, size_t end) { // were modified. We will delete references to these later. std::set<GURL> urls_modified; for (size_t i = begin; i <= end; i++) { - urls_modified.insert(results_[i]->url()); + urls_modified.insert(results_[i].url()); } // Now just delete that range in the vector en masse (the STL ending is diff --git a/chromium/components/history/core/browser/history_types.h b/chromium/components/history/core/browser/history_types.h index 9189b068c5a..17b97b4a038 100644 --- a/chromium/components/history/core/browser/history_types.h +++ b/chromium/components/history/core/browser/history_types.h @@ -18,7 +18,6 @@ #include "base/containers/stack_container.h" #include "base/macros.h" #include "base/memory/ref_counted_memory.h" -#include "base/memory/scoped_vector.h" #include "base/strings/string16.h" #include "base/time/time.h" #include "components/favicon_base/favicon_types.h" @@ -134,38 +133,22 @@ struct PageVisit { // a given URL appears in those results. class QueryResults { public: - typedef std::vector<URLResult*> URLResultVector; + typedef std::vector<URLResult> URLResultVector; QueryResults(); ~QueryResults(); - // Indicates the first time that the query includes results for (queries are - // clipped at the beginning, so it will always include to the end of the time - // queried). - // - // If the number of results was clipped as a result of the max count, this - // will be the time of the first query returned. If there were fewer results - // than we were allowed to return, this represents the first date considered - // in the query (this will be before the first result if there was time - // queried with no results). - // - // TODO(brettw): bug 1203054: This field is not currently set properly! Do - // not use until the bug is fixed. - base::Time first_time_searched() const { return first_time_searched_; } - void set_first_time_searched(base::Time t) { first_time_searched_ = t; } - // Note: If you need end_time_searched, it can be added. - void set_reached_beginning(bool reached) { reached_beginning_ = reached; } bool reached_beginning() { return reached_beginning_; } size_t size() const { return results_.size(); } bool empty() const { return results_.empty(); } - URLResult& back() { return *results_.back(); } - const URLResult& back() const { return *results_.back(); } + URLResult& back() { return results_.back(); } + const URLResult& back() const { return results_.back(); } - URLResult& operator[](size_t i) { return *results_[i]; } - const URLResult& operator[](size_t i) const { return *results_[i]; } + URLResult& operator[](size_t i) { return results_[i]; } + const URLResult& operator[](size_t i) const { return results_[i]; } URLResultVector::const_iterator begin() const { return results_.begin(); } URLResultVector::const_iterator end() const { return results_.end(); } @@ -189,10 +172,9 @@ class QueryResults { // efficiently transferred without copying. void Swap(QueryResults* other); - // Adds the given result to the map, using swap() on the members to avoid - // copying (there are a lot of strings and vectors). This means the parameter - // object will be cleared after this call. - void AppendURLBySwapping(URLResult* result); + // Set the result vector, the parameter vector will be moved to results_. + // It means the parameter vector will be empty after calling this method. + void SetURLResults(std::vector<URLResult>&& results); // Removes all instances of the given URL from the result set. void DeleteURL(const GURL& url); @@ -215,14 +197,12 @@ class QueryResults { // (this is inclusive). This is used when inserting or deleting. void AdjustResultMap(size_t begin, size_t end, ptrdiff_t delta); - base::Time first_time_searched_; - // Whether the query reaches the beginning of the database. bool reached_beginning_; // The ordered list of results. The pointers inside this are owned by this // QueryResults object. - ScopedVector<URLResult> results_; + URLResultVector results_; // Maps URLs to entries in results_. URLToResultIndices url_to_results_; @@ -526,6 +506,24 @@ struct FaviconBitmapIDSize { gfx::Size pixel_size; }; +enum FaviconBitmapType { + // The bitmap gets downloaded while visiting its page. Their life-time is + // bound to the life-time of the corresponding visit in history. + // - These bitmaps are re-downloaded when visiting the page again and the + // last_updated timestamp is old enough. + ON_VISIT, + + // The bitmap gets downloaded because it is demanded by some Chrome UI (while + // not visiting its page). For this reason, their life-time cannot be bound to + // the life-time of the corresponding visit in history. + // - These bitmaps are evicted from the database based on the last time they + // were requested. + // - Furthermore, on-demand bitmaps are immediately marked as expired. Hence, + // they are always replaced by ON_VISIT favicons whenever their page gets + // visited. + ON_DEMAND +}; + // Defines a favicon bitmap stored in the history backend. struct FaviconBitmap { FaviconBitmap(); diff --git a/chromium/components/history/core/browser/history_types_unittest.cc b/chromium/components/history/core/browser/history_types_unittest.cc index 8641baa3304..09dd46cefb3 100644 --- a/chromium/components/history/core/browser/history_types_unittest.cc +++ b/chromium/components/history/core/browser/history_types_unittest.cc @@ -40,14 +40,14 @@ const char kURL2[] = "http://news.google.com/"; void AddSimpleData(QueryResults* results) { GURL url1(kURL1); GURL url2(kURL2); - URLResult result1(url1, base::Time::Now()); - URLResult result2(url1, base::Time::Now()); - URLResult result3(url2, base::Time::Now()); + std::vector<URLResult> test_vector; + + test_vector.push_back(URLResult(url1, base::Time::Now())); + test_vector.push_back(URLResult(url1, base::Time::Now())); + test_vector.push_back(URLResult(url2, base::Time::Now())); // The URLResults are invalid after being inserted. - results->AppendURLBySwapping(&result1); - results->AppendURLBySwapping(&result2); - results->AppendURLBySwapping(&result3); + results->SetURLResults(std::move(test_vector)); CheckHistoryResultConsistency(*results); } diff --git a/chromium/components/history/core/browser/thumbnail_database.cc b/chromium/components/history/core/browser/thumbnail_database.cc index 60445eb48f0..c8b430f4f71 100644 --- a/chromium/components/history/core/browser/thumbnail_database.cc +++ b/chromium/components/history/core/browser/thumbnail_database.cc @@ -64,13 +64,17 @@ namespace history { // icon_id The ID of the favicon that the bitmap is associated to. // last_updated The time at which this favicon was inserted into the // table. This is used to determine if it needs to be -// redownloaded from the web. +// redownloaded from the web. Value 0 denotes that the bitmap +// has been explicitly expired. // image_data PNG encoded data of the favicon. // width Pixel width of |image_data|. // height Pixel height of |image_data|. -// last_requested The time at which this bitmap was last requested. This is -// used to determine the priority with which the bitmap -// should be retained on cleanup. +// last_requested The time at which this bitmap was last requested. This +// entry is non-zero iff the bitmap is of type ON_DEMAND. +// This info is used for clearing old ON_DEMAND bitmaps. +// (On-demand bitmaps cannot get cleared along with expired +// visits in history DB because there is no corresponding +// visit.) namespace { @@ -503,12 +507,16 @@ bool ThumbnailDatabase::GetFaviconBitmap( FaviconBitmapID ThumbnailDatabase::AddFaviconBitmap( favicon_base::FaviconID icon_id, const scoped_refptr<base::RefCountedMemory>& icon_data, + FaviconBitmapType type, base::Time time, const gfx::Size& pixel_size) { DCHECK(icon_id); - sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE, - "INSERT INTO favicon_bitmaps (icon_id, image_data, last_updated, width, " - "height) VALUES (?, ?, ?, ?, ?)")); + + sql::Statement statement(db_.GetCachedStatement( + SQL_FROM_HERE, + "INSERT INTO favicon_bitmaps (icon_id, image_data, last_updated, " + "last_requested, width, height) VALUES (?, ?, ?, ?, ?, ?)")); + statement.BindInt64(0, icon_id); if (icon_data.get() && icon_data->size()) { statement.BindBlob(1, icon_data->front(), @@ -516,9 +524,19 @@ FaviconBitmapID ThumbnailDatabase::AddFaviconBitmap( } else { statement.BindNull(1); } - statement.BindInt64(2, time.ToInternalValue()); - statement.BindInt(3, pixel_size.width()); - statement.BindInt(4, pixel_size.height()); + + // On-visit bitmaps: + // - keep track of last_updated: last write time is used for expiration; + // - always have last_requested==0: no need to keep track of last read time. + statement.BindInt64(2, type == ON_VISIT ? time.ToInternalValue() : 0); + // On-demand bitmaps: + // - always have last_updated==0: last write time is not stored as they are + // always expired and thus ready to be replaced by ON_VISIT icons; + // - keep track of last_requested: last read time is used for cache eviction. + statement.BindInt64(3, type == ON_DEMAND ? time.ToInternalValue() : 0); + + statement.BindInt(4, pixel_size.width()); + statement.BindInt(5, pixel_size.height()); if (!statement.Run()) return 0; @@ -530,8 +548,13 @@ bool ThumbnailDatabase::SetFaviconBitmap( scoped_refptr<base::RefCountedMemory> bitmap_data, base::Time time) { DCHECK(bitmap_id); - sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE, - "UPDATE favicon_bitmaps SET image_data=?, last_updated=? WHERE id=?")); + // By updating last_updated timestamp, we assume the icon is of type ON_VISIT. + // If it is ON_DEMAND, reset last_requested to 0 and thus silently change the + // type to ON_VISIT. + sql::Statement statement( + db_.GetCachedStatement(SQL_FROM_HERE, + "UPDATE favicon_bitmaps SET image_data=?, " + "last_updated=?, last_requested=? WHERE id=?")); if (bitmap_data.get() && bitmap_data->size()) { statement.BindBlob(0, bitmap_data->front(), static_cast<int>(bitmap_data->size())); @@ -539,7 +562,8 @@ bool ThumbnailDatabase::SetFaviconBitmap( statement.BindNull(0); } statement.BindInt64(1, time.ToInternalValue()); - statement.BindInt64(2, bitmap_id); + statement.BindInt64(2, 0); + statement.BindInt64(3, bitmap_id); return statement.Run(); } @@ -548,22 +572,47 @@ bool ThumbnailDatabase::SetFaviconBitmapLastUpdateTime( FaviconBitmapID bitmap_id, base::Time time) { DCHECK(bitmap_id); - sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE, - "UPDATE favicon_bitmaps SET last_updated=? WHERE id=?")); + // By updating last_updated timestamp, we assume the icon is of type ON_VISIT. + // If it is ON_DEMAND, reset last_requested to 0 and thus silently change the + // type to ON_VISIT. + sql::Statement statement( + db_.GetCachedStatement(SQL_FROM_HERE, + "UPDATE favicon_bitmaps SET last_updated=?, " + "last_requested=? WHERE id=?")); statement.BindInt64(0, time.ToInternalValue()); - statement.BindInt64(1, bitmap_id); + statement.BindInt64(1, 0); + statement.BindInt64(2, bitmap_id); return statement.Run(); } -bool ThumbnailDatabase::SetFaviconBitmapLastRequestedTime( - FaviconBitmapID bitmap_id, - base::Time time) { - DCHECK(bitmap_id); - sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE, - "UPDATE favicon_bitmaps SET last_requested=? WHERE id=?")); - statement.BindInt64(0, time.ToInternalValue()); - statement.BindInt64(1, bitmap_id); - return statement.Run(); +bool ThumbnailDatabase::TouchOnDemandFavicon(const GURL& icon_url, + base::Time time) { + // Look up the icon ids for the url. + sql::Statement id_statement(db_.GetCachedStatement( + SQL_FROM_HERE, "SELECT id FROM favicons WHERE url=?")); + id_statement.BindString(0, URLDatabase::GURLToDatabaseURL(icon_url)); + + base::Time max_time = + time - base::TimeDelta::FromDays(kFaviconUpdateLastRequestedAfterDays); + + while (id_statement.Step()) { + favicon_base::FaviconID icon_id = id_statement.ColumnInt64(0); + + // Update the time only for ON_DEMAND bitmaps (i.e. with last_requested > + // 0). For performance reasons, update the time only if the currently stored + // time is old enough (UPDATEs where the WHERE condition does not match any + // entries are way faster than UPDATEs that really change some data). + sql::Statement statement(db_.GetCachedStatement( + SQL_FROM_HERE, + "UPDATE favicon_bitmaps SET last_requested=? WHERE icon_id=? AND " + "last_requested>0 AND last_requested<=?")); + statement.BindInt64(0, time.ToInternalValue()); + statement.BindInt64(1, icon_id); + statement.BindInt64(2, max_time.ToInternalValue()); + if (!statement.Run()) + return false; + } + return true; } bool ThumbnailDatabase::DeleteFaviconBitmap(FaviconBitmapID bitmap_id) { @@ -634,10 +683,11 @@ favicon_base::FaviconID ThumbnailDatabase::AddFavicon( const GURL& icon_url, favicon_base::IconType icon_type, const scoped_refptr<base::RefCountedMemory>& icon_data, + FaviconBitmapType type, base::Time time, const gfx::Size& pixel_size) { favicon_base::FaviconID icon_id = AddFavicon(icon_url, icon_type); - if (!icon_id || !AddFaviconBitmap(icon_id, icon_data, time, pixel_size)) + if (!icon_id || !AddFaviconBitmap(icon_id, icon_data, type, time, pixel_size)) return 0; return icon_id; diff --git a/chromium/components/history/core/browser/thumbnail_database.h b/chromium/components/history/core/browser/thumbnail_database.h index ec21369d438..697faf3abec 100644 --- a/chromium/components/history/core/browser/thumbnail_database.h +++ b/chromium/components/history/core/browser/thumbnail_database.h @@ -26,6 +26,10 @@ namespace history { class HistoryBackendClient; +// The minimum number of days after which last_requested field gets updated. +// All earlier updates are ignored. +static const int kFaviconUpdateLastRequestedAfterDays = 14; + // This database interface is owned by the history backend and runs on the // history thread. It is a totally separate component from history partially // because we may want to move it to its own thread in the future. The @@ -87,37 +91,36 @@ class ThumbnailDatabase { scoped_refptr<base::RefCountedMemory>* png_icon_data, gfx::Size* pixel_size); - // Adds a bitmap component at |pixel_size| for the favicon with |icon_id|. - // Only favicons representing a .ico file should have multiple favicon bitmaps - // per favicon. + // Adds a bitmap component of |type| at |pixel_size| for the favicon with + // |icon_id|. Only favicons representing a .ico file should have multiple + // favicon bitmaps per favicon. // |icon_data| is the png encoded data. - // The |time| indicates the access time, and is used to detect when the - // favicon should be refreshed. + // The |type| indicates how the lifetime of this icon should be managed. + // The |time| is used for lifetime management of the bitmap (should be Now()). // |pixel_size| is the pixel dimensions of |icon_data|. // Returns the id of the added bitmap or 0 if unsuccessful. FaviconBitmapID AddFaviconBitmap( favicon_base::FaviconID icon_id, const scoped_refptr<base::RefCountedMemory>& icon_data, + FaviconBitmapType type, base::Time time, const gfx::Size& pixel_size); // Sets the bitmap data and the last updated time for the favicon bitmap at - // |bitmap_id|. + // |bitmap_id|. Should not be called for bitmaps of type ON_DEMAND as they + // should never get updated (the call silently changes the type to ON_VISIT). // Returns true if successful. bool SetFaviconBitmap(FaviconBitmapID bitmap_id, scoped_refptr<base::RefCountedMemory> bitmap_data, base::Time time); - // Sets the last updated time for the favicon bitmap at |bitmap_id|. - // Returns true if successful. + // Sets the last_updated time for the favicon bitmap at |bitmap_id|. Should + // not be called for bitmaps of type ON_DEMAND as last_updated time is only + // tracked for ON_VISIT bitmaps (the call silently changes the type to + // ON_VISIT). Returns true if successful. bool SetFaviconBitmapLastUpdateTime(FaviconBitmapID bitmap_id, base::Time time); - // Sets the last requested time for the favicon bitmap at |bitmap_id|. - // Returns true if successful. - bool SetFaviconBitmapLastRequestedTime(FaviconBitmapID bitmap_id, - base::Time time); - // Deletes the favicon bitmap with |bitmap_id|. // Returns true if successful. bool DeleteFaviconBitmap(FaviconBitmapID bitmap_id); @@ -128,6 +131,16 @@ class ThumbnailDatabase { // of the bitmaps for |icon_id| to be out of date. bool SetFaviconOutOfDate(favicon_base::FaviconID icon_id); + // Mark all bitmaps of type ON_DEMAND at |icon_url| as requested at |time|. + // This postpones their automatic eviction from the database. Not all calls + // end up in a write into the DB: + // - it is no-op if the bitmaps are not of type ON_DEMAND; + // - the updates of the "last requested time" have limited frequency for each + // particular bitmap (e.g. once per week). This limits the overhead of + // cache management for on-demand favicons. + // Returns true if successful. + bool TouchOnDemandFavicon(const GURL& icon_url, base::Time time); + // Returns the id of the entry in the favicon database with the specified url // and icon type. // Returns 0 if no entry exists for the specified url. @@ -146,11 +159,12 @@ class ThumbnailDatabase { favicon_base::IconType icon_type); // Adds a favicon with a single bitmap. This call is equivalent to calling - // AddFavicon and AddFaviconBitmap. + // AddFavicon and AddFaviconBitmap of type |type|. favicon_base::FaviconID AddFavicon( const GURL& icon_url, favicon_base::IconType icon_type, const scoped_refptr<base::RefCountedMemory>& icon_data, + FaviconBitmapType type, base::Time time, const gfx::Size& pixel_size); diff --git a/chromium/components/history/core/browser/thumbnail_database_unittest.cc b/chromium/components/history/core/browser/thumbnail_database_unittest.cc index 0ece892cfe4..4dbd0c6f64e 100644 --- a/chromium/components/history/core/browser/thumbnail_database_unittest.cc +++ b/chromium/components/history/core/browser/thumbnail_database_unittest.cc @@ -85,7 +85,8 @@ void AddAndMapFaviconSimple(ThumbnailDatabase* db, scoped_refptr<base::RefCountedStaticMemory> data( new base::RefCountedStaticMemory(kBlob1, sizeof(kBlob1))); favicon_base::FaviconID favicon_id = - db->AddFavicon(icon_url, icon_type, data, base::Time::Now(), gfx::Size()); + db->AddFavicon(icon_url, icon_type, data, FaviconBitmapType::ON_VISIT, + base::Time::Now(), gfx::Size()); db->AddIconMapping(page_url, favicon_id); } @@ -213,7 +214,8 @@ TEST_F(ThumbnailDatabaseTest, AddIconMapping) { GURL url("http://google.com"); base::Time time = base::Time::Now(); favicon_base::FaviconID id = - db.AddFavicon(url, favicon_base::TOUCH_ICON, favicon, time, gfx::Size()); + db.AddFavicon(url, favicon_base::TOUCH_ICON, favicon, + FaviconBitmapType::ON_VISIT, time, gfx::Size()); EXPECT_NE(0, id); EXPECT_NE(0, db.AddIconMapping(url, id)); @@ -224,37 +226,145 @@ TEST_F(ThumbnailDatabaseTest, AddIconMapping) { EXPECT_EQ(id, icon_mappings.front().icon_id); } -TEST_F(ThumbnailDatabaseTest, LastRequestedTime) { - ThumbnailDatabase db(NULL); +TEST_F(ThumbnailDatabaseTest, + AddOnDemandFaviconBitmapCreatesCorrectTimestamps) { + ThumbnailDatabase db(nullptr); ASSERT_EQ(sql::INIT_OK, db.Init(file_name_)); db.BeginTransaction(); + base::Time add_time; + ASSERT_TRUE( + base::Time::FromUTCExploded({2017, 5, 0, 1, 0, 0, 0, 0}, &add_time)); std::vector<unsigned char> data(kBlob1, kBlob1 + sizeof(kBlob1)); scoped_refptr<base::RefCountedBytes> favicon(new base::RefCountedBytes(data)); GURL url("http://google.com"); - base::Time now = base::Time::Now(); - favicon_base::FaviconID id = - db.AddFavicon(url, favicon_base::TOUCH_ICON, favicon, now, gfx::Size()); - ASSERT_NE(0, id); - - // Fetching the last requested time of a non-existent bitmap should fail. - base::Time last_requested = base::Time::UnixEpoch(); - EXPECT_FALSE(db.GetFaviconBitmap(id + 1, NULL, &last_requested, NULL, NULL)); - EXPECT_EQ(last_requested, base::Time::UnixEpoch()); // Remains unchanged. - - // Fetching the last requested time of a bitmap that has no last request - // should return a null timestamp. - last_requested = base::Time::UnixEpoch(); - EXPECT_TRUE(db.GetFaviconBitmap(id, NULL, &last_requested, NULL, NULL)); - EXPECT_TRUE(last_requested.is_null()); - - // Setting the last requested time of an existing bitmap should succeed, and - // the set time should be returned by the corresponding "Get". - last_requested = base::Time::UnixEpoch(); - EXPECT_TRUE(db.SetFaviconBitmapLastRequestedTime(id, now)); - EXPECT_TRUE(db.GetFaviconBitmap(id, NULL, &last_requested, NULL, NULL)); - EXPECT_EQ(last_requested, now); + favicon_base::FaviconID icon = db.AddFavicon(url, favicon_base::FAVICON); + ASSERT_NE(0, icon); + FaviconBitmapID bitmap = db.AddFaviconBitmap( + icon, favicon, FaviconBitmapType::ON_DEMAND, add_time, gfx::Size()); + ASSERT_NE(0, bitmap); + + base::Time last_updated; + base::Time last_requested; + ASSERT_TRUE(db.GetFaviconBitmap(bitmap, &last_updated, &last_requested, + nullptr, nullptr)); + EXPECT_EQ(base::Time(), last_updated); + EXPECT_EQ(add_time, last_requested); +} + +TEST_F(ThumbnailDatabaseTest, AddFaviconBitmapCreatesCorrectTimestamps) { + ThumbnailDatabase db(nullptr); + ASSERT_EQ(sql::INIT_OK, db.Init(file_name_)); + db.BeginTransaction(); + + base::Time add_time; + ASSERT_TRUE( + base::Time::FromUTCExploded({2017, 5, 0, 1, 0, 0, 0, 0}, &add_time)); + std::vector<unsigned char> data(kBlob1, kBlob1 + sizeof(kBlob1)); + scoped_refptr<base::RefCountedBytes> favicon(new base::RefCountedBytes(data)); + + GURL url("http://google.com"); + favicon_base::FaviconID icon = db.AddFavicon(url, favicon_base::FAVICON); + ASSERT_NE(0, icon); + FaviconBitmapID bitmap = db.AddFaviconBitmap( + icon, favicon, FaviconBitmapType::ON_VISIT, add_time, gfx::Size()); + ASSERT_NE(0, bitmap); + + base::Time last_updated; + base::Time last_requested; + ASSERT_TRUE(db.GetFaviconBitmap(bitmap, &last_updated, &last_requested, + nullptr, nullptr)); + EXPECT_EQ(add_time, last_updated); + EXPECT_EQ(base::Time(), last_requested); +} + +TEST_F(ThumbnailDatabaseTest, TouchUpdatesOnDemandFavicons) { + ThumbnailDatabase db(nullptr); + ASSERT_EQ(sql::INIT_OK, db.Init(file_name_)); + db.BeginTransaction(); + + base::Time start; + ASSERT_TRUE(base::Time::FromUTCExploded({2017, 5, 0, 1, 0, 0, 0, 0}, &start)); + std::vector<unsigned char> data(kBlob1, kBlob1 + sizeof(kBlob1)); + scoped_refptr<base::RefCountedBytes> favicon(new base::RefCountedBytes(data)); + + // Create an on-demand favicon. + GURL url("http://google.com"); + favicon_base::FaviconID icon = db.AddFavicon(url, favicon_base::FAVICON); + ASSERT_NE(0, icon); + FaviconBitmapID bitmap = db.AddFaviconBitmap( + icon, favicon, FaviconBitmapType::ON_DEMAND, start, gfx::Size()); + ASSERT_NE(0, bitmap); + + base::Time end = + start + base::TimeDelta::FromDays(kFaviconUpdateLastRequestedAfterDays); + EXPECT_TRUE(db.TouchOnDemandFavicon(url, end)); + + base::Time last_updated; + base::Time last_requested; + EXPECT_TRUE(db.GetFaviconBitmap(bitmap, &last_updated, &last_requested, + nullptr, nullptr)); + // Does not mess with the last_updated field. + EXPECT_EQ(base::Time(), last_updated); + EXPECT_EQ(end, last_requested); // Updates the last_requested field. +} + +TEST_F(ThumbnailDatabaseTest, TouchUpdatesOnlyInfrequently) { + ThumbnailDatabase db(nullptr); + ASSERT_EQ(sql::INIT_OK, db.Init(file_name_)); + db.BeginTransaction(); + + base::Time start; + ASSERT_TRUE(base::Time::FromUTCExploded({2017, 5, 0, 1, 0, 0, 0, 0}, &start)); + std::vector<unsigned char> data(kBlob1, kBlob1 + sizeof(kBlob1)); + scoped_refptr<base::RefCountedBytes> favicon(new base::RefCountedBytes(data)); + + // Create an on-demand favicon. + GURL url("http://google.com"); + favicon_base::FaviconID icon = db.AddFavicon(url, favicon_base::FAVICON); + ASSERT_NE(0, icon); + FaviconBitmapID bitmap = db.AddFaviconBitmap( + icon, favicon, FaviconBitmapType::ON_DEMAND, start, gfx::Size()); + ASSERT_NE(0, bitmap); + + base::Time end = start + base::TimeDelta::FromMinutes(1); + EXPECT_TRUE(db.TouchOnDemandFavicon(url, end)); + + base::Time last_requested; + EXPECT_TRUE( + db.GetFaviconBitmap(bitmap, nullptr, &last_requested, nullptr, nullptr)); + EXPECT_EQ(start, last_requested); // No update. +} + +TEST_F(ThumbnailDatabaseTest, TouchDoesNotUpdateStandardFavicons) { + ThumbnailDatabase db(nullptr); + ASSERT_EQ(sql::INIT_OK, db.Init(file_name_)); + db.BeginTransaction(); + + base::Time start; + ASSERT_TRUE(base::Time::FromUTCExploded({2017, 5, 0, 1, 0, 0, 0, 0}, &start)); + std::vector<unsigned char> data(kBlob1, kBlob1 + sizeof(kBlob1)); + scoped_refptr<base::RefCountedBytes> favicon(new base::RefCountedBytes(data)); + + // Create a standard favicon. + GURL url("http://google.com"); + favicon_base::FaviconID icon = db.AddFavicon(url, favicon_base::FAVICON); + EXPECT_NE(0, icon); + FaviconBitmapID bitmap = db.AddFaviconBitmap( + icon, favicon, FaviconBitmapType::ON_VISIT, start, gfx::Size()); + EXPECT_NE(0, bitmap); + + base::Time end = + start + base::TimeDelta::FromDays(kFaviconUpdateLastRequestedAfterDays); + db.TouchOnDemandFavicon(url, end); + + base::Time last_updated; + base::Time last_requested; + EXPECT_TRUE(db.GetFaviconBitmap(bitmap, &last_updated, &last_requested, + nullptr, nullptr)); + EXPECT_EQ(start, last_updated); // Does not mess with last_updated. + EXPECT_EQ(base::Time(), last_requested); // No update. } TEST_F(ThumbnailDatabaseTest, DeleteIconMappings) { @@ -268,7 +378,8 @@ TEST_F(ThumbnailDatabaseTest, DeleteIconMappings) { GURL url("http://google.com"); favicon_base::FaviconID id = db.AddFavicon(url, favicon_base::TOUCH_ICON); base::Time time = base::Time::Now(); - db.AddFaviconBitmap(id, favicon, time, gfx::Size()); + db.AddFaviconBitmap(id, favicon, FaviconBitmapType::ON_VISIT, time, + gfx::Size()); EXPECT_LT(0, db.AddIconMapping(url, id)); favicon_base::FaviconID id2 = db.AddFavicon(url, favicon_base::FAVICON); @@ -299,13 +410,16 @@ TEST_F(ThumbnailDatabaseTest, GetIconMappingsForPageURL) { favicon_base::FaviconID id1 = db.AddFavicon(url, favicon_base::TOUCH_ICON); base::Time time = base::Time::Now(); - db.AddFaviconBitmap(id1, favicon, time, kSmallSize); - db.AddFaviconBitmap(id1, favicon, time, kLargeSize); + db.AddFaviconBitmap(id1, favicon, FaviconBitmapType::ON_VISIT, time, + kSmallSize); + db.AddFaviconBitmap(id1, favicon, FaviconBitmapType::ON_VISIT, time, + kLargeSize); EXPECT_LT(0, db.AddIconMapping(url, id1)); favicon_base::FaviconID id2 = db.AddFavicon(url, favicon_base::FAVICON); EXPECT_NE(id1, id2); - db.AddFaviconBitmap(id2, favicon, time, kSmallSize); + db.AddFaviconBitmap(id2, favicon, FaviconBitmapType::ON_VISIT, time, + kSmallSize); EXPECT_LT(0, db.AddIconMapping(url, id2)); std::vector<IconMapping> icon_mappings; @@ -341,19 +455,22 @@ TEST_F(ThumbnailDatabaseTest, RetainDataForPageUrls) { favicon_base::FaviconID kept_id1 = db.AddFavicon(kIconUrl1, favicon_base::FAVICON); - db.AddFaviconBitmap(kept_id1, favicon1, base::Time::Now(), kLargeSize); + db.AddFaviconBitmap(kept_id1, favicon1, FaviconBitmapType::ON_VISIT, + base::Time::Now(), kLargeSize); db.AddIconMapping(kPageUrl1, kept_id1); db.AddIconMapping(kPageUrl3, kept_id1); db.AddIconMapping(kPageUrl4, kept_id1); favicon_base::FaviconID unkept_id = db.AddFavicon(kIconUrl2, favicon_base::FAVICON); - db.AddFaviconBitmap(unkept_id, favicon1, base::Time::Now(), kLargeSize); + db.AddFaviconBitmap(unkept_id, favicon1, FaviconBitmapType::ON_VISIT, + base::Time::Now(), kLargeSize); db.AddIconMapping(kPageUrl2, unkept_id); favicon_base::FaviconID kept_id2 = db.AddFavicon(kIconUrl5, favicon_base::FAVICON); - db.AddFaviconBitmap(kept_id2, favicon2, base::Time::Now(), kLargeSize); + db.AddFaviconBitmap(kept_id2, favicon2, FaviconBitmapType::ON_VISIT, + base::Time::Now(), kLargeSize); db.AddIconMapping(kPageUrl5, kept_id2); // RetainDataForPageUrls() uses schema manipulations for efficiency. @@ -406,8 +523,8 @@ TEST_F(ThumbnailDatabaseTest, RetainDataForPageUrlsExpiresRetainedFavicons) { scoped_refptr<base::RefCountedStaticMemory> favicon1( new base::RefCountedStaticMemory(kBlob1, sizeof(kBlob1))); favicon_base::FaviconID kept_id = db.AddFavicon( - kIconUrl1, favicon_base::FAVICON, favicon1, base::Time::Now(), - gfx::Size()); + kIconUrl1, favicon_base::FAVICON, favicon1, FaviconBitmapType::ON_VISIT, + base::Time::Now(), gfx::Size()); db.AddIconMapping(kPageUrl1, kept_id); EXPECT_TRUE(db.RetainDataForPageUrls(std::vector<GURL>(1u, kPageUrl1))); @@ -439,8 +556,10 @@ TEST_F(ThumbnailDatabaseTest, DeleteFavicon) { GURL url("http://google.com"); favicon_base::FaviconID id = db.AddFavicon(url, favicon_base::FAVICON); base::Time last_updated = base::Time::Now(); - db.AddFaviconBitmap(id, favicon1, last_updated, kSmallSize); - db.AddFaviconBitmap(id, favicon2, last_updated, kLargeSize); + db.AddFaviconBitmap(id, favicon1, FaviconBitmapType::ON_VISIT, last_updated, + kSmallSize); + db.AddFaviconBitmap(id, favicon2, FaviconBitmapType::ON_VISIT, last_updated, + kLargeSize); EXPECT_TRUE(db.GetFaviconBitmaps(id, NULL)); @@ -461,8 +580,9 @@ TEST_F(ThumbnailDatabaseTest, GetIconMappingsForPageURLForReturnOrder) { GURL icon_url("http://google.com/favicon.ico"); base::Time time = base::Time::Now(); - favicon_base::FaviconID id = db.AddFavicon( - icon_url, favicon_base::FAVICON, favicon, time, gfx::Size()); + favicon_base::FaviconID id = + db.AddFavicon(icon_url, favicon_base::FAVICON, favicon, + FaviconBitmapType::ON_VISIT, time, gfx::Size()); EXPECT_NE(0, db.AddIconMapping(page_url, id)); std::vector<IconMapping> icon_mappings; EXPECT_TRUE(db.GetIconMappingsForPageURL(page_url, &icon_mappings)); @@ -477,8 +597,9 @@ TEST_F(ThumbnailDatabaseTest, GetIconMappingsForPageURLForReturnOrder) { scoped_refptr<base::RefCountedBytes> favicon2 = new base::RefCountedBytes(data); - favicon_base::FaviconID id2 = db.AddFavicon( - icon_url, favicon_base::TOUCH_ICON, favicon2, time, gfx::Size()); + favicon_base::FaviconID id2 = + db.AddFavicon(icon_url, favicon_base::TOUCH_ICON, favicon2, + FaviconBitmapType::ON_VISIT, time, gfx::Size()); EXPECT_NE(0, db.AddIconMapping(page_url, id2)); icon_mappings.clear(); @@ -494,11 +615,8 @@ TEST_F(ThumbnailDatabaseTest, GetIconMappingsForPageURLForReturnOrder) { new base::RefCountedBytes(data2); favicon_base::FaviconID id3 = - db.AddFavicon(icon_url, - favicon_base::TOUCH_PRECOMPOSED_ICON, - favicon3, - time, - gfx::Size()); + db.AddFavicon(icon_url, favicon_base::TOUCH_PRECOMPOSED_ICON, favicon3, + FaviconBitmapType::ON_VISIT, time, gfx::Size()); EXPECT_NE(0, db.AddIconMapping(page_url, id3)); icon_mappings.clear(); @@ -548,31 +666,23 @@ TEST_F(ThumbnailDatabaseTest, HasMappingFor) { // Add a favicon which will have icon_mappings base::Time time = base::Time::Now(); - favicon_base::FaviconID id1 = db.AddFavicon(GURL("http://google.com"), - favicon_base::FAVICON, - favicon, - time, - gfx::Size()); + favicon_base::FaviconID id1 = + db.AddFavicon(GURL("http://google.com"), favicon_base::FAVICON, favicon, + FaviconBitmapType::ON_VISIT, time, gfx::Size()); EXPECT_NE(id1, 0); // Add another type of favicon time = base::Time::Now(); - favicon_base::FaviconID id2 = - db.AddFavicon(GURL("http://www.google.com/icon"), - favicon_base::TOUCH_ICON, - favicon, - time, - gfx::Size()); + favicon_base::FaviconID id2 = db.AddFavicon( + GURL("http://www.google.com/icon"), favicon_base::TOUCH_ICON, favicon, + FaviconBitmapType::ON_VISIT, time, gfx::Size()); EXPECT_NE(id2, 0); // Add 3rd favicon time = base::Time::Now(); - favicon_base::FaviconID id3 = - db.AddFavicon(GURL("http://www.google.com/icon"), - favicon_base::TOUCH_ICON, - favicon, - time, - gfx::Size()); + favicon_base::FaviconID id3 = db.AddFavicon( + GURL("http://www.google.com/icon"), favicon_base::TOUCH_ICON, favicon, + FaviconBitmapType::ON_VISIT, time, gfx::Size()); EXPECT_NE(id3, 0); // Add 2 icon mapping diff --git a/chromium/components/history/core/browser/top_sites_backend.cc b/chromium/components/history/core/browser/top_sites_backend.cc index 7124e827998..f343d5b3e3a 100644 --- a/chromium/components/history/core/browser/top_sites_backend.cc +++ b/chromium/components/history/core/browser/top_sites_backend.cc @@ -14,6 +14,8 @@ #include "base/metrics/histogram_macros.h" #include "base/single_thread_task_runner.h" #include "base/task/cancelable_task_tracker.h" +#include "base/task_scheduler/post_task.h" +#include "base/task_scheduler/task_traits.h" #include "base/time/time.h" #include "base/trace_event/trace_event.h" #include "components/history/core/browser/top_sites_database.h" @@ -21,9 +23,10 @@ namespace history { -TopSitesBackend::TopSitesBackend( - const scoped_refptr<base::SingleThreadTaskRunner>& db_task_runner) - : db_(new TopSitesDatabase()), db_task_runner_(db_task_runner) { +TopSitesBackend::TopSitesBackend() + : db_(new TopSitesDatabase()), + db_task_runner_(base::CreateSequencedTaskRunnerWithTraits( + {base::TaskPriority::USER_VISIBLE, base::MayBlock()})) { DCHECK(db_task_runner_); } @@ -83,7 +86,7 @@ TopSitesBackend::~TopSitesBackend() { } void TopSitesBackend::InitDBOnDBThread(const base::FilePath& path) { - DCHECK(db_task_runner_->BelongsToCurrentThread()); + DCHECK(db_task_runner_->RunsTasksInCurrentSequence()); if (!db_->Init(path)) { LOG(ERROR) << "Failed to initialize database."; db_.reset(); @@ -91,13 +94,13 @@ void TopSitesBackend::InitDBOnDBThread(const base::FilePath& path) { } void TopSitesBackend::ShutdownDBOnDBThread() { - DCHECK(db_task_runner_->BelongsToCurrentThread()); + DCHECK(db_task_runner_->RunsTasksInCurrentSequence()); db_.reset(); } void TopSitesBackend::GetMostVisitedThumbnailsOnDBThread( scoped_refptr<MostVisitedThumbnails> thumbnails) { - DCHECK(db_task_runner_->BelongsToCurrentThread()); + DCHECK(db_task_runner_->RunsTasksInCurrentSequence()); if (db_) { db_->GetPageThumbnails(&(thumbnails->most_visited), @@ -139,7 +142,7 @@ void TopSitesBackend::SetPageThumbnailOnDBThread(const MostVisitedURL& url, } void TopSitesBackend::ResetDatabaseOnDBThread(const base::FilePath& file_path) { - DCHECK(db_task_runner_->BelongsToCurrentThread()); + DCHECK(db_task_runner_->RunsTasksInCurrentSequence()); db_.reset(NULL); sql::Connection::Delete(db_path_); db_.reset(new TopSitesDatabase()); diff --git a/chromium/components/history/core/browser/top_sites_backend.h b/chromium/components/history/core/browser/top_sites_backend.h index 807f3374b61..620a76ac245 100644 --- a/chromium/components/history/core/browser/top_sites_backend.h +++ b/chromium/components/history/core/browser/top_sites_backend.h @@ -16,7 +16,7 @@ namespace base { class CancelableTaskTracker; class FilePath; -class SingleThreadTaskRunner; +class SequencedTaskRunner; } namespace history { @@ -42,8 +42,7 @@ class TopSitesBackend : public base::RefCountedThreadSafe<TopSitesBackend> { typedef base::Callback<void(const scoped_refptr<MostVisitedThumbnails>&)> GetMostVisitedThumbnailsCallback; - explicit TopSitesBackend( - const scoped_refptr<base::SingleThreadTaskRunner>& db_task_runner); + TopSitesBackend(); void Init(const base::FilePath& path); @@ -103,7 +102,7 @@ class TopSitesBackend : public base::RefCountedThreadSafe<TopSitesBackend> { base::FilePath db_path_; std::unique_ptr<TopSitesDatabase> db_; - scoped_refptr<base::SingleThreadTaskRunner> db_task_runner_; + scoped_refptr<base::SequencedTaskRunner> db_task_runner_; DISALLOW_COPY_AND_ASSIGN(TopSitesBackend); }; diff --git a/chromium/components/history/core/browser/top_sites_impl.cc b/chromium/components/history/core/browser/top_sites_impl.cc index 53171ff079e..aa758327087 100644 --- a/chromium/components/history/core/browser/top_sites_impl.cc +++ b/chromium/components/history/core/browser/top_sites_impl.cc @@ -51,7 +51,7 @@ void RunOrPostGetMostVisitedURLsCallback( const MostVisitedURLList& nonforced_urls) { const MostVisitedURLList& urls = include_forced_urls ? all_urls : nonforced_urls; - if (task_runner->RunsTasksOnCurrentThread()) + if (task_runner->RunsTasksInCurrentSequence()) callback.Run(urls); else task_runner->PostTask(FROM_HERE, base::Bind(callback, urls)); @@ -120,12 +120,10 @@ TopSitesImpl::TopSitesImpl(PrefService* pref_service, DCHECK(!can_add_url_to_history_.is_null()); } -void TopSitesImpl::Init( - const base::FilePath& db_name, - const scoped_refptr<base::SingleThreadTaskRunner>& db_task_runner) { +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. - backend_ = new TopSitesBackend(db_task_runner); + backend_ = new TopSitesBackend(); backend_->Init(db_name); backend_->GetMostVisitedThumbnails( base::Bind(&TopSitesImpl::OnGotMostVisitedThumbnails, @@ -136,38 +134,25 @@ void TopSitesImpl::Init( bool TopSitesImpl::SetPageThumbnail(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 false; - } - - bool add_temp_thumbnail = false; - if (!IsKnownURL(url)) { - if (IsNonForcedFull()) - return false; // We're full, and this URL is not known to us. - - add_temp_thumbnail = true; - } - - if (!can_add_url_to_history_.Run(url)) - return false; // It's not a real webpage. + ThumbnailEvent result = SetPageThumbnailImpl(url, thumbnail, score); - scoped_refptr<base::RefCountedBytes> thumbnail_data; - if (!EncodeBitmap(thumbnail, &thumbnail_data)) - return false; + UMA_HISTOGRAM_ENUMERATION("Thumbnails.AddedToTopSites", result, + THUMBNAIL_EVENT_COUNT); - 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 true; + 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 SetPageThumbnailEncoded(url, thumbnail_data.get(), score); + return false; } // WARNING: this function may be invoked on any thread. @@ -489,11 +474,51 @@ void TopSitesImpl::DiffMostVisited(const MostVisitedURLList& old_list, } } -bool TopSitesImpl::SetPageThumbnailNoDB( +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) { - // This should only be invoked when we know about the url. DCHECK(cache_->IsKnownURL(url)); const MostVisitedURL& most_visited = @@ -524,13 +549,12 @@ bool TopSitesImpl::SetPageThumbnailEncoded( const GURL& url, const base::RefCountedMemory* thumbnail, const ThumbnailScore& score) { - if (!SetPageThumbnailNoDB(url, thumbnail, score)) - return false; + DCHECK(cache_->IsKnownURL(url)); - // Update the database. - if (!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]; @@ -743,8 +767,16 @@ void TopSitesImpl::SetTopSites(const MostVisitedURLList& new_top_sites, for (TempImages::iterator it = temp_images_.begin(); it != temp_images_.end(); ++it) { if (canonical_url == cache_->GetCanonicalURL(it->first)) { - SetPageThumbnailEncoded( + 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; } diff --git a/chromium/components/history/core/browser/top_sites_impl.h b/chromium/components/history/core/browser/top_sites_impl.h index 3024d448a4c..e8d10234207 100644 --- a/chromium/components/history/core/browser/top_sites_impl.h +++ b/chromium/components/history/core/browser/top_sites_impl.h @@ -39,7 +39,6 @@ namespace base { class FilePath; class RefCountedBytes; class RefCountedMemory; -class SingleThreadTaskRunner; } namespace history { @@ -64,8 +63,7 @@ class TopSitesImpl : public TopSites, public HistoryServiceObserver { const CanAddURLToHistoryFn& can_add_url_to_history); // Initializes TopSitesImpl. - void Init(const base::FilePath& db_name, - const scoped_refptr<base::SingleThreadTaskRunner>& db_task_runner); + void Init(const base::FilePath& db_name); // TopSites implementation. bool SetPageThumbnail(const GURL& url, @@ -119,6 +117,21 @@ class TopSitesImpl : public TopSites, public HistoryServiceObserver { 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); @@ -146,14 +159,22 @@ class TopSitesImpl : public TopSites, public HistoryServiceObserver { const MostVisitedURLList& new_list, TopSitesDelta* delta); - // Sets the thumbnail without writing to the database. - // Returns true if the thumbnail was set, false if the existing one is better. - bool SetPageThumbnailNoDB(const GURL& url, - const base::RefCountedMemory* thumbnail_data, - const ThumbnailScore& score); + // 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); - // A version of SetPageThumbnail that takes RefCountedBytes as - // returned by HistoryService. + // 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); 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 25c83ff3cf0..00d1e10793b 100644 --- a/chromium/components/history/core/browser/top_sites_impl_unittest.cc +++ b/chromium/components/history/core/browser/top_sites_impl_unittest.cc @@ -11,10 +11,10 @@ #include "base/files/scoped_temp_dir.h" #include "base/macros.h" #include "base/memory/weak_ptr.h" -#include "base/message_loop/message_loop.h" #include "base/run_loop.h" #include "base/strings/utf_string_conversions.h" #include "base/task/cancelable_task_tracker.h" +#include "base/test/scoped_task_environment.h" #include "build/build_config.h" #include "components/history/core/browser/history_client.h" #include "components/history/core/browser/history_constants.h" @@ -72,13 +72,14 @@ class TopSitesQuerier { 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()), + weak_ptr_factory_.GetWeakPtr(), &run_loop), include_forced_urls); if (wait && start_number_of_callbacks == number_of_callbacks_) { waiting_ = true; - base::RunLoop().Run(); + run_loop.Run(); } } @@ -91,11 +92,12 @@ class TopSitesQuerier { private: // Callback for TopSitesImpl::GetMostVisitedURLs. - void OnTopSitesAvailable(const history::MostVisitedURLList& data) { + void OnTopSitesAvailable(base::RunLoop* run_loop, + const history::MostVisitedURLList& data) { urls_ = data; number_of_callbacks_++; if (waiting_) { - base::MessageLoop::current()->QuitWhenIdle(); + run_loop->QuitWhenIdle(); waiting_ = false; } } @@ -166,15 +168,6 @@ class TopSitesImplTest : public HistoryUnitTestBase { BlockUntilHistoryProcessesPendingRequests(history_service()); } - // Waits for top sites to finish processing a task. This is useful if you need - // to wait until top sites finishes processing a task. - void WaitForTopSites() { - top_sites()->backend_->DoEmptyRequest( - base::Bind(&TopSitesImplTest::QuitCallback, base::Unretained(this)), - &top_sites_tracker_); - base::RunLoop().Run(); - } - TopSitesImpl* top_sites() { return top_sites_impl_.get(); } HistoryService* history_service() { return history_service_.get(); } @@ -196,10 +189,6 @@ class TopSitesImplTest : public HistoryUnitTestBase { } } - // Quit the current message loop when invoked. Useful when running a nested - // message loop. - void QuitCallback() { base::MessageLoop::current()->QuitWhenIdle(); } - // Adds a page to history. void AddPageToHistory(const GURL& url) { RedirectList redirects; @@ -299,8 +288,7 @@ class TopSitesImplTest : public HistoryUnitTestBase { top_sites_impl_ = new TopSitesImpl( pref_service_.get(), history_service_.get(), prepopulated_pages, base::Bind(MockCanAddURLToHistory)); - top_sites_impl_->Init(scoped_temp_dir_.GetPath().Append(kTopSitesFilename), - message_loop_.task_runner()); + top_sites_impl_->Init(scoped_temp_dir_.GetPath().Append(kTopSitesFilename)); } void DestroyTopSites() { @@ -308,8 +296,7 @@ class TopSitesImplTest : public HistoryUnitTestBase { top_sites_impl_->ShutdownOnUIThread(); top_sites_impl_ = nullptr; - if (base::MessageLoop::current()) - base::RunLoop().RunUntilIdle(); + scoped_task_environment_.RunUntilIdle(); } } @@ -320,8 +307,9 @@ class TopSitesImplTest : public HistoryUnitTestBase { } private: + base::test::ScopedTaskEnvironment scoped_task_environment_; + base::ScopedTempDir scoped_temp_dir_; - base::MessageLoopForUI message_loop_; std::unique_ptr<TestingPrefServiceSimple> pref_service_; std::unique_ptr<HistoryService> history_service_; diff --git a/chromium/components/history/core/browser/typed_url_sync_bridge.cc b/chromium/components/history/core/browser/typed_url_sync_bridge.cc index 40d57d67272..150ae7c5058 100644 --- a/chromium/components/history/core/browser/typed_url_sync_bridge.cc +++ b/chromium/components/history/core/browser/typed_url_sync_bridge.cc @@ -4,47 +4,185 @@ #include "components/history/core/browser/typed_url_sync_bridge.h" +#include "base/big_endian.h" #include "base/memory/ptr_util.h" +#include "base/metrics/histogram_macros.h" +#include "base/strings/utf_string_conversions.h" +#include "components/history/core/browser/history_backend.h" +#include "components/sync/model/mutable_data_batch.h" #include "components/sync/model_impl/sync_metadata_store_change_list.h" +#include "net/base/url_util.h" + +using sync_pb::TypedUrlSpecifics; +using syncer::EntityChange; +using syncer::EntityChangeList; +using syncer::EntityData; +using syncer::MetadataChangeList; +using syncer::ModelError; +using syncer::MutableDataBatch; namespace history { +namespace { + +// The server backend can't handle arbitrarily large node sizes, so to keep +// the size under control we limit the visit array. +static const int kMaxTypedUrlVisits = 100; + +// There's no limit on how many visits the history DB could have for a given +// typed URL, so we limit how many we fetch from the DB to avoid crashes due to +// running out of memory (http://crbug.com/89793). This value is different +// from kMaxTypedUrlVisits, as some of the visits fetched from the DB may be +// RELOAD visits, which will be stripped. +static const int kMaxVisitsToFetch = 1000; + +// Enforce oldest to newest visit order. +static bool CheckVisitOrdering(const VisitVector& visits) { + int64_t previous_visit_time = 0; + for (VisitVector::const_iterator visit = visits.begin(); + visit != visits.end(); ++visit) { + if (visit != visits.begin() && + previous_visit_time > visit->visit_time.ToInternalValue()) + return false; + + previous_visit_time = visit->visit_time.ToInternalValue(); + } + return true; +} + +std::string GetStorageKeyFromURLRow(const URLRow& row) { + std::string storage_key(sizeof(row.id()), 0); + base::WriteBigEndian<URLID>(&storage_key[0], row.id()); + return storage_key; +} + +bool HasTypedUrl(const VisitVector& visits) { + auto typed_url_visit = std::find_if( + visits.begin(), visits.end(), [](const history::VisitRow& visit) { + return ui::PageTransitionCoreTypeIs(visit.transition, + ui::PAGE_TRANSITION_TYPED); + }); + return typed_url_visit != visits.end(); +} + +} // namespace + TypedURLSyncBridge::TypedURLSyncBridge( HistoryBackend* history_backend, - syncer::SyncMetadataStore* sync_metadata_store, + TypedURLSyncMetadataDatabase* sync_metadata_database, const ChangeProcessorFactory& change_processor_factory) : ModelTypeSyncBridge(change_processor_factory, syncer::TYPED_URLS), history_backend_(history_backend), - sync_metadata_store_(sync_metadata_store) { + sync_metadata_database_(sync_metadata_database), + num_db_accesses_(0), + num_db_errors_(0), + history_backend_observer_(this) { DCHECK(history_backend_); DCHECK(sequence_checker_.CalledOnValidSequence()); - DCHECK(sync_metadata_store_); - NOTIMPLEMENTED(); + DCHECK(sync_metadata_database_); } TypedURLSyncBridge::~TypedURLSyncBridge() { - // TODO(gangwu): unregister as HistoryBackendObserver, can use ScopedObserver - // to do it. } -std::unique_ptr<syncer::MetadataChangeList> +std::unique_ptr<MetadataChangeList> TypedURLSyncBridge::CreateMetadataChangeList() { DCHECK(sequence_checker_.CalledOnValidSequence()); return base::MakeUnique<syncer::SyncMetadataStoreChangeList>( - sync_metadata_store_, syncer::TYPED_URLS); + sync_metadata_database_, syncer::TYPED_URLS); } -base::Optional<syncer::ModelError> TypedURLSyncBridge::MergeSyncData( - std::unique_ptr<syncer::MetadataChangeList> metadata_change_list, - syncer::EntityDataMap entity_data_map) { +base::Optional<ModelError> TypedURLSyncBridge::MergeSyncData( + std::unique_ptr<MetadataChangeList> metadata_change_list, + EntityChangeList entity_data) { DCHECK(sequence_checker_.CalledOnValidSequence()); - NOTIMPLEMENTED(); - return {}; + + // Create a mapping of all local data by URLID. These will be narrowed down + // by CreateOrUpdateUrl() to include only the entries different from sync + // server data. + TypedURLMap new_db_urls; + + // Get all the visits and map the URLRows by URL. + URLVisitVectorMap local_visit_vectors; + + if (!GetValidURLsAndVisits(&local_visit_vectors, &new_db_urls)) { + return ModelError( + FROM_HERE, "Could not get the typed_url entries from HistoryBackend."); + } + + // New sync data organized for different write operations to history backend. + history::URLRows new_synced_urls; + history::URLRows updated_synced_urls; + TypedURLVisitVector new_synced_visits; + + // Iterate through entity_data and check for all the urls that + // sync already knows about. CreateOrUpdateUrl() will remove urls that + // are the same as the synced ones from |new_db_urls|. + for (const EntityChange& entity_change : entity_data) { + DCHECK(entity_change.data().specifics.has_typed_url()); + const TypedUrlSpecifics& specifics = + entity_change.data().specifics.typed_url(); + if (ShouldIgnoreUrl(GURL(specifics.url()))) + continue; + + // Ignore old sync urls that don't have any transition data stored with + // them, or transition data that does not match the visit data (will be + // deleted below). + if (specifics.visit_transitions_size() == 0 || + specifics.visit_transitions_size() != specifics.visits_size()) { + // Generate a debug assertion to help track down http://crbug.com/91473, + // even though we gracefully handle this case by overwriting this node. + DCHECK_EQ(specifics.visits_size(), specifics.visit_transitions_size()); + DVLOG(1) << "Ignoring obsolete sync url with no visit transition info."; + + continue; + } + UpdateUrlFromServer(specifics, &new_db_urls, &local_visit_vectors, + &new_synced_urls, &new_synced_visits, + &updated_synced_urls); + } + + for (const auto& kv : new_db_urls) { + if (!HasTypedUrl(local_visit_vectors[kv.first])) { + // This URL has no TYPED visits, don't sync it + continue; + } + std::string storage_key = GetStorageKeyFromURLRow(kv.second); + change_processor()->Put( + storage_key, CreateEntityData(kv.second, local_visit_vectors[kv.first]), + metadata_change_list.get()); + } + + base::Optional<ModelError> error = WriteToHistoryBackend( + &new_synced_urls, &updated_synced_urls, NULL, &new_synced_visits, NULL); + + if (error) + return error; + + for (const EntityChange& entity_change : entity_data) { + DCHECK(entity_change.data().specifics.has_typed_url()); + std::string storage_key = + GetStorageKeyInternal(entity_change.data().specifics.typed_url().url()); + if (storage_key.empty()) { + // ignore entity change + } else { + change_processor()->UpdateStorageKey(entity_change.data(), storage_key, + metadata_change_list.get()); + } + } + + UMA_HISTOGRAM_PERCENTAGE("Sync.TypedUrlMergeAndStartSyncingErrors", + GetErrorPercentage()); + ClearErrorStats(); + + return static_cast<syncer::SyncMetadataStoreChangeList*>( + metadata_change_list.get()) + ->TakeError(); } -base::Optional<syncer::ModelError> TypedURLSyncBridge::ApplySyncChanges( - std::unique_ptr<syncer::MetadataChangeList> metadata_change_list, - syncer::EntityChangeList entity_changes) { +base::Optional<ModelError> TypedURLSyncBridge::ApplySyncChanges( + std::unique_ptr<MetadataChangeList> metadata_change_list, + EntityChangeList entity_changes) { DCHECK(sequence_checker_.CalledOnValidSequence()); NOTIMPLEMENTED(); return {}; @@ -58,15 +196,31 @@ void TypedURLSyncBridge::GetData(StorageKeyList storage_keys, void TypedURLSyncBridge::GetAllData(DataCallback callback) { DCHECK(sequence_checker_.CalledOnValidSequence()); - NOTIMPLEMENTED(); + + history::URLRows typed_urls; + ++num_db_accesses_; + if (!history_backend_->GetAllTypedURLs(&typed_urls)) { + ++num_db_errors_; + change_processor()->ReportError(FROM_HERE, + "Could not get the typed_url entries."); + return; + } + + auto batch = base::MakeUnique<MutableDataBatch>(); + for (history::URLRow& url : typed_urls) { + VisitVector visits_vector; + FixupURLAndGetVisits(&url, &visits_vector); + batch->Put(GetStorageKeyFromURLRow(url), + CreateEntityData(url, visits_vector)); + } + callback.Run(std::move(batch)); } // Must be exactly the value of GURL::spec() for backwards comparability with // the previous (Directory + SyncableService) iteration of sync integration. // This can be large but it is assumed that this is not held in memory at steady // state. -std::string TypedURLSyncBridge::GetClientTag( - const syncer::EntityData& entity_data) { +std::string TypedURLSyncBridge::GetClientTag(const EntityData& entity_data) { DCHECK(sequence_checker_.CalledOnValidSequence()); DCHECK(entity_data.specifics.has_typed_url()) << "EntityData does not have typed urls specifics."; @@ -76,13 +230,15 @@ std::string TypedURLSyncBridge::GetClientTag( // Prefer to use URLRow::id() to uniquely identify entities when coordinating // with sync because it has a significantly low memory cost than a URL. -std::string TypedURLSyncBridge::GetStorageKey( - const syncer::EntityData& entity_data) { - DCHECK(sequence_checker_.CalledOnValidSequence()); - NOTIMPLEMENTED(); +std::string TypedURLSyncBridge::GetStorageKey(const EntityData& entity_data) { + NOTREACHED() << "TypedURLSyncBridge do not support GetStorageKey."; return std::string(); } +bool TypedURLSyncBridge::SupportsGetStorageKey() const { + return false; +} + void TypedURLSyncBridge::OnURLVisited(history::HistoryBackend* history_backend, ui::PageTransition transition, const history::URLRow& row, @@ -108,4 +264,641 @@ void TypedURLSyncBridge::OnURLsDeleted(history::HistoryBackend* history_backend, NOTIMPLEMENTED(); } +void TypedURLSyncBridge::Init() { + DCHECK(sequence_checker_.CalledOnValidSequence()); + + history_backend_observer_.Add(history_backend_); + LoadMetadata(); +} + +int TypedURLSyncBridge::GetErrorPercentage() const { + return num_db_accesses_ ? (100 * num_db_errors_ / num_db_accesses_) : 0; +} + +bool TypedURLSyncBridge::WriteToTypedUrlSpecifics( + const URLRow& url, + const VisitVector& visits, + TypedUrlSpecifics* typed_url) { + DCHECK(!url.last_visit().is_null()); + DCHECK(!visits.empty()); + DCHECK_EQ(url.last_visit().ToInternalValue(), + visits.back().visit_time.ToInternalValue()); + + typed_url->set_url(url.url().spec()); + typed_url->set_title(base::UTF16ToUTF8(url.title())); + typed_url->set_hidden(url.hidden()); + + DCHECK(CheckVisitOrdering(visits)); + + bool only_typed = false; + int skip_count = 0; + + if (std::find_if(visits.begin(), visits.end(), + [](const history::VisitRow& visit) { + return ui::PageTransitionCoreTypeIs( + visit.transition, ui::PAGE_TRANSITION_TYPED); + }) == visits.end()) { + // This URL has no TYPED visits, don't sync it + return false; + } + + if (visits.size() > static_cast<size_t>(kMaxTypedUrlVisits)) { + int typed_count = 0; + int total = 0; + // Walk the passed-in visit vector and count the # of typed visits. + for (VisitRow visit : visits) { + // We ignore reload visits. + if (PageTransitionCoreTypeIs(visit.transition, + ui::PAGE_TRANSITION_RELOAD)) { + continue; + } + ++total; + if (PageTransitionCoreTypeIs(visit.transition, + ui::PAGE_TRANSITION_TYPED)) { + ++typed_count; + } + } + + // We should have at least one typed visit. This can sometimes happen if + // the history DB has an inaccurate count for some reason (there's been + // bugs in the history code in the past which has left users in the wild + // with incorrect counts - http://crbug.com/84258). + DCHECK(typed_count > 0); + + if (typed_count > kMaxTypedUrlVisits) { + only_typed = true; + skip_count = typed_count - kMaxTypedUrlVisits; + } else if (total > kMaxTypedUrlVisits) { + skip_count = total - kMaxTypedUrlVisits; + } + } + + for (const auto& visit : visits) { + // Skip reload visits. + if (PageTransitionCoreTypeIs(visit.transition, ui::PAGE_TRANSITION_RELOAD)) + continue; + + // If we only have room for typed visits, then only add typed visits. + if (only_typed && !PageTransitionCoreTypeIs(visit.transition, + ui::PAGE_TRANSITION_TYPED)) { + continue; + } + + if (skip_count > 0) { + // We have too many entries to fit, so we need to skip the oldest ones. + // Only skip typed URLs if there are too many typed URLs to fit. + if (only_typed || !PageTransitionCoreTypeIs(visit.transition, + ui::PAGE_TRANSITION_TYPED)) { + --skip_count; + continue; + } + } + typed_url->add_visits(visit.visit_time.ToInternalValue()); + typed_url->add_visit_transitions(visit.transition); + } + DCHECK_EQ(skip_count, 0); + + CHECK_GT(typed_url->visits_size(), 0); + CHECK_LE(typed_url->visits_size(), kMaxTypedUrlVisits); + CHECK_EQ(typed_url->visits_size(), typed_url->visit_transitions_size()); + + return true; +} + +// static +TypedURLSyncBridge::MergeResult TypedURLSyncBridge::MergeUrls( + const TypedUrlSpecifics& sync_url, + const history::URLRow& url, + history::VisitVector* visits, + history::URLRow* new_url, + std::vector<history::VisitInfo>* new_visits) { + DCHECK(new_url); + DCHECK_EQ(sync_url.url(), url.url().spec()); + DCHECK_EQ(sync_url.url(), new_url->url().spec()); + DCHECK(visits->size()); + DCHECK_GT(sync_url.visits_size(), 0); + CHECK_EQ(sync_url.visits_size(), sync_url.visit_transitions_size()); + + // Convert these values only once. + base::string16 sync_url_title(base::UTF8ToUTF16(sync_url.title())); + base::Time sync_url_last_visit = base::Time::FromInternalValue( + sync_url.visits(sync_url.visits_size() - 1)); + + // This is a bitfield representing what we'll need to update with the output + // value. + MergeResult different = DIFF_NONE; + + // Check if the non-incremented values changed. + if ((sync_url_title != url.title()) || (sync_url.hidden() != url.hidden())) { + // Use the values from the most recent visit. + if (sync_url_last_visit >= url.last_visit()) { + new_url->set_title(sync_url_title); + new_url->set_hidden(sync_url.hidden()); + different |= DIFF_LOCAL_ROW_CHANGED; + } else { + new_url->set_title(url.title()); + new_url->set_hidden(url.hidden()); + different |= DIFF_UPDATE_NODE; + } + } else { + // No difference. + new_url->set_title(url.title()); + new_url->set_hidden(url.hidden()); + } + + size_t sync_url_num_visits = sync_url.visits_size(); + size_t history_num_visits = visits->size(); + size_t sync_url_visit_index = 0; + size_t history_visit_index = 0; + base::Time earliest_history_time = (*visits)[0].visit_time; + // Walk through the two sets of visits and figure out if any new visits were + // added on either side. + while (sync_url_visit_index < sync_url_num_visits || + history_visit_index < history_num_visits) { + // Time objects are initialized to "earliest possible time". + base::Time sync_url_time, history_time; + if (sync_url_visit_index < sync_url_num_visits) + sync_url_time = + base::Time::FromInternalValue(sync_url.visits(sync_url_visit_index)); + if (history_visit_index < history_num_visits) + history_time = (*visits)[history_visit_index].visit_time; + if (sync_url_visit_index >= sync_url_num_visits || + (history_visit_index < history_num_visits && + sync_url_time > history_time)) { + // We found a visit in the history DB that doesn't exist in the sync DB, + // so mark the sync_url as modified so the caller will update the sync + // node. + different |= DIFF_UPDATE_NODE; + ++history_visit_index; + } else if (history_visit_index >= history_num_visits || + sync_url_time < history_time) { + // Found a visit in the sync node that doesn't exist in the history DB, so + // add it to our list of new visits and set the appropriate flag so the + // caller will update the history DB. + // If the sync_url visit is older than any existing visit in the history + // DB, don't re-add it - this keeps us from resurrecting visits that were + // aged out locally. + // + // TODO(sync): This extra check should be unnecessary now that filtering + // expired visits is performed separately. Non-expired visits older than + // the earliest existing history visits should still be synced, so this + // check should be removed. + if (sync_url_time > earliest_history_time) { + different |= DIFF_LOCAL_VISITS_ADDED; + new_visits->push_back(history::VisitInfo( + sync_url_time, ui::PageTransitionFromInt(sync_url.visit_transitions( + sync_url_visit_index)))); + } + // This visit is added to visits below. + ++sync_url_visit_index; + } else { + // Same (already synced) entry found in both DBs - no need to do anything. + ++sync_url_visit_index; + ++history_visit_index; + } + } + + DCHECK(CheckVisitOrdering(*visits)); + if (different & DIFF_LOCAL_VISITS_ADDED) { + // If the server does not have the same visits as the local db, then the + // new visits from the server need to be added to the vector containing + // local visits. These visits will be passed to the server. + // Insert new visits into the appropriate place in the visits vector. + history::VisitVector::iterator visit_ix = visits->begin(); + for (std::vector<history::VisitInfo>::iterator new_visit = + new_visits->begin(); + new_visit != new_visits->end(); ++new_visit) { + while (visit_ix != visits->end() && + new_visit->first > visit_ix->visit_time) { + ++visit_ix; + } + visit_ix = + visits->insert(visit_ix, history::VisitRow(url.id(), new_visit->first, + 0, new_visit->second, 0)); + ++visit_ix; + } + } + DCHECK(CheckVisitOrdering(*visits)); + + new_url->set_last_visit(visits->back().visit_time); + return different; +} + +// static +void TypedURLSyncBridge::UpdateURLRowFromTypedUrlSpecifics( + const TypedUrlSpecifics& typed_url, + history::URLRow* new_url) { + DCHECK_GT(typed_url.visits_size(), 0); + CHECK_EQ(typed_url.visit_transitions_size(), typed_url.visits_size()); + if (!new_url->url().is_valid()) { + new_url->set_url(GURL(typed_url.url())); + } + new_url->set_title(base::UTF8ToUTF16(typed_url.title())); + new_url->set_hidden(typed_url.hidden()); + // Only provide the initial value for the last_visit field - after that, let + // the history code update the last_visit field on its own. + if (new_url->last_visit().is_null()) { + new_url->set_last_visit(base::Time::FromInternalValue( + typed_url.visits(typed_url.visits_size() - 1))); + } +} + +void TypedURLSyncBridge::LoadMetadata() { + if (!history_backend_ || !sync_metadata_database_) { + change_processor()->ReportError( + FROM_HERE, "Failed to load TypedURLSyncMetadataDatabase."); + return; + } + + auto batch = base::MakeUnique<syncer::MetadataBatch>(); + if (!sync_metadata_database_->GetAllSyncMetadata(batch.get())) { + change_processor()->ReportError( + FROM_HERE, + "Failed reading typed url metadata from TypedURLSyncMetadataDatabase."); + return; + } + change_processor()->ModelReadyToSync(std::move(batch)); +} + +void TypedURLSyncBridge::ClearErrorStats() { + num_db_accesses_ = 0; + num_db_errors_ = 0; +} + +void TypedURLSyncBridge::UpdateUrlFromServer( + const sync_pb::TypedUrlSpecifics& server_typed_url, + TypedURLMap* local_typed_urls, + URLVisitVectorMap* local_visit_vectors, + history::URLRows* new_synced_urls, + TypedURLVisitVector* new_synced_visits, + history::URLRows* updated_synced_urls) { + DCHECK(server_typed_url.visits_size() != 0); + DCHECK_EQ(server_typed_url.visits_size(), + server_typed_url.visit_transitions_size()); + + // Ignore empty urls. + if (server_typed_url.url().empty()) { + DVLOG(1) << "Ignoring empty URL in sync DB"; + return; + } + // Now, get rid of the expired visits. If there are no un-expired visits + // left, ignore this url - any local data should just replace it. + TypedUrlSpecifics sync_url = FilterExpiredVisits(server_typed_url); + if (sync_url.visits_size() == 0) { + DVLOG(1) << "Ignoring expired URL in sync DB: " << sync_url.url(); + return; + } + + // Check if local db already has the url from sync. + TypedURLMap::iterator it = local_typed_urls->find(GURL(sync_url.url())); + if (it == local_typed_urls->end()) { + // There are no matching typed urls from the local db, check for untyped + history::URLRow untyped_url(GURL(sync_url.url())); + + // The URL may still exist in the local db if it is an untyped url. + // An untyped url will transition to a typed url after receiving visits + // from sync, and sync should receive any visits already existing locally + // for the url, so the full list of visits is consistent. + bool is_existing_url = + history_backend_->GetURL(untyped_url.url(), &untyped_url); + if (is_existing_url) { + // Add a new entry to |local_typed_urls|, and set the iterator to it. + history::VisitVector untyped_visits; + if (!FixupURLAndGetVisits(&untyped_url, &untyped_visits)) { + return; + } + (*local_visit_vectors)[untyped_url.url()] = untyped_visits; + + // Store row info that will be used to update sync's visits. + (*local_typed_urls)[untyped_url.url()] = untyped_url; + + // Set iterator |it| to point to this entry. + it = local_typed_urls->find(untyped_url.url()); + DCHECK(it != local_typed_urls->end()); + // Continue with merge below. + } else { + // The url is new to the local history DB. + // Create new db entry for url. + history::URLRow new_url(GURL(sync_url.url())); + UpdateURLRowFromTypedUrlSpecifics(sync_url, &new_url); + new_synced_urls->push_back(new_url); + + // Add entries for url visits. + std::vector<history::VisitInfo> added_visits; + size_t visit_count = sync_url.visits_size(); + + for (size_t index = 0; index < visit_count; ++index) { + base::Time visit_time = + base::Time::FromInternalValue(sync_url.visits(index)); + ui::PageTransition transition = + ui::PageTransitionFromInt(sync_url.visit_transitions(index)); + added_visits.push_back(history::VisitInfo(visit_time, transition)); + } + new_synced_visits->push_back( + std::pair<GURL, std::vector<history::VisitInfo>>(new_url.url(), + added_visits)); + return; + } + } + + // Same URL exists in sync data and in history data - compare the + // entries to see if there's any difference. + history::VisitVector& visits = (*local_visit_vectors)[it->first]; + std::vector<history::VisitInfo> added_visits; + + // Empty URLs should be filtered out by ShouldIgnoreUrl() previously. + DCHECK(!it->second.url().spec().empty()); + + // Initialize fields in |new_url| to the same values as the fields in + // the existing URLRow in the history DB. This is needed because we + // overwrite the existing value in WriteToHistoryBackend(), but some of + // the values in that structure are not synced (like typed_count). + history::URLRow new_url(it->second); + + MergeResult difference = + MergeUrls(sync_url, it->second, &visits, &new_url, &added_visits); + + if (difference != DIFF_NONE) { + it->second = new_url; + if (difference & DIFF_UPDATE_NODE) { + // We don't want to resurrect old visits that have been aged out by + // other clients, so remove all visits that are older than the + // earliest existing visit in the sync node. + // + // TODO(sync): This logic should be unnecessary now that filtering of + // expired visits is performed separately. Non-expired visits older than + // the earliest existing sync visits should still be synced, so this + // logic should be removed. + if (sync_url.visits_size() > 0) { + base::Time earliest_visit = + base::Time::FromInternalValue(sync_url.visits(0)); + for (history::VisitVector::iterator i = visits.begin(); + i != visits.end() && i->visit_time < earliest_visit;) { + i = visits.erase(i); + } + // Should never be possible to delete all the items, since the + // visit vector contains newer local visits it will keep and/or the + // visits in typed_url.visits newer than older local visits. + DCHECK(visits.size() > 0); + } + DCHECK_EQ(new_url.last_visit().ToInternalValue(), + visits.back().visit_time.ToInternalValue()); + } + if (difference & DIFF_LOCAL_ROW_CHANGED) { + // Add entry to updated_synced_urls to update the local db. + DCHECK_EQ(it->second.id(), new_url.id()); + updated_synced_urls->push_back(new_url); + } + if (difference & DIFF_LOCAL_VISITS_ADDED) { + // Add entry with new visits to new_synced_visits to update the local db. + new_synced_visits->push_back( + std::pair<GURL, std::vector<history::VisitInfo>>(it->first, + added_visits)); + } + } else { + // No difference in urls, erase from map + local_typed_urls->erase(it); + } +} + +base::Optional<ModelError> TypedURLSyncBridge::WriteToHistoryBackend( + const history::URLRows* new_urls, + const history::URLRows* updated_urls, + const std::vector<GURL>* deleted_urls, + const TypedURLVisitVector* new_visits, + const history::VisitVector* deleted_visits) { + if (deleted_urls && !deleted_urls->empty()) + history_backend_->DeleteURLs(*deleted_urls); + + if (new_urls) { + history_backend_->AddPagesWithDetails(*new_urls, history::SOURCE_SYNCED); + } + + if (updated_urls) { + ++num_db_accesses_; + // This is an existing entry in the URL database. We don't verify the + // visit_count or typed_count values here, because either one (or both) + // could be zero in the case of bookmarks, or in the case of a URL + // transitioning from non-typed to typed as a result of this sync. + // In the field we sometimes run into errors on specific URLs. It's OK + // to just continue on (we can try writing again on the next model + // association). + size_t num_successful_updates = history_backend_->UpdateURLs(*updated_urls); + num_db_errors_ += updated_urls->size() - num_successful_updates; + } + + if (new_visits) { + for (const auto& visits : *new_visits) { + // If there are no visits to add, just skip this. + if (visits.second.empty()) + continue; + ++num_db_accesses_; + if (!history_backend_->AddVisits(visits.first, visits.second, + history::SOURCE_SYNCED)) { + ++num_db_errors_; + return ModelError(FROM_HERE, "Could not add visits to HistoryBackend."); + } + } + } + + if (deleted_visits) { + ++num_db_accesses_; + if (!history_backend_->RemoveVisits(*deleted_visits)) { + ++num_db_errors_; + return ModelError(FROM_HERE, + "Could not remove visits from HistoryBackend."); + // This is bad news, since it means we may end up resurrecting history + // entries on the next reload. It's unavoidable so we'll just keep on + // syncing. + } + } + + return {}; +} + +TypedUrlSpecifics TypedURLSyncBridge::FilterExpiredVisits( + const TypedUrlSpecifics& source) { + // Make a copy of the source, then regenerate the visits. + TypedUrlSpecifics specifics(source); + specifics.clear_visits(); + specifics.clear_visit_transitions(); + for (int i = 0; i < source.visits_size(); ++i) { + base::Time time = base::Time::FromInternalValue(source.visits(i)); + if (!history_backend_->IsExpiredVisitTime(time)) { + specifics.add_visits(source.visits(i)); + specifics.add_visit_transitions(source.visit_transitions(i)); + } + } + DCHECK(specifics.visits_size() == specifics.visit_transitions_size()); + return specifics; +} + +bool TypedURLSyncBridge::ShouldIgnoreUrl(const GURL& url) { + // Ignore empty URLs. Not sure how this can happen (maybe import from other + // busted browsers, or misuse of the history API, or just plain bugs) but we + // can't deal with them. + if (url.spec().empty()) + return true; + + // Ignore local file URLs. + if (url.SchemeIsFile()) + return true; + + // Ignore localhost URLs. + if (net::IsLocalhost(url.host_piece())) + return true; + + // Ignore username and password, sonce history backend will remove user name + // and password in URLDatabase::GURLToDatabaseURL and send username/password + // removed url to sync later. + if (url.has_username() || url.has_password()) + return true; + + return false; +} + +bool TypedURLSyncBridge::ShouldIgnoreVisits( + const history::VisitVector& visits) { + // We ignore URLs that were imported, but have never been visited by + // chromium. + static const int kFirstImportedSource = history::SOURCE_FIREFOX_IMPORTED; + history::VisitSourceMap map; + if (!history_backend_->GetVisitsSource(visits, &map)) + return false; // If we can't read the visit, assume it's not imported. + + // Walk the list of visits and look for a non-imported item. + for (const auto& visit : visits) { + if (map.count(visit.visit_id) == 0 || + map[visit.visit_id] < kFirstImportedSource) { + return false; + } + } + // We only saw imported visits, so tell the caller to ignore them. + return true; +} + +bool TypedURLSyncBridge::FixupURLAndGetVisits(URLRow* url, + VisitVector* visits) { + ++num_db_accesses_; + if (!history_backend_->GetMostRecentVisitsForURL(url->id(), kMaxVisitsToFetch, + visits)) { + ++num_db_errors_; + // Couldn't load the visits for this URL due to some kind of DB error. + // Don't bother writing this URL to the history DB (if we ignore the + // error and continue, we might end up duplicating existing visits). + DLOG(ERROR) << "Could not load visits for url: " << url->url(); + return false; + } + + // Sometimes (due to a bug elsewhere in the history or sync code, or due to + // a crash between adding a URL to the history database and updating the + // visit DB) the visit vector for a URL can be empty. If this happens, just + // create a new visit whose timestamp is the same as the last_visit time. + // This is a workaround for http://crbug.com/84258. + if (visits->empty()) { + DVLOG(1) << "Found empty visits for URL: " << url->url(); + if (url->last_visit().is_null()) { + // If modified URL is bookmarked, history backend treats it as modified + // even if all its visits are deleted. Return false to stop further + // processing because sync expects valid visit time for modified entry. + return false; + } + + VisitRow visit(url->id(), url->last_visit(), 0, ui::PAGE_TRANSITION_TYPED, + 0); + visits->push_back(visit); + } + + // GetMostRecentVisitsForURL() returns the data in the opposite order that + // we need it, so reverse it. + std::reverse(visits->begin(), visits->end()); + + // Sometimes, the last_visit field in the URL doesn't match the timestamp of + // the last visit in our visit array (they come from different tables, so + // crashes/bugs can cause them to mismatch), so just set it here. + url->set_last_visit(visits->back().visit_time); + DCHECK(CheckVisitOrdering(*visits)); + + // Removes all visits that are older than the current expiration time. Visits + // are in ascending order now, so we can check from beginning to check how + // many expired visits. + size_t num_expired_visits = 0; + for (auto& visit : *visits) { + base::Time time = visit.visit_time; + if (history_backend_->IsExpiredVisitTime(time)) { + ++num_expired_visits; + } else { + break; + } + } + if (num_expired_visits != 0) { + if (num_expired_visits == visits->size()) { + DVLOG(1) << "All visits are expired for url: " << url->url(); + visits->clear(); + return false; + } + visits->erase(visits->begin(), visits->begin() + num_expired_visits); + } + DCHECK(CheckVisitOrdering(*visits)); + + return true; +} + +std::unique_ptr<EntityData> TypedURLSyncBridge::CreateEntityData( + const URLRow& row, + const VisitVector& visits) { + auto entity_data = base::MakeUnique<EntityData>(); + TypedUrlSpecifics* specifics = entity_data->specifics.mutable_typed_url(); + + if (!WriteToTypedUrlSpecifics(row, visits, specifics)) { + // Cannot write to specifics, ex. no TYPED visits. + return base::MakeUnique<EntityData>(); + } + entity_data->non_unique_name = row.url().spec(); + + return entity_data; +} + +bool TypedURLSyncBridge::GetValidURLsAndVisits(URLVisitVectorMap* url_to_visit, + TypedURLMap* url_to_urlrow) { + DCHECK(url_to_visit); + DCHECK(url_to_urlrow); + + history::URLRows local_typed_urls; + ++num_db_accesses_; + if (!history_backend_->GetAllTypedURLs(&local_typed_urls)) { + ++num_db_errors_; + return false; + } + for (history::URLRow& url : local_typed_urls) { + DCHECK_EQ(0U, url_to_visit->count(url.url())); + if (!FixupURLAndGetVisits(&url, &((*url_to_visit)[url.url()])) || + ShouldIgnoreUrl(url.url()) || + ShouldIgnoreVisits((*url_to_visit)[url.url()])) { + // Ignore this URL if we couldn't load the visits or if there's some + // other problem with it (it was empty, or imported and never visited). + } else { + // Add url to url_to_urlrow. + (*url_to_urlrow)[url.url()] = url; + } + } + return true; +} + +std::string TypedURLSyncBridge::GetStorageKeyInternal(const std::string& url) { + DCHECK(history_backend_); + + URLRow existing_url; + ++num_db_accesses_; + bool is_existing_url = history_backend_->GetURL(GURL(url), &existing_url); + + if (!is_existing_url) { + // The typed url did not save to local history database, so return empty + // string. + return std::string(); + } + + return GetStorageKeyFromURLRow(existing_url); +} + } // namespace history diff --git a/chromium/components/history/core/browser/typed_url_sync_bridge.h b/chromium/components/history/core/browser/typed_url_sync_bridge.h index 59eb0756af9..13a8bbd9bc5 100644 --- a/chromium/components/history/core/browser/typed_url_sync_bridge.h +++ b/chromium/components/history/core/browser/typed_url_sync_bridge.h @@ -5,15 +5,13 @@ #ifndef COMPONENTS_HISTORY_CORE_BROWSER_TYPED_URL_SYNC_BRIDGE_H_ #define COMPONENTS_HISTORY_CORE_BROWSER_TYPED_URL_SYNC_BRIDGE_H_ +#include "base/scoped_observer.h" #include "components/history/core/browser/history_backend_observer.h" +#include "components/history/core/browser/typed_url_sync_metadata_database.h" #include "components/sync/model/metadata_change_list.h" #include "components/sync/model/model_type_sync_bridge.h" #include "components/sync/model/sync_error.h" -namespace syncer { -class SyncMetadataStore; -} - namespace history { class TypedURLSyncBridge : public syncer::ModelTypeSyncBridge, @@ -22,7 +20,7 @@ class TypedURLSyncBridge : public syncer::ModelTypeSyncBridge, // |sync_metadata_store| is owned by |history_backend|, and must outlive // TypedURLSyncBridge. TypedURLSyncBridge(HistoryBackend* history_backend, - syncer::SyncMetadataStore* sync_metadata_store, + TypedURLSyncMetadataDatabase* sync_metadata_store, const ChangeProcessorFactory& change_processor_factory); ~TypedURLSyncBridge() override; @@ -31,7 +29,7 @@ class TypedURLSyncBridge : public syncer::ModelTypeSyncBridge, override; base::Optional<syncer::ModelError> MergeSyncData( std::unique_ptr<syncer::MetadataChangeList> metadata_change_list, - syncer::EntityDataMap entity_data_map) override; + syncer::EntityChangeList entity_data) override; base::Optional<syncer::ModelError> ApplySyncChanges( std::unique_ptr<syncer::MetadataChangeList> metadata_change_list, syncer::EntityChangeList entity_changes) override; @@ -39,6 +37,7 @@ class TypedURLSyncBridge : public syncer::ModelTypeSyncBridge, void GetAllData(DataCallback callback) override; std::string GetClientTag(const syncer::EntityData& entity_data) override; std::string GetStorageKey(const syncer::EntityData& entity_data) override; + bool SupportsGetStorageKey() const override; // history::HistoryBackendObserver: void OnURLVisited(history::HistoryBackend* history_backend, @@ -54,19 +53,148 @@ class TypedURLSyncBridge : public syncer::ModelTypeSyncBridge, const history::URLRows& deleted_rows, const std::set<GURL>& favicon_urls) override; + // Must be called after creation and before any operations. + void Init(); + + // Returns the percentage of DB accesses that have resulted in an error. + int GetErrorPercentage() const; + + // Return true if this function successfully converts the passed URL + // information to a TypedUrlSpecifics structure for writing to the sync DB. + static bool WriteToTypedUrlSpecifics(const URLRow& url, + const VisitVector& visits, + sync_pb::TypedUrlSpecifics* specifics) + WARN_UNUSED_RESULT; + private: + friend class TypedURLSyncBridgeTest; + + typedef std::vector<std::pair<GURL, std::vector<VisitInfo>>> + TypedURLVisitVector; + + // This is a helper map used only in Merge functions. + typedef std::map<GURL, URLRow> TypedURLMap; + + // This is a helper map used to associate visit vectors from the history db + // to the typed urls in the above map |TypedURLMap|. + typedef std::map<GURL, VisitVector> URLVisitVectorMap; + + // Bitfield returned from MergeUrls to specify the result of a merge. + typedef uint32_t MergeResult; + static const MergeResult DIFF_NONE = 0; + static const MergeResult DIFF_UPDATE_NODE = 1 << 0; + static const MergeResult DIFF_LOCAL_ROW_CHANGED = 1 << 1; + static const MergeResult DIFF_LOCAL_VISITS_ADDED = 1 << 2; + + // Merges the URL information in |typed_url| with the URL information from the + // history database in |url| and |visits|, and returns a bitmask with the + // results of the merge: + // DIFF_UPDATE_NODE - changes have been made to |new_url| and |visits| which + // should be persisted to the sync node. + // DIFF_LOCAL_ROW_CHANGED - The history data in |new_url| should be persisted + // to the history DB. + // DIFF_LOCAL_VISITS_ADDED - |new_visits| contains a list of visits that + // should be written to the history DB for this URL. Deletions are not + // written to the DB - each client is left to age out visits on their own. + static MergeResult MergeUrls(const sync_pb::TypedUrlSpecifics& typed_url, + const history::URLRow& url, + history::VisitVector* visits, + history::URLRow* new_url, + std::vector<history::VisitInfo>* new_visits); + + // Fills |new_url| with formatted data from |typed_url|. + static void UpdateURLRowFromTypedUrlSpecifics( + const sync_pb::TypedUrlSpecifics& typed_url, + history::URLRow* new_url); + + // Synchronously load sync metadata from the TypedURLSyncMetadataDatabase and + // pass it to the processor so that it can start tracking changes. + void LoadMetadata(); + + // Helper function that clears our error counters (used to reset stats after + // merge so we can track merge errors separately). + void ClearErrorStats(); + + // Compares |server_typed_url| from the server against local history to decide + // how to merge any existing data, and updates appropriate data containers to + // write to server and backend. + void UpdateUrlFromServer(const sync_pb::TypedUrlSpecifics& server_typed_url, + TypedURLMap* local_typed_urls, + URLVisitVectorMap* visit_vectors, + history::URLRows* new_synced_urls, + TypedURLVisitVector* new_synced_visits, + history::URLRows* updated_synced_urls); + + // Writes new typed url data from sync server to history backend. + base::Optional<syncer::ModelError> WriteToHistoryBackend( + const history::URLRows* new_urls, + const history::URLRows* updated_urls, + const std::vector<GURL>* deleted_urls, + const TypedURLVisitVector* new_visits, + const history::VisitVector* deleted_visits); + + // Given a TypedUrlSpecifics object, removes all visits that are older than + // the current expiration time. Note that this can result in having no visits + // at all. + sync_pb::TypedUrlSpecifics FilterExpiredVisits( + const sync_pb::TypedUrlSpecifics& specifics); + + // Helper function that determines if we should ignore a URL for the purposes + // of sync, because it contains invalid data. + bool ShouldIgnoreUrl(const GURL& url); + + // Helper function that determines if we should ignore a URL for the purposes + // of sync, based on the visits the URL had. + bool ShouldIgnoreVisits(const history::VisitVector& visits); + + // Fetches visits from the history DB corresponding to the passed URL. This + // function compensates for the fact that the history DB has rather poor data + // integrity (duplicate visits, visit timestamps that don't match the + // last_visit timestamp, huge data sets that exhaust memory when fetched, + // expired visits that are not deleted by |ExpireHistoryBackend|, etc) by + // modifying the passed |url| object and |visits| vector. The order of + // |visits| will be from the oldest to the newest order. + // Returns false in two cases. + // 1. we could not fetch the visits for the passed URL, DB error. + // 2. No visits for the passed url, or all the visits are expired. + bool FixupURLAndGetVisits(URLRow* url, VisitVector* visits); + + // Create an EntityData by URL |row| and its visits |visits|. + std::unique_ptr<syncer::EntityData> CreateEntityData( + const URLRow& row, + const VisitVector& visits); + + // Get all the typed urls and visits from the history db, after filtering + // them, put them into |url_to_visit| and |url_to_urlrow|. + // Return false if cannot get urls from HistoryBackend. + bool GetValidURLsAndVisits(URLVisitVectorMap* url_to_visit, + TypedURLMap* url_to_urlrow); + + // Get URLID from HistoryBackend, and return URLID as storage key. + std::string GetStorageKeyInternal(const std::string& url); + // A non-owning pointer to the backend, which we're syncing local changes from // and sync changes to. HistoryBackend* const history_backend_; // A non-owning pointer to the database, which is for storing typed urls sync // metadata and state. - syncer::SyncMetadataStore* const sync_metadata_store_; + TypedURLSyncMetadataDatabase* const sync_metadata_database_; + + // Statistics for the purposes of tracking the percentage of DB accesses that + // fail for each client via UMA. + int num_db_accesses_; + int num_db_errors_; // Since HistoryBackend use SequencedTaskRunner, so should use SequenceChecker // here. base::SequenceChecker sequence_checker_; + // Tracks observed history backend, for receiving updates from history + // backend. + ScopedObserver<history::HistoryBackend, history::HistoryBackendObserver> + history_backend_observer_; + DISALLOW_COPY_AND_ASSIGN(TypedURLSyncBridge); }; diff --git a/chromium/components/history/core/browser/typed_url_sync_bridge_unittest.cc b/chromium/components/history/core/browser/typed_url_sync_bridge_unittest.cc new file mode 100644 index 00000000000..4a36d3aca53 --- /dev/null +++ b/chromium/components/history/core/browser/typed_url_sync_bridge_unittest.cc @@ -0,0 +1,409 @@ +// Copyright 2017 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "components/history/core/browser/typed_url_sync_bridge.h" + +#include "base/files/scoped_temp_dir.h" +#include "base/message_loop/message_loop.h" +#include "base/strings/utf_string_conversions.h" +#include "base/threading/thread_task_runner_handle.h" +#include "components/history/core/browser/history_backend.h" +#include "components/history/core/browser/history_backend_client.h" +#include "components/history/core/browser/history_database_params.h" +#include "components/history/core/browser/in_memory_history_backend.h" +#include "components/history/core/test/test_history_database.h" +#include "components/sync/model/data_batch.h" +#include "components/sync/model/recording_model_type_change_processor.h" +#include "components/sync/model/sync_metadata_store.h" +#include "testing/gtest/include/gtest/gtest.h" + +using sync_pb::TypedUrlSpecifics; +using syncer::DataBatch; +using syncer::EntityChange; +using syncer::EntityChangeList; +using syncer::EntityData; +using syncer::EntityDataPtr; +using syncer::KeyAndData; +using syncer::MetadataBatch; +using syncer::RecordingModelTypeChangeProcessor; + +namespace history { + +namespace { + +// Visits with this timestamp are treated as expired. +const int kExpiredVisit = -1; + +// Helper constants for tests. +const char kTitle[] = "pie"; +const char kTitle2[] = "cookie"; +const char kURL[] = "http://pie.com/"; +const char kURL2[] = "http://cookie.com/"; + +// Create a new row object and the typed visit çorresponding with the time at +// |last_visit| in the |visits| vector. +URLRow MakeTypedUrlRow(const std::string& url, + const std::string& title, + int typed_count, + int64_t last_visit, + bool hidden, + VisitVector* visits) { + // Give each URL a unique ID, to mimic the behavior of the real database. + GURL gurl(url); + URLRow history_url(gurl); + history_url.set_title(base::UTF8ToUTF16(title)); + history_url.set_typed_count(typed_count); + history_url.set_hidden(hidden); + + base::Time last_visit_time = base::Time::FromInternalValue(last_visit); + history_url.set_last_visit(last_visit_time); + + if (typed_count > 0) { + // Add a typed visit for time |last_visit|. + visits->push_back(VisitRow(history_url.id(), last_visit_time, 0, + ui::PAGE_TRANSITION_TYPED, 0)); + } else { + // Add a non-typed visit for time |last_visit|. + visits->push_back(VisitRow(history_url.id(), last_visit_time, 0, + ui::PAGE_TRANSITION_RELOAD, 0)); + } + + history_url.set_visit_count(visits->size()); + return history_url; +} + +void VerifyEqual(const TypedUrlSpecifics& s1, const TypedUrlSpecifics& s2) { + // Instead of just comparing serialized strings, manually check fields to show + // differences on failure. + EXPECT_EQ(s1.url(), s2.url()); + EXPECT_EQ(s1.title(), s2.title()); + EXPECT_EQ(s1.hidden(), s2.hidden()); + EXPECT_EQ(s1.visits_size(), s2.visits_size()); + EXPECT_EQ(s1.visit_transitions_size(), s2.visit_transitions_size()); + EXPECT_EQ(s1.visits_size(), s1.visit_transitions_size()); + int size = s1.visits_size(); + for (int i = 0; i < size; ++i) { + EXPECT_EQ(s1.visits(i), s2.visits(i)) << "visits differ at index " << i; + EXPECT_EQ(s1.visit_transitions(i), s2.visit_transitions(i)) + << "visit_transitions differ at index " << i; + } +} + +void VerifyDataBatch(std::map<std::string, TypedUrlSpecifics> expected, + std::unique_ptr<DataBatch> batch) { + while (batch->HasNext()) { + const KeyAndData& pair = batch->Next(); + auto iter = expected.find(pair.first); + ASSERT_NE(iter, expected.end()); + VerifyEqual(iter->second, pair.second->specifics.typed_url()); + // Removing allows us to verify we don't see the same item multiple times, + // and that we saw everything we expected. + expected.erase(iter); + } + EXPECT_TRUE(expected.empty()); +} + +class TestHistoryBackendDelegate : public HistoryBackend::Delegate { + public: + TestHistoryBackendDelegate() {} + + void NotifyProfileError(sql::InitStatus init_status, + const std::string& diagnostics) override {} + void SetInMemoryBackend( + std::unique_ptr<InMemoryHistoryBackend> backend) override {} + void NotifyFaviconsChanged(const std::set<GURL>& page_urls, + const GURL& icon_url) override {} + void NotifyURLVisited(ui::PageTransition transition, + const URLRow& row, + const RedirectList& redirects, + base::Time visit_time) override {} + void NotifyURLsModified(const URLRows& changed_urls) override {} + void NotifyURLsDeleted(bool all_history, + bool expired, + const URLRows& deleted_rows, + const std::set<GURL>& favicon_urls) override {} + void NotifyKeywordSearchTermUpdated(const URLRow& row, + KeywordID keyword_id, + const base::string16& term) override {} + void NotifyKeywordSearchTermDeleted(URLID url_id) override {} + void DBLoaded() override {} + + private: + DISALLOW_COPY_AND_ASSIGN(TestHistoryBackendDelegate); +}; + +class TestHistoryBackend : public HistoryBackend { + public: + TestHistoryBackend() + : HistoryBackend(new TestHistoryBackendDelegate(), + nullptr, + base::ThreadTaskRunnerHandle::Get()) {} + + bool IsExpiredVisitTime(const base::Time& time) override { + return time.ToInternalValue() == kExpiredVisit; + } + + URLID GetIdByUrl(const GURL& gurl) { + return db()->GetRowForURL(gurl, nullptr); + } + + void SetVisitsForUrl(URLRow& new_url, const VisitVector visits) { + std::vector<history::VisitInfo> added_visits; + URLRows new_urls; + DeleteURL(new_url.url()); + for (const auto& visit : visits) { + added_visits.push_back( + history::VisitInfo(visit.visit_time, visit.transition)); + } + new_urls.push_back(new_url); + AddPagesWithDetails(new_urls, history::SOURCE_SYNCED); + AddVisits(new_url.url(), added_visits, history::SOURCE_SYNCED); + new_url.set_id(GetIdByUrl(new_url.url())); + } + + private: + ~TestHistoryBackend() override {} +}; + +} // namespace + +class TypedURLSyncBridgeTest : public testing::Test { + public: + TypedURLSyncBridgeTest() : typed_url_sync_bridge_(NULL) {} + ~TypedURLSyncBridgeTest() override {} + + void SetUp() override { + fake_history_backend_ = new TestHistoryBackend(); + ASSERT_TRUE(test_dir_.CreateUniqueTempDir()); + fake_history_backend_->Init( + false, TestHistoryDatabaseParamsForPath(test_dir_.GetPath())); + std::unique_ptr<TypedURLSyncBridge> bridge = + base::MakeUnique<TypedURLSyncBridge>( + fake_history_backend_.get(), fake_history_backend_->db(), + RecordingModelTypeChangeProcessor::FactoryForBridgeTest(&processor_, + false)); + typed_url_sync_bridge_ = bridge.get(); + typed_url_sync_bridge_->Init(); + typed_url_sync_bridge_->history_backend_observer_.RemoveAll(); + fake_history_backend_->SetTypedURLSyncBridgeForTest(std::move(bridge)); + } + + void TearDown() override { fake_history_backend_->Closing(); } + + // Starts sync for |typed_url_sync_bridge_| with |initial_data| as the + // initial sync data. + void StartSyncing(const std::vector<TypedUrlSpecifics>& specifics) { + // Set change processor. + const auto error = + bridge()->MergeSyncData(bridge()->CreateMetadataChangeList(), + CreateEntityChangeList(specifics)); + + EXPECT_FALSE(error); + } + + // Fills |specifics| with the sync data for |url| and |visits|. + static bool WriteToTypedUrlSpecifics(const URLRow& url, + const VisitVector& visits, + TypedUrlSpecifics* specifics) { + return TypedURLSyncBridge::WriteToTypedUrlSpecifics(url, visits, specifics); + } + + std::string GetStorageKey(const TypedUrlSpecifics& specifics) { + std::string key = bridge()->GetStorageKeyInternal(specifics.url()); + return key; + } + + EntityDataPtr SpecificsToEntity(const TypedUrlSpecifics& specifics) { + EntityData data; + data.client_tag_hash = "ignored"; + *data.specifics.mutable_typed_url() = specifics; + return data.PassToPtr(); + } + + EntityChangeList CreateEntityChangeList( + const std::vector<TypedUrlSpecifics>& specifics_vector) { + EntityChangeList entity_change_list; + for (const auto& specifics : specifics_vector) { + entity_change_list.push_back(EntityChange::CreateAdd( + GetStorageKey(specifics), SpecificsToEntity(specifics))); + } + return entity_change_list; + } + + std::map<std::string, TypedUrlSpecifics> ExpectedMap( + const std::vector<TypedUrlSpecifics>& specifics_vector) { + std::map<std::string, TypedUrlSpecifics> map; + for (const auto& specifics : specifics_vector) { + map[GetStorageKey(specifics)] = specifics; + } + return map; + } + + void VerifyLocalHistoryData(const std::vector<TypedUrlSpecifics>& expected) { + bridge()->GetAllData(base::Bind(&VerifyDataBatch, ExpectedMap(expected))); + } + + TypedURLSyncBridge* bridge() { return typed_url_sync_bridge_; } + + TypedURLSyncMetadataDatabase* metadata_store() { + return bridge()->sync_metadata_database_; + } + + const RecordingModelTypeChangeProcessor& processor() { return *processor_; } + + protected: + base::MessageLoop message_loop_; + base::ScopedTempDir test_dir_; + scoped_refptr<TestHistoryBackend> fake_history_backend_; + TypedURLSyncBridge* typed_url_sync_bridge_; + // A non-owning pointer to the processor given to the bridge. Will be null + // before being given to the bridge, to make ownership easier. + RecordingModelTypeChangeProcessor* processor_ = nullptr; +}; + +// Add two typed urls locally and verify bridge can get them from GetAllData. +TEST_F(TypedURLSyncBridgeTest, GetAllData) { + // Add two urls to backend. + 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); + + // Create the same data in sync. + TypedUrlSpecifics typed_url1, typed_url2; + WriteToTypedUrlSpecifics(row1, visits1, &typed_url1); + WriteToTypedUrlSpecifics(row2, visits2, &typed_url2); + + // Check that the local cache is still correct. + VerifyLocalHistoryData({typed_url1, typed_url2}); +} + +// Add a typed url locally and one to sync with the same data. Starting sync +// should result in no changes. +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); + + // Create the same data in sync. + sync_pb::EntitySpecifics entity_specifics; + sync_pb::TypedUrlSpecifics* typed_url = entity_specifics.mutable_typed_url(); + WriteToTypedUrlSpecifics(row, visits, typed_url); + + StartSyncing({*typed_url}); + EXPECT_TRUE(processor().put_multimap().empty()); + + // Check that the local cache was is still correct. + VerifyLocalHistoryData({*typed_url}); +} + +// Add a corupted typed url locally, has typed url count 1, but no real typed +// url visit. Starting sync should not pick up this url. +TEST_F(TypedURLSyncBridgeTest, MergeUrlNoTypedUrl) { + // Add a url to backend. + VisitVector visits; + URLRow row = MakeTypedUrlRow(kURL, kTitle, 0, 3, false, &visits); + + // Mark typed_count to 1 even when there is no typed url visit. + row.set_typed_count(1); + fake_history_backend_->SetVisitsForUrl(row, visits); + + StartSyncing(std::vector<TypedUrlSpecifics>()); + EXPECT_TRUE(processor().put_multimap().empty()); + + MetadataBatch metadata_batch; + metadata_store()->GetAllSyncMetadata(&metadata_batch); + EXPECT_EQ(0u, metadata_batch.TakeAllMetadata().size()); +} + +// Starting sync with no sync data should just push the local url to sync. +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); + + StartSyncing(std::vector<TypedUrlSpecifics>()); + + // Check that the local cache was is still correct. + sync_pb::EntitySpecifics entity_specifics; + sync_pb::TypedUrlSpecifics* typed_url = entity_specifics.mutable_typed_url(); + WriteToTypedUrlSpecifics(row, visits, typed_url); + VerifyLocalHistoryData({*typed_url}); + + // Check that the server was updated correctly. + ASSERT_EQ(1U, processor().put_multimap().size()); + auto recorded_specifics_iterator = + processor().put_multimap().find(GetStorageKey(*typed_url)); + EXPECT_NE(processor().put_multimap().end(), recorded_specifics_iterator); + TypedUrlSpecifics recorded_specifics = + recorded_specifics_iterator->second->specifics.typed_url(); + + ASSERT_EQ(1, recorded_specifics.visits_size()); + EXPECT_EQ(3, recorded_specifics.visits(0)); + ASSERT_EQ(1, recorded_specifics.visit_transitions_size()); + EXPECT_EQ(static_cast<const int>(visits[0].transition), + recorded_specifics.visit_transitions(0)); +} + +// Starting sync with no local data should just push the synced url into the +// backend. +TEST_F(TypedURLSyncBridgeTest, MergeUrlEmptyLocal) { + // Create the sync data. + VisitVector visits; + URLRow row = MakeTypedUrlRow(kURL, kTitle, 1, 3, false, &visits); + sync_pb::EntitySpecifics entity_specifics; + sync_pb::TypedUrlSpecifics* typed_url = entity_specifics.mutable_typed_url(); + WriteToTypedUrlSpecifics(row, visits, typed_url); + + StartSyncing({*typed_url}); + EXPECT_EQ(0u, processor().put_multimap().size()); + + // Check that the backend was updated correctly. + VisitVector all_visits; + base::Time server_time = base::Time::FromInternalValue(3); + URLID url_id = fake_history_backend_->GetIdByUrl(GURL(kURL)); + ASSERT_NE(0, url_id); + fake_history_backend_->GetVisitsForURL(url_id, &all_visits); + ASSERT_EQ(1U, all_visits.size()); + EXPECT_EQ(server_time, all_visits[0].visit_time); + EXPECT_TRUE(ui::PageTransitionTypeIncludingQualifiersIs( + all_visits[0].transition, visits[0].transition)); +} + +// Starting sync with both local and sync have same typed URL, but different +// visit. After merge, both local and sync should have two same visits. +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); + + // Create the sync data. + VisitVector visits2; + URLRow row2 = MakeTypedUrlRow(kURL, kTitle, 1, 4, false, &visits2); + sync_pb::EntitySpecifics entity_specifics; + sync_pb::TypedUrlSpecifics* typed_url = entity_specifics.mutable_typed_url(); + WriteToTypedUrlSpecifics(row2, visits2, typed_url); + + StartSyncing({*typed_url}); + EXPECT_EQ(1u, processor().put_multimap().size()); + + // Check that the backend was updated correctly. + VisitVector all_visits; + URLID url_id = fake_history_backend_->GetIdByUrl(GURL(kURL)); + 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_TRUE(ui::PageTransitionTypeIncludingQualifiersIs( + all_visits[0].transition, visits1[0].transition)); + EXPECT_TRUE(ui::PageTransitionTypeIncludingQualifiersIs( + all_visits[1].transition, visits2[0].transition)); +} + +} // namespace history diff --git a/chromium/components/history/core/browser/typed_url_sync_metadata_database.cc b/chromium/components/history/core/browser/typed_url_sync_metadata_database.cc index f0b2ce6ddf3..81e9da0a8e9 100644 --- a/chromium/components/history/core/browser/typed_url_sync_metadata_database.cc +++ b/chromium/components/history/core/browser/typed_url_sync_metadata_database.cc @@ -4,8 +4,9 @@ #include "components/history/core/browser/typed_url_sync_metadata_database.h" +#include "base/big_endian.h" #include "base/logging.h" -#include "base/strings/string_number_conversions.h" +#include "components/history/core/browser/url_row.h" #include "sql/statement.h" namespace history { @@ -55,9 +56,11 @@ bool TypedURLSyncMetadataDatabase::UpdateSyncMetadata( << "Only the TYPED_URLS model type is supported"; int64_t storage_key_int = 0; - if (!base::StringToInt64(storage_key, &storage_key_int)) { - return false; - } + DCHECK_EQ(storage_key.size(), sizeof(storage_key_int)); + base::ReadBigEndian(storage_key.data(), &storage_key_int); + // Make sure storage_key_int is set. + DCHECK_NE(storage_key_int, 0); + sql::Statement s(GetDB().GetUniqueStatement( "INSERT OR REPLACE INTO typed_url_sync_metadata " "(storage_key, value) VALUES(?, ?)")); @@ -74,9 +77,11 @@ bool TypedURLSyncMetadataDatabase::ClearSyncMetadata( << "Only the TYPED_URLS model type is supported"; int64_t storage_key_int = 0; - if (!base::StringToInt64(storage_key, &storage_key_int)) { - return false; - } + DCHECK_EQ(storage_key.size(), sizeof(storage_key_int)); + base::ReadBigEndian(storage_key.data(), &storage_key_int); + // Make sure storage_key_int is set. + DCHECK_NE(storage_key_int, 0); + sql::Statement s(GetDB().GetUniqueStatement( "DELETE FROM typed_url_sync_metadata WHERE storage_key=?")); s.BindInt64(0, storage_key_int); @@ -122,7 +127,8 @@ bool TypedURLSyncMetadataDatabase::GetAllSyncEntityMetadata( "SELECT storage_key, value FROM typed_url_sync_metadata")); while (s.Step()) { - std::string storage_key = base::Int64ToString(s.ColumnInt64(0)); + std::string storage_key(sizeof(URLID), 0); + base::WriteBigEndian<URLID>(&storage_key[0], s.ColumnInt64(0)); std::string serialized_metadata = s.ColumnString(1); sync_pb::EntityMetadata entity_metadata; if (entity_metadata.ParseFromString(serialized_metadata)) { diff --git a/chromium/components/history/core/browser/typed_url_sync_metadata_database_unittest.cc b/chromium/components/history/core/browser/typed_url_sync_metadata_database_unittest.cc index c37bf3eeb7e..ec0f22f11da 100644 --- a/chromium/components/history/core/browser/typed_url_sync_metadata_database_unittest.cc +++ b/chromium/components/history/core/browser/typed_url_sync_metadata_database_unittest.cc @@ -4,8 +4,10 @@ #include "components/history/core/browser/typed_url_sync_metadata_database.h" +#include "base/big_endian.h" #include "base/files/file_path.h" #include "base/files/scoped_temp_dir.h" +#include "components/history/core/browser/url_row.h" #include "components/sync/protocol/model_type_state.pb.h" #include "sql/statement.h" #include "testing/gtest/include/gtest/gtest.h" @@ -17,6 +19,16 @@ using syncer::MetadataBatch; namespace history { +namespace { + +std::string IntToStorageKey(int i) { + std::string storage_key(sizeof(URLID), 0); + base::WriteBigEndian<URLID>(&storage_key[0], i); + return storage_key; +} + +} // namespace + class TypedURLSyncMetadataDatabaseTest : public testing::Test, public TypedURLSyncMetadataDatabase { public: @@ -60,8 +72,8 @@ TEST_F(TypedURLSyncMetadataDatabaseTest, TypedURLNoMetadata) { TEST_F(TypedURLSyncMetadataDatabaseTest, TypedURLGetAllSyncMetadata) { EntityMetadata metadata; - std::string storage_key = "1"; - std::string storage_key2 = "2"; + std::string storage_key = IntToStorageKey(1); + std::string storage_key2 = IntToStorageKey(2); metadata.set_sequence_number(1); EXPECT_TRUE(UpdateSyncMetadata(syncer::TYPED_URLS, storage_key, metadata)); @@ -96,7 +108,7 @@ TEST_F(TypedURLSyncMetadataDatabaseTest, TypedURLGetAllSyncMetadata) { TEST_F(TypedURLSyncMetadataDatabaseTest, TypedURLWriteThenDeleteSyncMetadata) { EntityMetadata metadata; MetadataBatch metadata_batch; - std::string storage_key = "1"; + std::string storage_key = IntToStorageKey(1); ModelTypeState model_type_state; model_type_state.set_initial_sync_done(true); diff --git a/chromium/components/history/core/browser/typed_url_syncable_service_unittest.cc b/chromium/components/history/core/browser/typed_url_syncable_service_unittest.cc index 936739677d4..844b182be35 100644 --- a/chromium/components/history/core/browser/typed_url_syncable_service_unittest.cc +++ b/chromium/components/history/core/browser/typed_url_syncable_service_unittest.cc @@ -154,23 +154,23 @@ class TestHistoryBackendDelegate : public HistoryBackend::Delegate { void NotifyProfileError(sql::InitStatus init_status, const std::string& diagnostics) override {} void SetInMemoryBackend( - std::unique_ptr<InMemoryHistoryBackend> backend) override{}; + std::unique_ptr<InMemoryHistoryBackend> backend) override {} void NotifyFaviconsChanged(const std::set<GURL>& page_urls, - const GURL& icon_url) override{}; + const GURL& icon_url) override {} void NotifyURLVisited(ui::PageTransition transition, const URLRow& row, const RedirectList& redirects, - base::Time visit_time) override{}; - void NotifyURLsModified(const URLRows& changed_urls) override{}; + base::Time visit_time) override {} + void NotifyURLsModified(const URLRows& changed_urls) override {} void NotifyURLsDeleted(bool all_history, bool expired, const URLRows& deleted_rows, - const std::set<GURL>& favicon_urls) override{}; + const std::set<GURL>& favicon_urls) override {} void NotifyKeywordSearchTermUpdated(const URLRow& row, KeywordID keyword_id, - const base::string16& term) override{}; - void NotifyKeywordSearchTermDeleted(URLID url_id) override{}; - void DBLoaded() override{}; + const base::string16& term) override {} + void NotifyKeywordSearchTermDeleted(URLID url_id) override {} + void DBLoaded() override {} private: DISALLOW_COPY_AND_ASSIGN(TestHistoryBackendDelegate); diff --git a/chromium/components/history/core/browser/web_history_service.cc b/chromium/components/history/core/browser/web_history_service.cc index 0ce75efc45b..f2516b3010d 100644 --- a/chromium/components/history/core/browser/web_history_service.cc +++ b/chromium/components/history/core/browser/web_history_service.cc @@ -5,6 +5,7 @@ #include "components/history/core/browser/web_history_service.h" #include <memory> +#include <utility> #include "base/bind.h" #include "base/command_line.h" @@ -89,7 +90,8 @@ class RequestImpl : public WebHistoryService::Request, SigninManagerBase* signin_manager, const scoped_refptr<net::URLRequestContextGetter>& request_context, const GURL& url, - const WebHistoryService::CompletionCallback& callback) + const WebHistoryService::CompletionCallback& callback, + const net::PartialNetworkTrafficAnnotationTag& partial_traffic_annotation) : OAuth2TokenService::Consumer("web_history"), token_service_(token_service), signin_manager_(signin_manager), @@ -99,7 +101,8 @@ class RequestImpl : public WebHistoryService::Request, response_code_(0), auth_retry_count_(0), callback_(callback), - is_pending_(false) { + is_pending_(false), + partial_traffic_annotation_(partial_traffic_annotation) { DCHECK(token_service_); DCHECK(signin_manager_); DCHECK(request_context_); @@ -177,8 +180,23 @@ class RequestImpl : public WebHistoryService::Request, const std::string& access_token) { net::URLFetcher::RequestType request_type = post_data_ ? net::URLFetcher::POST : net::URLFetcher::GET; + net::NetworkTrafficAnnotationTag traffic_annotation = + net::CompleteNetworkTrafficAnnotation("web_history_service", + partial_traffic_annotation_, + R"( + semantics { + sender: "Web History" + destination: GOOGLE_OWNED_SERVICE + } + policy { + cookies_allowed: false + setting: + "To disable this feature, users can either sign out or disable " + "history sync via unchecking 'History' setting under 'Advanced " + "sync settings." + })"); std::unique_ptr<net::URLFetcher> fetcher = - net::URLFetcher::Create(url_, request_type, this); + net::URLFetcher::Create(url_, request_type, this, traffic_annotation); data_use_measurement::DataUseUserData::AttachToFetcher( fetcher.get(), data_use_measurement::DataUseUserData::WEB_HISTORY_SERVICE); @@ -255,6 +273,10 @@ class RequestImpl : public WebHistoryService::Request, // True if the request was started and has not yet completed, otherwise false. bool is_pending_; + + // Partial Network traffic annotation used to create URLFetcher for this + // request. + const net::PartialNetworkTrafficAnnotationTag partial_traffic_annotation_; }; // Converts a time into a string for use as a parameter in a request to the @@ -356,9 +378,10 @@ void WebHistoryService::RemoveObserver(WebHistoryServiceObserver* observer) { WebHistoryService::Request* WebHistoryService::CreateRequest( const GURL& url, - const CompletionCallback& callback) { + const CompletionCallback& callback, + const net::PartialNetworkTrafficAnnotationTag& partial_traffic_annotation) { return new RequestImpl(token_service_, signin_manager_, request_context_, url, - callback); + callback, partial_traffic_annotation); } // static @@ -379,20 +402,23 @@ std::unique_ptr<base::DictionaryValue> WebHistoryService::ReadResponse( std::unique_ptr<WebHistoryService::Request> WebHistoryService::QueryHistory( const base::string16& text_query, const QueryOptions& options, - const WebHistoryService::QueryWebHistoryCallback& callback) { + const WebHistoryService::QueryWebHistoryCallback& callback, + const net::PartialNetworkTrafficAnnotationTag& partial_traffic_annotation) { // Wrap the original callback into a generic completion callback. CompletionCallback completion_callback = base::Bind( &WebHistoryService::QueryHistoryCompletionCallback, callback); GURL url = GetQueryUrl(text_query, options, server_version_info_); - std::unique_ptr<Request> request(CreateRequest(url, completion_callback)); + std::unique_ptr<Request> request( + CreateRequest(url, completion_callback, partial_traffic_annotation)); request->Start(); return request; } void WebHistoryService::ExpireHistory( const std::vector<ExpireHistoryArgs>& expire_list, - const ExpireWebHistoryCallback& callback) { + const ExpireWebHistoryCallback& callback, + const net::PartialNetworkTrafficAnnotationTag& partial_traffic_annotation) { base::DictionaryValue delete_request; std::unique_ptr<base::ListValue> deletions(new base::ListValue); base::Time now = base::Time::Now(); @@ -414,7 +440,7 @@ void WebHistoryService::ExpireHistory( if (expire.urls.empty()) deletions->Append(CreateDeletion(min_timestamp, max_timestamp, GURL())); } - delete_request.Set("del", deletions.release()); + delete_request.Set("del", std::move(deletions)); std::string post_data; base::JSONWriter::Write(delete_request, &post_data); @@ -431,7 +457,8 @@ void WebHistoryService::ExpireHistory( weak_ptr_factory_.GetWeakPtr(), callback); - std::unique_ptr<Request> request(CreateRequest(url, completion_callback)); + std::unique_ptr<Request> request( + CreateRequest(url, completion_callback, partial_traffic_annotation)); request->SetPostData(post_data); Request* request_ptr = request.get(); pending_expire_requests_[request_ptr] = std::move(request); @@ -442,16 +469,18 @@ void WebHistoryService::ExpireHistoryBetween( const std::set<GURL>& restrict_urls, base::Time begin_time, base::Time end_time, - const ExpireWebHistoryCallback& callback) { + const ExpireWebHistoryCallback& callback, + const net::PartialNetworkTrafficAnnotationTag& partial_traffic_annotation) { std::vector<ExpireHistoryArgs> expire_list(1); expire_list.back().urls = restrict_urls; expire_list.back().begin_time = begin_time; expire_list.back().end_time = end_time; - ExpireHistory(expire_list, callback); + ExpireHistory(expire_list, callback, partial_traffic_annotation); } void WebHistoryService::GetAudioHistoryEnabled( - const AudioWebHistoryCallback& callback) { + const AudioWebHistoryCallback& callback, + const net::PartialNetworkTrafficAnnotationTag& partial_traffic_annotation) { // Wrap the original callback into a generic completion callback. CompletionCallback completion_callback = base::Bind(&WebHistoryService::AudioHistoryCompletionCallback, @@ -459,15 +488,18 @@ void WebHistoryService::GetAudioHistoryEnabled( callback); GURL url(kHistoryAudioHistoryUrl); - std::unique_ptr<Request> request(CreateRequest(url, completion_callback)); + + std::unique_ptr<Request> request( + CreateRequest(url, completion_callback, partial_traffic_annotation)); request->Start(); Request* request_ptr = request.get(); pending_audio_history_requests_[request_ptr] = std::move(request); } void WebHistoryService::SetAudioHistoryEnabled( - bool new_enabled_value, - const AudioWebHistoryCallback& callback) { + bool new_enabled_value, + const AudioWebHistoryCallback& callback, + const net::PartialNetworkTrafficAnnotationTag& partial_traffic_annotation) { // Wrap the original callback into a generic completion callback. CompletionCallback completion_callback = base::Bind(&WebHistoryService::AudioHistoryCompletionCallback, @@ -475,7 +507,8 @@ void WebHistoryService::SetAudioHistoryEnabled( callback); GURL url(kHistoryAudioHistoryChangeUrl); - std::unique_ptr<Request> request(CreateRequest(url, completion_callback)); + std::unique_ptr<Request> request( + CreateRequest(url, completion_callback, partial_traffic_annotation)); base::DictionaryValue enable_audio_history; enable_audio_history.SetBoolean("enable_history_recording", @@ -495,7 +528,8 @@ size_t WebHistoryService::GetNumberOfPendingAudioHistoryRequests() { } void WebHistoryService::QueryWebAndAppActivity( - const QueryWebAndAppActivityCallback& callback) { + const QueryWebAndAppActivityCallback& callback, + const net::PartialNetworkTrafficAnnotationTag& partial_traffic_annotation) { // Wrap the original callback into a generic completion callback. CompletionCallback completion_callback = base::Bind(&WebHistoryService::QueryWebAndAppActivityCompletionCallback, @@ -503,14 +537,16 @@ void WebHistoryService::QueryWebAndAppActivity( callback); GURL url(kQueryWebAndAppActivityUrl); - Request* request = CreateRequest(url, completion_callback); + Request* request = + CreateRequest(url, completion_callback, partial_traffic_annotation); pending_web_and_app_activity_requests_[request] = base::WrapUnique(request); request->Start(); } void WebHistoryService::QueryOtherFormsOfBrowsingHistory( version_info::Channel channel, - const QueryOtherFormsOfBrowsingHistoryCallback& callback) { + const QueryOtherFormsOfBrowsingHistoryCallback& callback, + const net::PartialNetworkTrafficAnnotationTag& partial_traffic_annotation) { // Wrap the original callback into a generic completion callback. CompletionCallback completion_callback = base::Bind( &WebHistoryService::QueryOtherFormsOfBrowsingHistoryCompletionCallback, @@ -527,7 +563,8 @@ void WebHistoryService::QueryOtherFormsOfBrowsingHistory( url = url.ReplaceComponents(replace_path); DCHECK(url.is_valid()); - Request* request = CreateRequest(url, completion_callback); + Request* request = + CreateRequest(url, completion_callback, partial_traffic_annotation); // Set the Sync-specific user agent. std::string user_agent = syncer::MakeUserAgentForSync( diff --git a/chromium/components/history/core/browser/web_history_service.h b/chromium/components/history/core/browser/web_history_service.h index b25ad05b8a2..3016fad0e91 100644 --- a/chromium/components/history/core/browser/web_history_service.h +++ b/chromium/components/history/core/browser/web_history_service.h @@ -19,6 +19,7 @@ #include "base/observer_list.h" #include "components/history/core/browser/history_types.h" #include "components/keyed_service/core/keyed_service.h" +#include "net/traffic_annotation/network_traffic_annotation.h" namespace base { class DictionaryValue; @@ -112,30 +113,44 @@ class WebHistoryService : public KeyedService { std::unique_ptr<Request> QueryHistory( const base::string16& text_query, const QueryOptions& options, - const QueryWebHistoryCallback& callback); + const QueryWebHistoryCallback& callback, + const net::PartialNetworkTrafficAnnotationTag& + partial_traffic_annotation); // Removes all visits to specified URLs in specific time ranges. // This is the of equivalent HistoryService::ExpireHistory(). void ExpireHistory(const std::vector<ExpireHistoryArgs>& expire_list, - const ExpireWebHistoryCallback& callback); + const ExpireWebHistoryCallback& callback, + const net::PartialNetworkTrafficAnnotationTag& + partial_traffic_annotation); // Removes all visits to specified URLs in the given time range. // This is the of equivalent HistoryService::ExpireHistoryBetween(). void ExpireHistoryBetween(const std::set<GURL>& restrict_urls, base::Time begin_time, base::Time end_time, - const ExpireWebHistoryCallback& callback); + const ExpireWebHistoryCallback& callback, + const net::PartialNetworkTrafficAnnotationTag& + partial_traffic_annotation); // Requests whether audio history recording is enabled. - virtual void GetAudioHistoryEnabled(const AudioWebHistoryCallback& callback); + virtual void GetAudioHistoryEnabled( + const AudioWebHistoryCallback& callback, + const net::PartialNetworkTrafficAnnotationTag& + partial_traffic_annotation); // Sets the state of audio history recording to |new_enabled_value|. - virtual void SetAudioHistoryEnabled(bool new_enabled_value, - const AudioWebHistoryCallback& callback); + virtual void SetAudioHistoryEnabled( + bool new_enabled_value, + const AudioWebHistoryCallback& callback, + const net::PartialNetworkTrafficAnnotationTag& + partial_traffic_annotation); // Queries whether web and app activity is enabled on the server. virtual void QueryWebAndAppActivity( - const QueryWebAndAppActivityCallback& callback); + const QueryWebAndAppActivityCallback& callback, + const net::PartialNetworkTrafficAnnotationTag& + partial_traffic_annotation); // Used for tests. size_t GetNumberOfPendingAudioHistoryRequests(); @@ -143,13 +158,17 @@ class WebHistoryService : public KeyedService { // Whether there are other forms of browsing history stored on the server. void QueryOtherFormsOfBrowsingHistory( version_info::Channel channel, - const QueryOtherFormsOfBrowsingHistoryCallback& callback); + const QueryOtherFormsOfBrowsingHistoryCallback& callback, + const net::PartialNetworkTrafficAnnotationTag& + partial_traffic_annotation); protected: // This function is pulled out for testing purposes. Caller takes ownership of // the new Request. virtual Request* CreateRequest(const GURL& url, - const CompletionCallback& callback); + const CompletionCallback& callback, + const net::PartialNetworkTrafficAnnotationTag& + partial_traffic_annotation); // Extracts a JSON-encoded HTTP response into a DictionaryValue. // If |request|'s HTTP response code indicates failure, or if the response diff --git a/chromium/components/history/core/browser/web_history_service_unittest.cc b/chromium/components/history/core/browser/web_history_service_unittest.cc index 7d528f3bcf3..f20cb048ad1 100644 --- a/chromium/components/history/core/browser/web_history_service_unittest.cc +++ b/chromium/components/history/core/browser/web_history_service_unittest.cc @@ -17,6 +17,7 @@ #include "components/signin/core/browser/fake_signin_manager.h" #include "components/signin/core/browser/test_signin_client.h" #include "net/http/http_status_code.h" +#include "net/traffic_annotation/network_traffic_annotation_test_helper.h" #include "net/url_request/url_request_test_util.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" @@ -42,7 +43,10 @@ class TestingWebHistoryService : public WebHistoryService { ~TestingWebHistoryService() override {} WebHistoryService::Request* CreateRequest( - const GURL& url, const CompletionCallback& callback) override; + const GURL& url, + const CompletionCallback& callback, + const net::PartialNetworkTrafficAnnotationTag& partial_traffic_annotation) + override; // This is sorta an override but override and static don't mix. // This function just calls WebHistoryService::ReadResponse. @@ -156,7 +160,9 @@ class TestRequest : public WebHistoryService::Request { }; WebHistoryService::Request* TestingWebHistoryService::CreateRequest( - const GURL& url, const CompletionCallback& callback) { + const GURL& url, + const CompletionCallback& callback, + const net::PartialNetworkTrafficAnnotationTag& partial_traffic_annotation) { EXPECT_EQ(expected_url_, url); WebHistoryService::Request* request = new TestRequest(url, callback, this); @@ -249,7 +255,8 @@ TEST_F(WebHistoryServiceTest, GetAudioHistoryEnabled) { web_history_service()->SetExpectedAudioHistoryValue(true); web_history_service()->GetAudioHistoryEnabled( base::Bind(&TestingWebHistoryService::GetAudioHistoryCallback, - base::Unretained(web_history_service()))); + base::Unretained(web_history_service())), + PARTIAL_TRAFFIC_ANNOTATION_FOR_TESTS); base::ThreadTaskRunnerHandle::Get()->PostTask( FROM_HERE, base::Bind(&TestingWebHistoryService::EnsureNoPendingRequestsRemain, @@ -263,8 +270,10 @@ TEST_F(WebHistoryServiceTest, SetAudioHistoryEnabledTrue) { web_history_service()->SetExpectedPostData( "{\"client\":\"audio\",\"enable_history_recording\":true}"); web_history_service()->SetAudioHistoryEnabled( - true, base::Bind(&TestingWebHistoryService::SetAudioHistoryCallback, - base::Unretained(web_history_service()))); + true, + base::Bind(&TestingWebHistoryService::SetAudioHistoryCallback, + base::Unretained(web_history_service())), + PARTIAL_TRAFFIC_ANNOTATION_FOR_TESTS); base::ThreadTaskRunnerHandle::Get()->PostTask( FROM_HERE, base::Bind(&TestingWebHistoryService::EnsureNoPendingRequestsRemain, @@ -278,8 +287,10 @@ TEST_F(WebHistoryServiceTest, SetAudioHistoryEnabledFalse) { web_history_service()->SetExpectedPostData( "{\"client\":\"audio\",\"enable_history_recording\":false}"); web_history_service()->SetAudioHistoryEnabled( - false, base::Bind(&TestingWebHistoryService::SetAudioHistoryCallback, - base::Unretained(web_history_service()))); + false, + base::Bind(&TestingWebHistoryService::SetAudioHistoryCallback, + base::Unretained(web_history_service())), + PARTIAL_TRAFFIC_ANNOTATION_FOR_TESTS); base::ThreadTaskRunnerHandle::Get()->PostTask( FROM_HERE, base::Bind(&TestingWebHistoryService::EnsureNoPendingRequestsRemain, @@ -293,15 +304,18 @@ TEST_F(WebHistoryServiceTest, MultipleRequests) { web_history_service()->SetExpectedPostData( "{\"client\":\"audio\",\"enable_history_recording\":false}"); web_history_service()->SetAudioHistoryEnabled( - false, base::Bind(&TestingWebHistoryService::MultipleRequestsCallback, - base::Unretained(web_history_service()))); + false, + base::Bind(&TestingWebHistoryService::MultipleRequestsCallback, + base::Unretained(web_history_service())), + PARTIAL_TRAFFIC_ANNOTATION_FOR_TESTS); web_history_service()->SetExpectedURL( GURL("https://history.google.com/history/api/lookup?client=audio")); web_history_service()->SetExpectedPostData(""); web_history_service()->GetAudioHistoryEnabled( base::Bind(&TestingWebHistoryService::MultipleRequestsCallback, - base::Unretained(web_history_service()))); + base::Unretained(web_history_service())), + PARTIAL_TRAFFIC_ANNOTATION_FOR_TESTS); // Check that both requests are no longer pending. base::ThreadTaskRunnerHandle::Get()->PostTask( |