Added HistoryService::GetHistoryCount() and helper functions in components/history

components/history/core/browser/visit_database.cc

Index: components/history/core/browser/visit_database.cc
diff --git a/components/history/core/browser/visit_database.cc b/components/history/core/browser/visit_database.cc
index 6d5819c36621ae11058b478811aaca64af2736e8..ae730d130e14fe5f33fb1eaea339bf231c51aa0a 100644
--- a/components/history/core/browser/visit_database.cc
+++ b/components/history/core/browser/visit_database.cc
@@ -536,6 +536,29 @@ bool VisitDatabase::GetVisibleVisitCountToHost(const GURL& url,
   return true;
 }
 
+int VisitDatabase::GetHistoryCount() {

[msramek, 2015/09/14 17:28:16]

Please follow the convention that methods return boolean indicating if the SQL
statement was successful. Return the result through an out-parameter.

[lwchkg, 2015/09/15 14:27:08]

Sounds good.

+  sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
+      "SELECT COUNT(*) FROM ("

[msramek, 2015/09/14 17:28:16]

Why not SELECT COUNT(DISTINCT ...) FROM ...?

[lwchkg, 2015/09/15 14:27:08]

I was using COUNT(*) where DISTINCT is not available. I'll change to SELECT
COUNT(DISTINCT ...) as it looks like more efficient.

[lwchkg, 2015/09/15 18:03:07]

Sorry for wrong reply. SELECT COUNT(DISTINCT ...) FROM ... doesn't work because
we need to count distinct (url, date) pairs.

[msramek, 2015/09/16 11:16:30]

Just to be sure we're talking about the same thing:

SELECT COUNT(DISTINCT url, DATE(...)) FROM visits

doesn't work? (SQLite has limited syntax, so I wouldn't be surprised, I'm just
wondering).

[lwchkg, 2015/09/16 17:32:34]

No, it doesn't work in SQLite. Error message: wrong number of arguments to
function COUNT()

Anyway, just checked the syntax of a few SQL engines. MySQL has the syntax you
mentioned, but Oracle has not.

see
https://dev.mysql.com/doc/refman/5.7/en/group-by-functions.html#function_count
and
http://docs.oracle.com/database/121/nav/portal_5.htm

Is that an non-standard feature of MySQL?

+      "SELECT DISTINCT url, "
+      // Convert unit of timestamp from the numbers of microseconds since
+      // Windows Epoch to the number of seconds from Unix Epoch.

[msramek, 2015/09/14 17:28:16]

Shouldn't this be DATE(timestamp, 'unixepoch', 'localtime')? First, we interpret
the timestamp as Unix Epoch, and then convert it to local time?

https://www.sqlite.org/lang_datefunc.html

Also note that base::Time() timestamps are from "1601-01-01", not from
"1970-01-01", i.e. not Unix Epoch. But since AFAIK neither of them handles leap
seconds, it should be fine. This should be explained in a comment.

[lwchkg, 2015/09/15 14:27:08]

You're right. Confirmed that my wrong version calculates times incorrectly. This
is very embarrassing.
Thanks for the information and I'll include in the comments. MS filetime seems
to be completely ignorant about the concepts of leap seconds.

[msramek, 2015/09/16 11:16:30]

Hm, I didn't finish my thought properly. Since both epochs start at midnight of
their respective time, and they don't use leap seconds, it holds that

DATE(x, 'unixepoch', 'localtime') == DATE(y, 'unixepoch', 'localtime')

if and only if

DATE(x-11644473600, 'unixepoch', 'localtime') == DATE(y-11644473600,
'unixepoch', 'localtime')

So the conversion with kTimeTToMicrosecondsOffset is not even needed, because we
don't need to get the exact date, just to round to the current date.

However, what you currently have is better for readability and probably not
observably slower.

Let's keep this for now, and see if other reviewers have different opinions.

[lwchkg, 2015/09/16 17:32:34]

Not correct. The leap years are different before and after conversion. So the
dates will be different. This has a effect in Daylight Saving Time because
timestamps are corrected with one more hour of offset.

[msramek, 2015/09/17 15:14:25]

Ah, of course, DST. Thanks for catching this, and sorry for misguiding you.

+      "DATE(visit_time/1000000-11644473600, 'localtime', 'unixepoch') "

[msramek, 2015/09/14 17:28:16]

Please use a constant, such as Time::kTimeTToMicrosecondsOffset or similar.

[lwchkg, 2015/09/15 14:27:08]

Time::kTimeTToMicrosecondsOffset sounds good. Anyway the comment "...
platform-dependent epoch" (line 408) in base/time/time.h is scary.

[msramek, 2015/09/16 11:16:30]

It does. I looked around and it seems to be used everywhere for these
conversions, see e.g.:

https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.cc&...

and it actually has the same value defined for posix, Win, and Mac, so I assume
that this is correct.

[lwchkg, 2015/09/16 17:32:34]

Agree... looks like the comment is incorrect. Should we file a bug?

[msramek, 2015/09/17 15:14:25]

I investigated a bit more - the constant is used specifically as the offset
between base::Time and time_t. base::Time is guaranteed to start on 1601-01-01,
but time_t is *not* guaranteed to start on 1971-01-01 (although in practice it
does). That's why the constant is platform-dependent.

Now in this case, we're transforming base::Time to unixepoch, not to time_t, so
the constant is platform-independent.

So while in practice kTimeTToMicrosecondsOffset is the correct constant to use,
in theory it's not. I'll fix this shortly.

+      "FROM visits "
+      "WHERE (transition & ?) != 0 "  // CHAIN_END
+      "AND (transition & ?) NOT IN (?, ?, ?)"  // NO SUBFRAME or
+                                               // KEYWORD_GENERATED
+      ")"));
+
+  statement.BindInt(0, ui::PAGE_TRANSITION_CHAIN_END);
+  statement.BindInt(1, ui::PAGE_TRANSITION_CORE_MASK);
+  statement.BindInt(2, ui::PAGE_TRANSITION_AUTO_SUBFRAME);
+  statement.BindInt(3, ui::PAGE_TRANSITION_MANUAL_SUBFRAME);
+  statement.BindInt(4, ui::PAGE_TRANSITION_KEYWORD_GENERATED);
+
+  statement.Step();
+  return statement.ColumnInt(0);
+}
+
 bool VisitDatabase::GetStartDate(base::Time* first_visit) {
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "SELECT MIN(visit_time) FROM visits WHERE visit_time != 0"));