Changes to add duration into history database.

Changes to add duration into history database. This completes Issue 9605037 which can compute suggestion sites based on time slicing as well as visit duration. A new database table was added for those new fields we may need to experiment on.

BUG=none
TEST=unit-tests

[GeorgeY, 2012-03-22 00:36:49]

http://codereview.chromium.org/9789001/diff/7002/chrome/browser/history/histo...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/9789001/diff/7002/chrome/browser/history/histo...
chrome/browser/history/history.h:223: void UpdatePageInfo(const void* host,
Add comments, please,

and why do you use void* here? if it's a host string it should be const
std::string&, if it's an url it should be const GURL&

http://codereview.chromium.org/9789001/diff/7002/chrome/browser/history/histo...
File chrome/browser/history/history_backend.h (right):

http://codereview.chromium.org/9789001/diff/7002/chrome/browser/history/histo...
chrome/browser/history/history_backend.h:138: void UpdatePageInfo(const void*
host,
The same problem with comments and const void*
There should be *no* raw pointers going on between the threads: if it is a
meaningful pointer wrap it up into smart_..., if it is an object - use object
instead.
EDIT: I looked at the code and *this* use for const void* is OK, but, please,
comment the same way it is commented elsewhere in the file.

http://codereview.chromium.org/9789001/diff/7002/chrome/browser/history/histo...
File chrome/browser/history/history_types.h (right):

http://codereview.chromium.org/9789001/diff/7002/chrome/browser/history/histo...
chrome/browser/history/history_types.h:204: };
We should not create extra tables when we have 1 to 1 relationship between them.
We should update the visits table as we do the migration anyway.

http://codereview.chromium.org/9789001/diff/7002/chrome/browser/history/histo...
chrome/browser/history/history_types.h:253: base::Time visit_duration;
This just reiterates my point: visit_duration is a part of the visit, makes
sense that it should be the part of the visit table as well.

For active_duration, on the other hand we could use a completely different
approach: have a table with |visit_id| and |time| and add to it every time user
switches tab/focus.

http://codereview.chromium.org/9789001/diff/7002/chrome/browser/history/visit...
File chrome/browser/history/visit_database.cc (right):

http://codereview.chromium.org/9789001/diff/7002/chrome/browser/history/visit...
chrome/browser/history/visit_database.cc:880: "UPDATE visit_details SET
id=?,duration=? WHERE id=?"));
Why do you need to re-set |id|? - you should not do that.

http://codereview.chromium.org/9789001/diff/7002/chrome/browser/history/visit...
chrome/browser/history/visit_database.cc:905: sql.append("WHERE id IN (");
Have you considered how long it would take to parse such SQLite statement? It
*probably* will work (the limits for SQLite  are huge), but it would work
slowly.
Another point for *not* doing the separate table

[Wei Li, 2012-03-22 20:05:38]

The left comments are all about adding new visit table fields instead of a new
table. I will make a separate patch set after the offline discussion.

http://codereview.chromium.org/9789001/diff/7002/chrome/browser/history/histo...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/9789001/diff/7002/chrome/browser/history/histo...
chrome/browser/history/history.h:223: void UpdatePageInfo(const void* host,
On 2012/03/22 00:36:49, GeorgeY wrote:
> Add comments, please,
> 
> and why do you use void* here? if it's a host string it should be const
> std::string&, if it's an url it should be const GURL&

Done.

http://codereview.chromium.org/9789001/diff/7002/chrome/browser/history/histo...
File chrome/browser/history/history_backend.h (right):

http://codereview.chromium.org/9789001/diff/7002/chrome/browser/history/histo...
chrome/browser/history/history_backend.h:138: void UpdatePageInfo(const void*
host,
On 2012/03/22 00:36:49, GeorgeY wrote:
> The same problem with comments and const void*
> There should be *no* raw pointers going on between the threads: if it is a
> meaningful pointer wrap it up into smart_..., if it is an object - use object
> instead.
> EDIT: I looked at the code and *this* use for const void* is OK, but, please,
> comment the same way it is commented elsewhere in the file.

Done.

http://codereview.chromium.org/9789001/diff/7002/chrome/browser/history/visit...
File chrome/browser/history/visit_database.cc (right):

http://codereview.chromium.org/9789001/diff/7002/chrome/browser/history/visit...
chrome/browser/history/visit_database.cc:880: "UPDATE visit_details SET
id=?,duration=? WHERE id=?"));
On 2012/03/22 00:36:49, GeorgeY wrote:
> Why do you need to re-set |id|? - you should not do that.

Done.

[brettw, 2012-03-26 20:25:06]

I think adding a new column to Visits is the appropriate change here. Thanks for
checking.

http://codereview.chromium.org/9789001/diff/18002/chrome/browser/history/hist...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/9789001/diff/18002/chrome/browser/history/hist...
chrome/browser/history/history.h:229: void UpdatePageInfo(const void* host,
Updating a page's ending time stamp seems pretty different than updating
"PageInfo" which is what the function is called. Can you pick a more appropriate
name for this?

[Wei Li, 2012-03-27 18:11:53]

Changed to adding a new column instead of a new table in visits db as both of
you suggested. Pls take a look, thanks.

http://codereview.chromium.org/9789001/diff/18002/chrome/browser/history/hist...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/9789001/diff/18002/chrome/browser/history/hist...
chrome/browser/history/history.h:229: void UpdatePageInfo(const void* host,
On 2012/03/26 20:25:06, brettw wrote:
> Updating a page's ending time stamp seems pretty different than updating
> "PageInfo" which is what the function is called. Can you pick a more
appropriate
> name for this?

Changed to UpdateDBWithPageInfo which could be more specific . But I would think
this as a general service method name. What do you think?

[GeorgeY, 2012-03-27 21:15:57]

https://chromiumcodereview.appspot.com/9789001/diff/18002/chrome/browser/hist...
File chrome/browser/history/history_backend.h (right):

https://chromiumcodereview.appspot.com/9789001/diff/18002/chrome/browser/hist...
chrome/browser/history/history_backend.h:137: void
AddPageNoVisitForBookmark(const GURL& url);
nit: please insert an empty line before the comment

https://chromiumcodereview.appspot.com/9789001/diff/27001/chrome/browser/hist...
File chrome/browser/history/history_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/9789001/diff/27001/chrome/browser/hist...
chrome/browser/history/history_tab_helper.cc:178: // We need to use tab value
directly.
May be
// We need to use tab value directly as a hash.
To indicate that it is used as a weak pointer?

https://chromiumcodereview.appspot.com/9789001/diff/27001/chrome/browser/hist...
File chrome/browser/history/visit_database.cc (right):

https://chromiumcodereview.appspot.com/9789001/diff/27001/chrome/browser/hist...
chrome/browser/history/visit_database.cc:26: "
id,url,visit_time,from_visit,transition,segment_id,is_indexed," \
nit: I think we should have 4 spaces indent here and on the next line.

https://chromiumcodereview.appspot.com/9789001/diff/27001/chrome/browser/hist...
chrome/browser/history/visit_database.cc:362: if
(!GetDB().DoesColumnExist("visits", "visit_duration")) {
Why do we need to do here as well in addition to MigrateVisitsWithoutDuration()?

[Wei Li, 2012-03-27 23:01:49]

http://codereview.chromium.org/9789001/diff/18002/chrome/browser/history/hist...
File chrome/browser/history/history_backend.h (right):

http://codereview.chromium.org/9789001/diff/18002/chrome/browser/history/hist...
chrome/browser/history/history_backend.h:137: void
AddPageNoVisitForBookmark(const GURL& url);
On 2012/03/27 21:15:57, GeorgeY wrote:
> nit: please insert an empty line before the comment

Done.

http://codereview.chromium.org/9789001/diff/27001/chrome/browser/history/hist...
File chrome/browser/history/history_tab_helper.cc (right):

http://codereview.chromium.org/9789001/diff/27001/chrome/browser/history/hist...
chrome/browser/history/history_tab_helper.cc:178: // We need to use tab value
directly.
On 2012/03/27 21:15:57, GeorgeY wrote:
> May be
> // We need to use tab value directly as a hash.
> To indicate that it is used as a weak pointer?

It is not used as a hash, but a pointer as it passes in.

http://codereview.chromium.org/9789001/diff/27001/chrome/browser/history/visi...
File chrome/browser/history/visit_database.cc (right):

http://codereview.chromium.org/9789001/diff/27001/chrome/browser/history/visi...
chrome/browser/history/visit_database.cc:26: "
id,url,visit_time,from_visit,transition,segment_id,is_indexed," \
On 2012/03/27 21:15:57, GeorgeY wrote:
> nit: I think we should have 4 spaces indent here and on the next line.

Done.

http://codereview.chromium.org/9789001/diff/27001/chrome/browser/history/visi...
chrome/browser/history/visit_database.cc:362: if
(!GetDB().DoesColumnExist("visits", "visit_duration")) {
On 2012/03/27 21:15:57, GeorgeY wrote:
> Why do we need to do here as well in addition to
MigrateVisitsWithoutDuration()?

I used this to guard some manual errors such as version in meta_table changed
while the new column was not added. For normal uses, this should not happen, so
I removed it.

[brettw, 2012-03-28 16:33:48]

http://codereview.chromium.org/9789001/diff/18002/chrome/browser/history/hist...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/9789001/diff/18002/chrome/browser/history/hist...
chrome/browser/history/history.h:229: void UpdatePageInfo(const void* host,
My problem wasn't that "update" was ambiguous, but that "page info" is. This
updates the visit time. It should have that in the name, the fact that it
updates the DB is implied.

[Wei Li, 2012-03-28 17:37:38]

http://codereview.chromium.org/9789001/diff/18002/chrome/browser/history/hist...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/9789001/diff/18002/chrome/browser/history/hist...
chrome/browser/history/history.h:229: void UpdatePageInfo(const void* host,
On 2012/03/28 16:33:48, brettw wrote:
> My problem wasn't that "update" was ambiguous, but that "page info" is. This
> updates the visit time. It should have that in the name, the fact that it
> updates the DB is implied.

Hope the name is clearer this time.

[brettw, 2012-03-28 17:51:45]

lgtm

http://codereview.chromium.org/9789001/diff/37002/chrome/browser/history/hist...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/9789001/diff/37002/chrome/browser/history/hist...
chrome/browser/history/history.h:232: const base::Time end_ts);
You don't need the const here (since you pass by value which is fine for the
Time class).

[Wei Li, 2012-03-28 18:53:13]

http://codereview.chromium.org/9789001/diff/37002/chrome/browser/history/hist...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/9789001/diff/37002/chrome/browser/history/hist...
chrome/browser/history/history.h:232: const base::Time end_ts);
On 2012/03/28 17:51:45, brettw wrote:
> You don't need the const here (since you pass by value which is fine for the
> Time class).

Done.

[GeorgeY, 2012-03-28 19:15:14]

On 2012/03/28 18:53:13, Wei wrote:
>
http://codereview.chromium.org/9789001/diff/37002/chrome/browser/history/hist...
> File chrome/browser/history/history.h (right):
> 
>
http://codereview.chromium.org/9789001/diff/37002/chrome/browser/history/hist...
> chrome/browser/history/history.h:232: const base::Time end_ts);
> On 2012/03/28 17:51:45, brettw wrote:
> > You don't need the const here (since you pass by value which is fine for the
> > Time class).
> 
> Done.

lgtm

[commit-bot: I haz the power, 2012-03-28 19:16:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/weili@chromium.org/9789001/10018

[commit-bot: I haz the power, 2012-03-28 20:07:22]

Try job failure for 9789001-10018 (retry) on linux_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Wei Li, 2012-03-31 01:01:50]

Change patched and committed by georgey thru Issue 9963038. Close it now.