Remove TextDatabase from the history service.

Remove TextDatabase from the history service.

The full text indexing feature is no longer used by anything.
 - Remove the TextDatabase, TextDatabaseManager and related files.
 - Remove is_indexed field from VisitRow objects.
 - Modify ExpireHistoryBackend and HistoryBackend to no longer call TextDatabase
 - Have HistoryBackend delete the "History Index *" files from the users profile
 - Remove ChromeViewHostMsg_PageContents IPC message since the browser process
   no longer requires to have the page contents of recently loaded pages.

BUG=247418
TESTS=unit tests, manual testing and run through trybots.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212459

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213442

[rmcilroy, 2013-06-14 15:31:41]

This CL follows on from CL:16776004, by removing all code which was required for
the now unused TextDatabase.

[Scott Hess - ex-Googler, 2013-06-14 19:39:56]

LGTM.

As I mentioned to Brett when he originally brought this up, is there any chance
that we could revert?  Once we're deleting databases off the filesystem, they're
gone even if we revert the code.

https://chromiumcodereview.appspot.com/16951015/diff/1/chrome/browser/history...
File chrome/browser/history/history_backend.cc (right):

https://chromiumcodereview.appspot.com/16951015/diff/1/chrome/browser/history...
chrome/browser/history/history_backend.cc:1765: file_util::Delete(current_file,
false);
This will potentially leave junk files.  Please use sql::Connection::Delete(),
which doesn't exist until someone reviews
https://codereview.chromium.org/17058004 .

https://chromiumcodereview.appspot.com/16951015/diff/1/chrome/browser/history...
chrome/browser/history/history_backend.cc:1766: }
I think it would be reasonable to have this histogram either the number of
databases deleted, or the number of users for which any databases were deleted.

https://chromiumcodereview.appspot.com/16951015/diff/1/chrome/browser/history...
File chrome/browser/history/visit_database.cc (right):

https://chromiumcodereview.appspot.com/16951015/diff/1/chrome/browser/history...
chrome/browser/history/visit_database.cc:37: // TODO(rmcilroy): remove unused
is_indexed field.
Removing involves copying the entire table over.  I don't think BOOLEAN is
interpreted by SQLite, but if you store a NULL (or nothing), 0, or 1 here, I
believe it will require a single byte of space per row.  NULL is the value I'm
most certain about requiring a single byte.  I think it's reasonable to just
leave it as a wart.

[rmcilroy, 2013-06-17 14:11:49]

> As I mentioned to Brett when he originally brought this up, is there any
chance
> that we could revert?  Once we're deleting databases off the filesystem,
they're
> gone even if we revert the code.

If we disable / remove TextDatabase, then the index will no longer be updated
with new website visits, so it will be out-of-data later on, even if we end up
re-instating the FTS for some reason.  I think it is better just to delete the
index to avoid potential issues with it being out-of-date, and if necessary,
recreate what we can from the URL database on a revert.  I'm happy to hold off
on the delete logic for a while if the consensus is that that would be best?

Brett: do you have any comments on this?

https://codereview.chromium.org/16951015/diff/1/chrome/browser/history/histor...
File chrome/browser/history/history_backend.cc (right):

https://codereview.chromium.org/16951015/diff/1/chrome/browser/history/histor...
chrome/browser/history/history_backend.cc:1765: file_util::Delete(current_file,
false);
On 2013/06/14 19:39:56, shess wrote:
> This will potentially leave junk files.  Please use sql::Connection::Delete(),
> which doesn't exist until someone reviews
> https://codereview.chromium.org/17058004 .

Done.

https://codereview.chromium.org/16951015/diff/1/chrome/browser/history/histor...
chrome/browser/history/history_backend.cc:1766: }
On 2013/06/14 19:39:56, shess wrote:
> I think it would be reasonable to have this histogram either the number of
> databases deleted, or the number of users for which any databases were
deleted.

Done.

https://codereview.chromium.org/16951015/diff/1/chrome/browser/history/visit_...
File chrome/browser/history/visit_database.cc (right):

https://codereview.chromium.org/16951015/diff/1/chrome/browser/history/visit_...
chrome/browser/history/visit_database.cc:37: // TODO(rmcilroy): remove unused
is_indexed field.
On 2013/06/14 19:39:56, shess wrote:
> Removing involves copying the entire table over.  I don't think BOOLEAN is
> interpreted by SQLite, but if you store a NULL (or nothing), 0, or 1 here, I
> believe it will require a single byte of space per row.  NULL is the value I'm
> most certain about requiring a single byte.  I think it's reasonable to just
> leave it as a wart.

Ack.

[rmcilroy, 2013-06-24 15:51:13]

All preceding CLs have landed.  Adding OWNERS for LGTMs on specific subsystems:
   brettw: chrome/browser/history
   sky: chrome/, chrome/renderer/, chrome/browser/ui/omnibox
   palmer: chrome/common/render_messages.h

Please let me know if you have any questions.

[palmer, 2013-06-24 18:53:53]

I love the smell of deleted code in the morning. LGTM.

[rmcilroy, 2013-06-28 18:44:33]

Ping? Brett, Sky?

[sky, 2013-06-28 19:38:31]

Brett should review this one.

[rmcilroy, 2013-07-04 14:22:10]

