diff options
| -rw-r--r-- | chrome/browser/history/delete_directive_handler.cc | 340 | ||||
| -rw-r--r-- | chrome/browser/history/delete_directive_handler.h | 76 | ||||
| -rw-r--r-- | chrome/browser/history/history_backend.cc | 64 | ||||
| -rw-r--r-- | chrome/browser/history/history_backend.h | 10 | ||||
| -rw-r--r-- | chrome/browser/history/history_backend_unittest.cc | 47 | ||||
| -rw-r--r-- | chrome/browser/history/history_service.cc | 160 | ||||
| -rw-r--r-- | chrome/browser/history/history_service.h | 22 | ||||
| -rw-r--r-- | chrome/browser/history/history_types.h | 5 | ||||
| -rw-r--r-- | chrome/browser/history/history_unittest.cc | 278 | ||||
| -rw-r--r-- | chrome/browser/history/visit_database.cc | 26 | ||||
| -rw-r--r-- | chrome/browser/history/visit_database_unittest.cc | 9 | ||||
| -rw-r--r-- | chrome/chrome_browser.gypi | 2 | ||||
| -rw-r--r-- | sync/api/sync_data.h | 2 | ||||
| -rw-r--r-- | sync/api/syncable_service.h | 2 |
14 files changed, 711 insertions, 332 deletions
diff --git a/chrome/browser/history/delete_directive_handler.cc b/chrome/browser/history/delete_directive_handler.cc new file mode 100644 index 0000000..783186a --- /dev/null +++ b/chrome/browser/history/delete_directive_handler.cc @@ -0,0 +1,340 @@ +// Copyright (c) 2013 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/history/delete_directive_handler.h" + +#include "base/json/json_writer.h" +#include "base/rand_util.h" +#include "base/time.h" +#include "base/values.h" +#include "chrome/browser/history/history_backend.h" +#include "chrome/browser/history/history_db_task.h" +#include "chrome/browser/history/history_service.h" +#include "sync/api/sync_change.h" +#include "sync/protocol/history_delete_directive_specifics.pb.h" +#include "sync/protocol/proto_value_conversions.h" +#include "sync/protocol/sync.pb.h" + +namespace { + +std::string DeleteDirectiveToString( + const sync_pb::HistoryDeleteDirectiveSpecifics& delete_directive) { + scoped_ptr<base::DictionaryValue> value( + syncer::HistoryDeleteDirectiveSpecificsToValue(delete_directive)); + std::string str; + base::JSONWriter::Write(value.get(), &str); + return str; +} + +// Compare time range directives first by start time, then by end time. +bool TimeRangeLessThan(const syncer::SyncData& data1, + const syncer::SyncData& data2) { + const sync_pb::TimeRangeDirective& range1 = + data1.GetSpecifics().history_delete_directive().time_range_directive(); + const sync_pb::TimeRangeDirective& range2 = + data2.GetSpecifics().history_delete_directive().time_range_directive(); + if (range1.start_time_usec() < range2.start_time_usec()) + return true; + if (range1.start_time_usec() > range2.start_time_usec()) + return false; + return range1.end_time_usec() < range2.end_time_usec(); +} + +base::Time UnixUsecToTime(int64 usec) { + return base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(usec); +} + +// Converts global IDs in |global_id_directive| to times. +void GetTimesFromGlobalIds( + const sync_pb::GlobalIdDirective& global_id_directive, + std::set<base::Time> *times) { + for (int i = 0; i < global_id_directive.global_id_size(); ++i) { + times->insert( + base::Time::FromInternalValue(global_id_directive.global_id(i))); + } +} + +} // anonymous namespace + +namespace history { + +class DeleteDirectiveHandler::DeleteDirectiveTask : public HistoryDBTask { + public: + DeleteDirectiveTask( + base::WeakPtr<DeleteDirectiveHandler> delete_directive_handler, + const syncer::SyncDataList& delete_directive, + DeleteDirectiveHandler::PostProcessingAction post_processing_action) + : delete_directive_handler_(delete_directive_handler), + delete_directives_(delete_directive), + post_processing_action_(post_processing_action) {} + + // Implements HistoryDBTask. + virtual bool RunOnDBThread(history::HistoryBackend* backend, + history::HistoryDatabase* db) OVERRIDE; + virtual void DoneRunOnMainThread() OVERRIDE; + + private: + ~DeleteDirectiveTask() {} + + // Process a list of global Id directives. Delete all visits to a URL in + // time ranges of directives if the timestamp of one visit matches with one + // global id. + void ProcessGlobalIdDeleteDirectives( + history::HistoryBackend* history_backend, + const syncer::SyncDataList& global_id_directives); + + // Process a list of time range directives, all history entries within the + // time ranges are deleted. |time_range_directives| should be sorted by + // |start_time_usec| and |end_time_usec| already. + void ProcessTimeRangeDeleteDirectives( + history::HistoryBackend* history_backend, + const syncer::SyncDataList& time_range_directives); + + base::WeakPtr<DeleteDirectiveHandler> delete_directive_handler_; + syncer::SyncDataList delete_directives_; + DeleteDirectiveHandler::PostProcessingAction post_processing_action_; +}; + +bool DeleteDirectiveHandler::DeleteDirectiveTask::RunOnDBThread( + history::HistoryBackend* backend, + history::HistoryDatabase* db) { + syncer::SyncDataList global_id_directives; + syncer::SyncDataList time_range_directives; + for (syncer::SyncDataList::const_iterator it = delete_directives_.begin(); + it != delete_directives_.end(); ++it) { + DCHECK_EQ(it->GetDataType(), syncer::HISTORY_DELETE_DIRECTIVES); + const sync_pb::HistoryDeleteDirectiveSpecifics& delete_directive = + it->GetSpecifics().history_delete_directive(); + if (delete_directive.has_global_id_directive()) { + global_id_directives.push_back(*it); + } else { + time_range_directives.push_back(*it); + } + } + + ProcessGlobalIdDeleteDirectives(backend, global_id_directives); + std::sort(time_range_directives.begin(), time_range_directives.end(), + TimeRangeLessThan); + ProcessTimeRangeDeleteDirectives(backend, time_range_directives); + return true; +} + +void DeleteDirectiveHandler::DeleteDirectiveTask::DoneRunOnMainThread() { + if (delete_directive_handler_.get()) { + delete_directive_handler_->FinishProcessing(post_processing_action_, + delete_directives_); + } +} + +void +DeleteDirectiveHandler::DeleteDirectiveTask::ProcessGlobalIdDeleteDirectives( + history::HistoryBackend* history_backend, + const syncer::SyncDataList& global_id_directives) { + if (global_id_directives.empty()) + return; + + // Group times represented by global IDs by time ranges of delete directives. + // It's more efficient for backend to process all directives with same time + // range at once. + typedef std::map<std::pair<base::Time, base::Time>, std::set<base::Time> > + GlobalIdTimesGroup; + GlobalIdTimesGroup id_times_group; + for (size_t i = 0; i < global_id_directives.size(); ++i) { + DVLOG(1) << "Processing delete directive: " + << DeleteDirectiveToString( + global_id_directives[i].GetSpecifics() + .history_delete_directive()); + + const sync_pb::GlobalIdDirective& id_directive = + global_id_directives[i].GetSpecifics().history_delete_directive() + .global_id_directive(); + if (id_directive.global_id_size() == 0 || + !id_directive.has_start_time_usec() || + !id_directive.has_end_time_usec()) { + DLOG(ERROR) << "Invalid global id directive."; + continue; + } + GetTimesFromGlobalIds( + id_directive, + &id_times_group[ + std::make_pair(UnixUsecToTime(id_directive.start_time_usec()), + UnixUsecToTime(id_directive.end_time_usec()))]); + } + + if (id_times_group.empty()) + return; + + // Call backend to expire history of directives in each group. + for (GlobalIdTimesGroup::const_iterator group_it = id_times_group.begin(); + group_it != id_times_group.end(); ++group_it) { + // Add 1us to cover history entries visited at the end time because time + // range in directive is inclusive. + history_backend->ExpireHistoryForTimes( + group_it->second, + group_it->first.first, + group_it->first.second + base::TimeDelta::FromMicroseconds(1)); + } +} + +void +DeleteDirectiveHandler::DeleteDirectiveTask::ProcessTimeRangeDeleteDirectives( + history::HistoryBackend* history_backend, + const syncer::SyncDataList& time_range_directives) { + if (time_range_directives.empty()) + return; + + // Iterate through time range directives. Expire history in combined + // time range for multiple directives whose time ranges overlap. + base::Time current_start_time; + base::Time current_end_time; + for (size_t i = 0; i < time_range_directives.size(); ++i) { + const sync_pb::HistoryDeleteDirectiveSpecifics& delete_directive = + time_range_directives[i].GetSpecifics().history_delete_directive(); + DVLOG(1) << "Processing time range directive: " + << DeleteDirectiveToString(delete_directive); + + const sync_pb::TimeRangeDirective& time_range_directive = + delete_directive.time_range_directive(); + if (!time_range_directive.has_start_time_usec() || + !time_range_directive.has_end_time_usec() || + time_range_directive.start_time_usec() >= + time_range_directive.end_time_usec()) { + DLOG(ERROR) << "Invalid time range directive."; + continue; + } + + base::Time directive_start_time = + UnixUsecToTime(time_range_directive.start_time_usec()); + base::Time directive_end_time = + UnixUsecToTime(time_range_directive.end_time_usec()); + if (directive_start_time > current_end_time) { + if (!current_start_time.is_null()) { + // Add 1us to cover history entries visited at the end time because + // time range in directive is inclusive. + history_backend->ExpireHistoryBetween( + std::set<GURL>(), current_start_time, + current_end_time + base::TimeDelta::FromMicroseconds(1)); + } + current_start_time = directive_start_time; + } + if (directive_end_time > current_end_time) + current_end_time = directive_end_time; + } + + if (!current_start_time.is_null()) { + history_backend->ExpireHistoryBetween( + std::set<GURL>(), current_start_time, + current_end_time + base::TimeDelta::FromMicroseconds(1)); + } +} + +DeleteDirectiveHandler::DeleteDirectiveHandler() + : weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {} + +DeleteDirectiveHandler::~DeleteDirectiveHandler() { + weak_ptr_factory_.InvalidateWeakPtrs(); +} + +void DeleteDirectiveHandler::Start( + HistoryService* history_service, + const syncer::SyncDataList& initial_sync_data, + scoped_ptr<syncer::SyncChangeProcessor> sync_processor) { + DCHECK(thread_checker_.CalledOnValidThread()); + sync_processor_ = sync_processor.Pass(); + if (!initial_sync_data.empty()) { + // Drop processed delete directives during startup. + history_service->ScheduleDBTask( + new DeleteDirectiveTask(weak_ptr_factory_.GetWeakPtr(), + initial_sync_data, + DROP_AFTER_PROCESSING), + &internal_consumer_); + } +} + +void DeleteDirectiveHandler::Stop() { + DCHECK(thread_checker_.CalledOnValidThread()); + sync_processor_.reset(); +} + +syncer::SyncError DeleteDirectiveHandler::ProcessLocalDeleteDirective( + const sync_pb::HistoryDeleteDirectiveSpecifics& delete_directive) { + DCHECK(thread_checker_.CalledOnValidThread()); + if (!sync_processor_.get()) { + return syncer::SyncError( + FROM_HERE, + "Cannot send local delete directive to sync", + syncer::HISTORY_DELETE_DIRECTIVES); + } + // Generate a random sync tag since history delete directives don't + // have a 'built-in' ID. 8 bytes should suffice. + std::string sync_tag = base::RandBytesAsString(8); + sync_pb::EntitySpecifics entity_specifics; + entity_specifics.mutable_history_delete_directive()->CopyFrom( + delete_directive); + syncer::SyncData sync_data = + syncer::SyncData::CreateLocalData( + sync_tag, sync_tag, entity_specifics); + syncer::SyncChange change( + FROM_HERE, syncer::SyncChange::ACTION_ADD, sync_data); + syncer::SyncChangeList changes(1, change); + return sync_processor_->ProcessSyncChanges(FROM_HERE, changes); +} + +syncer::SyncError DeleteDirectiveHandler::ProcessSyncChanges( + HistoryService* history_service, + const syncer::SyncChangeList& change_list) { + DCHECK(thread_checker_.CalledOnValidThread()); + if (!sync_processor_.get()) { + return syncer::SyncError( + FROM_HERE, "Sync is disabled.", syncer::HISTORY_DELETE_DIRECTIVES); + } + + syncer::SyncDataList delete_directives; + for (syncer::SyncChangeList::const_iterator it = change_list.begin(); + it != change_list.end(); ++it) { + switch (it->change_type()) { + case syncer::SyncChange::ACTION_ADD: + delete_directives.push_back(it->sync_data()); + break; + case syncer::SyncChange::ACTION_DELETE: + // TODO(akalin): Keep track of existing delete directives. + break; + default: + NOTREACHED(); + break; + } + } + + if (!delete_directives.empty()) { + // Don't drop real-time delete directive so that sync engine can detect + // redelivered delete directives to avoid processing them again and again + // in one chrome session. + history_service->ScheduleDBTask( + new DeleteDirectiveTask(weak_ptr_factory_.GetWeakPtr(), + delete_directives, KEEP_AFTER_PROCESSING), + &internal_consumer_); + } + return syncer::SyncError(); +} + +void DeleteDirectiveHandler::FinishProcessing( + PostProcessingAction post_processing_action, + const syncer::SyncDataList& delete_directives) { + DCHECK(thread_checker_.CalledOnValidThread()); + + // If specified, drop processed delete directive in sync model because they + // only need to be applied once. + if (sync_processor_.get() && + post_processing_action == DROP_AFTER_PROCESSING) { + syncer::SyncChangeList change_list; + for (size_t i = 0; i < delete_directives.size(); ++i) { + change_list.push_back( + syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_DELETE, + delete_directives[i])); + } + sync_processor_->ProcessSyncChanges(FROM_HERE, change_list); + } +} + +} // namespace history diff --git a/chrome/browser/history/delete_directive_handler.h b/chrome/browser/history/delete_directive_handler.h new file mode 100644 index 0000000..51412ab --- /dev/null +++ b/chrome/browser/history/delete_directive_handler.h @@ -0,0 +1,76 @@ +// Copyright (c) 2013 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CHROME_BROWSER_HISTORY_DELETE_DIRECTIVE_HANDLER_H_ +#define CHROME_BROWSER_HISTORY_DELETE_DIRECTIVE_HANDLER_H_ + +#include "base/memory/scoped_ptr.h" +#include "base/memory/weak_ptr.h" +#include "base/threading/thread_checker.h" +#include "chrome/browser/common/cancelable_request.h" +#include "sync/api/sync_change_processor.h" +#include "sync/api/sync_data.h" + +namespace sync_pb { +class HistoryDeleteDirectiveSpecifics; +} + +class HistoryService; + +namespace history { + +// DeleteDirectiveHandler sends delete directives created locally to sync +// engine to propagate to other clients. It also expires local history entries +// according to given delete directives from server. +class DeleteDirectiveHandler { + public: + DeleteDirectiveHandler(); + ~DeleteDirectiveHandler(); + + // Start/stop processing delete directives when sync is enabled/disabled. + void Start(HistoryService* history_service, + const syncer::SyncDataList& initial_sync_data, + scoped_ptr<syncer::SyncChangeProcessor> sync_processor); + void Stop(); + + // Sends the given |delete_directive| to SyncChangeProcessor (if it exists). + // Returns any error resulting from sending the delete directive to sync. + // NOTE: the given |delete_directive| is not processed to remove local + // history entries that match. Caller still needs to call other + // interfaces, e.g. HistoryService::ExpireHistoryBetween(), to delete + // local history entries. + syncer::SyncError ProcessLocalDeleteDirective( + const sync_pb::HistoryDeleteDirectiveSpecifics& delete_directive); + + // Expires local history entries according to delete directives from server. + syncer::SyncError ProcessSyncChanges( + HistoryService* history_service, + const syncer::SyncChangeList& change_list); + + private: + class DeleteDirectiveTask; + friend class DeleteDirectiveTask; + + // Action to take on processed delete directives. + enum PostProcessingAction { + KEEP_AFTER_PROCESSING, + DROP_AFTER_PROCESSING + }; + + // Callback when history backend finishes deleting visits according to + // |delete_directives|. + void FinishProcessing(PostProcessingAction post_processing_action, + const syncer::SyncDataList& delete_directives); + + CancelableRequestConsumer internal_consumer_; + scoped_ptr<syncer::SyncChangeProcessor> sync_processor_; + base::ThreadChecker thread_checker_; + base::WeakPtrFactory<DeleteDirectiveHandler> weak_ptr_factory_; + + DISALLOW_COPY_AND_ASSIGN(DeleteDirectiveHandler); +}; + +} // namespace history + +#endif // CHROME_BROWSER_HISTORY_DELETE_DIRECTIVE_HANDLER_H_ diff --git a/chrome/browser/history/history_backend.cc b/chrome/browser/history/history_backend.cc index 202c7a7..95980cf 100644 --- a/chrome/browser/history/history_backend.cc +++ b/chrome/browser/history/history_backend.cc @@ -2732,29 +2732,57 @@ void HistoryBackend::ExpireHistoryBetween( } void HistoryBackend::ExpireHistoryForTimes( - const std::vector<base::Time>& times) { - // Put the times in reverse chronological order and remove - // duplicates (for expirer_.ExpireHistoryForTimes()). - std::vector<base::Time> sorted_times = times; - std::sort(sorted_times.begin(), sorted_times.end(), - std::greater<base::Time>()); - sorted_times.erase( - std::unique(sorted_times.begin(), sorted_times.end()), - sorted_times.end()); + const std::set<base::Time>& times, + base::Time begin_time, base::Time end_time) { + if (times.empty() || !db_.get()) + return; - if (sorted_times.empty()) + DCHECK(*times.begin() >= begin_time) + << "Min time is before begin time: " + << times.begin()->ToJsTime() << " v.s. " << begin_time.ToJsTime(); + DCHECK(*times.rbegin() < end_time) + << "Max time is after end time: " + << times.rbegin()->ToJsTime() << " v.s. " << end_time.ToJsTime(); + + history::QueryOptions options; + options.begin_time = begin_time; + options.end_time = end_time; + options.duplicate_policy = QueryOptions::KEEP_ALL_DUPLICATES; + QueryResults results; + QueryHistoryBasic(db_.get(), db_.get(), options, &results); + + // 1st pass: find URLs that are visited at one of |times|. + std::set<GURL> urls; + for (size_t i = 0; i < results.size(); ++i) { + if (times.count(results[i].visit_time()) > 0) + urls.insert(results[i].url()); + } + if (urls.empty()) return; - if (db_.get()) { - expirer_.ExpireHistoryForTimes(sorted_times); - // Force a commit, if the user is deleting something for privacy reasons, - // we want to get it on disk ASAP. - Commit(); + // 2nd pass: collect all visit times of those URLs. + std::vector<base::Time> times_to_expire; + for (size_t i = 0; i < results.size(); ++i) { + if (urls.count(results[i].url())) + times_to_expire.push_back(results[i].visit_time()); } - // Update the first recorded time if we've expired it. - if (std::binary_search(sorted_times.begin(), sorted_times.end(), - first_recorded_time_, std::greater<base::Time>())) + // Put the times in reverse chronological order and remove + // duplicates (for expirer_.ExpireHistoryForTimes()). + std::sort(times_to_expire.begin(), times_to_expire.end(), + std::greater<base::Time>()); + times_to_expire.erase( + std::unique(times_to_expire.begin(), times_to_expire.end()), + times_to_expire.end()); + + // Expires by times and commit. + DCHECK(!times_to_expire.empty()); + expirer_.ExpireHistoryForTimes(times_to_expire); + Commit(); + + DCHECK(times_to_expire.back() >= first_recorded_time_); + // Update |first_recorded_time_| if we expired it. + if (times_to_expire.back() == first_recorded_time_) db_->GetStartDate(&first_recorded_time_); } diff --git a/chrome/browser/history/history_backend.h b/chrome/browser/history/history_backend.h index 5df4d1c..facd6e2 100644 --- a/chrome/browser/history/history_backend.h +++ b/chrome/browser/history/history_backend.h @@ -442,8 +442,13 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>, base::Time begin_time, base::Time end_time); - // Calls ExpireHistoryBackend::ExpireHistoryForTimes and commits the change. - void ExpireHistoryForTimes(const std::vector<base::Time>& times); + // Finds the URLs visited at |times| and expires all their visits within + // [|begin_time|, |end_time|). All times in |times| should be in + // [|begin_time|, |end_time|). This is used when expiration request is from + // server side, i.e. web history deletes, where only visit times (possibly + // incomplete) are transmitted to protect user's privacy. + void ExpireHistoryForTimes(const std::set<base::Time>& times, + base::Time begin_time, base::Time end_time); // Calls ExpireHistoryBetween() once for each element in the vector. // The fields of |ExpireHistoryArgs| map directly to the arguments of @@ -548,6 +553,7 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>, CloneFaviconIsRestrictedToSameDomain); FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, QueryFilteredURLs); FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, UpdateVisitDuration); + FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, ExpireHistoryForTimes); friend class ::TestingProfile; diff --git a/chrome/browser/history/history_backend_unittest.cc b/chrome/browser/history/history_backend_unittest.cc index 04955d5..9d84486 100644 --- a/chrome/browser/history/history_backend_unittest.cc +++ b/chrome/browser/history/history_backend_unittest.cc @@ -2492,9 +2492,10 @@ TEST_F(HistoryBackendTest, AddPageNoVisitForBookmark) { TEST_F(HistoryBackendTest, ExpireHistoryForTimes) { ASSERT_TRUE(backend_.get()); - HistoryAddPageArgs args[5]; + HistoryAddPageArgs args[10]; for (size_t i = 0; i < arraysize(args); ++i) { - args[i].url = GURL("http://example" + base::IntToString(i) + ".com"); + args[i].url = GURL("http://example" + + std::string((i % 2 == 0 ? ".com" : ".net"))); args[i].time = base::Time::FromInternalValue(i); backend_->AddPage(args[i]); } @@ -2505,22 +2506,38 @@ TEST_F(HistoryBackendTest, ExpireHistoryForTimes) { EXPECT_TRUE(backend_->GetURL(args[i].url, &row)); } - std::vector<base::Time> times; - times.push_back(args[0].time); - // Insert twice to make sure we handle duplicate times correctly. - times.push_back(args[2].time); - times.push_back(args[2].time); - times.push_back(args[4].time); - backend_->ExpireHistoryForTimes(times); + std::set<base::Time> times; + times.insert(args[5].time); + backend_->ExpireHistoryForTimes(times, + base::Time::FromInternalValue(2), + base::Time::FromInternalValue(8)); - EXPECT_EQ(base::Time::FromInternalValue(1), + EXPECT_EQ(base::Time::FromInternalValue(0), backend_->GetFirstRecordedTimeForTest()); - EXPECT_FALSE(backend_->GetURL(args[0].url, &row)); - EXPECT_TRUE(backend_->GetURL(args[1].url, &row)); - EXPECT_FALSE(backend_->GetURL(args[2].url, &row)); - EXPECT_TRUE(backend_->GetURL(args[3].url, &row)); - EXPECT_FALSE(backend_->GetURL(args[4].url, &row)); + // Visits to http://example.com are untouched. + VisitVector visit_vector; + EXPECT_TRUE(backend_->GetVisitsForURL( + backend_->db_->GetRowForURL(GURL("http://example.com"), NULL), + &visit_vector)); + ASSERT_EQ(5u, visit_vector.size()); + EXPECT_EQ(base::Time::FromInternalValue(0), visit_vector[0].visit_time); + EXPECT_EQ(base::Time::FromInternalValue(2), visit_vector[1].visit_time); + EXPECT_EQ(base::Time::FromInternalValue(4), visit_vector[2].visit_time); + EXPECT_EQ(base::Time::FromInternalValue(6), visit_vector[3].visit_time); + EXPECT_EQ(base::Time::FromInternalValue(8), visit_vector[4].visit_time); + + // Visits to http://example.net between [2,8] are removed. + visit_vector.clear(); + EXPECT_TRUE(backend_->GetVisitsForURL( + backend_->db_->GetRowForURL(GURL("http://example.net"), NULL), + &visit_vector)); + ASSERT_EQ(2u, visit_vector.size()); + EXPECT_EQ(base::Time::FromInternalValue(1), visit_vector[0].visit_time); + EXPECT_EQ(base::Time::FromInternalValue(9), visit_vector[1].visit_time); + + EXPECT_EQ(base::Time::FromInternalValue(0), + backend_->GetFirstRecordedTimeForTest()); } TEST_F(HistoryBackendTest, ExpireHistory) { diff --git a/chrome/browser/history/history_service.cc b/chrome/browser/history/history_service.cc index e0225a6..8f3b69b 100644 --- a/chrome/browser/history/history_service.cc +++ b/chrome/browser/history/history_service.cc @@ -28,15 +28,11 @@ #include "base/callback.h" #include "base/command_line.h" #include "base/compiler_specific.h" -#include "base/json/json_writer.h" #include "base/location.h" #include "base/memory/ref_counted.h" #include "base/message_loop.h" #include "base/path_service.h" #include "base/prefs/pref_service.h" -#include "base/rand_util.h" -#include "base/sequenced_task_runner.h" -#include "base/string_util.h" #include "base/thread_task_runner_handle.h" #include "base/threading/thread.h" #include "base/time.h" @@ -67,14 +63,7 @@ #include "content/public/browser/notification_service.h" #include "grit/chromium_strings.h" #include "grit/generated_resources.h" -#include "sync/api/sync_change.h" -#include "sync/api/sync_change_processor.h" -#include "sync/api/sync_data.h" -#include "sync/api/sync_error.h" #include "sync/api/sync_error_factory.h" -#include "sync/protocol/history_delete_directive_specifics.pb.h" -#include "sync/protocol/proto_value_conversions.h" -#include "sync/protocol/sync.pb.h" #include "third_party/skia/include/core/SkBitmap.h" using base::Time; @@ -84,15 +73,6 @@ namespace { static const char* kHistoryThreadName = "Chrome_HistoryThread"; -std::string DeleteDirectiveToString( - const sync_pb::HistoryDeleteDirectiveSpecifics& delete_directive) { - scoped_ptr<base::DictionaryValue> value( - syncer::HistoryDeleteDirectiveSpecificsToValue(delete_directive)); - std::string str; - base::JSONWriter::Write(value.get(), &str); - return str; -} - void DerefDownloadDbHandle( const HistoryService::DownloadCreateCallback& callback, int64* db_handle) { @@ -1032,12 +1012,6 @@ base::WeakPtr<HistoryService> HistoryService::AsWeakPtr() { return weak_ptr_factory_.GetWeakPtr(); } -void HistoryService::ProcessDeleteDirectiveForTest( - const sync_pb::HistoryDeleteDirectiveSpecifics& delete_directive) { - DCHECK(thread_checker_.CalledOnValidThread()); - ProcessDeleteDirective(delete_directive); -} - syncer::SyncMergeResult HistoryService::MergeDataAndStartSyncing( syncer::ModelType type, const syncer::SyncDataList& initial_sync_data, @@ -1045,19 +1019,15 @@ syncer::SyncMergeResult HistoryService::MergeDataAndStartSyncing( scoped_ptr<syncer::SyncErrorFactory> error_handler) { DCHECK(thread_checker_.CalledOnValidThread()); DCHECK_EQ(type, syncer::HISTORY_DELETE_DIRECTIVES); - for (syncer::SyncDataList::const_iterator it = initial_sync_data.begin(); - it != initial_sync_data.end(); ++it) { - DCHECK_EQ(it->GetDataType(), syncer::HISTORY_DELETE_DIRECTIVES); - ProcessDeleteDirective(it->GetSpecifics().history_delete_directive()); - } - sync_change_processor_ = sync_processor.Pass(); + delete_directive_handler_.Start(this, initial_sync_data, + sync_processor.Pass()); return syncer::SyncMergeResult(type); } void HistoryService::StopSyncing(syncer::ModelType type) { DCHECK(thread_checker_.CalledOnValidThread()); DCHECK_EQ(type, syncer::HISTORY_DELETE_DIRECTIVES); - sync_change_processor_.reset(); + delete_directive_handler_.Stop(); } syncer::SyncDataList HistoryService::GetAllSyncData( @@ -1071,47 +1041,15 @@ syncer::SyncDataList HistoryService::GetAllSyncData( syncer::SyncError HistoryService::ProcessSyncChanges( const tracked_objects::Location& from_here, const syncer::SyncChangeList& change_list) { - for (syncer::SyncChangeList::const_iterator it = change_list.begin(); - it != change_list.end(); ++it) { - switch (it->change_type()) { - case syncer::SyncChange::ACTION_ADD: - ProcessDeleteDirective( - it->sync_data().GetSpecifics().history_delete_directive()); - break; - case syncer::SyncChange::ACTION_DELETE: - // TODO(akalin): Keep track of existing delete directives. - break; - default: - NOTREACHED(); - break; - } - } + delete_directive_handler_.ProcessSyncChanges(this, change_list); return syncer::SyncError(); } syncer::SyncError HistoryService::ProcessLocalDeleteDirective( const sync_pb::HistoryDeleteDirectiveSpecifics& delete_directive) { DCHECK(thread_checker_.CalledOnValidThread()); - ProcessDeleteDirective(delete_directive); - if (!sync_change_processor_.get()) { - return syncer::SyncError( - FROM_HERE, - "Cannot send local delete directive to sync", - syncer::HISTORY_DELETE_DIRECTIVES); - } - // Generate a random sync tag since history delete directives don't - // have a 'built-in' ID. 8 bytes should suffice. - std::string sync_tag = base::RandBytesAsString(8); - sync_pb::EntitySpecifics entity_specifics; - entity_specifics.mutable_history_delete_directive()->CopyFrom( + return delete_directive_handler_.ProcessLocalDeleteDirective( delete_directive); - syncer::SyncData sync_data = - syncer::SyncData::CreateLocalData( - sync_tag, sync_tag, entity_specifics); - syncer::SyncChange change( - FROM_HERE, syncer::SyncChange::ACTION_ADD, sync_data); - syncer::SyncChangeList changes(1, change); - return sync_change_processor_->ProcessSyncChanges(FROM_HERE, changes); } void HistoryService::SetInMemoryBackend(int backend_id, @@ -1300,91 +1238,3 @@ void HistoryService::NotifyVisitDBObserversOnAddVisit( FOR_EACH_OBSERVER(history::VisitDatabaseObserver, visit_database_observers_, OnAddVisit(info)); } - -void HistoryService::ProcessDeleteDirective( - const sync_pb::HistoryDeleteDirectiveSpecifics& delete_directive) { - DCHECK(thread_checker_.CalledOnValidThread()); - - DVLOG(1) << "Processing delete directive: " - << DeleteDirectiveToString(delete_directive); - - base::Closure done_callback = - base::Bind(&HistoryService::OnDeleteDirectiveProcessed, - weak_ptr_factory_.GetWeakPtr(), - delete_directive); - - // Execute |done_callback| on return unless we pass it to something - // else. - base::ScopedClosureRunner scoped_done_callback(done_callback); - - // Exactly one directive must be filled in. If not, ignore. - if (delete_directive.has_global_id_directive() == - delete_directive.has_time_range_directive()) { - return; - } - - base::Closure backend_task; - - if (delete_directive.has_global_id_directive()) { - const sync_pb::GlobalIdDirective& global_id_directive = - delete_directive.global_id_directive(); - std::vector<base::Time> times; - for (int i = 0; i < global_id_directive.global_id_size(); ++i) { - int64 global_id = global_id_directive.global_id(i); - // The global id is just an internal time value. - base::Time time = base::Time::FromInternalValue(global_id); - times.push_back(time); - } - - if (!times.empty()) { - backend_task = - base::Bind(&HistoryBackend::ExpireHistoryForTimes, - history_backend_, times); - } - } else if (delete_directive.has_time_range_directive()) { - const sync_pb::TimeRangeDirective& time_range_directive = - delete_directive.time_range_directive(); - // {start,end}_time_usec must both be filled in. If not, ignore. - if (!time_range_directive.has_start_time_usec() || - !time_range_directive.has_end_time_usec()) { - return; - } - - // The directive is for the closed interval [start_time_usec, - // end_time_usec], but ExpireHistoryBetween() expects the - // half-open interval [begin_time, end_time), so add 1 to - // end_time_usec before converting. - base::Time begin_time = - base::Time::UnixEpoch() + - base::TimeDelta::FromMicroseconds( - time_range_directive.start_time_usec()); - base::Time end_time = - base::Time::UnixEpoch() + - base::TimeDelta::FromMicroseconds( - time_range_directive.end_time_usec() + 1); - - backend_task = - base::Bind(&HistoryBackend::ExpireHistoryBetween, - history_backend_, std::set<GURL>(), - begin_time, end_time); - } - - if (!backend_task.is_null()) { - LoadBackendIfNecessary(); - DCHECK(thread_); - if (thread_->message_loop_proxy()->PostTaskAndReply( - FROM_HERE, - backend_task, - done_callback)) { - ignore_result(scoped_done_callback.Release()); - } - } -} - -void HistoryService::OnDeleteDirectiveProcessed( - const sync_pb::HistoryDeleteDirectiveSpecifics& delete_directive) { - DVLOG(1) << "Processed delete directive: " - << DeleteDirectiveToString(delete_directive); - // TODO(akalin): Keep track of which delete directives we've already - // processed. -} diff --git a/chrome/browser/history/history_service.h b/chrome/browser/history/history_service.h index 61966b8..c4f8a67 100644 --- a/chrome/browser/history/history_service.h +++ b/chrome/browser/history/history_service.h @@ -22,6 +22,7 @@ #include "base/time.h" #include "chrome/browser/common/cancelable_request.h" #include "chrome/browser/favicon/favicon_service.h" +#include "chrome/browser/history/delete_directive_handler.h" #include "chrome/browser/history/history_types.h" #include "chrome/browser/profiles/profile_keyed_service.h" #include "chrome/browser/search_engines/template_url_id.h" @@ -75,10 +76,6 @@ struct HistoryDetails; } // namespace history -namespace sync_pb { -class HistoryDeleteDirectiveSpecifics; -} - // The history service records page titles, and visit times, as well as // (eventually) information about autocomplete. // @@ -602,9 +599,6 @@ class HistoryService : public CancelableRequestProvider, base::WeakPtr<HistoryService> AsWeakPtr(); - void ProcessDeleteDirectiveForTest( - const sync_pb::HistoryDeleteDirectiveSpecifics& delete_directive); - // syncer::SyncableService implementation. virtual syncer::SyncMergeResult MergeDataAndStartSyncing( syncer::ModelType type, @@ -835,15 +829,6 @@ class HistoryService : public CancelableRequestProvider, // specified priority. The task will have ownership taken. void ScheduleTask(SchedulePriority priority, const base::Closure& task); - // Delete local history according to the given directive (from - // sync). - void ProcessDeleteDirective( - const sync_pb::HistoryDeleteDirectiveSpecifics& delete_directive); - - // Called when a delete directive has been processed. - void OnDeleteDirectiveProcessed( - const sync_pb::HistoryDeleteDirectiveSpecifics& delete_directive); - // Schedule ------------------------------------------------------------------ // // Functions for scheduling operations on the history thread that have a @@ -1066,9 +1051,6 @@ class HistoryService : public CancelableRequestProvider, // TODO(mrossetti): Consider changing ownership. See http://crbug.com/138321 scoped_ptr<history::InMemoryHistoryBackend> in_memory_backend_; - // Used to propagate local delete directives to sync. - scoped_ptr<syncer::SyncChangeProcessor> sync_change_processor_; - // The profile, may be null when testing. Profile* profile_; @@ -1099,6 +1081,8 @@ class HistoryService : public CancelableRequestProvider, ObserverList<history::VisitDatabaseObserver> visit_database_observers_; + history::DeleteDirectiveHandler delete_directive_handler_; + DISALLOW_COPY_AND_ASSIGN(HistoryService); }; diff --git a/chrome/browser/history/history_types.h b/chrome/browser/history/history_types.h index 3d6ff1f..68bd3f2 100644 --- a/chrome/browser/history/history_types.h +++ b/chrome/browser/history/history_types.h @@ -461,7 +461,10 @@ struct QueryOptions { // Omit visits for which there is a more recent visit to the same URL on // the same day. Each URL will appear no more than once per day, where the // day is defined by the local timezone. - REMOVE_DUPLICATES_PER_DAY + REMOVE_DUPLICATES_PER_DAY, + + // Return all visits without deduping. + KEEP_ALL_DUPLICATES }; // Allows the caller to specify how duplicate URLs in the result set should diff --git a/chrome/browser/history/history_unittest.cc b/chrome/browser/history/history_unittest.cc index 5f8e5fd..fcb8aca 100644 --- a/chrome/browser/history/history_unittest.cc +++ b/chrome/browser/history/history_unittest.cc @@ -38,6 +38,7 @@ #include "base/path_service.h" #include "base/string_util.h" #include "base/stringprintf.h" +#include "base/threading/platform_thread.h" #include "base/time.h" #include "base/utf_string_conversions.h" #include "chrome/browser/history/download_row.h" @@ -65,6 +66,7 @@ #include "sync/api/sync_error_factory.h" #include "sync/api/sync_merge_result.h" #include "sync/protocol/history_delete_directive_specifics.pb.h" +#include "sync/protocol/sync.pb.h" #include "testing/gtest/include/gtest/gtest.h" #include "third_party/skia/include/core/SkBitmap.h" #include "ui/gfx/codec/jpeg_codec.h" @@ -1242,107 +1244,6 @@ TEST_F(HistoryTest, HistoryDBTaskCanceled) { ASSERT_FALSE(task->done_invoked); } -// Create a delete directive for a few specific history entries, -// including ones that don't exist. The expected entries should be -// deleted. -TEST_F(HistoryTest, ProcessGlobalIdDeleteDirective) { - ASSERT_TRUE(history_service_.get()); - const GURL test_url("http://www.google.com/"); - for (int64 i = 1; i <= 10; ++i) { - base::Time t = - base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(i); - history_service_->AddPage(test_url, t, NULL, 0, GURL(), - history::RedirectList(), - content::PAGE_TRANSITION_LINK, - history::SOURCE_BROWSED, false); - } - - EXPECT_TRUE(QueryURL(history_service_.get(), test_url)); - EXPECT_EQ(10, query_url_row_.visit_count()); - - sync_pb::HistoryDeleteDirectiveSpecifics delete_directive; - sync_pb::GlobalIdDirective* global_id_directive = - delete_directive.mutable_global_id_directive(); - global_id_directive->add_global_id( - (base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(0)) - .ToInternalValue()); - global_id_directive->add_global_id( - (base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(2)) - .ToInternalValue()); - global_id_directive->add_global_id( - (base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(5)) - .ToInternalValue()); - global_id_directive->add_global_id( - (base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(10)) - .ToInternalValue()); - global_id_directive->add_global_id( - (base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(20)) - .ToInternalValue()); - - history_service_->ProcessDeleteDirectiveForTest(delete_directive); - - EXPECT_TRUE(QueryURL(history_service_.get(), test_url)); - EXPECT_EQ(7, query_url_row_.visit_count()); -} - -// Create a delete directive for a given time range. The expected -// entries should be deleted. -TEST_F(HistoryTest, ProcessTimeRangeDeleteDirective) { - ASSERT_TRUE(history_service_.get()); - const GURL test_url("http://www.google.com/"); - for (int64 i = 1; i <= 10; ++i) { - base::Time t = - base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(i); - history_service_->AddPage(test_url, t, NULL, 0, GURL(), - history::RedirectList(), - content::PAGE_TRANSITION_LINK, - history::SOURCE_BROWSED, false); - } - - EXPECT_TRUE(QueryURL(history_service_.get(), test_url)); - EXPECT_EQ(10, query_url_row_.visit_count()); - - sync_pb::HistoryDeleteDirectiveSpecifics delete_directive; - sync_pb::TimeRangeDirective* time_range_directive = - delete_directive.mutable_time_range_directive(); - time_range_directive->set_start_time_usec(2); - time_range_directive->set_end_time_usec(9); - - history_service_->ProcessDeleteDirectiveForTest(delete_directive); - - EXPECT_TRUE(QueryURL(history_service_.get(), test_url)); - EXPECT_EQ(2, query_url_row_.visit_count()); -} - -// Create a local delete directive and process it while sync is -// offline. The expected entries should be deleted, and an error -// should be returned. -TEST_F(HistoryTest, ProcessLocalDeleteDirectiveSyncOffline) { - ASSERT_TRUE(history_service_.get()); - const GURL test_url("http://www.google.com/"); - for (int64 i = 1; i <= 10; ++i) { - base::Time t = - base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(i); - history_service_->AddPage(test_url, t, NULL, 0, GURL(), - history::RedirectList(), - content::PAGE_TRANSITION_LINK, - history::SOURCE_BROWSED, false); - } - - sync_pb::HistoryDeleteDirectiveSpecifics delete_directive; - sync_pb::GlobalIdDirective* global_id_directive = - delete_directive.mutable_global_id_directive(); - global_id_directive->add_global_id( - (base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(1)) - .ToInternalValue()); - - syncer::SyncError err = - history_service_->ProcessLocalDeleteDirective(delete_directive); - EXPECT_TRUE(err.IsSet()); - EXPECT_TRUE(QueryURL(history_service_.get(), test_url)); - EXPECT_EQ(9, query_url_row_.visit_count()); -} - // Dummy SyncChangeProcessor used to help review what SyncChanges are pushed // back up to Sync. // @@ -1399,9 +1300,8 @@ class SyncChangeProcessorDelegate : public syncer::SyncChangeProcessor { }; // Create a local delete directive and process it while sync is -// online, and then when offline. The expected entries should be -// deleted, the delete directive should be sent to sync, no error -// should be returned for the first time, and an error should be +// online, and then when offline. The delete directive should be sent to sync, +// no error should be returned for the first time, and an error should be // returned for the second time. TEST_F(HistoryTest, ProcessLocalDeleteDirectiveSyncOnline) { ASSERT_TRUE(history_service_.get()); @@ -1438,15 +1338,179 @@ TEST_F(HistoryTest, ProcessLocalDeleteDirectiveSyncOnline) { EXPECT_FALSE(err.IsSet()); EXPECT_EQ(1u, change_processor.GetChanges().size()); - EXPECT_TRUE(QueryURL(history_service_.get(), test_url)); - EXPECT_EQ(9, query_url_row_.visit_count()); - history_service_->StopSyncing(syncer::HISTORY_DELETE_DIRECTIVES); err = history_service_->ProcessLocalDeleteDirective(delete_directive); EXPECT_TRUE(err.IsSet()); EXPECT_EQ(1u, change_processor.GetChanges().size()); } +// Closure function that runs periodically to check result of delete directive +// processing. Stop when timeout or processing ends indicated by the creation +// of sync changes. +void CheckDirectiveProcessingResult( + Time timeout, const TestChangeProcessor* change_processor, + uint32 num_changes) { + if (base::Time::Now() > timeout || + change_processor->GetChanges().size() >= num_changes) { + return; + } + + base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100)); + MessageLoop::current()->PostTask( + FROM_HERE, + base::Bind(&CheckDirectiveProcessingResult, timeout, + change_processor, num_changes)); +} + +// Create a delete directive for a few specific history entries, +// including ones that don't exist. The expected entries should be +// deleted. +TEST_F(HistoryTest, ProcessGlobalIdDeleteDirective) { + ASSERT_TRUE(history_service_.get()); + const GURL test_url("http://www.google.com/"); + for (int64 i = 1; i <= 20; i++) { + base::Time t = + base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(i); + history_service_->AddPage(test_url, t, NULL, 0, GURL(), + history::RedirectList(), + content::PAGE_TRANSITION_LINK, + history::SOURCE_BROWSED, false); + } + + EXPECT_TRUE(QueryURL(history_service_.get(), test_url)); + EXPECT_EQ(20, query_url_row_.visit_count()); + + syncer::SyncDataList directives; + // 1st directive. + sync_pb::EntitySpecifics entity_specs; + sync_pb::GlobalIdDirective* global_id_directive = + entity_specs.mutable_history_delete_directive() + ->mutable_global_id_directive(); + global_id_directive->add_global_id( + (base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(6)) + .ToInternalValue()); + global_id_directive->set_start_time_usec(3); + global_id_directive->set_end_time_usec(10); + directives.push_back( + syncer::SyncData::CreateRemoteData(1, entity_specs)); + + // 2nd directive. + global_id_directive->Clear(); + global_id_directive->add_global_id( + (base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(17)) + .ToInternalValue()); + global_id_directive->set_start_time_usec(13); + global_id_directive->set_end_time_usec(19); + directives.push_back( + syncer::SyncData::CreateRemoteData(2, entity_specs)); + + TestChangeProcessor change_processor; + EXPECT_FALSE( + history_service_->MergeDataAndStartSyncing( + syncer::HISTORY_DELETE_DIRECTIVES, + directives, + scoped_ptr<syncer::SyncChangeProcessor>( + new SyncChangeProcessorDelegate(&change_processor)), + scoped_ptr<syncer::SyncErrorFactory>()).error().IsSet()); + + // Inject a task to check status and keep message loop filled before directive + // processing finishes. + MessageLoop::current()->PostTask( + FROM_HERE, + base::Bind(&CheckDirectiveProcessingResult, + base::Time::Now() + base::TimeDelta::FromSeconds(10), + &change_processor, 2)); + MessageLoop::current()->RunUntilIdle(); + EXPECT_TRUE(QueryURL(history_service_.get(), test_url)); + ASSERT_EQ(5, query_url_row_.visit_count()); + EXPECT_EQ(base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(1), + query_url_visits_[0].visit_time); + EXPECT_EQ(base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(2), + query_url_visits_[1].visit_time); + EXPECT_EQ(base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(11), + query_url_visits_[2].visit_time); + EXPECT_EQ(base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(12), + query_url_visits_[3].visit_time); + EXPECT_EQ(base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(20), + query_url_visits_[4].visit_time); + + // Expect two sync changes for deleting processed directives. + const syncer::SyncChangeList& sync_changes = change_processor.GetChanges(); + ASSERT_EQ(2u, sync_changes.size()); + EXPECT_EQ(syncer::SyncChange::ACTION_DELETE, sync_changes[0].change_type()); + EXPECT_EQ(1, sync_changes[0].sync_data().GetRemoteId()); + EXPECT_EQ(syncer::SyncChange::ACTION_DELETE, sync_changes[1].change_type()); + EXPECT_EQ(2, sync_changes[1].sync_data().GetRemoteId()); +} + +// Create delete directives for time ranges. The expected entries should be +// deleted. +TEST_F(HistoryTest, ProcessTimeRangeDeleteDirective) { + ASSERT_TRUE(history_service_.get()); + const GURL test_url("http://www.google.com/"); + for (int64 i = 1; i <= 10; ++i) { + base::Time t = + base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(i); + history_service_->AddPage(test_url, t, NULL, 0, GURL(), + history::RedirectList(), + content::PAGE_TRANSITION_LINK, + history::SOURCE_BROWSED, false); + } + + EXPECT_TRUE(QueryURL(history_service_.get(), test_url)); + EXPECT_EQ(10, query_url_row_.visit_count()); + + syncer::SyncDataList directives; + // 1st directive. + sync_pb::EntitySpecifics entity_specs; + sync_pb::TimeRangeDirective* time_range_directive = + entity_specs.mutable_history_delete_directive() + ->mutable_time_range_directive(); + time_range_directive->set_start_time_usec(2); + time_range_directive->set_end_time_usec(5); + directives.push_back(syncer::SyncData::CreateRemoteData(1, entity_specs)); + + // 2nd directive. + time_range_directive->Clear(); + time_range_directive->set_start_time_usec(8); + time_range_directive->set_end_time_usec(10); + directives.push_back(syncer::SyncData::CreateRemoteData(2, entity_specs)); + + TestChangeProcessor change_processor; + EXPECT_FALSE( + history_service_->MergeDataAndStartSyncing( + syncer::HISTORY_DELETE_DIRECTIVES, + directives, + scoped_ptr<syncer::SyncChangeProcessor>( + new SyncChangeProcessorDelegate(&change_processor)), + scoped_ptr<syncer::SyncErrorFactory>()).error().IsSet()); + + // Inject a task to check status and keep message loop filled before + // directive processing finishes. + MessageLoop::current()->PostTask( + FROM_HERE, + base::Bind(&CheckDirectiveProcessingResult, + base::Time::Now() + base::TimeDelta::FromSeconds(10), + &change_processor, 2)); + MessageLoop::current()->RunUntilIdle(); + EXPECT_TRUE(QueryURL(history_service_.get(), test_url)); + ASSERT_EQ(3, query_url_row_.visit_count()); + EXPECT_EQ(base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(1), + query_url_visits_[0].visit_time); + EXPECT_EQ(base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(6), + query_url_visits_[1].visit_time); + EXPECT_EQ(base::Time::UnixEpoch() + base::TimeDelta::FromMicroseconds(7), + query_url_visits_[2].visit_time); + + // Expect two sync changes for deleting processed directives. + const syncer::SyncChangeList& sync_changes = change_processor.GetChanges(); + ASSERT_EQ(2u, sync_changes.size()); + EXPECT_EQ(syncer::SyncChange::ACTION_DELETE, sync_changes[0].change_type()); + EXPECT_EQ(1, sync_changes[0].sync_data().GetRemoteId()); + EXPECT_EQ(syncer::SyncChange::ACTION_DELETE, sync_changes[1].change_type()); + EXPECT_EQ(2, sync_changes[1].sync_data().GetRemoteId()); +} + TEST_F(HistoryBackendDBTest, MigratePresentations) { // Create the db we want. Use 22 since segments didn't change in that time // frame. diff --git a/chrome/browser/history/visit_database.cc b/chrome/browser/history/visit_database.cc index 289ce9d..ff76b6a 100644 --- a/chrome/browser/history/visit_database.cc +++ b/chrome/browser/history/visit_database.cc @@ -348,21 +348,21 @@ bool VisitDatabase::GetVisibleVisitsInRange(const QueryOptions& options, VisitRow visit; FillVisitRow(statement, &visit); - if (options.duplicate_policy == QueryOptions::REMOVE_DUPLICATES_PER_DAY && - found_urls_midnight != visit.visit_time.LocalMidnight()) { - found_urls.clear(); - found_urls_midnight = visit.visit_time.LocalMidnight(); - } - - // Make sure the URL this visit corresponds to is unique. - if (found_urls.find(visit.url_id) == found_urls.end()) { + if (options.duplicate_policy != QueryOptions::KEEP_ALL_DUPLICATES) { + if (options.duplicate_policy == QueryOptions::REMOVE_DUPLICATES_PER_DAY && + found_urls_midnight != visit.visit_time.LocalMidnight()) { + found_urls.clear(); + found_urls_midnight = visit.visit_time.LocalMidnight(); + } + // Make sure the URL this visit corresponds to is unique. + if (found_urls.find(visit.url_id) != found_urls.end()) + continue; found_urls.insert(visit.url_id); - - if (static_cast<int>(visits->size()) >= options.EffectiveMaxCount()) - return true; - - visits->push_back(visit); } + + if (static_cast<int>(visits->size()) >= options.EffectiveMaxCount()) + return true; + visits->push_back(visit); } return false; } diff --git a/chrome/browser/history/visit_database_unittest.cc b/chrome/browser/history/visit_database_unittest.cc index bdc33fc..77a5440 100644 --- a/chrome/browser/history/visit_database_unittest.cc +++ b/chrome/browser/history/visit_database_unittest.cc @@ -321,6 +321,15 @@ TEST_F(VisitDatabaseTest, GetVisibleVisitsInRange) { EXPECT_TRUE(IsVisitInfoEqual(results[1], test_visit_rows[3])); EXPECT_TRUE(IsVisitInfoEqual(results[2], test_visit_rows[1])); + // Now try without de-duping, expect to see all visible visits. + options.duplicate_policy = QueryOptions::KEEP_ALL_DUPLICATES; + GetVisibleVisitsInRange(options, &results); + ASSERT_EQ(static_cast<size_t>(4), results.size()); + EXPECT_TRUE(IsVisitInfoEqual(results[0], test_visit_rows[5])); + EXPECT_TRUE(IsVisitInfoEqual(results[1], test_visit_rows[3])); + EXPECT_TRUE(IsVisitInfoEqual(results[2], test_visit_rows[1])); + EXPECT_TRUE(IsVisitInfoEqual(results[3], test_visit_rows[0])); + // Set the end time to exclude the second visit. The first visit should be // returned. Even though the second is a more recent visit, it's not in the // query range. diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index acb79d8..6a52c7d 100644 --- a/chrome/chrome_browser.gypi +++ b/chrome/chrome_browser.gypi @@ -763,6 +763,8 @@ 'browser/history/android/visit_sql_handler.h', 'browser/history/archived_database.cc', 'browser/history/archived_database.h', + 'browser/history/delete_directive_handler.cc', + 'browser/history/delete_directive_handler.h', 'browser/history/download_database.cc', 'browser/history/download_database.h', 'browser/history/download_row.cc', diff --git a/sync/api/sync_data.h b/sync/api/sync_data.h index c3404c3..33e41a8 100644 --- a/sync/api/sync_data.h +++ b/sync/api/sync_data.h @@ -117,6 +117,8 @@ class SYNC_EXPORT SyncData { // gmock printer helper. void PrintTo(const SyncData& sync_data, std::ostream* os); +typedef std::vector<SyncData> SyncDataList; + } // namespace syncer #endif // SYNC_API_SYNC_DATA_H_ diff --git a/sync/api/syncable_service.h b/sync/api/syncable_service.h index 8d187fc..13c282ef 100644 --- a/sync/api/syncable_service.h +++ b/sync/api/syncable_service.h @@ -21,8 +21,6 @@ namespace syncer { class SyncErrorFactory; -typedef std::vector<SyncData> SyncDataList; - // TODO(zea): remove SupportsWeakPtr in favor of having all SyncableService // implementers provide a way of getting a weak pointer to themselves. // See crbug.com/100114. |