Skip to content

Commit

Permalink
sql: Add BindTimeDelta() and ColumnTimeDelta() to sql::Statement
Browse files Browse the repository at this point in the history 
These helpers use the recommended approach to serialize/deserialize a
base::TimeDelta to/from int64.

These methods are added to encourage usage of built-in ways to insert
and retrieve a TimeDelta from a sql database, and discourage usage of
the `statement.BindInt64(delta.ToInternalValue())` pattern since TimeDelta::ToInternalValue is deprecated.

Bug: 1402777, 1195962

Change-Id: Ia6db6b8ff862be25d60b6ffa67f4e7e56204330d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4118621
Reviewed-by: Christos Froussios <[email protected]>
Reviewed-by: manuk hovanesian <[email protected]>
Commit-Queue: Kirubel Aklilu <[email protected]>
Reviewed-by: Evan Stade <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1085968}
  • Loading branch information
Kirubel Aklilu authored and Chromium LUCI CQ committed Dec 21, 2022
1 parent 609e0ae commit 97f422c
Show file tree
Hide file tree
Showing 6 changed files with 106 additions and 53 deletions.
6 changes: 3 additions & 3 deletions components/autofill/core/browser/webdata/autofill_table.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2173,7 +2173,7 @@ bool AutofillTable::AddServerCardMetadata(
InsertBuilder(db_, s, kServerCardMetadataTable,
{kUseCount, kUseDate, kBillingAddressId, kId});
s.BindInt64(0, card_metadata.use_count);
s.BindInt64(1, card_metadata.use_date.ToInternalValue());
s.BindTime(1, card_metadata.use_date);
s.BindString(2, card_metadata.billing_address_id);
s.BindString(3, card_metadata.id);
s.Run();
Expand All @@ -2191,7 +2191,7 @@ bool AutofillTable::UpdateServerCardMetadata(const CreditCard& credit_card) {
InsertBuilder(db_, s, kServerCardMetadataTable,
{kUseCount, kUseDate, kBillingAddressId, kId});
s.BindInt64(0, credit_card.use_count());
s.BindInt64(1, credit_card.use_date().ToInternalValue());
s.BindTime(1, credit_card.use_date());
s.BindString(2, credit_card.billing_address_id());
s.BindString(3, credit_card.server_id());
s.Run();
Expand All @@ -2208,7 +2208,7 @@ bool AutofillTable::UpdateServerCardMetadata(
InsertBuilder(db_, s, kServerCardMetadataTable,
{kUseCount, kUseDate, kBillingAddressId, kId});
s.BindInt64(0, card_metadata.use_count);
s.BindInt64(1, card_metadata.use_date.ToInternalValue());
s.BindTime(1, card_metadata.use_date);
s.BindString(2, card_metadata.billing_address_id);
s.BindString(3, card_metadata.id);
s.Run();
Expand Down
22 changes: 11 additions & 11 deletions components/history/core/browser/history_backend_db_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,8 +251,8 @@ TEST_F(HistoryBackendDBTest, MigrateDownloadsReasonPathsAndDangerType) {
// Implicit dependence on value of kDangerTypeNotDangerous from
// download_database.cc.
EXPECT_EQ(0, statement.ColumnInt(4));
EXPECT_EQ(nowish.ToInternalValue(), statement.ColumnInt64(5));
EXPECT_EQ(nowish.ToInternalValue(), statement.ColumnInt64(6));
EXPECT_EQ(nowish, statement.ColumnTime(5));
EXPECT_EQ(nowish, statement.ColumnTime(6));

EXPECT_TRUE(statement.Step());
EXPECT_EQ(2, statement.ColumnInt64(0));
Expand All @@ -261,8 +261,8 @@ TEST_F(HistoryBackendDBTest, MigrateDownloadsReasonPathsAndDangerType) {
EXPECT_EQ("/path/to/some/file", statement.ColumnString(2));
EXPECT_EQ("/path/to/some/file", statement.ColumnString(3));
EXPECT_EQ(0, statement.ColumnInt(4));
EXPECT_EQ(nowish.ToInternalValue(), statement.ColumnInt64(5));
EXPECT_EQ(nowish.ToInternalValue(), statement.ColumnInt64(6));
EXPECT_EQ(nowish, statement.ColumnTime(5));
EXPECT_EQ(nowish, statement.ColumnTime(6));

EXPECT_FALSE(statement.Step());
}
Expand Down Expand Up @@ -852,7 +852,7 @@ TEST_F(HistoryBackendDBTest, MigrateDownloadsLastAccessTimeAndTransient) {
sql::Statement s(db.GetUniqueStatement(
"SELECT last_access_time, transient from downloads"));
EXPECT_TRUE(s.Step());
EXPECT_EQ(base::Time(), base::Time::FromInternalValue(s.ColumnInt64(0)));
EXPECT_EQ(base::Time(), s.ColumnTime(0));
EXPECT_EQ(0, s.ColumnInt(1));
}
}
Expand Down Expand Up @@ -1405,7 +1405,7 @@ TEST_F(HistoryBackendDBTest, MigratePresentations) {
s.BindInt64(0, url_id);
s.BindString(1, url.spec());
s.BindString16(2, title);
s.BindInt64(3, segment_time.ToInternalValue());
s.BindTime(3, segment_time);
ASSERT_TRUE(s.Run());
}

Expand All @@ -1430,7 +1430,7 @@ TEST_F(HistoryBackendDBTest, MigratePresentations) {
"(?, ?, ?, ?)"));
s.BindInt64(0, 4); // id.
s.BindInt64(1, segment_id);
s.BindInt64(2, segment_time.ToInternalValue());
s.BindTime(2, segment_time);
s.BindInt(3, 5); // visit count.
ASSERT_TRUE(s.Run());
}
Expand Down Expand Up @@ -1515,7 +1515,7 @@ TEST_F(HistoryBackendDBTest, MigrateVisitSegmentNames) {
s.BindInt64(0, url_id1);
s.BindString(1, url1.spec());
s.BindString16(2, title1);
s.BindInt64(3, segment_time.ToInternalValue());
s.BindTime(3, segment_time);
ASSERT_TRUE(s.Run());
}

Expand All @@ -1539,7 +1539,7 @@ TEST_F(HistoryBackendDBTest, MigrateVisitSegmentNames) {
"(?, ?, ?, ?)"));
s.BindInt64(0, 4); // id.
s.BindInt64(1, segment_id1);
s.BindInt64(2, segment_time.ToInternalValue());
s.BindTime(2, segment_time);
s.BindInt(3, 11); // visit count.
ASSERT_TRUE(s.Run());
}
Expand All @@ -1553,7 +1553,7 @@ TEST_F(HistoryBackendDBTest, MigrateVisitSegmentNames) {
s.BindInt64(0, url_id2);
s.BindString(1, url2.spec());
s.BindString16(2, title2);
s.BindInt64(3, segment_time.ToInternalValue());
s.BindTime(3, segment_time);
ASSERT_TRUE(s.Run());
}

