Removed duplicate timestamp code from places component

CHANGES_UNRELEASED.md

@@ -8,3 +8,8 @@
 
 ### What's changed ###
 - Added a basic API and database layer for the autofill component. ([#3582](https://github.com/mozilla/application-services/pull/3582))
+
+## Places
+
+### What's changed
+- Removed the duplicate Timestamp logic from Places, which now exists in Support, and updated the references. ([#3593](https://github.com/mozilla/application-services/pull/3593))

Cargo.lock

@@ -700,6 +700,7 @@ dependencies = [
  "sql-support",
  "tempfile",
  "termion",
+ "types",
  "url",
 ]
 
@@ -721,6 +722,7 @@ dependencies = [
  "sync-guid",
  "sync15",
  "tempdir",
+ "types",
  "url",
 ]
 
@@ -1949,6 +1951,7 @@ dependencies = [
  "sync15",
  "tempfile",
  "thiserror",
+ "types",
  "url",
 ]
 
@@ -1961,6 +1964,7 @@ dependencies = [
  "serde_json",
  "sql-support",
  "tempdir",
+ "types",
  "url",
 ]
 
@@ -1978,6 +1982,7 @@ dependencies = [
  "sql-support",
  "sync-guid",
  "sync15",
+ "types",
  "url",
  "viaduct",
 ]
@@ -1995,6 +2000,7 @@ dependencies = [
  "sync-guid",
  "sync15",
  "tempfile",
+ "types",
  "url",
 ]
 

components/places/Cargo.toml

@@ -21,6 +21,7 @@ url = { version = "2.1", features = ["serde"] }
 percent-encoding = "2.1"
 caseless = "0.2"
 sql-support = { path = "../support/sql" }
+types = { path = "../support/types" }
 ffi-support = "0.4"
 bitflags = "1.2"
 idna = "0.2"

components/places/ffi/Cargo.toml

@@ -23,6 +23,7 @@ viaduct = { path = "../../viaduct" }
 interrupt-support = { path = "../../support/interrupt" }
 sql-support = { path = "../../support/sql" }
 sync-guid = { path = "../../support/guid" }
+types = { path = "../../support/types" }
 
 [dependencies.sync15]
 path = "../../sync15"

components/places/ffi/src/lib.rs

@@ -271,8 +271,8 @@ pub extern "C" fn places_get_visited_urls_in_range(
         let visited = storage::history::get_visited_urls(
             conn,
             // Probably should allow into()...
-            places::Timestamp(start.max(0) as u64),
-            places::Timestamp(end.max(0) as u64),
+            types::Timestamp(start.max(0) as u64),
+            types::Timestamp(end.max(0) as u64),
             include_remote != 0,
         )?;
         Ok(serde_json::to_string(&visited)?)
@@ -309,8 +309,8 @@ pub extern "C" fn places_delete_visits_between(
     CONNECTIONS.call_with_result(error, handle, |conn| -> places::Result<_> {
         storage::history::delete_visits_between(
             conn,
-            places::Timestamp(start.max(0) as u64),
-            places::Timestamp(end.max(0) as u64),
+            types::Timestamp(start.max(0) as u64),
+            types::Timestamp(end.max(0) as u64),
         )?;
         Ok(())
     })
@@ -330,7 +330,7 @@ pub extern "C" fn places_delete_visit(
                 storage::history::delete_place_visit_at_time(
                     conn,
                     &url,
-                    places::Timestamp(timestamp.max(0) as u64),
+                    types::Timestamp(timestamp.max(0) as u64),
                 )?;
             }
             Err(e) => {
@@ -338,7 +338,7 @@ pub extern "C" fn places_delete_visit(
                 storage::history::delete_place_visit_at_time_by_href(
                     conn,
                     url.as_str(),
-                    places::Timestamp(timestamp.max(0) as u64),
+                    types::Timestamp(timestamp.max(0) as u64),
                 )?;
             }
         };
@@ -400,8 +400,8 @@ pub extern "C" fn places_get_visit_infos(
     CONNECTIONS.call_with_result(error, handle, |conn| -> places::Result<_> {
         Ok(storage::history::get_visit_infos(
             conn,
-            places::Timestamp(start_date.max(0) as u64),
-            places::Timestamp(end_date.max(0) as u64),
+            types::Timestamp(start_date.max(0) as u64),
+            types::Timestamp(end_date.max(0) as u64),
             VisitTransitionSet::from_u16(exclude_types as u16)
                 .expect("Bug: Invalid VisitTransitionSet"),
         )?)

components/places/src/api/history.rs

@@ -7,6 +7,7 @@ use crate::db::PlacesDb;
 use crate::error::*;
 use crate::observation::VisitObservation;
 use crate::types::*;
+use types::Timestamp;
 use url::Url;
 // This module can become, roughly: PlacesUtils.history()
 

components/places/src/api/matcher.rs

@@ -615,7 +615,8 @@ mod tests {
     use crate::api::places_api::test::new_mem_connection;
     use crate::observation::VisitObservation;
     use crate::storage::history::apply_observation;
-    use crate::types::{Timestamp, VisitTransition};
+    use crate::types::VisitTransition;
+    use types::Timestamp;
 
     #[test]
     fn split() {

components/places/src/bookmark_sync/store.rs

@@ -19,7 +19,7 @@ use crate::storage::{
     },
     delete_pending_temp_tables, get_meta, put_meta,
 };
-use crate::types::{BookmarkType, SyncStatus, Timestamp};
+use crate::types::{BookmarkType, SyncStatus};
 use dogear::{
     self, AbortSignal, CompletionOps, Content, Item, MergedRoot, TelemetryEvent, Tree, UploadItem,
     UploadTombstone,
@@ -35,6 +35,7 @@ use sync15::{
     ServerTimestamp, Store, StoreSyncAssociation,
 };
 use sync_guid::Guid as SyncGuid;
+use types::Timestamp;
 pub const LAST_SYNC_META_KEY: &str = "bookmarks_last_sync_time";
 // Note that all engines in this crate should use a *different* meta key
 // for the global sync ID, because engines are reset individually.

components/places/src/bookmark_sync/tests/synced_item.rs

@@ -7,10 +7,10 @@ use crate::{
     db::PlacesDb,
     error::*,
     storage::RowId,
-    types::Timestamp,
 };
 use rusqlite::Row;
 use sync_guid::Guid as SyncGuid;
+use types::Timestamp;
 
 use sql_support::{self, ConnExt};
 use sync15::ServerTimestamp;

components/places/src/db/db.rs

@@ -278,10 +278,10 @@ pub(crate) mod sql_fns {
     use crate::api::matcher::{split_after_host_and_port, split_after_prefix};
     use crate::hash;
     use crate::match_impl::{AutocompleteMatch, MatchBehavior, SearchBehavior};
-    use crate::types::Timestamp;
     use rusqlite::{functions::Context, types::ValueRef, Error, Result};
     use std::sync::atomic::Ordering;
     use sync_guid::Guid as SyncGuid;
+    use types::Timestamp;
 
     // Helpers for define_functions
     fn get_raw_str<'a>(ctx: &'a Context<'_>, fname: &'static str, idx: usize) -> Result<&'a str> {

components/places/src/frecency.rs

@@ -3,8 +3,9 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use crate::error::*;
-use crate::types::{Timestamp, VisitTransition};
+use crate::types::VisitTransition;
 use rusqlite::Connection;
+use types::Timestamp;
 
 #[derive(Debug, Clone, Copy, PartialEq)]
 enum RedirectBonus {

components/places/src/history_sync/mod.rs

@@ -2,10 +2,10 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-use crate::types::Timestamp;
 use serde_derive::*;
 use std::fmt;
 use std::time::{SystemTime, UNIX_EPOCH};
+use types::Timestamp;
 
 mod plan;
 pub mod record;

components/places/src/history_sync/plan.rs

@@ -14,13 +14,14 @@ use crate::storage::{
         fetch_visits, finish_outgoing, FetchedVisit, FetchedVisitPage, OutgoingInfo,
     },
 };
-use crate::types::{Timestamp, VisitTransition};
+use crate::types::VisitTransition;
 use interrupt_support::Interruptee;
 use std::collections::HashSet;
 use std::time::{SystemTime, UNIX_EPOCH};
 use sync15::telemetry;
 use sync15::{IncomingChangeset, OutgoingChangeset, Payload};
 use sync_guid::Guid as SyncGuid;
+use types::Timestamp;
 use url::Url;
 
 /// Clamps a history visit date between the current date and the earliest
@@ -301,12 +302,13 @@ mod tests {
     use crate::observation::VisitObservation;
     use crate::storage::history::history_sync::fetch_visits;
     use crate::storage::history::{apply_observation, delete_visits_for, url_to_guid};
-    use crate::types::{SyncStatus, Timestamp};
+    use crate::types::SyncStatus;
     use interrupt_support::NeverInterrupts;
     use serde_json::json;
     use sql_support::ConnExt;
     use std::time::Duration;
     use sync15::{IncomingChangeset, ServerTimestamp};
+    use types::Timestamp;
     use url::Url;
 
     fn get_existing_guid(conn: &PlacesDb, url: &Url) -> SyncGuid {

components/places/src/import/common.rs

@@ -4,8 +4,8 @@
 
 use crate::api::places_api::SyncConn;
 use crate::error::*;
-use crate::types::Timestamp;
 use rusqlite::named_params;
+use types::Timestamp;
 use url::Url;
 
 // sanitize_timestamp can't use `Timestamp::now();` directly because it needs
@@ -23,9 +23,9 @@ lazy_static::lazy_static! {
 pub mod sql_fns {
     use crate::import::common::NOW;
     use crate::storage::URL_LENGTH_MAX;
-    use crate::types::Timestamp;
     use rusqlite::{functions::Context, types::ValueRef, Result};
     use std::convert::TryFrom;
+    use types::Timestamp;
     use url::Url;
 
     #[inline(never)]

components/places/src/observation.rs

@@ -4,6 +4,7 @@
 
 use crate::types::*;
 use serde_derive::*;
+use types::Timestamp;
 use url::Url;
 
 /// An "observation" based model for updating history.

components/places/src/storage/bookmarks.rs

@@ -10,7 +10,7 @@ use crate::bookmark_sync::store::{
 };
 use crate::db::PlacesDb;
 use crate::error::*;
-use crate::types::{BookmarkType, SyncStatus, Timestamp};
+use crate::types::{BookmarkType, SyncStatus};
 use rusqlite::types::ToSql;
 use rusqlite::{Connection, Row};
 use serde::{
@@ -25,6 +25,7 @@ use std::cmp::{max, min};
 use std::collections::HashMap;
 use sync15::StoreSyncAssociation;
 use sync_guid::Guid as SyncGuid;
+use types::Timestamp;
 use url::Url;
 
 pub use public_node::PublicNode;

components/places/src/storage/history.rs

@@ -16,13 +16,14 @@ use crate::msg_types::{
 };
 use crate::observation::VisitObservation;
 use crate::storage::{delete_meta, delete_pending_temp_tables, get_meta, put_meta};
-use crate::types::{SyncStatus, Timestamp, VisitTransition, VisitTransitionSet};
+use crate::types::{SyncStatus, VisitTransition, VisitTransitionSet};
 use rusqlite::types::ToSql;
 use rusqlite::Result as RusqliteResult;
 use rusqlite::{Row, NO_PARAMS};
 use sql_support::{self, ConnExt};
 use sync15::StoreSyncAssociation;
 use sync_guid::Guid as SyncGuid;
+use types::Timestamp;
 use url::Url;
 
 /// When `delete_everything` is called (to perform a permanent local deletion), in
@@ -1367,10 +1368,11 @@ mod tests {
     use super::*;
     use crate::api::places_api::ConnectionType;
     use crate::history_sync::record::{HistoryRecord, HistoryRecordVisit};
-    use crate::types::{Timestamp, VisitTransitionSet};
+    use crate::types::VisitTransitionSet;
     use pretty_assertions::assert_eq;
     use std::time::{Duration, SystemTime};
     use sync15::CollSyncIds;
+    use types::Timestamp;
 
     #[test]
     fn test_get_visited_urls() {

components/places/src/storage/mod.rs

@@ -12,7 +12,7 @@ pub mod tags;
 use crate::db::PlacesDb;
 use crate::error::{ErrorKind, InvalidPlaceInfo, Result};
 use crate::msg_types::{HistoryVisitInfo, TopFrecentSiteInfo};
-use crate::types::{SyncStatus, Timestamp, VisitTransition};
+use crate::types::{SyncStatus, VisitTransition};
 use rusqlite::types::{FromSql, FromSqlResult, ToSql, ToSqlOutput, ValueRef};
 use rusqlite::Result as RusqliteResult;
 use rusqlite::Row;
@@ -20,6 +20,7 @@ use serde_derive::*;
 use sql_support::{self, ConnExt};
 use std::fmt;
 use sync_guid::Guid as SyncGuid;
+use types::Timestamp;
 use url::Url;
 
 /// From https://searchfox.org/mozilla-central/rev/93905b660f/toolkit/components/places/PlacesUtils.jsm#189

components/places/src/tests.rs

@@ -8,10 +8,11 @@ use serde_json::Value;
 use crate::{
     db::PlacesDb,
     storage::bookmarks::{fetch_tree, get_raw_bookmark, insert_tree, BookmarkTreeNode, FetchDepth},
-    types::{BookmarkType, Timestamp},
+    types::BookmarkType,
 };
 use sql_support::ConnExt;
 use sync_guid::Guid as SyncGuid;
+use types::Timestamp;
 
 use pretty_assertions::assert_eq;
 

components/places/src/types.rs

@@ -5,100 +5,12 @@
 use rusqlite::types::{FromSql, FromSqlError, FromSqlResult, ToSql, ToSqlOutput, ValueRef};
 use rusqlite::Result as RusqliteResult;
 use serde::ser::{Serialize, Serializer};
-use serde_derive::*;
 use std::convert::TryFrom;
 use std::fmt;
-use std::time::{Duration, SystemTime, UNIX_EPOCH};
 
 mod visit_transition_set;
 pub use visit_transition_set::VisitTransitionSet;
 
-// Typesafe way to manage timestamps.
-// We should probably work out how to share this too?
-#[derive(
-    Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Deserialize, Serialize, Default,
-)]
-pub struct Timestamp(pub u64);
-
-impl Timestamp {
-    pub fn now() -> Self {
-        SystemTime::now().into()
-    }
-
-    /// Returns None if `other` is later than `self` (Duration may not represent
-    /// negative timespans in rust).
-    #[inline]
-    pub fn duration_since(self, other: Timestamp) -> Option<Duration> {
-        // just do this via SystemTime.
-        SystemTime::from(self).duration_since(other.into()).ok()
-    }
-
-    #[inline]
-    pub fn checked_sub(self, d: Duration) -> Option<Timestamp> {
-        SystemTime::from(self).checked_sub(d).map(Timestamp::from)
-    }
-
-    pub fn as_millis(self) -> u64 {
-        self.0
-    }
-    /// In desktop sync, bookmarks are clamped to Jan 23, 1993 (which is 727747200000)
-    /// There's no good reason history records could be older than that, so we do
-    /// the same here (even though desktop's history currently doesn't)
-    /// XXX - there's probably a case to be made for this being, say, 5 years ago -
-    /// then all requests earlier than that are collapsed into a single visit at
-    /// this timestamp.
-    pub const EARLIEST: Timestamp = Timestamp(727_747_200_000);
-}
-
-impl From<Timestamp> for u64 {
-    #[inline]
-    fn from(ts: Timestamp) -> Self {
-        ts.0
-    }
-}
-
-impl From<SystemTime> for Timestamp {
-    #[inline]
-    fn from(st: SystemTime) -> Self {
-        let d = st.duration_since(UNIX_EPOCH).unwrap(); // hrmph - unwrap doesn't seem ideal
-        Timestamp((d.as_secs() as u64) * 1000 + (u64::from(d.subsec_nanos()) / 1_000_000))
-    }
-}
-
-impl From<Timestamp> for SystemTime {
-    #[inline]
-    fn from(ts: Timestamp) -> Self {
-        UNIX_EPOCH + Duration::from_millis(ts.into())
-    }
-}
-
-impl From<u64> for Timestamp {
-    #[inline]
-    fn from(ts: u64) -> Self {
-        assert!(ts != 0);
-        Timestamp(ts)
-    }
-}
-
-impl fmt::Display for Timestamp {
-    #[inline]
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "{}", self.0)
-    }
-}
-
-impl ToSql for Timestamp {
-    fn to_sql(&self) -> RusqliteResult<ToSqlOutput<'_>> {
-        Ok(ToSqlOutput::from(self.0 as i64)) // hrm - no u64 in rusqlite
-    }
-}
-
-impl FromSql for Timestamp {
-    fn column_result(value: ValueRef<'_>) -> FromSqlResult<Self> {
-        value.as_i64().map(|v| Timestamp(v as u64)) // hrm - no u64
-    }
-}
-
 #[derive(thiserror::Error, Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
 #[error("Invalid visit type")]
 pub struct InvalidVisitType;

examples/places-autocomplete/Cargo.toml

@@ -16,6 +16,7 @@ log = "0.4"
 url = "2.1"
 anyhow = "1.0"
 sql-support = { path = "../../components/support/sql" }
+types = { path = "../../components/support/types" }
 clap = "2.33"
 tempfile = "3.1"
 rand = "0.7"

examples/places-autocomplete/src/autocomplete.rs

@@ -73,7 +73,7 @@ impl LegacyPlace {
                 .with_visit_type(
                     VisitTransition::from_primitive(v.visit_type).unwrap_or(VisitTransition::Link),
                 )
-                .with_at(places::Timestamp((v.date / 1000) as u64))
+                .with_at(types::Timestamp((v.date / 1000) as u64))
                 .with_title(self.title.clone())
                 .with_is_remote(rand::random::<f64>() < options.remote_probability);
             places::storage::history::apply_observation_direct(db, obs)?;

examples/places-utils/Cargo.toml

@@ -13,6 +13,7 @@ path = "src/places-utils.rs"
 [dev-dependencies]
 places = { path = "../../components/places" }
 sync-guid = { path = "../../components/support/guid" }
+types = { path = "../../components/support/types" }
 sync15 = { path = "../../components/sync15" }
 serde = "1"
 serde_derive = "1"

examples/places-utils/src/places-utils.rs

@@ -11,7 +11,7 @@ use places::storage::bookmarks::{
     fetch_tree, insert_tree, BookmarkNode, BookmarkRootGuid, BookmarkTreeNode, FetchDepth,
     FolderNode, SeparatorNode,
 };
-use places::types::{BookmarkType, Timestamp};
+use places::types::BookmarkType;
 use places::{ConnectionType, PlacesApi, PlacesDb};
 use serde_derive::*;
 use std::fs::File;
@@ -22,6 +22,7 @@ use sync15::{
     Sync15StorageClient,
 };
 use sync_guid::Guid as SyncGuid;
+use types::Timestamp;
 use url::Url;
 
 use anyhow::Result;

testing/separated/places-bench/Cargo.toml

@@ -16,5 +16,6 @@ criterion = "0.3"
 tempdir = "0.3"
 places = { path = "../../../components/places" }
 sql-support = { path = "../../../components/support/sql" }
+types = { path = "../../../components/support/types" }
 serde_json = "1.0"
 url = "2.1"

testing/separated/places-bench/src/database.rs

@@ -10,6 +10,7 @@ use places::PlacesDb;
 use sql_support::ConnExt;
 use std::rc::Rc;
 use tempdir::TempDir;
+use types::Timestamp;
 
 #[derive(Clone, Debug)]
 struct DummyHistoryEntry {
@@ -32,7 +33,7 @@ fn init_db(db: &mut PlacesDb) -> places::Result<()> {
     let tx = db.unchecked_transaction()?;
     let entries = get_dummy_data();
     let day_ms = 24 * 60 * 60 * 1000;
-    let now: places::Timestamp = std::time::SystemTime::now().into();
+    let now: Timestamp = std::time::SystemTime::now().into();
     for entry in entries {
         let url = url::Url::parse(&entry.url).unwrap();
         for i in 0..20 {
@@ -40,7 +41,7 @@ fn init_db(db: &mut PlacesDb) -> places::Result<()> {
                 .with_title(entry.title.clone())
                 .with_is_remote(i < 10)
                 .with_visit_type(places::VisitTransition::Link)
-                .with_at(places::Timestamp(now.0 - day_ms * (1 + i)));
+                .with_at(Timestamp(now.0 - day_ms * (1 + i)));
             places::storage::history::apply_observation_direct(&db, obs)?;
         }
     }

testing/separated/places-tests/Cargo.toml

@@ -12,6 +12,7 @@ name = "places-integration-tests"
 [dev-dependencies]
 sql-support = { path = "../../../components/support/sql" }
 sync-guid = { path = "../../../components/support/guid" }
+types = { path = "../../../components/support/types" }
 places = { path = "../../../components/places" }
 sync15 = { path = "../../../components/sync15" }
 serde_json = "1.0"

testing/separated/places-tests/src/fennec_bookmarks.rs

@@ -3,13 +3,14 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use places::import::fennec::bookmarks::BookmarksMigrationResult;
-use places::{api::places_api::PlacesApi, ErrorKind, Result, Timestamp};
+use places::{api::places_api::PlacesApi, ErrorKind, Result};
 use rusqlite::types::{ToSql, ToSqlOutput};
 use rusqlite::{Connection, NO_PARAMS};
 use std::path::Path;
 use std::sync::atomic::{AtomicUsize, Ordering};
 use sync_guid::Guid;
 use tempfile::tempdir;
+use types::Timestamp;
 
 fn empty_fennec_db(path: &Path) -> Result<Connection> {
     let conn = Connection::open(path)?;

testing/separated/places-tests/src/fennec_history.rs

@@ -3,12 +3,13 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use places::import::fennec::history::HistoryMigrationResult;
-use places::{api::places_api::PlacesApi, types::VisitTransition, ErrorKind, Result, Timestamp};
+use places::{api::places_api::PlacesApi, types::VisitTransition, ErrorKind, Result};
 use rusqlite::{Connection, NO_PARAMS};
 use std::path::Path;
 use std::sync::atomic::{AtomicUsize, Ordering};
 use sync_guid::Guid;
 use tempfile::tempdir;
+use types::Timestamp;
 
 fn empty_fennec_db(path: &Path) -> Result<Connection> {
     let conn = Connection::open(path)?;

testing/separated/places-tests/src/ios_bookmarks.rs

@@ -7,7 +7,7 @@ use places::{
     api::places_api::{ConnectionType, PlacesApi},
     import::ios_bookmarks::IosBookmarkType,
     storage::bookmarks,
-    Result, Timestamp,
+    Result,
 };
 use rusqlite::Connection;
 use std::collections::HashMap;
@@ -15,6 +15,7 @@ use std::path::Path;
 use std::sync::atomic::{AtomicUsize, Ordering};
 use sync_guid::Guid as SyncGuid;
 use tempfile::tempdir;
+use types::Timestamp;
 
 fn empty_ios_db(path: &Path) -> Result<Connection> {
     let conn = Connection::open(path)?;