Resolve TODO for changing all_timestamps to use base::Time

Convert all_timestamps to use base::Time and update all related code to use
base::Time values.

Bug: 335282446
Change-Id: Ifa73a9f353e954fdc9c2d4e70968cc423a1f2ae0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6169127
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1420525}

AUTHORS

@@ -1030,6 +1030,7 @@ Mohamed Mansour <m0.interactive@gmail.com>
 Mohamed Hany Youns <mohamedhyouns@gmail.com>
 Mohammad Azam <m.azam@samsung.com>
 MohammadSabri <mohammad.kh.sabri@exalt.ps>
+Mohammed Ashraf <mohammedashraf4599@gmail.com>
 Mohammed Wajahat Ali Siddiqui <wajahat.s@samsung.com>
 Mohan Reddy <mohan.reddy@samsung.com>
 Mohit Bhalla <bhallam@amazon.com>

chrome/browser/android/history/browsing_history_bridge.cc

@@ -127,11 +127,12 @@ void BrowsingHistoryBridge::OnQueryComplete(
 
     // This relies on |all_timestamps| being a sorted data structure.
     int64_t most_recent_java_timestamp =
-        base::Time::FromInternalValue(*entry.all_timestamps.rbegin())
-            .InMillisecondsSinceUnixEpoch();
-    std::vector<int64_t> native_timestamps(entry.all_timestamps.begin(),
-                                           entry.all_timestamps.end());
-
+        entry.all_timestamps.rbegin()->InMillisecondsSinceUnixEpoch();
+    std::vector<int64_t> native_timestamps;
+    for (const base::Time& val : entry.all_timestamps) {
+      native_timestamps.push_back(
+          val.ToDeltaSinceWindowsEpoch().InMicroseconds());
+    }
     Java_BrowsingHistoryBridge_createHistoryItemAndAddToList(
         env, j_query_result_obj_,
         url::GURLAndroid::FromNativeGURL(env, entry.url),
@@ -165,8 +166,10 @@ void BrowsingHistoryBridge::MarkItemForRemoval(
   entry.app_id = j_app_id
                      ? base::android::ConvertJavaStringToUTF8(env, j_app_id)
                      : history::kNoAppIdFilter;
-  entry.all_timestamps.insert(timestamps.begin(), timestamps.end());
-
+  for (int64_t val : timestamps) {
+    entry.all_timestamps.insert(
+        base::Time::FromDeltaSinceWindowsEpoch(base::Microseconds(val)));
+  }
   items_to_remove_.push_back(entry);
 }
 

chrome/browser/sync/test/integration/two_client_history_sync_test.cc

@@ -299,8 +299,8 @@ IN_PROC_BROWSER_TEST_F(TwoClientHistorySyncTest, SyncsVisitsDeletion) {
   // Logic similar to `BrowsingHistoryHandler::HandleRemoveVisits()`.
   history::BrowsingHistoryService::HistoryEntry entry1;
   entry1.url = url1;
-  entry1.all_timestamps.insert(time1a.ToInternalValue());
-  entry1.all_timestamps.insert(time1b.ToInternalValue());
+  entry1.all_timestamps.insert(time1a);
+  entry1.all_timestamps.insert(time1b);
   std::vector<history::BrowsingHistoryService::HistoryEntry> items_to_remove;
   items_to_remove.push_back(entry1);
 

chrome/browser/ui/webui/history/browsing_history_handler.cc

@@ -237,10 +237,8 @@ history::mojom::HistoryEntryPtr HistoryEntryToMojom(
 
   // Pass the timestamps in a list.
   std::vector<double> timestamps;
-  for (int64_t timestamp : entry.all_timestamps) {
-    timestamps.push_back(
-        base::Time::FromDeltaSinceWindowsEpoch(base::Microseconds(timestamp))
-            .InMillisecondsFSinceUnixEpoch());
+  for (const base::Time& timestamp : entry.all_timestamps) {
+    timestamps.push_back(timestamp.InMillisecondsFSinceUnixEpoch());
   }
   result_mojom->all_timestamps = std::move(timestamps);
 
@@ -436,7 +434,7 @@ void BrowsingHistoryHandler::RemoveVisits(
     for (const auto& timestamp : timestamps) {
       base::Time visit_time =
           base::Time::FromMillisecondsSinceUnixEpoch(timestamp);
-      entry.all_timestamps.insert(visit_time.ToInternalValue());
+      entry.all_timestamps.insert(visit_time);
     }
 
     items_to_remove.push_back(entry);

chrome/browser/ui/webui/history_clusters/history_clusters_handler.cc

@@ -390,14 +390,13 @@ void HistoryClustersHandler::RemoveVisits(
     {
       history::BrowsingHistoryService::HistoryEntry entry;
       entry.url = visit->raw_visit_data->url;
-      entry.all_timestamps.insert(
-          visit->raw_visit_data->visit_time.ToInternalValue());
+      entry.all_timestamps.insert(visit->raw_visit_data->visit_time);
       items_to_remove.push_back(std::move(entry));
     }
     for (const auto& duplicate : visit->duplicates) {
       history::BrowsingHistoryService::HistoryEntry entry;
       entry.url = duplicate->url;
-      entry.all_timestamps.insert(duplicate->visit_time.ToInternalValue());
+      entry.all_timestamps.insert(duplicate->visit_time);
       items_to_remove.push_back(std::move(entry));
     }
   }
@@ -432,8 +431,7 @@ void HistoryClustersHandler::RemoveVisitByUrlAndTime(
   history::BrowsingHistoryService::HistoryEntry entry;
   entry.url = url;
   base::Time visit_time = base::Time::FromMillisecondsSinceUnixEpoch(timestamp);
-  entry.all_timestamps.insert(
-      visit_time.ToDeltaSinceWindowsEpoch().InMicroseconds());
+  entry.all_timestamps.insert(visit_time);
   browsing_history_service_->RemoveVisits({entry});
 }
 

components/history/core/browser/browsing_history_service.cc

@@ -134,7 +134,7 @@ BrowsingHistoryService::HistoryEntry::HistoryEntry(
       visit_count(visit_count),
       typed_count(typed_count),
       app_id(app_id) {
-  all_timestamps.insert(time.ToInternalValue());
+  all_timestamps.insert(time);
 }
 
 BrowsingHistoryService::HistoryEntry::HistoryEntry()
@@ -175,8 +175,9 @@ BrowsingHistoryService::BrowsingHistoryService(
   DCHECK(driver_);
 
   // Get notifications when history is cleared.
-  if (local_history_)
+  if (local_history_) {
     history_service_observation_.Observe(local_history_.get());
+  }
 
   // Get notifications when web history is deleted.
   WebHistoryService* web_history = driver_->GetWebHistoryService();
@@ -223,8 +224,9 @@ void BrowsingHistoryService::WebHistoryTimeout(
 
   // Don't reset `web_history_request_` so we can still record histogram.
   // TODO(dubroy): Communicate the failure to the front end.
-  if (!query_task_tracker_.HasTrackedTasks())
+  if (!query_task_tracker_.HasTrackedTasks()) {
     ReturnResultsToDriver(std::move(state));
+  }
 }
 
 void BrowsingHistoryService::QueryHistory(const std::u16string& search_text,
@@ -407,17 +409,17 @@ void BrowsingHistoryService::RemoveVisits(
         delete_directive.mutable_global_id_directive();
     ExpireHistoryArgs* expire_args = nullptr;
 
-    for (int64_t timestamp : entry.all_timestamps) {
+    for (base::Time timestamp : entry.all_timestamps) {
       if (!expire_args) {
         GURL gurl(entry.url);
         expire_list.resize(expire_list.size() + 1);
         expire_args = &expire_list.back();
-        expire_args->SetTimeRangeForOneDay(
-            base::Time::FromInternalValue(timestamp));
+        expire_args->SetTimeRangeForOneDay(timestamp);
         expire_args->urls.insert(gurl);
       }
       // The local visit time is treated as a global ID for the visit.
-      global_id_directive->add_global_id(timestamp);
+      global_id_directive->add_global_id(
+          timestamp.ToDeltaSinceWindowsEpoch().InMicroseconds());
     }
 
     // Set the start and end time in microseconds since the Unix epoch.
@@ -435,8 +437,9 @@ void BrowsingHistoryService::RemoveVisits(
     expire_args->restrict_app_id = entry.app_id;
 
     // TODO(dubroy): Figure out the proper way to handle an error here.
-    if (web_history && local_history_)
+    if (web_history && local_history_) {
       local_history_->ProcessLocalDeleteDirective(delete_directive);
+    }
   }
 
   if (local_history_) {
@@ -616,8 +619,9 @@ void BrowsingHistoryService::QueryComplete(
   state->local_status =
       results.reached_beginning() ? REACHED_BEGINNING : MORE_RESULTS;
 
-  if (!web_history_timer_->IsRunning())
+  if (!web_history_timer_->IsRunning()) {
     ReturnResultsToDriver(std::move(state));
+  }
 }
 
 void BrowsingHistoryService::OnGetAllAppIds(GetAllAppIdsResult result) {
@@ -668,8 +672,9 @@ void BrowsingHistoryService::WebHistoryQueryComplete(
     base::optional_ref<const base::Value::Dict> results_dict) {
   // If the response came in too late, do nothing.
   // TODO(dubroy): Maybe show a banner, and prompt the user to reload?
-  if (!web_history_timer_->IsRunning())
+  if (!web_history_timer_->IsRunning()) {
     return;
+  }
   web_history_timer_->Stop();
   web_history_request_.reset();
 
@@ -705,13 +710,15 @@ void BrowsingHistoryService::WebHistoryQueryComplete(
         if (state->original_options.host_only) {
           // Do post filter to skip entries that do not have the correct
           // hostname.
-          if (gurl.host() != host_name_utf8)
+          if (gurl.host() != host_name_utf8) {
             continue;
+          }
         }
 
         // Ignore any URLs that should not be shown in the history page.
-        if (driver_->ShouldHideWebHistoryUrl(gurl))
+        if (driver_->ShouldHideWebHistoryUrl(gurl)) {
           continue;
+        }
 
         std::u16string title;
 
@@ -765,8 +772,9 @@ void BrowsingHistoryService::WebHistoryQueryComplete(
     state->remote_status = FAILURE;
   }
 
-  if (!query_task_tracker_.HasTrackedTasks())
+  if (!query_task_tracker_.HasTrackedTasks()) {
     ReturnResultsToDriver(std::move(state));
+  }
 }
 
 void BrowsingHistoryService::OtherFormsOfBrowsingHistoryQueryComplete(
@@ -779,16 +787,18 @@ void BrowsingHistoryService::OtherFormsOfBrowsingHistoryQueryComplete(
 void BrowsingHistoryService::RemoveComplete() {
   // Notify the driver that the deletion request is complete, but only if
   // web history delete request is not still pending.
-  if (!has_pending_delete_request_)
+  if (!has_pending_delete_request_) {
     driver_->OnRemoveVisitsComplete();
+  }
 }
 
 void BrowsingHistoryService::RemoveWebHistoryComplete(bool success) {
   has_pending_delete_request_ = false;
   // TODO(dubroy): Should we handle failure somehow? Delete directives will
   // ensure that the visits are eventually deleted, so maybe it's not necessary.
-  if (!delete_task_tracker_.HasTrackedTasks())
+  if (!delete_task_tracker_.HasTrackedTasks()) {
     RemoveComplete();
+  }
 }
 
 void BrowsingHistoryService::OnHistoryDeletions(

components/history/core/browser/browsing_history_service.h

@@ -94,8 +94,7 @@ class BrowsingHistoryService : public HistoryServiceObserver,
     std::string client_id;
 
     // Timestamps of all local or remote visits the same URL on the same day.
-    // TODO(skym): These should probably be converted to base::Time.
-    std::set<int64_t> all_timestamps;
+    std::set<base::Time> all_timestamps;
 
     // If true, this entry is a search result.
     bool is_search_result;

ios/chrome/browser/history/ui_bundled/base_history_view_controller.mm

@@ -470,8 +470,7 @@ static const base::TimeDelta kDelayUntilReadyToRemoveLoadingIndicatorsMs =
         [self.tableViewModel itemAtIndexPath:indexPath]);
     BrowsingHistoryService::HistoryEntry entry;
     entry.url = object.URL;
-    // TODO(crbug.com/40479288) Remove base::TimeXXX::ToInternalValue().
-    entry.all_timestamps.insert(object.timestamp.ToInternalValue());
+    entry.all_timestamps.insert(object.timestamp);
     entries.push_back(entry);
   }
   self.historyService->RemoveVisits(entries);