Expand All @@ -1577,7 +1577,7 @@ TEST_F(HistoryBackendDBTest, MigrateVisitSegmentNames) {
"(?, ?, ?, ?)"));
s.BindInt64(0, 5); // id.
s.BindInt64(1, segment_id2);
s.BindInt64(2, segment_time.ToInternalValue());
s.BindTime(2, segment_time);
s.BindInt(3, 13); // visit count.
ASSERT_TRUE(s.Run());
}
Expand Down
68 changes: 31 additions & 37 deletions components/history/core/browser/visit_database.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,12 +231,11 @@ bool VisitDatabase::DropVisitTable() {
void VisitDatabase::FillVisitRow(sql::Statement& statement, VisitRow* visit) {
visit->visit_id = statement.ColumnInt64(0);
visit->url_id = statement.ColumnInt64(1);
visit->visit_time = base::Time::FromInternalValue(statement.ColumnInt64(2));
visit->visit_time = statement.ColumnTime(2);
visit->referring_visit = statement.ColumnInt64(3);
visit->transition = ui::PageTransitionFromInt(statement.ColumnInt(4));
visit->segment_id = statement.ColumnInt64(5);
visit->visit_duration =
base::TimeDelta::FromInternalValue(statement.ColumnInt64(6));
visit->visit_duration = statement.ColumnTimeDelta(6);
visit->incremented_omnibox_typed_score = statement.ColumnBool(7);
visit->opener_visit = statement.ColumnInt64(8);
visit->originator_cache_guid = statement.ColumnString(9);
Expand Down Expand Up @@ -310,11 +309,11 @@ VisitID VisitDatabase::AddVisit(VisitRow* visit, VisitSource source) {
// Although some columns are NULLable, we never write NULL. We write 0 or ""
// instead for simplicity. See the CREATE TABLE comments for details.
statement.BindInt64(0, visit->url_id);
statement.BindInt64(1, visit->visit_time.ToInternalValue());
statement.BindTime(1, visit->visit_time);
statement.BindInt64(2, visit->referring_visit);
statement.BindInt64(3, visit->transition);
statement.BindInt64(4, visit->segment_id);
statement.BindInt64(5, visit->visit_duration.ToInternalValue());
statement.BindTimeDelta(5, visit->visit_duration);
statement.BindBool(6, visit->incremented_omnibox_typed_score);
statement.BindInt64(7, visit->opener_visit);
statement.BindString(8, visit->originator_cache_guid);
Expand Down Expand Up @@ -402,7 +401,7 @@ bool VisitDatabase::GetLastRowForVisitByVisitTime(base::Time visit_time,
SQL_FROM_HERE,
"SELECT" HISTORY_VISIT_ROW_FIELDS
"FROM visits WHERE visit_time=? ORDER BY id DESC LIMIT 1"));
statement.BindInt64(0, visit_time.ToInternalValue());
statement.BindTime(0, visit_time);

if (!statement.Step())
return false;
Expand Down Expand Up @@ -451,11 +450,11 @@ bool VisitDatabase::UpdateVisitRow(const VisitRow& visit) {
// Although some columns are NULLable, we never write NULL. We write 0 or ""
// instead for simplicity. See the CREATE TABLE comments for details.
statement.BindInt64(0, visit.url_id);
statement.BindInt64(1, visit.visit_time.ToInternalValue());
statement.BindTime(1, visit.visit_time);
statement.BindInt64(2, visit.referring_visit);
statement.BindInt64(3, visit.transition);
statement.BindInt64(4, visit.segment_id);
statement.BindInt64(5, visit.visit_duration.ToInternalValue());
statement.BindTimeDelta(5, visit.visit_duration);
statement.BindBool(6, visit.incremented_omnibox_typed_score);
statement.BindInt64(7, visit.opener_visit);
statement.BindString(8, visit.originator_cache_guid);
Expand Down Expand Up @@ -511,7 +510,7 @@ bool VisitDatabase::GetVisitsForTimes(const std::vector<base::Time>& times,
SQL_FROM_HERE, "SELECT" HISTORY_VISIT_ROW_FIELDS "FROM visits "
"WHERE visit_time == ?"));

statement.BindInt64(0, time.ToInternalValue());
statement.BindTime(0, time);

if (!FillVisitVector(statement, visits))
return false;
Expand All @@ -532,7 +531,7 @@ bool VisitDatabase::GetAllVisitsInRange(base::Time begin_time,

// See GetVisibleVisitsInRange for more info on how these times are bound.
int64_t end = end_time.ToInternalValue();
statement.BindInt64(0, begin_time.ToInternalValue());
statement.BindTime(0, begin_time);
statement.BindInt64(1, end ? end : std::numeric_limits<int64_t>::max());
statement.BindInt64(
2, max_results ? max_results : std::numeric_limits<int64_t>::max());
Expand All @@ -556,7 +555,7 @@ bool VisitDatabase::GetVisitsInRangeForTransition(base::Time begin_time,

// See GetVisibleVisitsInRange for more info on how these times are bound.
int64_t end = end_time.ToInternalValue();
statement.BindInt64(0, begin_time.ToInternalValue());
statement.BindTime(0, begin_time);
statement.BindInt64(1, end ? end : std::numeric_limits<int64_t>::max());
statement.BindInt64(2, ui::PAGE_TRANSITION_CORE_MASK);
statement.BindInt64(3, transition);
Expand Down Expand Up @@ -751,9 +750,7 @@ bool VisitDatabase::GetVisibleVisitCountToHost(const GURL& url,
if (!TransitionIsVisible(statement.ColumnInt(1)))
continue;
++visit_count;
min_visit_time =
std::min(base::Time::FromInternalValue(statement.ColumnInt64(0)),
min_visit_time);
min_visit_time = std::min(statement.ColumnTime(0), min_visit_time);
}

if (!statement.Succeeded())
Expand All @@ -777,17 +774,16 @@ bool VisitDatabase::GetHistoryCount(const base::Time& begin_time,
"FROM visits "
"WHERE visit_time >= ? AND visit_time < ?"));

statement.BindInt64(0, begin_time.ToInternalValue());
statement.BindInt64(1, end_time.ToInternalValue());
statement.BindTime(0, begin_time);
statement.BindTime(1, end_time);

// Set of (date, url) pairs.
std::set<std::pair<base::Time, std::string>> url_days;
while (statement.Step()) {
if (!TransitionIsVisible(statement.ColumnInt(2)))
continue;
url_days.emplace(
base::Time::FromInternalValue(statement.ColumnInt64(1)).LocalMidnight(),
statement.ColumnString(0));
url_days.emplace(statement.ColumnTime(1).LocalMidnight(),
statement.ColumnString(0));
}

*count = url_days.size();
Expand Down Expand Up @@ -829,13 +825,13 @@ bool VisitDatabase::GetLastVisitToHost(const std::string& host,
statement.BindString(5, bounds.at(2).second);
statement.BindString(6, bounds.at(3).first);
statement.BindString(7, bounds.at(3).second);
statement.BindInt64(8, begin_time.ToInternalValue());
statement.BindInt64(9, end_time.ToInternalValue());
statement.BindTime(8, begin_time);
statement.BindTime(9, end_time);

while (statement.Step()) {
if (ui::PageTransitionIsMainFrame(
ui::PageTransitionFromInt(statement.ColumnInt(1)))) {
*last_visit = base::Time::FromInternalValue(statement.ColumnInt64(0));
*last_visit = statement.ColumnTime(0);
return true;
}
}
Expand Down Expand Up @@ -871,8 +867,8 @@ bool VisitDatabase::GetLastVisitToOrigin(const url::Origin& origin,
"LIMIT 1"));
statement.BindString(0, origin_bounds.first);
statement.BindString(1, origin_bounds.second);
statement.BindInt64(2, begin_time.ToInternalValue());
statement.BindInt64(3, end_time.ToInternalValue());
statement.BindTime(2, begin_time);
statement.BindTime(3, end_time);

if (!statement.Step()) {
// If there are no entries from the statement, the host may not have been
Expand All @@ -882,7 +878,7 @@ bool VisitDatabase::GetLastVisitToOrigin(const url::Origin& origin,
return statement.Succeeded();
}

*last_visit = base::Time::FromInternalValue(statement.ColumnInt64(0));
*last_visit = statement.ColumnTime(0);
return true;
}

Expand All @@ -903,7 +899,7 @@ bool VisitDatabase::GetLastVisitToURL(const GURL& url,
"ORDER BY v.visit_time DESC "
"LIMIT 1"));
statement.BindString(0, url.spec());
statement.BindInt64(1, end_time.ToInternalValue());
statement.BindTime(1, end_time);

if (!statement.Step()) {
// If there are no entries from the statement, the URL may not have been
Expand All @@ -913,7 +909,7 @@ bool VisitDatabase::GetLastVisitToURL(const GURL& url,
return statement.Succeeded();
}

*last_visit = base::Time::FromInternalValue(statement.ColumnInt64(0));
*last_visit = statement.ColumnTime(0);
return true;
}

Expand Down Expand Up @@ -943,16 +939,15 @@ DailyVisitsResult VisitDatabase::GetDailyVisitsToHost(const GURL& host,

statement.BindString(0, host_bounds.first);
statement.BindString(1, host_bounds.second);
statement.BindInt64(2, begin_time.ToInternalValue());
statement.BindInt64(3, end_time.ToInternalValue());
statement.BindTime(2, begin_time);
statement.BindTime(3, end_time);

std::vector<base::Time> dates;
while (statement.Step()) {
if (!TransitionIsVisible(statement.ColumnInt(1)))
continue;
++result.total_visits;
dates.push_back(base::Time::FromInternalValue(statement.ColumnInt64(0))
.LocalMidnight());
dates.push_back(statement.ColumnTime(0).LocalMidnight());
}
std::sort(dates.begin(), dates.end());
result.days_with_visits =
Expand All @@ -970,7 +965,7 @@ bool VisitDatabase::GetStartDate(base::Time* first_visit) {
*first_visit = base::Time::Now();
return false;
}
*first_visit = base::Time::FromInternalValue(statement.ColumnInt64(0));
*first_visit = statement.ColumnTime(0);
return true;
}

Expand Down Expand Up @@ -1116,12 +1111,11 @@ bool VisitDatabase::MigrateVisitsWithoutIncrementedOmniboxTypedScore() {
VisitRow row;
row.visit_id = read.ColumnInt64(0);
row.url_id = read.ColumnInt64(1);
row.visit_time = base::Time::FromInternalValue(read.ColumnInt64(2));
row.visit_time = read.ColumnTime(2);
row.referring_visit = read.ColumnInt64(3);
row.transition = ui::PageTransitionFromInt(read.ColumnInt(4));
row.segment_id = read.ColumnInt64(5);
row.visit_duration =
base::TimeDelta::FromInternalValue(read.ColumnInt64(6));
row.visit_duration = read.ColumnTimeDelta(6);
// Check if the visit row is in an invalid state and if it is then
// leave the new field as the default value.
if (row.visit_id == row.referring_visit)
Expand All @@ -1136,11 +1130,11 @@ bool VisitDatabase::MigrateVisitsWithoutIncrementedOmniboxTypedScore() {
"visit_duration=?,incremented_omnibox_typed_score=? "
"WHERE id=?"));
statement.BindInt64(0, row.url_id);
statement.BindInt64(1, row.visit_time.ToInternalValue());
statement.BindTime(1, row.visit_time);
statement.BindInt64(2, row.referring_visit);
statement.BindInt64(3, row.transition);
statement.BindInt64(4, row.segment_id);
statement.BindInt64(5, row.visit_duration.ToInternalValue());
statement.BindTimeDelta(5, row.visit_duration);
statement.BindBool(6, row.incremented_omnibox_typed_score);
statement.BindInt64(7, row.visit_id);

Expand Down
4 changes: 2 additions & 2 deletions components/omnibox/browser/in_memory_url_index_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,14 +216,14 @@ void InMemoryURLIndexTest::SetUp() {
sql::Statement s(db.GetUniqueStatement(
"UPDATE urls SET last_visit_time = ? - ? * last_visit_time"));
s.BindTime(0, time_right_now);
s.BindInt64(1, day_delta.ToInternalValue());
s.BindTimeDelta(1, day_delta);
ASSERT_TRUE(s.Run());
}
{
sql::Statement s(db.GetUniqueStatement(
"UPDATE visits SET visit_time = ? - ? * visit_time"));
s.BindTime(0, time_right_now);
s.BindInt64(1, day_delta.ToInternalValue());
s.BindTimeDelta(1, day_delta);
ASSERT_TRUE(s.Run());
}
}));
Expand Down
Loading

0 comments on commit 97f422c

Please sign in to comment.