On 2013/06/28 19:38:31, sky wrote:
> Brett should review this one.

Ping - Brett?

[rmcilroy, 2013-07-12 10:57:56]

brettw: Please could you give this CL a review?  It's been hanging around for
one month and is important for our memory reduction efforts on Android.

sky: I think I still need your approval fro the changes to
chrome/renderer/chrome_render_view_observer.cc.  Could you please review these
changes.

[palmer, 2013-07-12 18:21:02]

https://codereview.chromium.org/16951015/diff/10001/chrome/browser/history/hi...
File chrome/browser/history/history_backend_unittest.cc (right):

https://codereview.chromium.org/16951015/diff/10001/chrome/browser/history/hi...
chrome/browser/history/history_backend_unittest.cc:604:
EXPECT_TRUE(bookmark_model_.IsBookmarked(row1.url()));
It'd be good to add some unit tests for the new
|HistoryBackend::DeleteFTSIndexDatabases| function, including unit tests that
ensure that the targets of symlinks are not deleted. (I think they won't be, but
a test will ensure it.)

[rmcilroy, 2013-07-15 12:47:18]

Rebased and added DeleteFTSIndexDatabases unit test.  PTAL.

https://codereview.chromium.org/16951015/diff/10001/chrome/browser/history/hi...
File chrome/browser/history/history_backend_unittest.cc (right):

https://codereview.chromium.org/16951015/diff/10001/chrome/browser/history/hi...
chrome/browser/history/history_backend_unittest.cc:604:
EXPECT_TRUE(bookmark_model_.IsBookmarked(row1.url()));
On 2013/07/12 18:21:02, Chromium Palmer wrote:
> It'd be good to add some unit tests for the new
> |HistoryBackend::DeleteFTSIndexDatabases| function, including unit tests that
> ensure that the targets of symlinks are not deleted. (I think they won't be,
but
> a test will ensure it.)

Done.

[palmer, 2013-07-15 18:51:26]

Thanks! Still LGTM.

[brettw, 2013-07-17 17:39:56]

lgtm

https://codereview.chromium.org/16951015/diff/37001/chrome/browser/history/vi...
File chrome/browser/history/visit_database.cc (right):

https://codereview.chromium.org/16951015/diff/37001/chrome/browser/history/vi...
chrome/browser/history/visit_database.cc:37: "is_indexed BOOLEAN,"  // unused
field - kept to avoid recreating DB.
We should be able to remove this line and tolerate some older DBs that have it
and new ones that don't (we should probably write a comment here explaining how
some DB's will have this column and why). As long as we never read or write to
it, everything will be fine.

https://codereview.chromium.org/16951015/diff/37001/chrome/browser/history/vi...
chrome/browser/history/visit_database.cc:40: } else if
(!GetDB().DoesColumnExist("visits", "is_indexed")) {
They we can also remove this block.

[rmcilroy, 2013-07-18 11:42:52]

Thanks Brett.

Scott Violet: I still need an LGTM from someone in chrome/OWNERS for:
    chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
    chrome/renderer/chrome_render_view_observer.cc
    chrome/test/perf/generate_profile.cc
    chrome/test/perf/generate_profile.h
Could you please take a look please?

https://codereview.chromium.org/16951015/diff/37001/chrome/browser/history/vi...
File chrome/browser/history/visit_database.cc (right):

https://codereview.chromium.org/16951015/diff/37001/chrome/browser/history/vi...
chrome/browser/history/visit_database.cc:37: "is_indexed BOOLEAN,"  // unused
field - kept to avoid recreating DB.
On 2013/07/17 17:39:56, brettw wrote:
> We should be able to remove this line and tolerate some older DBs that have it
> and new ones that don't (we should probably write a comment here explaining
how
> some DB's will have this column and why). As long as we never read or write to
> it, everything will be fine.

Done.

https://codereview.chromium.org/16951015/diff/37001/chrome/browser/history/vi...
chrome/browser/history/visit_database.cc:40: } else if
(!GetDB().DoesColumnExist("visits", "is_indexed")) {
On 2013/07/17 17:39:56, brettw wrote:
> They we can also remove this block.

Done.

[sky, 2013-07-18 16:17:48]

Wow, weird to see someone use my full real name in a review;)

Said files LGTM

[rmcilroy, 2013-07-18 16:50:43]

On 2013/07/18 16:17:48, sky wrote:
> Wow, weird to see someone use my full real name in a review;)
> 
> Said files LGTM

I had to differentiate between yourself and Scott Hess ;).  Thanks!

[commit-bot: I haz the power, 2013-07-18 17:10:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/16951015/66001

[commit-bot: I haz the power, 2013-07-18 22:46:12]

Change committed as 212459

[rmcilroy, 2013-07-24 13:48:06]

Attempting to re-land this after it was reverted due to a unittest failure which
didn't occur on the trybots.  I have landed r213413 beforehand to help figure
out what the issue is on the non-trybot buildbots if this happens again.

[commit-bot: I haz the power, 2013-07-24 13:48:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/16951015/81001

[commit-bot: I haz the power, 2013-07-24 13:55:41]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-07-24 14:15:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/16951015/81001

[commit-bot: I haz the power, 2013-07-24 16:13:20]

Change committed as 213442