From 37437cbb433dc0c96691ef7f19880533375703f6 Mon Sep 17 00:00:00 2001 From: shess Date: Wed, 11 Mar 2015 13:24:46 -0700 Subject: [PATCH] [sql] Stop building fts2. Long ago, Chromium used fts2 for history full-text search. It was later replaced by fts3, and even later that feature was deleted entirely. fts2 is no longer used in the browser at all, so stop compiling it. Since SQLite is used by WebSQL, in theory this could affect web authors, but WebSQL uses an authorizer to allow only specific virtual table types. fts2 is not one of those types, I have verified manually that fts2 tables cannot be created using WebSQL. BUG=455817 Review URL: https://codereview.chromium.org/999573003 Cr-Commit-Position: refs/heads/master@{#320135} --- build/common.gypi | 8 -------- sql/sql.gyp | 5 ----- sql/sqlite_features_unittest.cc | 38 ++++++++++---------------------------- third_party/sqlite/BUILD.gn | 19 +------------------ third_party/sqlite/sqlite.gyp | 30 ------------------------------ 5 files changed, 11 insertions(+), 89 deletions(-) diff --git a/build/common.gypi b/build/common.gypi index 317cfd34b9a7..2cc762b0d32a 100644 --- a/build/common.gypi +++ b/build/common.gypi @@ -815,7 +815,6 @@ 'enable_supervised_users%': 0, 'enable_task_manager%': 0, 'use_system_libcxx%': 1, - 'support_pre_M6_history_database%': 0, }], # Use GPU accelerated cross process image transport by default @@ -1070,9 +1069,6 @@ 'google_default_client_secret%': '', # Native Client is enabled by default. 'disable_nacl%': '0', - - # Set to 1 to support old history files - 'support_pre_M6_history_database%': '1', }, # Copy conditionally-set variables out one scope. @@ -1222,7 +1218,6 @@ 'use_lto_o2%': '<(use_lto_o2)', 'gold_icf_level%': '<(gold_icf_level)', 'video_hole%': '<(video_hole)', - 'support_pre_M6_history_database%': '<(support_pre_M6_history_database)', 'v8_use_external_startup_data%': '<(v8_use_external_startup_data)', 'cfi_vptr%': '<(cfi_vptr)', @@ -2405,9 +2400,6 @@ }], ], - # older history files use fts2 instead of fts3 - 'sqlite_enable_fts2%': '<(support_pre_M6_history_database)', - # The path to the ANGLE library. 'angle_path': '<(DEPTH)/third_party/angle', diff --git a/sql/sql.gyp b/sql/sql.gyp index 711abf9d3001..825d3d07bab2 100644 --- a/sql/sql.gyp +++ b/sql/sql.gyp @@ -116,11 +116,6 @@ '../testing/android/native_test.gyp:native_test_native_code', ], }], - ['sqlite_enable_fts2', { - 'defines': [ - 'SQLITE_ENABLE_FTS2', - ], - }], ], # TODO(jschuh): crbug.com/167187 fix size_t to int truncations. 'msvs_disabled_warnings': [4267, ], diff --git a/sql/sqlite_features_unittest.cc b/sql/sqlite_features_unittest.cc index 2be1fd2a77f6..2b95bb8d3e72 100644 --- a/sql/sqlite_features_unittest.cc +++ b/sql/sqlite_features_unittest.cc @@ -65,41 +65,23 @@ TEST_F(SQLiteFeaturesTest, NoFTS1) { "CREATE VIRTUAL TABLE foo USING fts1(x)")); } -#if defined(SQLITE_ENABLE_FTS2) -// fts2 is used for older history files, so we're signed on for keeping our -// version up-to-date. -// TODO(shess): Think up a crazy way to get out from having to support -// this forever. -TEST_F(SQLiteFeaturesTest, FTS2) { - ASSERT_TRUE(db().Execute("CREATE VIRTUAL TABLE foo USING fts2(x)")); -} - -// A standard SQLite will not include our patch. This includes iOS. -#if !defined(USE_SYSTEM_SQLITE) -// Chromium fts2 was patched to treat "foo*" as a prefix search, though the icu -// tokenizer will return it as two tokens {"foo", "*"}. -TEST_F(SQLiteFeaturesTest, FTS2_Prefix) { - const char kCreateSql[] = - "CREATE VIRTUAL TABLE foo USING fts2(x, tokenize icu)"; - ASSERT_TRUE(db().Execute(kCreateSql)); - - ASSERT_TRUE(db().Execute("INSERT INTO foo (x) VALUES ('test')")); - - sql::Statement s(db().GetUniqueStatement( - "SELECT x FROM foo WHERE x MATCH 'te*'")); - ASSERT_TRUE(s.Step()); - EXPECT_EQ("test", s.ColumnString(0)); +// Do not include fts2 support, it is not useful, and nobody is +// looking at it. +TEST_F(SQLiteFeaturesTest, NoFTS2) { + ASSERT_EQ(SQLITE_ERROR, db().ExecuteAndReturnErrorCode( + "CREATE VIRTUAL TABLE foo USING fts2(x)")); } -#endif -#endif -// fts3 is used for current history files, and also for WebDatabase. +// fts3 used to be used for history files, and may also be used by WebDatabase +// clients. TEST_F(SQLiteFeaturesTest, FTS3) { ASSERT_TRUE(db().Execute("CREATE VIRTUAL TABLE foo USING fts3(x)")); } #if !defined(USE_SYSTEM_SQLITE) -// Test that fts3 doesn't need fts2's patch (see above). +// Originally history used fts2, which Chromium patched to treat "foo*" as a +// prefix search, though the icu tokenizer would return it as two tokens {"foo", +// "*"}. Test that fts3 works correctly. TEST_F(SQLiteFeaturesTest, FTS3_Prefix) { const char kCreateSql[] = "CREATE VIRTUAL TABLE foo USING fts3(x, tokenize icu)"; diff --git a/third_party/sqlite/BUILD.gn b/third_party/sqlite/BUILD.gn index 686863b1b384..80e1b377e8d0 100644 --- a/third_party/sqlite/BUILD.gn +++ b/third_party/sqlite/BUILD.gn @@ -10,25 +10,11 @@ source_set("sqlite") { sources = [ "amalgamation/sqlite3.c", "amalgamation/sqlite3.h", - - # fts2.c currently has a lot of conflicts when added to - # the amalgamation. It is probably not worth fixing that. - "src/ext/fts2/fts2.c", - "src/ext/fts2/fts2.h", - "src/ext/fts2/fts2_hash.c", - "src/ext/fts2/fts2_hash.h", - "src/ext/fts2/fts2_icu.c", - "src/ext/fts2/fts2_porter.c", - "src/ext/fts2/fts2_tokenizer.c", - "src/ext/fts2/fts2_tokenizer.h", - "src/ext/fts2/fts2_tokenizer1.c", ] cflags = [] defines = [ "SQLITE_CORE", - "SQLITE_ENABLE_BROKEN_FTS2", - "SQLITE_ENABLE_FTS2", "SQLITE_ENABLE_FTS3", "SQLITE_DISABLE_FTS3_UNICODE", "SQLITE_DISABLE_FTS4_DEFERRED", @@ -57,10 +43,7 @@ source_set("sqlite") { ] } - include_dirs = [ - "amalgamation", - "src/src", # Needed for fts2 to build. - ] + include_dirs = [ "amalgamation" ] configs -= [ "//build/config/compiler:chromium_code" ] configs += [ "//build/config/compiler:no_chromium_code" ] diff --git a/third_party/sqlite/sqlite.gyp b/third_party/sqlite/sqlite.gyp index d514ebf4b23e..ed64e98d8d0f 100644 --- a/third_party/sqlite/sqlite.gyp +++ b/third_party/sqlite/sqlite.gyp @@ -109,14 +109,6 @@ 'amalgamation/sqlite3.h', 'amalgamation/sqlite3.c', ], - - # TODO(shess): Previously fts1 and rtree files were - # explicitly excluded from the build. Make sure they are - # logically still excluded. - - # TODO(shess): Should all of the sources be listed and then - # excluded? For editing purposes? - 'include_dirs': [ 'amalgamation', ], @@ -172,28 +164,6 @@ 'LOCAL_FDO_SUPPORT': 'true', }, }], - ['sqlite_enable_fts2', { - 'defines': [ - 'SQLITE_ENABLE_BROKEN_FTS2', - 'SQLITE_ENABLE_FTS2', - ], - 'sources': [ - # fts2.c currently has a lot of conflicts when added to - # the amalgamation. It is probably not worth fixing that. - 'src/ext/fts2/fts2.c', - 'src/ext/fts2/fts2.h', - 'src/ext/fts2/fts2_hash.c', - 'src/ext/fts2/fts2_hash.h', - 'src/ext/fts2/fts2_icu.c', - 'src/ext/fts2/fts2_porter.c', - 'src/ext/fts2/fts2_tokenizer.c', - 'src/ext/fts2/fts2_tokenizer.h', - 'src/ext/fts2/fts2_tokenizer1.c', - ], - 'include_dirs': [ - 'src/src', - ], - }], ], }], ], -- 2.11.4.GIT