1030277 - Create content provider for search terms

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Search Activity, defect)

Description

[Eric Edens [:eedens]]

First iteration of a content provider for search history. The underlying data source will support these three columns:

 term            string
 lastVisited     timestamp
 visits          count

Implementation will parallel ReadingListProvider: the underlying storage will be a table within browserDB, but the content provider code will exist in its own class.

Later additions could support for sync and references to the actual search engine.

Comment 1

[Eric Edens [:eedens]]

Attachment: part1_db_and_tests.diff

Includes:
 - content provider (SearchHistoryProvider) with tests
 - plumbing in BrowserContract and BrowserDatabaseHelper


Some questions:
 1) Is it safe to not support updateInTransaction?
 2) With respect to locale support, do you see any issues with SearchHistoryProvider.normalizeSearchTerm?
 3) What indexes do you think we need?

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8447345 [details] [diff] [review]
part1_db_and_tests.diff

Review of attachment 8447345 [details] [diff] [review]:
-----------------------------------------------------------------

Good start.

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +34,5 @@
>  
>  final class BrowserDatabaseHelper extends SQLiteOpenHelper {
>  
>      private static final String LOGTAG = "GeckoBrowserDBHelper";
> +    public static final int DATABASE_VERSION = 19;

You're racing Wes and Chris here.

@@ +1390,5 @@
>          }
>      }
>  
> +    private void upgradeDatabaseFrom18to19(SQLiteDatabase db) {
> +        createSearchHistoryTable(db);

Bug 1014712 is relevant to your interests here. Small modifications to that will make this more modular while still allowing you to have a separate CP.

::: mobile/android/base/db/SearchHistoryProvider.java
@@ +17,5 @@
> +
> +public class SearchHistoryProvider extends SharedBrowserDatabaseProvider {
> +
> +    /**
> +     * Collapse whitepsace and convert to lower case.

whitespace

@@ +27,5 @@
> +
> +        // Collapse whitespace
> +        term = term.trim().replaceAll("\\s+", " ");
> +
> +        return term.toLowerCase();

Collapsing whitespace seems innocuous. But are you confident that lowercasing search terms is always going to be meaning-preserving, including for all 52 languages we support?

For example, \u00cc ("Ì") in Lithuanian lowercases to "i", losing the accent. Famously, "I" in Turkish lowercases to "ı", and "i" uppercases to "İ".

In general, don't attempt to alter the case of user input; store it directly. You need to think carefully about how this is going to work elsewhere in the world before you commit to an approach like this.

Java has .equalsIgnoreCase, which does Unicode case folding, in case you need to compare in memory. If you need to query based on existing terms, SQLite's LIKE operator already only works with ASCII (see Bug 937179), and we need to fix that.

@@ +53,5 @@
> +
> +        SQLiteDatabase db = getWritableDatabase(uri);
> +
> +        long id = db.insertWithOnConflict(SearchHistory.TABLE_NAME,
> +                null, cv, SQLiteDatabase.CONFLICT_IGNORE);

This is buggy in Android. Don't use it.

https://code.google.com/p/android/issues/detail?id=13045

Instead, try the UPDATE first. See how BrowserProvider#updateOrInsertBookmark does things.

@@ +79,5 @@
> +    }
> +
> +    /**
> +     * Since we are managing counts and the full-text db, an update
> +     * could mangle the internal state. So we disable it.

Isn't it as simple as "we're modeling an append-only operation log using a relational DB, so UPDATE is a meaningless concept"?

::: mobile/android/base/tests/testSearchHistoryProvider.java
@@ +17,5 @@
> +import android.database.sqlite.SQLiteDatabase;
> +
> +public class testSearchHistoryProvider extends ContentProviderTest {
> +
> +    // Translations of "United Kingdom" in several different languages

Props for thinking about locales up front!

@@ +124,5 @@
> +                mProvider.insert(SearchHistory.CONTENT_URI, cv);
> +            }
> +
> +            Cursor c = mProvider.query(SearchHistory.CONTENT_URI, null, null, null, null);
> +            mAsserter.is(c.getCount(), testStrings.length, "Should have one row for each insert");

You haven't tested here your lowercasing collision. Figuring out how to test that -- and, indeed, what kind of collisions you think are valid, and which values you should retrieve -- will help you decide how to model this.

For example, if you had here a test that the cursor contained all of the inputted strings, it would fail. 

If you had a test that lowercased all of the input strings *in their own locale*, the test would fail, because the value inserted into the DB was lowercased using a different locale. 

If you had a test that had "semantically equivalent lowercase strings", and checked for those instead -- that is, you found a locale XX where someString.toLowerCase(XX) didn't mean the same thing as someString -- it could fail.

Comment 3

[Richard Newman [:rnewman]]

> For example, \u00cc ("Ì") in Lithuanian lowercases to "i", losing the
> accent. Famously, "I" in Turkish lowercases to "ı", and "i" uppercases to
> "İ".

I should add to this with a concrete example.

Imagine that I'm Turkish, my phone is set to Turkish, and I'm searching for "Ian" (McKellen).

Lowercasing just turned my search into "ıan". And look how that works out:

http://www.imdb.com/find?q=%C4%B1an&s=all
=>
No results found for "ıan"

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Eric - Why did you add the normalization in the first place? I'm just wondering if you had a use case in mind.

Also, can we change "term" to "query" ?

Comment 5

[Eric Edens [:eedens]]

Mark --

> Why did you add the normalization in the first place?
The primary reason is to avoid duplicates in the history table. If a user searches for "sushi" and then later searches for "Sushi", my thought is to increment the existing `visits` counter rather than insert a new row into the table.

> Also, can we change "term" to "query" ?
Sounds good to me!




Richard --

Thanks for all the feedback :) I was aware of the case folding issues for Turkey, but I hadn't considered how transformations would affect words outside of their locale (eg: Ian).

> Collapsing whitespace seems innocuous. But are you confident that
> lowercasing search terms is always going to be meaning-preserving,
> including for all 52 languages we support?

How about the default code path does not perform toLoweCase.  For those locales where it is safe to perform case folding, we can provide an override through a locale-qualified resource directory:

  if (getResources().getBoolean(R.bool.lowercase_is_safe)) {
      query = query.toLowerCase();
  }

> Bug 1014712 is relevant to your interests here. Small modifications
> to that will make this more modular while still allowing you
> to have a separate CP.

I see the bug is still under development. If it were already in the codebase and stable, then I'd say we jump on the opportunity. Maybe at a later time, if there's a strong benefit, we could go back and refactor this CP to use wesj's Table interface?

> This is buggy in Android. Don't use it.
Sounds good

Comment 6

[Richard Newman [:rnewman]]

(In reply to Eric Edens [:eedens] from comment #5)

> How about the default code path does not perform toLoweCase.  For those
> locales where it is safe to perform case folding, we can provide an override
> through a locale-qualified resource directory:

Better, certainly (albeit now highlighting the assumptions that users are searching for strings in their current locale's language -- the Ian problem -- and that our definition of safety is correct), but I'd probably suggest simply leaving this kind of operation until the point of retrieval, rather than modifying the input data, unless you have a really killer use-case that demands it.

Once you merge two things, it's hard to un-merge them.

Instead, how about using something like COLLATE NOCASE, optionally undoing that on a per-query basis? Example from StackOverflow: <http://stackoverflow.com/questions/14947945/sqlite-case-sensitive-search-with-collate-nocase-field>.

This allows you to store unmodified data, and query it either with or without explicit case sensitivity.

Regardless, if your interface is 'right' -- users pick from history -- and our expectations of Latin text entry are borne out -- users mostly enter a consistent case -- then the lack of explicit lowercasing shouldn't be a disadvantage at all, while of course saving us from edge-case lowercasing bugs.


> I see the bug is still under development. If it were already in the codebase
> and stable, then I'd say we jump on the opportunity. Maybe at a later time,
> if there's a strong benefit, we could go back and refactor this CP to use
> wesj's Table interface?

Sure, but let's see if you beat Wes to land first :D

Comment 7

[Eric Edens [:eedens]]

Attachment: part1_db_and_tests.patch

Considering the potential issues with case folding, I've removed it altogether. 

Overall changes:

* Removed case folding
* Added index on DATE_LAST_VISITED
* Replaced `term` with `query` throughout.
* Removed insertWithOnConflict
* Added test for querying strings with non-ascii characters.

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8448201 [details] [diff] [review]
part1_db_and_tests.patch

Review of attachment 8448201 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/db/SearchHistoryProvider.java
@@ +42,5 @@
> +         * FIRST: Try incrementing the VISITS counter and updating the DATE_LAST_VISITED.
> +         */
> +        String sql = "UPDATE " + SearchHistory.TABLE_NAME + " SET " +
> +                SearchHistory.VISITS + " = " + SearchHistory.VISITS + " + 1, " +
> +                SearchHistory.DATE_LAST_VISITED + " = " + String.valueOf(System.currentTimeMillis()) +

No need for the valueOf. Java will automatically coerce.

@@ +43,5 @@
> +         */
> +        String sql = "UPDATE " + SearchHistory.TABLE_NAME + " SET " +
> +                SearchHistory.VISITS + " = " + SearchHistory.VISITS + " + 1, " +
> +                SearchHistory.DATE_LAST_VISITED + " = " + String.valueOf(System.currentTimeMillis()) +
> +                " WHERE " + SearchHistory.QUERY + " = ?;";

No trailing ";" in the query.

@@ +44,5 @@
> +        String sql = "UPDATE " + SearchHistory.TABLE_NAME + " SET " +
> +                SearchHistory.VISITS + " = " + SearchHistory.VISITS + " + 1, " +
> +                SearchHistory.DATE_LAST_VISITED + " = " + String.valueOf(System.currentTimeMillis()) +
> +                " WHERE " + SearchHistory.QUERY + " = ?;";
> +        Cursor c = db.rawQuery(sql, new String[]{query});

Spaces around {}s.

@@ +46,5 @@
> +                SearchHistory.DATE_LAST_VISITED + " = " + String.valueOf(System.currentTimeMillis()) +
> +                " WHERE " + SearchHistory.QUERY + " = ?;";
> +        Cursor c = db.rawQuery(sql, new String[]{query});
> +
> +        if (c == null || c.getCount() > 1) {

c should only be null if your query is malformed. This should, of course, never happen. If it does, we'd like a NPE with a stack, so we can ignore that case.

Remember to close the cursor:

  final Cursor c = db.rawQuery(...);
  try {
    if (c.getCount() > 1) {
      return null;
    }
    ...
  } finally {
    c.close();
  }

@@ +52,5 @@
> +            // on the QUERY column, so there should only be one match.
> +            // We could put some error logging here, though, to detect
> +            // programming errors.
> +            return null;
> +        } else if (c.getCount() == 1) {

No need for else after return.

@@ +53,5 @@
> +            // We could put some error logging here, though, to detect
> +            // programming errors.
> +            return null;
> +        } else if (c.getCount() == 1) {
> +            c.moveToFirst();

If you already checked for > 1, just:

  if (c.moveToFirst()) {
    return ContentUris.…
  }

@@ +68,5 @@
> +
> +        long id = db.insert(SearchHistory.TABLE_NAME, null, cv);
> +
> +        if (id < 0) {
> +            // TODO: What would cause this error? How do we handle it?

Constraint violation, IIRC. Probably return null and expect the caller to check it. Sadpants.

Comment 9

[Eric Edens [:eedens]]

Attachment: part1_db_and_tests.patch

Fixed issues from comment #8.

Try: https://tbpl.mozilla.org/?tree=Try&rev=4121602a00ca

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 8448782 [details] [diff] [review]
part1_db_and_tests.patch

Review of attachment 8448782 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/db/BrowserContract.java
@@ +445,5 @@
> +        public static final Uri CONTENT_URI = Uri.withAppendedPath(SEARCH_HISTORY_AUTHORITY_URI, "searchhistory");
> +
> +        public static final String CONTENT_TYPE = "vnd.android.cursor.dir/searchhistory";
> +
> +        public static final String QUERY = "query";

Nit: fewer newlines. (Usually unnecessary between members of the same type and access level.)

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +758,5 @@
> +
> +        db.execSQL("CREATE TABLE " + SearchHistory.TABLE_NAME + "(" +
> +                    SearchHistory._ID + " INTEGER PRIMARY KEY AUTOINCREMENT, " +
> +                    SearchHistory.QUERY + " TEXT UNIQUE NOT NULL, " +
> +                    SearchHistory.DATE_LAST_VISITED + " TIMESTAMP DEFAULT CURRENT_TIMESTAMP, " +

Timestamps are a pain in the ass in sqlite -- they're timezoned, for one thing, and leave us pissing around with leaps.

Just use a numeric field with a Unix timestamp, either in seconds or milliseconds, a la:

                History.DATE_MODIFIED + " INTEGER," +

@@ +759,5 @@
> +        db.execSQL("CREATE TABLE " + SearchHistory.TABLE_NAME + "(" +
> +                    SearchHistory._ID + " INTEGER PRIMARY KEY AUTOINCREMENT, " +
> +                    SearchHistory.QUERY + " TEXT UNIQUE NOT NULL, " +
> +                    SearchHistory.DATE_LAST_VISITED + " TIMESTAMP DEFAULT CURRENT_TIMESTAMP, " +
> +                    SearchHistory.VISITS + " INTEGER ); ");

No need for trailing ";" in query.

::: mobile/android/base/db/SearchHistoryProvider.java
@@ +30,5 @@
> +
> +    @Override
> +    public Uri insertInTransaction(Uri uri, ContentValues cv) {
> +        // We don't support inserting empty search queries.
> +        if (TextUtils.isEmpty(cv.getAsString(SearchHistory.QUERY))) {

Extract `cv.getAsString(SearchHistory.QUERY))` to a local.

@@ +35,5 @@
> +            return null;
> +        }
> +
> +        SQLiteDatabase db = getWritableDatabase(uri);
> +        String query = stripWhitespace(cv.getAsString(SearchHistory.QUERY));

... and use it here.

But strip the whitespace *before* you check for isEmpty on line 34! Then add a test for this.

@@ +46,5 @@
> +                SearchHistory.DATE_LAST_VISITED + " = " + System.currentTimeMillis() +
> +                " WHERE " + SearchHistory.QUERY + " = ?";
> +        Cursor c = db.rawQuery(sql, new String[] { query });
> +
> +        if (c.getCount() > 1) {

try { finally …

@@ +51,5 @@
> +            // There is a UNIQUE constraint on the QUERY column,
> +            // so there should only be one match.
> +            return null;
> +        }
> +        

Nit: whitespace.

Comment 11

[Eric Edens [:eedens]]

Attachment: part1_db_and_tests.patch

https://tbpl.mozilla.org/?tree=Try&rev=b1b88c664fca

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8449013 [details] [diff] [review]
part1_db_and_tests.patch

Review of attachment 8449013 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/db/SearchHistoryProvider.java
@@ +29,5 @@
> +
> +
> +    @Override
> +    public Uri insertInTransaction(Uri uri, ContentValues cv) {
> +        String query = stripWhitespace(cv.getAsString(SearchHistory.QUERY));

final

@@ +36,5 @@
> +        if (TextUtils.isEmpty(query)) {
> +            return null;
> +        }
> +
> +        SQLiteDatabase db = getWritableDatabase(uri);

final

@@ +41,5 @@
> +
> +        /*
> +         * FIRST: Try incrementing the VISITS counter and updating the DATE_LAST_VISITED.
> +         */
> +        String sql = "UPDATE " + SearchHistory.TABLE_NAME + " SET " +

final

@@ +45,5 @@
> +        String sql = "UPDATE " + SearchHistory.TABLE_NAME + " SET " +
> +                SearchHistory.VISITS + " = " + SearchHistory.VISITS + " + 1, " +
> +                SearchHistory.DATE_LAST_VISITED + " = " + System.currentTimeMillis() +
> +                " WHERE " + SearchHistory.QUERY + " = ?";
> +        Cursor c = db.rawQuery(sql, new String[] { query });

final

@@ +59,5 @@
> +                    .withAppendedId(uri, c.getInt(c.getColumnIndex(SearchHistory._ID)));
> +            }
> +        } finally {
> +            if (c != null) {
> +                c.close();

No need for the null check.

::: mobile/android/base/tests/testSearchHistoryProvider.java
@@ +179,5 @@
> +            // system time.
> +            t1Real = System.currentTimeMillis();
> +            mProvider.insert(SearchHistory.CONTENT_URI, cv);
> +            Cursor c = mProvider.query(SearchHistory.CONTENT_URI, null, null, null, null);
> +            c.moveToFirst();

You need to close your cursors, even in tests.

@@ +181,5 @@
> +            mProvider.insert(SearchHistory.CONTENT_URI, cv);
> +            Cursor c = mProvider.query(SearchHistory.CONTENT_URI, null, null, null, null);
> +            c.moveToFirst();
> +            t1Db = c.getLong(c.getColumnIndex(SearchHistory.DATE_LAST_VISITED));
> +            mAsserter.ok(Math.abs(t1Real - t1Db) < twoSeconds, "DATE_LAST_VISITED",

Don't do this. Compute a timestamp prior to the insertion (t1Real), then a timestamp immediately after running the insert, and assert that the date is between those two values.

@@ +192,5 @@
> +            mProvider.insert(SearchHistory.CONTENT_URI, cv);
> +            c = mProvider.query(SearchHistory.CONTENT_URI, null, null, null, null);
> +            c.moveToFirst();
> +            t2Db = c.getLong(c.getColumnIndex(SearchHistory.DATE_LAST_VISITED));
> +            mAsserter.ok(Math.abs(t2Real - t2Db) < twoSeconds, "DATE_LAST_VISITED",

Same.

Comment 13

[Eric Edens [:eedens]]

Attachment: part1_db_and_tests.patch

Fixing nits from rnewman's review.

Comment 14

[Eric Edens [:eedens]]

Comment on attachment 8449116 [details] [diff] [review]
part1_db_and_tests.patch

Review of attachment 8449116 [details] [diff] [review]:
-----------------------------------------------------------------

Fixing nits from rnewman's review.

Comment 15

[Eric Edens [:eedens]]

Try tests passing: https://tbpl.mozilla.org/?tree=Try&rev=a3a841718fc4 
Moving to checkin-needed

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/1b9e9be1db29

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1b9e9be1db29

Comment 18

[Ed Morley [:emorley]]

Backed out for intermittent test failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=42926191&tree=Fx-Team
{
03:00:53     INFO -  58 INFO TEST-START | testSearchHistoryProvider - TestTimestamp
03:00:53     INFO -  testBrowserProvider: Database empty - Starting TestTimestamp.
03:00:53     INFO -  59 INFO TEST-PASS | testSearchHistoryProvider - TestTimestamp | DATE_LAST_VISITED - Date last visited should be set on insert.
03:00:53     INFO -  60 INFO TEST-PASS | testSearchHistoryProvider - TestTimestamp | DATE_LAST_VISITED - Date last visited should be set on insert.
03:00:53     INFO -  61 INFO TEST-PASS | testSearchHistoryProvider - TestTimestamp | DATE_LAST_VISITED - Date last visited should be set on insert.
03:00:53     INFO -  62 INFO TEST-PASS | testSearchHistoryProvider - TestTimestamp | DATE_LAST_VISITED - Date last visited should be set on insert.
03:00:53     INFO -  63 INFO TEST-UNEXPECTED-FAIL | testSearchHistoryProvider - TestTimestamp | DATE_LAST_VISITED - Date last visited should be updated on key increment.
03:00:53     INFO -  Exception caught during test!
03:00:53     INFO -  junit.framework.AssertionFailedError: 63 INFO TEST-UNEXPECTED-FAIL | testSearchHistoryProvider - TestTimestamp | DATE_LAST_VISITED - Date last visited should be updated on key increment.
03:00:53     INFO -  	at junit.framework.Assert.fail(Assert.java:50)
03:00:53     INFO -  	at org.mozilla.gecko.FennecMochitestAssert._logMochitestResult(FennecMochitestAssert.java:111)
03:00:53     INFO -  	at org.mozilla.gecko.FennecMochitestAssert.ok(FennecMochitestAssert.java:140)
03:00:53     INFO -  	at org.mozilla.gecko.tests.testSearchHistoryProvider$TestTimestamp.test(testSearchHistoryProvider.java:206)
03:00:53     INFO -  	at org.mozilla.gecko.tests.BaseTest$TestCase.run(BaseTest.java:897)
03:00:53     INFO -  	at org.mozilla.gecko.tests.testSearchHistoryProvider.testSearchHistory(testSearchHistoryProvider.java:110)
03:00:53     INFO -  	at java.lang.reflect.Method.invokeNative(Native Method)
03:00:53     INFO -  	at java.lang.reflect.Method.invoke(Method.java:511)
03:00:53     INFO -  	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214)
03:00:53     INFO -  	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199)
03:00:53     INFO -  	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192)
03:00:53     INFO -  	at org.mozilla.gecko.tests.BaseTest.runTest(BaseTest.java:142)
03:00:53     INFO -  	at junit.framework.TestCase.runBare(TestCase.java:134)
03:00:53     INFO -  	at junit.framework.TestResult$1.protect(TestResult.java:115)
03:00:53     INFO -  	at junit.framework.TestResult.runProtected(TestResult.java:133)
03:00:53     INFO -  	at junit.framework.TestResult.run(TestResult.java:118)
03:00:53     INFO -  	at junit.framework.TestCase.run(TestCase.java:124)
03:00:53     INFO -  	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:190)
03:00:53     INFO -  	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:175)
03:00:53     INFO -  	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:555)
03:00:53     INFO -  	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1661)
03:00:53     INFO -  64 INFO TEST-UNEXPECTED-FAIL | testSearchHistoryProvider - TestTimestamp | Exception caught - junit.framework.AssertionFailedError: 63 INFO TEST-UNEXPECTED-FAIL | testSearchHistoryProvider - TestTimestamp | DATE_LAST_VISITED - Date last visited should be updated on key increment.
03:00:53     INFO -  65 INFO TEST-END | testSearchHistoryProvider - TestTimestamp | finished in 65ms
}

remote:   https://hg.mozilla.org/integration/fx-team/rev/dfe3a1df68e7

Comment 19

[Eric Edens [:eedens]]

Attachment: part1_db_and_tests.patch v6

Try: https://tbpl.mozilla.org/?tree=Try&rev=ff41e020bafb

This patch changes the strict greater-than assertion (t2Db > t1Db) to a greater-than-or-equal assertion (t2Db >= t1Db). 

The change comes after running 12 iterations of the failing test [1][2]. Although the failure wasn't reproduced, the timing logs indicate that the test runs so quickly that the two insertion times could be equal at the millisecond level. Here is an example [3] where the two insertions completed within 1 ms of each other:

01:15:30     INFO -  First insert:
01:15:30     INFO -    insertStart 1404461288358
01:15:30     INFO -    insertFinish 1404461288359
01:15:30     INFO -    t1Db 1404461288359
01:15:30     INFO -  Second insert:
01:15:30     INFO -    insertStart 1404461288360
01:15:30     INFO -    insertFinish 1404461288362
01:15:30     INFO -    t2Db 1404461288360

[1] https://tbpl.mozilla.org/?tree=Try&rev=3256c39e2819
[2] https://tbpl.mozilla.org/?tree=Try&rev=d4f25ed68492
[3] https://tbpl.mozilla.org/php/getParsedLog.php?id=43115338&tree=Try&full=1

Comment 20

[Eric Edens [:eedens]]

Adding rnewman to coordinate landing with bug 1014712

Comment 21

[Richard Newman [:rnewman]]

Attachment: Create content provider for search terms. v2

Updated to be v20. Eric, watch logcat as you run the test.

Comment 22

[Eric Edens [:eedens]]

Attachment: part1_db_and_tests.patch

Fixed the logcat errors. Try run: https://tbpl.mozilla.org/?tree=Try&rev=88c9117d7ec9

Error 1: Warning for not closing this close within the loop (we close the cursor after the loop already).

mobile/android/base/tests/testSearchHistoryProvider.java
146  for (int i = 0; i < testStrings.length; i++) {
147      cv = new ContentValues();
148      cv.put(SearchHistory.QUERY, testStrings[i]);
149      mProvider.insert(SearchHistory.CONTENT_URI, cv);
150 	
151      c = mProvider.query(SearchHistory.CONTENT_URI, null, selection,

This was fixed by explicitly closing the cursor at the end of the loop iteration.

Error 2:  android.database.sqlite.SQLiteConstraintException: column query is not unique (code 19)

mobile/android/base/db/SearchHistoryProvider.java
49  final Cursor c = db.rawQuery(sql, new String[] { query });

The root issue was that the cursor did not contain the rows that had been updated. So all inserts -- even if they were already present in the table -- were sent on the code path to insert as a new value. Since the update had already succeeded, the VISITS value was set correctly in the database.

To fix this, we assume that the query is a new term; if the insert fails, then we catch the exception and do an increment.

Comment 23

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/a19387ced68e

Comment 24

[:Margaret Leibovic]

Did someone just miss closing this bug? Looks like this landed on m-c:
http://hg.mozilla.org/mozilla-central/rev/1b9e9be1db29

Comment 25

[Richard Newman [:rnewman]]

It was backed out the first time. Looks like the reland (Comment 23) merged as 

http://hg.mozilla.org/mozilla-central/rev/a19387ced68e

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a19387ced68e