959290 - Make ContentProvider for Reading List

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Sola Ogunsakin [:sola]]

Make a ContentProvider for reading list items.

Comment 1

[Sola Ogunsakin [:sola]]

Attachment: premilinary patch

so far only supports querying and responds with fake data. we haven't finalized the schema and SQL implementation yet. just wanted to get some feedback if any.

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8360240 [details] [diff] [review]
premilinary patch

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

Great start. Next steps:

* Fleshing out the real behavior
* Migration
* Tests!

::: mobile/android/base/db/BrowserContract.java
@@ +303,5 @@
>  
>          public static final String PROVIDER_ID = "provider_id";
>      }
> +
> +    public static final class ReadingListItems implements CommonColumns, URLColumns {

And SyncColumns.

@@ +314,5 @@
> +
> +        public static final String PREVIEW    = "preview";
> +        public static final String WORD_COUNT = "word_count";
> +
> +        public static final String[] DEFAULT_PROJECTION = new String[] { _ID, URL, TITLE, PREVIEW, WORD_COUNT };

And GUID?

::: mobile/android/base/db/BrowserDB.java
@@ +67,5 @@
>  
>          @RobocopTarget
>          public Cursor getBookmarksInFolder(ContentResolver cr, long folderId);
>  
> +        public Cursor getReadingList(ContentResolver cr);

Does this also need to be a RobocopTarget?

::: mobile/android/base/db/LocalBrowserDB.java
@@ +423,5 @@
>      }
>  
> +    @Override
> +    public Cursor getReadingList(ContentResolver cr) {
> +      return cr.query(mReadingListUriWithProfile,

4-space indent, and make sure the following lines align.

::: mobile/android/base/db/ReadingListProvider.java
@@ +140,5 @@
> +            }
> +            default:
> +                throw new UnsupportedOperationException("Unknown query URI " + uri);
> +        }
> +        //return cursor;

Kill this line for now.

@@ +148,5 @@
> +     * Returns a cursor populated with static fake data.
> +     */
> +    private Cursor queryFakeItems(Uri uri, String[] projection, String selection, String[] selectionArgs, String sortOrder) {
> +        final MatrixCursor c = new MatrixCursor(ReadingListItems.DEFAULT_PROJECTION);
> +        c.addRow(new Object[] { 1, "http://www.nytimes.com/2014/01/14/us/health-care-plans-attracting-more-older-less-healthy-people.html?hp", "Health Care Plans Attracting More Older, Less Healthy People", "WASHINGTON — People signing up for health insurance through the Affordable Care Act’s federal and state marketplaces tend to be older and potentially less healthy, officials said Monday, a demographic mix that could cause premiums to rise in the future if the pattern persists.", 1500 });

Newlines, please.

Comment 3

[:Margaret Leibovic]

Comment on attachment 8360240 [details] [diff] [review]
premilinary patch

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

Looking good so far! I agree with what rnewman had to say as well.

::: mobile/android/base/db/ReadingListProvider.java
@@ +60,5 @@
> +    public boolean onCreate() {
> +        if (DB_DISABLED) {
> +            return true;
> +        }
> +        return true;

I imagine you intend to put something else in here to initialize a database. So long as you don't actually have any real database stuff in here that you want to disable, you can get rid of this DB_DISABLED flag. The only reason that flag exists in HomeListsProvider is that I wanted to land that before we came up with a finalized db schema, but I don't think we'll have the same issue here.

@@ +94,5 @@
> +        trace("Calling delete on URI: " + uri);
> +        return 0;
> +    }
> +
> +    public int deleteInTransaction(Uri uri, String selection, String[] selectionArgs) {

This method is unused, but I imagine you just copied it from existing content provider code.

Over time, we actually copy/pasted this *InTransaction helper method pattern throughout our content provider code, such that the actual delete/update/etc. methods are basically all the same, and the meat of their implementations are all in these *InTransaction methods. Maybe now would be the time to actually factor out this pattern into a place that can be shared between all the content providers.

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8360240 [details] [diff] [review]
premilinary patch

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

Nice, it would be great if we had a common parent class between BrowserProvider, TabsProvider, and now ReadingListProvider and probably HomeDataProvider. Margaret, did you have a change to work on that 'PerProfileProvider' parent class idea?

::: mobile/android/base/db/ReadingListProvider.java
@@ +27,5 @@
> +public class ReadingListProvider extends ContentProvider {
> +    private static final String LOGTAG = "GeckoRLProvider";
> +
> +    // Flag to disable DB usage. This prevents us from creating (and later needing to upgrade) a DB.
> +    private static final boolean DB_DISABLED = true;

DB_DISABLED = true is a bit mind-boggling :-) I'd go with DB_ENABLED = false instead.

@@ +31,5 @@
> +    private static final boolean DB_DISABLED = true;
> +
> +    static final int ITEMS      = 101;
> +    static final int ITEMS_ID   = 102;
> +    static final int ITEMS_FAKE = 100;

nit: we don't usually indent multiple assignment like this.
nit: add empty line here.

@@ +37,5 @@
> +
> +    static {
> +        URI_MATCHER.addURI(BrowserContract.READING_LIST_AUTHORITY, "articles",      ITEMS);
> +        URI_MATCHER.addURI(BrowserContract.READING_LIST_AUTHORITY, "articles/#",    ITEMS_ID);
> +        URI_MATCHER.addURI(BrowserContract.READING_LIST_AUTHORITY, "articles/fake", ITEMS_FAKE);

nit: same here for indentation.

@@ +41,5 @@
> +        URI_MATCHER.addURI(BrowserContract.READING_LIST_AUTHORITY, "articles/fake", ITEMS_FAKE);
> +    }
> +
> +    private static boolean logDebug   = Log.isLoggable(LOGTAG, Log.DEBUG);
> +    private static boolean logVerbose = Log.isLoggable(LOGTAG, Log.VERBOSE);

ditto.

Comment 5

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #4)
> Comment on attachment 8360240 [details] [diff] [review]
> premilinary patch
> 
> Review of attachment 8360240 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice, it would be great if we had a common parent class between
> BrowserProvider, TabsProvider, and now ReadingListProvider and probably
> HomeDataProvider. Margaret, did you have a change to work on that
> 'PerProfileProvider' parent class idea?

The result of that was PerProfileDatabases, which moved the copy/pasted profile-related code into a common class.

The current PerProfileContentProvider is confusingly named, and should be renamed to SQLiteBridgeContentProvider. See bug 947506 for that (a volunteer is working on that, but I might just wrap that up and land it so I can use it for the home panel data stuff).

But yes, I've thought about creating a common parent class that implements the basic ContentProvider methods, with abstract *InTransaction methods for the individual content provider to implement. However, I never had time to do this. Maybe that could be a good bug for sola to file and fix :)

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8360240 [details] [diff] [review]
premilinary patch

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

Looks good - what do you need now to keep moving forward on the "// not implemented bits"? A quick note about how this relates to the other two ReadingList display bugs would be good context to have.

::: mobile/android/base/db/BrowserContract.java
@@ +311,5 @@
> +
> +        public static final String CONTENT_TYPE      = "vnd.android.cursor.dir/readinglistitem";
> +        public static final String CONTENT_ITEM_TYPE = "vnd.android.cursor.item/readinglistitem";
> +
> +        public static final String PREVIEW    = "preview";

If you decide to change the term to "excerpt" be sure to update everywhere.

@@ +314,5 @@
> +
> +        public static final String PREVIEW    = "preview";
> +        public static final String WORD_COUNT = "word_count";
> +
> +        public static final String[] DEFAULT_PROJECTION = new String[] { _ID, URL, TITLE, PREVIEW, WORD_COUNT };

EXCERPT, possibly

::: mobile/android/base/db/ReadingListProvider.java
@@ +24,5 @@
> +import android.text.TextUtils;
> +import android.util.Log;
> +
> +public class ReadingListProvider extends ContentProvider {
> +    private static final String LOGTAG = "GeckoRLProvider";

I'd make this slightly more readable - better to cut off Provider than the "ReadingList" part (if you run up against the Android 23-character Logtag limit - IllegalArgException at http://developer.android.com/reference/android/util/Log.html#isLoggable%28java.lang.String,%20int%29 )

Comment 7

[Sola Ogunsakin [:sola]]

Attachment: bug-959290-fix.patch

Implemented requested changes. 

To continue with the implementation, I need rough estimate of the reading_list table schema so I can start writing some queries. Bug #889351 (the UI) depends on this because there no real data for excerpt and readTime UI. Bug #959297 is blocked on this because there's no where to store the excerpt and readTime data from the article.

@margaret: i'll file a bug the make an abstract ContentProvider class and start working on it. Should this depend on that?

Comment 8

[Sola Ogunsakin [:sola]]

Can I start with this schema? https://etherpad.mozilla.org/MSS4ZtisSj

Comment 9

[Lucas Rocha (:lucasr)]

(In reply to Sola Ogunsakin [:sola] from comment #8)
> Can I start with this schema? https://etherpad.mozilla.org/MSS4ZtisSj

Drive-by: looks fine to me. Keep in mind that you'll probably have to update the IndexedDB schema accordingly. See:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7965

Comment 10

[:Margaret Leibovic]

(In reply to Sola Ogunsakin [:sola] from comment #8)
> Can I start with this schema? https://etherpad.mozilla.org/MSS4ZtisSj

Looks reasonable to me!

Comment 11

[Sola Ogunsakin [:sola]]

Attachment: bug-959290-fix.patch

Step taken to test migration:
-added reading list items and bookmarks w/ DB version 17.
-bumped DB version to 18
-after migration, saw the same reading list items in UI with same title and urls. No non-reading list bookmarks where moved over.

-tested adding/removing of bookmark items
-tested adding/removing of reading list items
-tested re-adding reading list item marked as 'deleted'


-will run tests on try to see if anything is broken

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 8364836 [details] [diff] [review]
bug-959290-fix.patch

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

This is looking good overall but there are some important things to fix before we go ahead with this.

::: mobile/android/base/db/BrowserContract.java
@@ +27,5 @@
>      public static final String HOME_LISTS_AUTHORITY = AppConstants.ANDROID_PACKAGE_NAME + ".db.homelists";
>      public static final Uri HOME_LISTS_AUTHORITY_URI = Uri.parse("content://" + HOME_LISTS_AUTHORITY);
>  
> +    public static final String READING_LIST_AUTHORITY = AppConstants.ANDROID_PACKAGE_NAME + ".db.readinglist";
> +    public static final Uri    READING_LIST_AUTHORITY_URI = Uri.parse("content://" + READING_LIST_AUTHORITY);

nit: we don't usually indent stuff like this. Keep it consistent with the rest of the file.

@@ +312,5 @@
> +
> +        public static final String CONTENT_TYPE = "vnd.android.cursor.dir/readinglistitem";
> +        public static final String CONTENT_ITEM_TYPE = "vnd.android.cursor.item/readinglistitem";
> +
> +        public static final String GUID = "guid";

You only need to include here the columns that are not defined in the implemented interfaces. Namely, GUID and DELETED should come from SyncColumns.

@@ +322,5 @@
> +
> +        // Hack: to get the reading list cursor to work with HomeListView.HomeContextMenuInfo. Need a way to distinguish
> +        // a reading list cursor from a bookmarks cursor. For legacy purposes add fake 'parent' column (hardcoded to FIXED_READING_LIST_ID)
> +        // to query's projection. Will refactor HomeContextMenuInfo bug#963404.
> +        public static final String PARENT_COLUMN_HACK = Bookmarks.FIXED_READING_LIST_ID + " AS " + Bookmarks.PARENT;

Ugh... Maybe refactor HomeContextMenuInfo first? It shouldn't be a lot of work and we'd avoid landing this hack (to then having to get back to fix it later).

::: mobile/android/base/db/BrowserDB.java
@@ +97,5 @@
>  
>          public void removeReadingListItemWithURL(ContentResolver cr, String uri);
>  
> +        public void removeReadingListItem(ContentResolver cr, int id);
> +        

nit: remove trailing spaces here.

@@ +265,5 @@
>  
>      public static void removeReadingListItemWithURL(ContentResolver cr, String uri) {
>          sDb.removeReadingListItemWithURL(cr, uri);
>      }
> +    

nit: remove trailing space here.

::: mobile/android/base/db/BrowserProvider.java
@@ +1842,5 @@
> +
> +            final String selection = Bookmarks.PARENT + " = ? ";
> +            final String[] selectionArgs = { String.valueOf(Bookmarks.FIXED_READING_LIST_ID) };
> +            final String[] projection = { Bookmarks._ID, Bookmarks.GUID, Bookmarks.URL, Bookmarks.DATE_MODIFIED,
> +                                    Bookmarks.DATE_CREATED, Bookmarks.TITLE };

nit:
final String[] projection = { Bookmarks._ID,
                              Bookmarks.GUID,
                              ...}

@@ +1868,5 @@
> +                    values.put(ReadingListItems.URL, url);
> +                    values.put(ReadingListItems.GUID, guid);
> +                    values.put(ReadingListItems.TITLE, title);
> +                    values.put(ReadingListItems.DATE_MODIFIED, modified);
> +                    values.put(ReadingListItems.DATE_CREATED, created);

You can use the DatabaseUtils.cursor*ToContentValues() methods to make this a bit simpler. See:
http://developer.android.com/reference/android/database/DatabaseUtils.html

@@ +1872,5 @@
> +                    values.put(ReadingListItems.DATE_CREATED, created);
> +                    db.insertOrThrow(TABLE_READING_LIST, null, values);
> +                }
> +                // Done
> +                db.setTransactionSuccessful();

Shouldn't you delete the bookmarks once they've been copied to the new table?

@@ +1875,5 @@
> +                // Done
> +                db.setTransactionSuccessful();
> +
> +            } catch (SQLException e) {
> +                Log.e(LOGTAG, "Error migrating reading list items", e);

Don't you have to rollback the transaction if an exception is thrown?

@@ +1877,5 @@
> +
> +            } catch (SQLException e) {
> +                Log.e(LOGTAG, "Error migrating reading list items", e);
> +            } finally {
> +                if (c != null) c.close();

nit:
if (c != null) {
    c.close();
}

::: mobile/android/base/db/ReadingListProvider.java
@@ +25,5 @@
> +import android.os.Build;
> +import android.text.TextUtils;
> +import android.util.Log;
> +
> +public class ReadingListProvider extends BrowserProvider {

I realize that ReadingListProvider and BrowserProvider will have to share the same schema creation and upgrade code. However, making ReadingListProvider inherit from BrowserProvider (as is) feels a bit too heavy-handed. ReadingListProvider is probably inheriting a bunch of unrelated code from BrowserProvider. I'd create a common base class for BrowserProvider and ReadingListProvider that only contains the necessary common code. This means, in practice, only the code to create and upgrade the database schema.

@@ +29,5 @@
> +public class ReadingListProvider extends BrowserProvider {
> +    private static final String LOGTAG = "GeckoReadingListProv";
> +
> +    static final int ARTICLES = 101;
> +    static final int ARTICLES_ID = 102;

This patch is using the terms 'item' and 'article' inconsistently to refer to reading list entries. Better be consistent. I'd go with 'item' as it's more generic and future-proof.

::: mobile/android/base/home/BookmarksPanel.java
@@ +19,5 @@
>  import android.database.Cursor;
>  import android.os.Bundle;
>  import android.support.v4.app.LoaderManager.LoaderCallbacks;
>  import android.support.v4.content.Loader;
> +import android.util.Log;

Unrelated?

::: mobile/android/base/home/HomeFragment.java
@@ +103,5 @@
>          final boolean canOpenInReader = (info.display == Combined.DISPLAY_READER);
>          menu.findItem(R.id.home_open_in_reader).setVisible(canOpenInReader);
>      }
>  
> +

Unrelated?

@@ +178,5 @@
>                  new RemoveHistoryTask(context, historyId).execute();
>                  return true;
>              }
>  
> +            if(info.inReadingList) {

nit: space 'if' and '('

@@ +236,2 @@
>  
> +        public RemoveBookmarkTask(Context context, int id, String url) {

The 'url' argument doesn't seem to be used anywhere in the constructor.

@@ +248,5 @@
>          }
>  
>          @Override
>          public void onPostExecute(Void result) {
> +            int messageId = R.string.bookmark_removed;

Make it final while at it.

@@ +282,5 @@
> +        }
> +
> +        @Override
> +        public void onPostExecute(Void result) {
> +            int messageId = R.string.reading_list_removed;

Make it final.

::: mobile/android/base/home/HomeListView.java
@@ +18,5 @@
>  import android.database.Cursor;
>  import android.graphics.drawable.Drawable;
>  import android.text.TextUtils;
>  import android.util.AttributeSet;
> +import android.util.Log;

Unused?

@@ +130,5 @@
>      public static class HomeContextMenuInfo extends AdapterContextMenuInfo {
>  
>          public int bookmarkId;
>          public int historyId;
> +        public int readingListArticleId;

readingListItemId for consistency.

Comment 13

[Sola Ogunsakin [:sola]]

Comment on attachment 8364836 [details] [diff] [review]
bug-959290-fix.patch

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

::: mobile/android/base/db/BrowserContract.java
@@ +322,5 @@
> +
> +        // Hack: to get the reading list cursor to work with HomeListView.HomeContextMenuInfo. Need a way to distinguish
> +        // a reading list cursor from a bookmarks cursor. For legacy purposes add fake 'parent' column (hardcoded to FIXED_READING_LIST_ID)
> +        // to query's projection. Will refactor HomeContextMenuInfo bug#963404.
> +        public static final String PARENT_COLUMN_HACK = Bookmarks.FIXED_READING_LIST_ID + " AS " + Bookmarks.PARENT;

alright, will do

::: mobile/android/base/db/BrowserProvider.java
@@ +1872,5 @@
> +                    values.put(ReadingListItems.DATE_CREATED, created);
> +                    db.insertOrThrow(TABLE_READING_LIST, null, values);
> +                }
> +                // Done
> +                db.setTransactionSuccessful();

wanted to leave them there to make sure we can recover if something went wrong.

@@ +1875,5 @@
> +                // Done
> +                db.setTransactionSuccessful();
> +
> +            } catch (SQLException e) {
> +                Log.e(LOGTAG, "Error migrating reading list items", e);

i believe the transaction will be rolled back when db.endTransaction() is called in the finally block without db.setTransactionSuccessful() being called prior

::: mobile/android/base/db/ReadingListProvider.java
@@ +25,5 @@
> +import android.os.Build;
> +import android.text.TextUtils;
> +import android.util.Log;
> +
> +public class ReadingListProvider extends BrowserProvider {

alright, will do

Comment 14

[Sola Ogunsakin [:sola]]

Attachment: bug-959290.patch

Make ReadingListProvider extend TransactionalProvider instead of BrowserProvider

Comment 15

[Lucas Rocha (:lucasr)]

Comment on attachment 8373618 [details] [diff] [review]
bug-959290.patch

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

This is looking pretty good overall but needs another round of fixes.

::: mobile/android/base/db/BrowserContract.java
@@ +24,5 @@
>      public static final String TABS_AUTHORITY = AppConstants.ANDROID_PACKAGE_NAME + ".db.tabs";
>      public static final Uri TABS_AUTHORITY_URI = Uri.parse("content://" + TABS_AUTHORITY);
>  
>      public static final String HOME_AUTHORITY = AppConstants.ANDROID_PACKAGE_NAME + ".db.home";
>      public static final Uri HOME_AUTHORITY_URI = Uri.parse("content://" + HOME_AUTHORITY);

There's probably some bits related to reading list in BrowserContract that can be removed once we switch to a separate content provider. FIXED_READING_LIST_ID and READING_LIST_FOLDER_GUID comes to mind. There might be more. Please, double check that.

@@ +380,5 @@
> +        public static final String CONTENT_ITEM_TYPE = "vnd.android.cursor.item/readinglistitem";
> +
> +        public static final String EXCERPT = "excerpt";
> +        public static final String READ = "read";
> +        public static final String LENGTH = "length";

Maybe WORD_COUNT is a more accurate name?

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +1580,5 @@
> +                ContentValues values = new ContentValues();
> +                DatabaseUtils.cursorStringToContentValues(cursor, Bookmarks.URL, values, ReadingListItems.URL);
> +                DatabaseUtils.cursorStringToContentValues(cursor, Bookmarks.GUID, values, ReadingListItems.GUID);
> +                DatabaseUtils.cursorStringToContentValues(cursor, Bookmarks.TITLE, values, ReadingListItems.TITLE);
> +                DatabaseUtils.cursorLongToContentValues(cursor, Bookmarks.DATE_CREATED, values, ReadingListItems.DATE_MODIFIED);

Is this intentionally flipped? Bookmarks.DATE_CREATED should go into ReadingListItems.DATE_CREATED, no?

::: mobile/android/base/db/DBUtils.java
@@ +102,5 @@
> +     * Return true of the query is from Firefox Sync
> +     * @param uri query URI
> +     */
> +    public static boolean isCallerSync(Uri uri) {
> +        String isSync = uri.getQueryParameter(BrowserContract.PARAM_IS_SYNC);

Having DBUtils code using BrowserContract feels a bit wrong to me. Either create a new util class like ProviderUtils or just move them to TransactionalProvider just like you did with cleanupSomeDeletedRecords(). I have a slight preference for the latter.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +7,5 @@
>  
> +import java.io.ByteArrayOutputStream;
> +import java.util.Collection;
> +import java.util.HashMap;
> +import java.util.List;

Unrelated. Keep these imports in their original position.

@@ -16,5 @@
>  import org.mozilla.gecko.db.BrowserContract.Thumbnails;
>  import org.mozilla.gecko.db.BrowserContract.URLColumns;
>  import org.mozilla.gecko.favicons.decoders.FaviconDecoder;
>  import org.mozilla.gecko.favicons.decoders.LoadFaviconResult;
> -import org.mozilla.gecko.gfx.BitmapUtils;

Unrelated? Remove unused imports in a separate patch.

@@ -24,5 @@
>  import android.content.ContentValues;
>  import android.database.ContentObserver;
>  import android.database.Cursor;
>  import android.database.CursorWrapper;
> -import android.database.DatabaseUtils;

Ditto.

@@ +50,5 @@
>  
>      private final String mProfile;
>  
>      // Map of folder GUIDs to IDs. Used for caching.
> +    private final HashMap<String, Long> mFolderIdMap;

Unrelated, please do these misc fixes in separate self-contained patches.

@@ +382,5 @@
>          // We always want to show mobile bookmarks in the root view.
>          if (folderId == Bookmarks.FIXED_ROOT_ID) {
>              folderId = getFolderIdFromGuid(cr, Bookmarks.MOBILE_FOLDER_GUID);
>  
> +            // We'll add a fake "Desktop Bookmarks" folder to the root view if desktop

Unrelated.

@@ +424,5 @@
>  
> +    @Override
> +    public Cursor getReadingList(ContentResolver cr) {
> +      return cr.query(mReadingListUriWithProfile,
> +                         ReadingListItems.DEFAULT_PROJECTION,

nit: fix indentation. Align all arguments neatly.

@@ +425,5 @@
> +    @Override
> +    public Cursor getReadingList(ContentResolver cr) {
> +      return cr.query(mReadingListUriWithProfile,
> +                         ReadingListItems.DEFAULT_PROJECTION,
> +                         "", null, null);

Can't you use null instead of empty String?

@@ +466,5 @@
>          Cursor c = null;
>          try {
> +            c = cr.query(mReadingListUriWithProfile,
> +                         new String[] { ReadingListItems._ID },
> +                         "", null, null);

Ditto. Use null if possible.

@@ +507,5 @@
> +            final String[] projection = { ReadingListItems._ID };
> +            final String selection = ReadingListItems.URL + " = ? ";
> +            final String[] selectionArgs = { uri };
> +            final String sortOrder = null;
> +            c = cr.query(mReadingListUriWithProfile, projection, selection, selectionArgs, sortOrder);

Use the values directly in the query() call. No need to declare local variables for that. Follow the same indentation style from other parts of this class.

@@ +664,5 @@
>      }
>  
>      @Override
>      public void addReadingListItem(ContentResolver cr, String title, String uri) {
> +        ContentValues values = new ContentValues();

final

@@ +673,5 @@
> +        // Restore deleted record if possible
> +        final Uri insertUri = mReadingListUriWithProfile
> +                                .buildUpon()
> +                                .appendQueryParameter(BrowserContract.PARAM_INSERT_IF_NEEDED, "true")
> +                                .build();

nit: Add empty line here. Also, follow the same indentation style from other parts of the class.

@@ +676,5 @@
> +                                .appendQueryParameter(BrowserContract.PARAM_INSERT_IF_NEEDED, "true")
> +                                .build();
> +        final String[] whereArgs = { uri };
> +        final String whereClause = ReadingListItems.URL + " = ? ";
> +        final int updated = cr.update(insertUri, values, whereClause, whereArgs);

Use the values directly in the update() call. No need to declare local variables.

@@ +684,5 @@
>      @Override
>      public void removeReadingListItemWithURL(ContentResolver cr, String uri) {
> +        final String[] whereArgs = { uri };
> +        final String whereClause = ReadingListItems.URL + " = ? ";
> +        cr.delete(mReadingListUriWithProfile, whereClause, whereArgs);

Ditto, use values directly in the delete() call.

@@ +691,5 @@
> +    @Override
> +    public void removeReadingListItem(ContentResolver cr, int id) {
> +        final String[] whereArgs = { String.valueOf(id) };
> +        final String whereClause = ReadingListItems._ID + " = ? ";
> +        cr.delete(mReadingListUriWithProfile, whereClause, whereArgs);

Ditto, use values directly in the delete() call.

@@ +1238,5 @@
>  
>          return (count > 0);
>      }
>  
> +    @Override

Unrelated.

::: mobile/android/base/db/ReadingListProvider.java
@@ +26,5 @@
> +    static final int ITEMS_ID = 102;
> +    static final UriMatcher URI_MATCHER = new UriMatcher(UriMatcher.NO_MATCH);
> +
> +    static {
> +        URI_MATCHER.addURI(BrowserContract.READING_LIST_AUTHORITY, "articles", ITEMS);

In BrowserContract you're using 'ReadingListItems' terminology but in ReadingListProvider you're using 'Articles'. Name everything consistently as 'ReadingListItems'. In the URI case though, I think it's fine to simply use 'items'.

@@ +38,5 @@
> +     * @return Number of articles updated or inserted
> +     */
> +    public int updateOrInsertArticle(Uri uri, ContentValues values, String selection, String[] selectionArgs) {
> +        int updated = updateArticles(uri, values, selection, selectionArgs);
> +        if ( !(updated > 0) ) {

Do you mean updated <= 0? :-)

@@ +49,5 @@
> +     * Updates articles that match the selection criteria.
> +     *
> +     * @return Number of articles updated or inserted
> +     */
> +    public int updateArticles(Uri uri, ContentValues values, String selection, String[] selectionArgs) {

Rename to updateItems()

@@ +55,5 @@
> +        final SQLiteDatabase db = getWritableDatabase(uri);
> +        if (!values.containsKey(ReadingListItems.DATE_MODIFIED)) {
> +            values.put(ReadingListItems.DATE_MODIFIED, System.currentTimeMillis());
> +        }
> +       return db.update(TABLE_READING_LIST, values, selection, selectionArgs);

nit: fix indentation.

@@ +64,5 @@
> +     * and GUID fields are generated if they are not specified.
> +     *
> +     * @return ID of the newly inserted article
> +     */
> +    long insertArticle(Uri uri, ContentValues values) {

Rename to insertItem()

@@ +90,5 @@
> +     * detect the change.
> +     *
> +     * @return Number of deleted articles
> +     */
> +    int deleteArticles(Uri uri, String selection, String[] selectionArgs) {

Rename to deleteItems()

@@ +112,5 @@
> +    public int updateInTransaction(Uri uri, ContentValues values, String selection, String[] selectionArgs) {
> +        trace("Calling update in transaction on URI: " + uri);
> +
> +        int updated = 0,
> +            match = URI_MATCHER.match(uri);

nit: we don't use this style of variable declaration anywhere else in our code. So:
int updated = 0;
int match = ...

@@ +115,5 @@
> +        int updated = 0,
> +            match = URI_MATCHER.match(uri);
> +
> +        switch (match) {
> +

nit: remove empty line here.

@@ +121,5 @@
> +                debug("Update on ITEMS_ID: " + uri);
> +                selection = DBUtils.concatenateWhere(selection, TABLE_READING_LIST + "._id = ?");
> +                selectionArgs = DBUtils.appendSelectionArgs(selectionArgs,
> +                        new String[] { Long.toString(ContentUris.parseId(uri)) });
> +

nit: remove extra empty line here.

@@ +155,5 @@
> +                debug("Deleting on ITEMS_ID: " + uri);
> +                selection = DBUtils.concatenateWhere(selection, TABLE_READING_LIST + "._id = ?");
> +                selectionArgs = DBUtils.appendSelectionArgs(selectionArgs,
> +                        new String[] { Long.toString(ContentUris.parseId(uri)) });
> +

nit: remove extra empty line here.

@@ +175,5 @@
> +    @Override
> +    public Uri insertInTransaction(Uri uri, ContentValues values) {
> +        trace("Calling insert in transaction on URI: " + uri);
> +        long id = -1;
> +        int match = URI_MATCHER.match(uri);

final

@@ +178,5 @@
> +        long id = -1;
> +        int match = URI_MATCHER.match(uri);
> +
> +        switch (match) {
> +            case ITEMS: {

nit: no need use {}'s here.

@@ +190,5 @@
> +        }
> +
> +        debug("Inserted ID in database: " + id);
> +
> +        if (id >= 0)

nit: use {}'s even on one-line if's. Audit the rest of the code for consistency.

@@ +205,5 @@
> +        String limit = uri.getQueryParameter(BrowserContract.PARAM_LIMIT);
> +
> +        final int match = URI_MATCHER.match(uri);
> +        switch (match) {
> +

nit: remove empty line here.

@@ +218,5 @@
> +                trace("Query on ITEMS: " + uri);
> +                if (!DBUtils.shouldShowDeleted(uri))
> +                    selection = DBUtils.concatenateWhere(ReadingListItems.IS_DELETED + " = 0", selection);
> +                break;
> +            }

nit: add empty line here.

@@ +240,5 @@
> +    public String getType(Uri uri) {
> +        trace("Getting URI type: " + uri);
> +
> +        final int match = URI_MATCHER.match(uri);
> +        switch (match) {

nit: remove empty line here.

@@ +242,5 @@
> +
> +        final int match = URI_MATCHER.match(uri);
> +        switch (match) {
> +
> +            case ITEMS: {

nit: no need to use {}'s here.

@@ +245,5 @@
> +
> +            case ITEMS: {
> +                trace("URI is ITEMS: " + uri);
> +                return ReadingListItems.CONTENT_TYPE;
> +            }

nit: add empty line here.

@@ +246,5 @@
> +            case ITEMS: {
> +                trace("URI is ITEMS: " + uri);
> +                return ReadingListItems.CONTENT_TYPE;
> +            }
> +            case ITEMS_ID: {

Ditto for {}'s

::: mobile/android/base/db/TransactionalProvider.java
@@ +261,5 @@
>      protected boolean isTest(Uri uri) {
>          String isTest = uri.getQueryParameter(BrowserContract.PARAM_IS_TEST);
>          return !TextUtils.isEmpty(isTest);
>      }
>  

Tip for future patches: this kind of refactoring that just moves methods around could be done in a separate patch as a self-contained change. This would reduce the amount of code the reviewer has to look at in his/her review.

@@ +312,5 @@
> +
> +                debug("Removed old deleted item with URI: " + uriWithId);
> +            }
> +        } finally {
> +            if (cursor != null)

nit: add {}'s for one-line if's (in a follow-up patch?)

::: mobile/android/base/home/HomeFragment.java
@@ +89,5 @@
>  
>          // Hide the "Edit" menuitem if this item isn't a bookmark,
>          // or if this is a reading list item.
>          if (!info.hasBookmarkId() || info.isInReadingList()) {
>              menu.findItem(R.id.home_edit_bookmark).setVisible(false);

You'll probably have to either fix the edit dialog to cover reading list items in addition to bookmark items or simply disable editing altogether for reading list items. I'm leaning more towards the latter as I don't think users expect to be able to edit reading list items anyway. Check of ibarlow.

@@ +94,5 @@
>          }
>  
> +        // Hide the "Remove" menuitem if this item isn't a reading list,
> +        // bookmark, or history item
> +        if (!info.hasBookmarkId() && !info.hasHistoryId() && !info.isInReadingList()) {

Maybe just create a method in HomeContextMenuInfo like canRemove() that does that internally?

@@ +247,5 @@
>  
>          @Override
>          public void onPostExecute(Void result) {
> +            int messageId = R.string.bookmark_removed;
> +            Toast.makeText(mContext, messageId, Toast.LENGTH_SHORT).show();

You can get rid of the messageId variable now. Use R.string.bookmark_remove directly in the makeText() call.

@@ +285,1 @@
>              Toast.makeText(mContext, messageId, Toast.LENGTH_SHORT).show();

Ditto. Just use reading_list_removed directly in the makeText() call.

@@ -264,5 @@
>          private final int mId;
>  
>          public RemoveHistoryTask(Context context, int id) {
>              super(ThreadUtils.getBackgroundHandler());
> -

Unrelated?

::: mobile/android/base/home/ReadingListPanel.java
@@ +5,5 @@
>  
>  package org.mozilla.gecko.home;
>  
>  import org.mozilla.gecko.R;
>  import org.mozilla.gecko.db.BrowserContract.Bookmarks;

This import is not unused I guess?

@@ +24,5 @@
>  import android.support.v4.widget.CursorAdapter;
>  import android.text.SpannableStringBuilder;
>  import android.text.Spanned;
>  import android.text.style.ImageSpan;
> +import android.util.Log;

Unused, remove.

@@ +39,5 @@
>  /**
>   * Fragment that displays reading list contents in a ListView.
>   */
>  public class ReadingListPanel extends HomeFragment {
> +    private static final String LOGTAG = "GeckoReadingListPanel";

nit: add empty line here.

Comment 16

[Brian Nicholson (:bnicholson)]

As Lucas mentioned in bug 971413, the patch there will bitrot this one slightly. You'll want to remove the Reader:ListCountUpdated and toast bits from RemoveReadingListItemTask -- Reader:Remove will take care of that on its own. Just a heads up to be on the lookout!

Comment 17

[Sola Ogunsakin [:sola]]

Alright, thanks for the heads up

Comment 18

[Sola Ogunsakin [:sola]]

(In reply to Lucas Rocha (:lucasr) from comment #15)
> Comment on attachment 8373618 [details] [diff] [review]
> bug-959290.patch
> 
> Review of attachment 8373618 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: mobile/android/base/db/BrowserContract.java
> >  
> >      public static final String HOME_AUTHORITY = AppConstants.ANDROID_PACKAGE_NAME + ".db.home";
> >      public static final Uri HOME_AUTHORITY_URI = Uri.parse("content://" + HOME_AUTHORITY);
> 
> There's probably some bits related to reading list in BrowserContract that
> can be removed once we switch to a separate content provider.
> FIXED_READING_LIST_ID and READING_LIST_FOLDER_GUID comes to mind. There
> might be more. Please, double check that.
> 

True.. we still need them for old migrations though. I'll move them into the Obsolete class.

> @@ +380,5 @@
> > +        public static final String CONTENT_ITEM_TYPE = "vnd.android.cursor.item/readinglistitem";
> > +
> > +        public static final String EXCERPT = "excerpt";
> > +        public static final String READ = "read";
> > +        public static final String LENGTH = "length";
> 
> Maybe WORD_COUNT is a more accurate name?

this field stores the length of the article in characters. 

> 
> ::: mobile/android/base/db/BrowserDatabaseHelper.java
> @@ +1580,5 @@
> > +                DatabaseUtils.cursorLongToContentValues(cursor, Bookmarks.DATE_CREATED, values, ReadingListItems.DATE_MODIFIED);
> 
> Is this intentionally flipped? Bookmarks.DATE_CREATED should go into
> ReadingListItems.DATE_CREATED, no?

Good catch! my bad it should be ReadingListItems.DATE_CREATED.

> ::: mobile/android/base/db/DBUtils.java
> > +    public static boolean isCallerSync(Uri uri) {
> > +        String isSync = uri.getQueryParameter(BrowserContract.PARAM_IS_SYNC);
> 
> Having DBUtils code using BrowserContract feels a bit wrong to me. Either
> create a new util class like ProviderUtils or just move them to
> TransactionalProvider just like you did with cleanupSomeDeletedRecords(). I
> have a slight preference for the latter.
> 

will move it to TransactionalProvider

> >  
> > +            // We'll add a fake "Desktop Bookmarks" folder to the root view if desktop
> 
> Unrelated.

My IDE auto-removes white spaces @ the end of lines :(

> Tip for future patches: this kind of refactoring that just moves methods
> around could be done in a separate patch as a self-contained change. This
> would reduce the amount of code the reviewer has to look at in his/her
> review.

gotcha

> >          // Hide the "Edit" menuitem if this item isn't a bookmark,
> >          // or if this is a reading list item.
> >          if (!info.hasBookmarkId() || info.isInReadingList()) {
> >              menu.findItem(R.id.home_edit_bookmark).setVisible(false);
> 
> You'll probably have to either fix the edit dialog to cover reading list
> items in addition to bookmark items or simply disable editing altogether for
> reading list items. I'm leaning more towards the latter as I don't think
> users expect to be able to edit reading list items anyway. Check of ibarlow.

this logic disables editing for reading list items

> > -
> 
> Unrelated?
> 

believe it was white-space

Comment 19

[Sola Ogunsakin [:sola]]

Attachment: bug-959290.patch

Implemented requested changes

Aside: added SCROLL_POSITION column to the reading list table (it defaults to 0). So we don't need a migration for bug 857990.

Comment 20

[Richard Newman [:rnewman]]

(In reply to Sola Ogunsakin [:sola] from comment #18)

> True.. we still need them for old migrations though. I'll move them into the
> Obsolete class. 

N.B.: any changes you make to BrowserContract need to be reflected in the copy of it in android-sync, and the /sync/ classes that touch it -- both as pull requests to <https://github.com/mozilla-services/android-sync>.

Comment 21

[Sola Ogunsakin [:sola]]

(In reply to Richard Newman [:rnewman] from comment #20)
> (In reply to Sola Ogunsakin [:sola] from comment #18)
> 
> > True.. we still need them for old migrations though. I'll move them into the
> > Obsolete class. 
> 
> N.B.: any changes you make to BrowserContract need to be reflected in the
> copy of it in android-sync, and the /sync/ classes that touch it -- both as
> pull requests to <https://github.com/mozilla-services/android-sync>.

Hmm.. alright seems like moving FIXED_READING_LIST_ID and READING_LIST_FOLDER_GUID will require touching a lot of stuff and is beyond the scope of this bug. I'll file another bug to make these changes and update the sync code.

Comment 22

[Lucas Rocha (:lucasr)]

Comment on attachment 8376565 [details] [diff] [review]
bug-959290.patch

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

Nice. What about some tests too? Tests for ReadingListProvider should be analogous to the BrowserProvider ones (but a lot smaller I suppose). Have a look at mobile/android/base/tests/testBrowserProvider.java.

::: mobile/android/base/db/DBUtils.java
@@ +7,5 @@
>  import org.mozilla.gecko.GeckoAppShell;
>  
>  import android.content.ContentValues;
>  import android.database.sqlite.SQLiteOpenHelper;
> +import android.net.Uri;

Unrelated, remove.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +521,5 @@
>              return c.getCount() > 0;
>          } catch (NullPointerException e) {
>              Log.e(LOGTAG, "NullPointerException in isReadingListItem");
>          } finally {
> +            if (c != null) {

Unrelated.

::: mobile/android/base/db/TransactionalProvider.java
@@ +346,5 @@
>      }
>  
>      // Calculate these once, at initialization. isLoggable is too expensive to
>      // have in-line in each log call.
> +    private static boolean logDebug = Log.isLoggable(LOGTAG, Log.DEBUG);

Unrelated?

::: mobile/android/base/home/ReadingListPanel.java
@@ +37,5 @@
>  /**
>   * Fragment that displays reading list contents in a ListView.
>   */
>  public class ReadingListPanel extends HomeFragment {
> +    private static final String LOGTAG = "GeckoReadingListPanel";

Unused?

Comment 23

[Sola Ogunsakin [:sola]]

Attachment: Part 1: Make ReadingListProvider

Comment 24

[Sola Ogunsakin [:sola]]

Attachment: Part 2: Tests for ReadingListProvider

Comment 25

[Lucas Rocha (:lucasr)]

Comment on attachment 8377924 [details] [diff] [review]
Part 1: Make ReadingListProvider

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

Awesome. Giving f+ instead of r+ to ensure the terminology fixes are done before landing.

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +1566,5 @@
> +                                        Bookmarks.TITLE };
> +        Cursor cursor = null;
> +        try {
> +            // Start transaction
> +            db.beginTransaction();

nit: add empty line here.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +702,5 @@
>  
>      @Override
>      public void addReadingListItem(ContentResolver cr, String title, String uri) {
> +        final ContentValues values = new ContentValues();
> +        values.put(Bookmarks.IS_DELETED, 0);

ReadingListItems.IS_DELETED

::: mobile/android/base/db/ReadingListProvider.java
@@ +34,5 @@
> +    /**
> +     * Updates articles that match the selection criteria. If no such articles is found
> +     * one is inserted with the attributes passed in. Returns 0 if no article updated.
> +     *
> +     * @return Number of articles updated or inserted

articles -> items (audit the rest of the files for consistent terminology)

@@ +45,5 @@
> +        return updated;
> +    }
> +
> +    /**
> +     * Updates articles that match the selection criteria.

Ditto: articles -> items

::: mobile/android/base/home/ReadingListPanel.java
@@ +114,5 @@
>              public HomeContextMenuInfo makeInfoForCursor(View view, int position, long id, Cursor cursor) {
>                  final HomeContextMenuInfo info = new HomeContextMenuInfo(view, position, id);
> +                info.url = cursor.getString(cursor.getColumnIndexOrThrow(ReadingListItems.URL));
> +                info.title = cursor.getString(cursor.getColumnIndexOrThrow(ReadingListItems.TITLE));
> +                info.readingListItemId = cursor.getInt(cursor.getColumnIndex(ReadingListItems._ID));

Any reason why the call uses getColumnIndex() instead of getColumnIndexOrThrow()? It seems all calls should be getColumnIndexOrThrow().

Comment 26

[Sola Ogunsakin [:sola]]

Attachment: Part 1: Make ReadingListProvider

Comment 27

[Lucas Rocha (:lucasr)]

Comment on attachment 8378339 [details] [diff] [review]
Part 1: Make ReadingListProvider

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

Great.

Comment 28

[Richard Newman [:rnewman]]

Comment on attachment 8378339 [details] [diff] [review]
Part 1: Make ReadingListProvider

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

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +1587,5 @@
> +                DatabaseUtils.cursorLongToContentValues(cursor, Bookmarks.DATE_MODIFIED, values, ReadingListItems.DATE_MODIFIED);
> +
> +                db.insertOrThrow(TABLE_READING_LIST, null, values);
> +            }
> +            // Done

Right here you also need to delete all children of FIXED_READING_LIST_ID, and otherwise cleanse it from the earth.

We don't support downgrades on Android, so this kind of backward-incompatible change is safe.

Comment 29

[Sola Ogunsakin [:sola]]

(In reply to Richard Newman [:rnewman] from comment #28)
> Comment on attachment 8378339 [details] [diff] [review]
> Part 1: Make ReadingListProvider
> 
> Review of attachment 8378339 [details] [diff] [review]:
> -----------------------------------------------------------------
> Right here you also need to delete all children of FIXED_READING_LIST_ID,
> and otherwise cleanse it from the earth.
> 
> We don't support downgrades on Android, so this kind of
> backward-incompatible change is safe.

sounds good :)

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8377927 [details] [diff] [review]
Part 2: Tests for ReadingListProvider

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

Good start in the right direction - I apologize in advance that I nit a lot! The additional tests (see my comments) can be done in a followup, if you want. The biggest issues are the synchronousity/asynchronousity of the deletion tests and the disk accesses on bulk insert.

nit: I personally find it much easier to read code with the helper methods below the major methods (e.g. so I don't have to memorize many things before I realize what's going on or alternatively have to scroll to the bottom and, then in reverse, to start understanding). If you prefer the way it is now, then I suppose it's fine, but I'd prefer if it was flipped.

::: mobile/android/base/tests/robocop.ini
@@ +15,5 @@
>  [testBookmarkFolders]
>  # [testBookmarklets] # see bug 915350
>  # [testBookmarkKeyword] # see bug 915350
>  [testBrowserProvider]
> +[testReadingListProvider]

nit: Alphabetize

::: mobile/android/base/tests/testReadingListProvider.java
@@ +26,5 @@
> +     * <p>
> +     * We want a fresh provider each test, so this should be invoked in
> +     * <code>setUp</code> before each individual test.
> +     */
> +    protected static Callable<ContentProvider> sProviderFactory = new Callable<ContentProvider>() {

nit: Why not private?

@@ +43,5 @@
> +     * Removes all items from the DB. Called in between tests.
> +     */
> +    private void ensureEmptyDatabase() throws Exception {
> +        Uri uri = appendUriParam(ReadingListItems.CONTENT_URI, BrowserContract.PARAM_IS_SYNC, "1");
> +        mProvider.delete(uri, null, null);

Using `ContentProvider.delete` in a testing setup/teardown method assumes it works, however, this test file is created to test this method (in addition to others) so we can't really use it here. Write something that accesses the DB directly (if possible) and deletes the contents.

@@ +46,5 @@
> +        Uri uri = appendUriParam(ReadingListItems.CONTENT_URI, BrowserContract.PARAM_IS_SYNC, "1");
> +        mProvider.delete(uri, null, null);
> +    }
> +
> +    private ContentValues createItem(String title, String url, String excerpt, int length) throws Exception {

nit: Ambiguous name. I'd go for `fillContentValues`. This also makes the code more understandable in the context of why "createAnItem" calls a method called "createItem".

@@ +57,5 @@
> +
> +        return values;
> +    }
> +
> +    private ContentValues createAnItem() throws Exception {

nit: Be more specific - e.g. `createFillerItem`. Also, a bit more nitty but what's "an item"? A DB row? A ReadingListItem? e.g. 'createFillerReadingListItem` sounds more better to me. Remember to apply an changes to the other methods you use here (e.g. `insertAnItem`).

@@ +58,5 @@
> +        return values;
> +    }
> +
> +    private ContentValues createAnItem() throws Exception {
> +        return createItem("Example", "http://example.com", "foo bar", "foo bar".length());

Make foo bar a final and use the variable instead - typing the string twice is fragile. But better yet, any reason not to put this measurement in `createItem`?

@@ +76,5 @@
> +    private Cursor getItemById(Uri uri, long id) throws Exception {
> +        return getItemById(uri, id, null);
> +    }
> +
> +    private long insertItemWithAssertion(ContentValues b) throws Exception {

nit: Why b? Similarly everywhere it's used. I'd prefer `v` at the least, but `values` is probably the best.

@@ +80,5 @@
> +    private long insertItemWithAssertion(ContentValues b) throws Exception {
> +        long id = ContentUris.parseId(mProvider.insert(ReadingListItems.CONTENT_URI, b));
> +
> +        Cursor c = getItemById(id);
> +        mAsserter.is(c.moveToFirst(), true, "Inserted item found");

`mAsserter.ok` - leave diag blank.

We seem to do this assertion a lot. Why not make a helper method, assertItemExistsByID(id)? It'd certainly make closing these cursors easier (don't forget finally: see below)!

@@ +81,5 @@
> +        long id = ContentUris.parseId(mProvider.insert(ReadingListItems.CONTENT_URI, b));
> +
> +        Cursor c = getItemById(id);
> +        mAsserter.is(c.moveToFirst(), true, "Inserted item found");
> +        c.close();

`try { ... } finally { c.close(); }`

@@ +87,5 @@
> +        return id;
> +    }
> +
> +    private long insertAnItem() throws Exception {
> +        return insertItemWithAssertion(createAnItem());

Why wrap this? Why not just make the assertion in `insertAnItem`? You're doing a lot of function calls and it can be hard to follow (particularly with my ordering nit).

@@ +92,5 @@
> +    }
> +
> +    @Override
> +    public void setUp() throws Exception {
> +        super.setUp(sProviderFactory, BrowserContract.READING_LIST_AUTHORITY, "browser.db");

DB name should be a constant.

@@ +94,5 @@
> +    @Override
> +    public void setUp() throws Exception {
> +        super.setUp(sProviderFactory, BrowserContract.READING_LIST_AUTHORITY, "browser.db");
> +
> +        mTests.add(new TestInsertItems());

nit: You could make a static final Array of all the of the tests at the top of the class, to improve visibility that these tests need to be registered to run. Then iterate over that Array, calling `mTests.add` on each item here.

@@ +111,5 @@
> +            test.run();
> +        }
> +    }
> +
> +    abstract class Test implements Runnable {

This looks like it came out of testHomeListsProvider - maybe you should pull it out to a common file instead. The name `Test` is a bit ambiguous to stand on it's own, so I'd opt for `TestRunnable` or something better.

I hate that this has to exist because this is why junit is designed the way it is (i.e. setUp & tearDown), but I guess it avoids having to restart Gecko. Make sure to add that to a comment in the file explaining why it exists.

If there's any other code that you also used as inspiration, consider abstracting it out to a separate file (preferably now, though alternatively file a followup).

@@ +119,5 @@
> +        public void run() {
> +            try {
> +                test();
> +            } catch (Exception e) {
> +                mAsserter.is(true, false, "Test " + this.getClass().getName() +

`mAsserter.ok` - just leave the diag string blank

@@ +128,5 @@
> +
> +    /**
> +     * Verify that we can insert a reading list item into the DB.
> +     */
> +    class TestInsertItems extends Test {

What about tests for inserting junk values besides null such as an ArrayList. I'm acutally not sure how SQLite handles this.

nit: private; same with the others

@@ +136,5 @@
> +            long id = -1;
> +
> +            try {
> +                id = ContentUris.parseId(mProvider.insert(ReadingListItems.CONTENT_URI, b));
> +            } catch (Exception e) {}

Add a comment explaining that we catch here so that we can see if the assertion failed or not (alternatively, fail if we get to the line after `id =`).

@@ +147,5 @@
> +            ContentValues b = createAnItem();
> +            long id = ContentUris.parseId(mProvider.insert(ReadingListItems.CONTENT_URI, b));
> +            Cursor c = getItemById(id);
> +
> +            mAsserter.is(c.moveToFirst(), true, "Inserted item found");

`.ok`

@@ +150,5 @@
> +
> +            mAsserter.is(c.moveToFirst(), true, "Inserted item found");
> +
> +            mAsserter.is(c.getString(c.getColumnIndex(ReadingListItems.TITLE)), b.getAsString(ReadingListItems.TITLE),
> +                         "Inserted item has correct title");

nit: You can wrap this assertion and make it more readable.

@@ +160,5 @@
> +                         "Inserted item has correct length");
> +
> +            id = insertWithNullCol(ReadingListItems.GUID);
> +            mAsserter.is(new Long(id), new Long(-1),
> +                         "Should not be able to item with null GUID");

nit: insert an item?

@@ +162,5 @@
> +            id = insertWithNullCol(ReadingListItems.GUID);
> +            mAsserter.is(new Long(id), new Long(-1),
> +                         "Should not be able to item with null GUID");
> +
> +            c.close();

Close in finally block

@@ +169,5 @@
> +
> +    /**
> +     * Verify that we can remove a reading list item from the DB.
> +     */
> +    class TestDeleteItems extends Test {

What about trying to delete non-existent items (or using the wrong values - make sure all the items are still in the DB)?

@@ +175,5 @@
> +        @Override
> +        public void test() throws Exception {
> +            long id = insertAnItem();
> +            testNonSyncDelete(id);
> +            testQueryOnDeleteItem(id);

If that is non-synchronous, don't we need to block for it to finish? That probably explains why the sychronous delete can pass too.

@@ +176,5 @@
> +        public void test() throws Exception {
> +            long id = insertAnItem();
> +            testNonSyncDelete(id);
> +            testQueryOnDeleteItem(id);
> +            testSyncDelete(id);

Don't we need to insert another item here?

@@ +188,5 @@
> +         * show up in a query.
> +         *
> +         * @param id of a deleted item to be queried
> +         */
> +        private void testQueryOnDeleteItem(long id) throws Exception {

`assertItemDoesNotExistByID`

@@ +206,5 @@
> +            final int deleted = mProvider.delete(ReadingListItems.CONTENT_URI,
> +                    ReadingListItems._ID + " = ?",
> +                    new String[] { String.valueOf(id) });
> +
> +            mAsserter.is((deleted == 1), true, "Inserted item was deleted");

`mAsserter.is(deleted, 1, "...")`. Same for the below.

@@ +247,5 @@
> +
> +    /**
> +     * Verify that we can update reading list items.
> +     */
> +    class TestUpdateItems extends Test {

How about updating with an invalid id? Make sure the items in the DB are not modified.

@@ +257,5 @@
> +            try {
> +                updated = mProvider.update(ReadingListItems.CONTENT_URI, u,
> +                                           ReadingListItems._ID + " = ?",
> +                                           new String[] { String.valueOf(id) });
> +            } catch (Exception e) {}

Similarly, comment why we're catching here.

@@ +263,5 @@
> +            return updated;
> +        }
> +
> +        @Override
> +        public void test() throws Exception {

nit: I'd prefer to see the test methods at the top too.

@@ +273,5 @@
> +
> +            long dateCreated = c.getLong(c.getColumnIndex(ReadingListItems.DATE_CREATED));
> +            long dateModified = c.getLong(c.getColumnIndex(ReadingListItems.DATE_MODIFIED));
> +
> +            ContentValues u = new ContentValues();

nit: Why u? updated? Why not `updatedValues`?

@@ +285,5 @@
> +                                           new String[] { String.valueOf(id) });
> +
> +            mAsserter.is((updated == 1), true, "Inserted item was updated");
> +
> +            c.close();

nit: finally

@@ +300,5 @@
> +            mAsserter.is(c.getString(c.getColumnIndex(ReadingListItems.LENGTH)), u.getAsString(ReadingListItems.LENGTH),
> +                         "Updated item has correct LENGTH");
> +
> +            mAsserter.is(new Long(c.getLong(c.getColumnIndex(ReadingListItems.DATE_CREATED))),
> +                         new Long(dateCreated),

nit: Does java not autobox this?

@@ +312,5 @@
> +            updated = updateWithNullCol(id, ReadingListItems.GUID);
> +            mAsserter.is((updated > 0), false,
> +                         "Should not be able to update item with null GUID");
> +
> +            c.close();

nit: finally

@@ +317,5 @@
> +        }
> +    }
> +
> +    class TestBatchOperations extends Test {
> +        static final int TESTCOUNT = 100;

nit: 100 may be excessive. I'd say any amount over 3 should be fine, I'd probably go with 10 (somewhat arbitrarily).

@@ +323,5 @@
> +        /**
> +         * Insert a bunch of items into the DB with and
> +         * verify that they are there.
> +         */
> +        public void testBulkInsert() throws Exception {

What happens when you insert crappy values (e.g. null) into the DB in a bulk insert? Should this be tested?

@@ +338,5 @@
> +            mAsserter.is(inserts, TESTCOUNT, "Excepted number of inserts matches");
> +
> +            boolean allFound = true;
> +            for (int i = 0; i < TESTCOUNT; i++) {
> +                Cursor cursor = mProvider.query(ReadingListItems.CONTENT_URI,

Accessing the disk this many times is probably expensive - is there a better way to do it? I imagine building a HashMap with perhaps the URLs, querying for the full DB, and iterating over that, checking against the HashMap (and removing after each find) should be sufficient. Presumably these in-memory operations are faster than touching the disk.

@@ +341,5 @@
> +            for (int i = 0; i < TESTCOUNT; i++) {
> +                Cursor cursor = mProvider.query(ReadingListItems.CONTENT_URI,
> +                                                null,
> +                                                ReadingListItems.URL + " = ?",
> +                                                new String[] { "http://www.test.org/" + i },

nit: Make that url a constant.

@@ +347,5 @@
> +
> +                if (!cursor.moveToFirst()) {
> +                    allFound = false;
> +                }
> +                cursor.close();

nit: finally

@@ +354,5 @@
> +        }
> +
> +        @Override
> +        public void test() throws Exception {
> +            testBulkInsert();

Why not just drop the contents of `testBulkInsert` in here?

@@ +366,5 @@
> +     * okay to test sequentially.
> +     */
> +    class TestBrowserProviderNotifications extends Test {
> +
> +        protected void ensureOnlyChangeNotifiedStartsWith(Uri expectedUri, String operation) {

Since you're only using expectedUri as a String, I'd say you should take it as a String - it makes "*StartsWith" make more sense as a name too.

nit: `ensureChangeNotificationURIStartsWith` or similar

@@ +383,5 @@
> +                            "Notification from " + operation + " was valid");
> +
> +            mAsserter.ok(uri.toString().startsWith(expectedUri.toString()),
> +                         "Content observer was notified exactly once by " + operation,
> +                         uri.toString() + " starts with expected prefix " + expectedUri);

Leave diag blank - I have no idea what it's for. :P

@@ +425,5 @@
> +
> +            ensureOnlyChangeNotifiedStartsWith(ReadingListItems.CONTENT_URI, "delete");
> +
> +            // Bulk insert
> +            final ContentValues[] hs = { createAnItem() };

Do at least 3.

Comment 31

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Note also that some comments (e.g. `finally` around Cursor.close()) I didn't mark all instances of.

Comment 32

[Sola Ogunsakin [:sola]]

Attachment: Part 2: Tests for ReadingListProvider

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8380107 [details] [diff] [review]
Part 2: Tests for ReadingListProvider

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

Moving a bit closer! A bunch of nits, but most importantly, we shouldn't be using the ContentProvider methods in helper methods until we know they work (and in general, it's preferable not to use them). See if you can come up with a solution.

::: mobile/android/base/tests/BaseTest.java
@@ +874,5 @@
>          return null;
>      }
> +
> +    /**
> +     * Abstract class for running test cases. Must implement tests in test() method.

Explain the motivation for this (when we could just be using setUp and tearDown).

@@ +876,5 @@
> +
> +    /**
> +     * Abstract class for running test cases. Must implement tests in test() method.
> +     */
> +    abstract class TestCase implements Runnable {

This should probably be protected.

::: mobile/android/base/tests/testBrowserProvider.java
@@ +247,5 @@
>              test.run();
>          }
>      }
>  
> +    class TestBatchOperations extends TestCase {

Since we're touching all of these anyway, is there any reason not to make them private?

::: mobile/android/base/tests/testReadingListProvider.java
@@ +21,5 @@
> +    private final TestCase[] TESTS_TO_RUN = { new TestInsertItems(),
> +                                              new TestDeleteItems(),
> +                                              new TestUpdateItems(),
> +                                              new TestBatchOperations(),
> +                                              new TestBrowserProviderNotifications() };

Add a comment mentioning that this TestCase uses the content provider methods (e.g. update, insert) and should always be run last.

@@ +44,5 @@
> +
> +    @Override
> +    public void setUp() throws Exception {
> +        super.setUp(sProviderFactory, BrowserContract.READING_LIST_AUTHORITY, DB_NAME);
> +        for(TestCase test: TESTS_TO_RUN) {

nit: `for (`

@@ +62,5 @@
> +
> +    /**
> +     * Verify that we can insert a reading list item into the DB.
> +     */
> +    private class TestInsertItems extends BaseTest.TestCase {

nit: Remove `BaseTest`

@@ +70,5 @@
> +            long id = ContentUris.parseId(mProvider.insert(ReadingListItems.CONTENT_URI, b));
> +            Cursor c = null;
> +
> +            try {
> +                c = getItemById(id);

Move the initialization outside `try`. If this fails, we have no reason to try to close the cursor anyway because it will be null. This removes the need for the null check in `finally`. Here and below.

@@ +92,5 @@
> +            b.putNull(colName);
> +
> +            try {
> +                ContentUris.parseId(mProvider.insert(ReadingListItems.CONTENT_URI, b));
> +                fail("Insertion succeeded with " + colName + " = null");

Actually, I realized 'fail' is an automatically imported junit method - use mAsserter.ok(false, ...)

nit: ==

@@ +94,5 @@
> +            try {
> +                ContentUris.parseId(mProvider.insert(ReadingListItems.CONTENT_URI, b));
> +                fail("Insertion succeeded with " + colName + " = null");
> +            } catch (Exception e) {}
> +

nit: ws

@@ +106,5 @@
> +
> +        @Override
> +        public void test() throws Exception {
> +            long id = insertAnItemWithAssertion();
> +            // Test that item is only marked as deleted and not deleted.

nit: You used the term "flagged" before... which do you mean?

nit: "the item", here and below

nit: "not deleted" -> "removed from the database"

@@ +108,5 @@
> +        public void test() throws Exception {
> +            long id = insertAnItemWithAssertion();
> +            // Test that item is only marked as deleted and not deleted.
> +            testNonFirefoxSyncDelete(id);
> +            // Test that 'deleted' item is doesn't show up in a query.

nit: grammar

@@ +125,5 @@
> +         *
> +         * @param id of a deleted item to be queried
> +         */
> +        private void testQueryOnDeleteItem(long id) throws Exception {
> +            assertItemDoesNotExistByID(id, "Inserted item can't be found after deletion");

nit: I'd take this out of the helper function and dump it in testNonFirefoxSyncDelete - it's part of the same "Firefox Sync" test case, imo. Just add a comment why assertItemExistsByID and assertItemDoesNotExistByID contradict each other (i.e. uris) ;).

@@ +130,5 @@
> +        }
> +
> +        /**
> +         * Delete an item with PARAM_IS_SYNC unset and verify that item was only flagged
> +         * as deleted and not actually deleted.

nit: "removed from the database."

@@ +168,5 @@
> +         * was actually deleted.
> +         *
> +         * @param id of the item to be deleted
> +         */
> +        private void testDeleteWithURI(long id) throws Exception {

nit: testDeleteWithItemURI or similar.

@@ +177,5 @@
> +
> +    /**
> +     * Verify that we can update reading list items.
> +     */
> +    private class TestUpdateItems extends BaseTest.TestCase {

nit: Remove BaseTest.

@@ +182,5 @@
> +
> +        @Override
> +        public void test() throws Exception {
> +            ContentValues original = createFillerReadingListItem();
> +            long id = ContentUris.parseId(mProvider.insert(ReadingListItems.CONTENT_URI, original));

We shouldn't use mProvider.insert here because we're not definitely sure that it works! Is it possible to insert these values by hand? If it's too messy (e.g. schema is likely to change and versioning is too hard - very likely :P and probably not worth your time), then add a comment to the tests to ensure that testInsert runs before testUpdate!

@@ +218,5 @@
> +                mAsserter.isnot(c.getLong(c.getColumnIndex(ReadingListItems.DATE_MODIFIED)),
> +                        originalDateModified,
> +                        "Date modified should have changed");
> +
> +                ContentValues expectedValues = updates;

nit: Do this above, outside the try, and add a comment mentioning this is a name change for clarity.

@@ +220,5 @@
> +                        "Date modified should have changed");
> +
> +                ContentValues expectedValues = updates;
> +                expectedValues.put(ReadingListItems.DATE_MODIFIED,
> +                                   c.getLong(c.getColumnIndex(ReadingListItems.DATE_MODIFIED)));

Rather than setting this with a value from the DB, add a version of assertRowEqualContentValues with a boolean that is "should I assert date modified?". I'd recommend overloading the method, and defaulting it to "yes", just so we force people to think about this.

Getting the value from the DB is risky because it may return two different values (e.g. concurrency).

If we can never check this value without getting it from the DB, then forget checking it at all.

@@ +225,5 @@
> +
> +                // DATE_CREATED and LENGTH should equal old values since they weren't updated.
> +                expectedValues.put(ReadingListItems.DATE_CREATED, originalDateCreated);
> +                expectedValues.put(ReadingListItems.LENGTH, original.getAsString(ReadingListItems.LENGTH));
> +                assertRowEqualsContentValues(c, updates);

nit: expectedValues

@@ +247,5 @@
> +         * @param id of the item to be deleted
> +         */
> +        private void testUpdateWithInvalidID() throws Exception {
> +            ensureEmptyDatabase();
> +            final long INVALID_ID = 1234567890;

You don't know that this will be an invalid id - the insertion can use this value! Rather, use the returned `id + 1`.

@@ +268,5 @@
> +                updated = mProvider.update(ReadingListItems.CONTENT_URI, updates,
> +                                           ReadingListItems._ID + " = ?",
> +                                           new String[] { String.valueOf(id) });
> +            } catch (Exception e) {
> +                // We catch here so that we can see if the assertion failed or not.

We can use `fail` here, right?

@@ +276,5 @@
> +        }
> +    }
> +
> +    private class TestBatchOperations extends TestCase {
> +        static final int TESTCOUNT = 10;

nit: private

nit: It's not a test count - it's an item count! Or a test item count.

@@ +279,5 @@
> +    private class TestBatchOperations extends TestCase {
> +        static final int TESTCOUNT = 10;
> +
> +        /**
> +         * Insert a bunch of items into the DB with and

with...?

@@ +303,5 @@
> +            Cursor c = null;
> +            boolean allFound = true;
> +            try {
> +                c = mProvider.query(ReadingListItems.CONTENT_URI, null, null, null, null, null);
> +                while(allFound && c.moveToNext()) {

nit: `while (`

@@ +304,5 @@
> +            boolean allFound = true;
> +            try {
> +                c = mProvider.query(ReadingListItems.CONTENT_URI, null, null, null, null, null);
> +                while(allFound && c.moveToNext()) {
> +                    allFound = urls.contains(c.getString(c.getColumnIndex(ReadingListItems.URL)));

Remove the url from the HashSet after it's found so we're not double-counting (e.g. DB inserts the same item 100 times - they'll all be in the HashSet and this will pass).

Rather than setting a boolean here, just `mAsserter.ok(urls.contains(...`

@@ +334,5 @@
> +        public void test() throws Exception {
> +            // Insert
> +            final ContentValues h = createFillerReadingListItem();
> +
> +            mResolver.notifyChangeList.clear();

Do this first, under the comment, to make it obvious that's it's required for all tests.

@@ +337,5 @@
> +
> +            mResolver.notifyChangeList.clear();
> +            long id = ContentUris.parseId(mProvider.insert(ReadingListItems.CONTENT_URI, h));
> +
> +            mAsserter.isnot(Long.valueOf(id),

nit: Java doesn't autobox this? Here and below.

@@ +341,5 @@
> +            mAsserter.isnot(Long.valueOf(id),
> +                            -1L,
> +                            "Inserted item has valid id");
> +
> +            ensureOnlyChangeNotifiedStartsWith(ReadingListItems.CONTENT_URI.toString(), "insert");

Rather than calling .toString() so many times, just set it as a constant. Can probably be `private final static`

@@ +397,5 @@
> +            mAsserter.isnot(uri,
> +                            null,
> +                            "Notification from " + operation + " was valid");
> +
> +            mAsserter.ok(uri.toString().startsWith(expectedUri),

nit: If you do "expectedURI.startsWith", you can remove the validation above. As an internal API, we don't really need to validate the args (unless they're assertions for sanity checks - in this case, we'd catch it with expectedUri.startsWith throwing a null pointer exception, granted if this code changes, that won't be the case... e.g. I'm conflicted here, leave it in if you'd like :).

@@ +402,5 @@
> +                         "Content observer was notified exactly once by " + operation,
> +                         "");
> +        }
> +    }
> +

nit: ws

@@ +409,5 @@
> +     * Removes all items from the DB.
> +     */
> +    private void ensureEmptyDatabase() throws Exception {
> +        Uri uri = appendUriParam(ReadingListItems.CONTENT_URI, BrowserContract.PARAM_IS_SYNC, "1");
> +        mProvider.delete(uri, null, null);

Again, you should delete directly from the DB if you can. If not, you probably want to comment that this should NEVER be called before testDelete is verified to work (and ensure that is the case! - maybe add a member var, "wasDeleteTested" or such).

@@ +416,5 @@
> +        Cursor c = null;
> +        try {
> +            c = mProvider.query(ReadingListItems.CONTENT_URI, null, null, null, null, null);
> +            mAsserter.is(c.getCount(), 0, "Database was successfully emptied");
> +        } catch(Exception e) {

Catch the specific exceptions we're looking for.

@@ +432,5 @@
> +     *
> +     * @param cursor over the row to be checked
> +     * @param values to be checked
> +     */
> +    private void assertRowEqualsContentValues(Cursor cursor, ContentValues values) {

nit: It would be slick to do this in a loop. You should have the strings you need for the messages in the ReadingListItems constants.

nit: `cursorWithActual` or similar. `expectedValues` or similar.

@@ +449,5 @@
> +        mAsserter.is(cursor.getString(cursor.getColumnIndex(ReadingListItems.LENGTH)),
> +                     values.getAsString(ReadingListItems.LENGTH),
> +                     "Item has correct LENGTH");
> +
> +        mAsserter.is(cursor.getLong(cursor.getColumnIndex(ReadingListItems.DATE_CREATED)),

Is ReadingListItems.DATE_CREATED in your previous patch? I don't see it defined.

@@ +471,5 @@
> +    }
> +
> +    private ContentValues createFillerReadingListItem() throws Exception {
> +        Random rand = new Random();
> +        return fillContentValues("Example", "http://example.com" + rand.nextInt(), "foo bar");

nit: `..."http://example.com/" + rand.nextInt()...` It's probably better to make it look like a real URL. :)

Comment 34

[Richard Newman [:rnewman]]

Note that testBrowserProvider just churned a little bit: Bug 975792.

Comment 35

[Sola Ogunsakin [:sola]]

(In reply to Michael Comella (:mcomella) from comment #33)
> Comment on attachment 8380107 [details] [diff] [review]
> Part 2: Tests for ReadingListProvider
> 
> Review of attachment 8380107 [details] [diff] [review]:
> ----------------------------------------------------------------
> @@ +220,5 @@
> > +                        "Date modified should have changed");
> > +
> > +                ContentValues expectedValues = updates;
> > +                expectedValues.put(ReadingListItems.DATE_MODIFIED,
> > +                                   c.getLong(c.getColumnIndex(ReadingListItems.DATE_MODIFIED)));
> 
> Rather than setting this with a value from the DB, add a version of
> assertRowEqualContentValues with a boolean that is "should I assert date
> modified?". I'd recommend overloading the method, and defaulting it to
> "yes", just so we force people to think about this.
> 
> Getting the value from the DB is risky because it may return two different
> values (e.g. concurrency).
> 
> If we can never check this value without getting it from the DB, then forget
> checking it at all.
> 

I believe all the tests/db updates are synchronous,

> @@ +449,5 @@
> > +        mAsserter.is(cursor.getString(cursor.getColumnIndex(ReadingListItems.LENGTH)),
> > +                     values.getAsString(ReadingListItems.LENGTH),
> > +                     "Item has correct LENGTH");
> > +
> > +        mAsserter.is(cursor.getLong(cursor.getColumnIndex(ReadingListItems.DATE_CREATED)),
> 
> Is ReadingListItems.DATE_CREATED in your previous patch? I don't see it
> defined.
yeah it was

Comment 36

[Sola Ogunsakin [:sola]]

Attachment: bug-959290-tests.patch

Comment 37

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Sola Ogunsakin [:sola] from comment #35)
> I believe all the tests/db updates are synchronous,

Concurrency was just an example. Other things can go wrong too (e.g. buggy disk driver, damaged disk), and concurrency may be added in the future!

All in all, there's little reason to get a value from a data structure and later assert that it's the same value when you try to get it again when you're not testing the data structure - it just introduces the potential for additional errors.

Comment 38

[Sola Ogunsakin [:sola]]

(In reply to Michael Comella (:mcomella) from comment #37)
> All in all, there's little reason to get a value from a data structure and
> later assert that it's the same value when you try to get it again when
> you're not testing the data structure - it just introduces the potential for
> additional errors.

Alright I'll take out that testcase

Comment 39

[Sola Ogunsakin [:sola]]

Attachment: bug-959290-tests.patch

Comment 40

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8381829 [details] [diff] [review]
bug-959290-tests.patch

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

Looking good - we're getting closer! Note that I may have repeated myself in some places.

::: mobile/android/base/tests/BaseTest.java
@@ +874,5 @@
>          return null;
>      }
> +
> +    /**
> +     * Abstract class for running small test cases within a BaseTest. Must implement tests in test() method.

nit: Add that we do this so we can avoid the overhead of starting Gecko and creating profiles.

Information about the test() method should go over the test method.

::: mobile/android/base/tests/ContentProviderTest.java
@@ +92,5 @@
>              return getInstrumentation().getContext().getApplicationInfo();
>          }
>      }
>  
> +    class DelegatingTestContentProvider extends ContentProvider {

nit: protected.

::: mobile/android/base/tests/testReadingListProvider.java
@@ +21,5 @@
> +    private static final String DB_NAME = "browser.db";
> +
> +    // List of tests to be run sorted by dependency.
> +    private final TestCase[] TESTS_TO_RUN = { new TestInsertItems(),
> +                                              // Dependency: TestInsertItems

This seems like an awkward place for these comments - I would probably put it in the method javadoc and append the comment above the test cases to say there are dependencies in the javadoc, but I could go either way. Your call.

@@ +56,5 @@
> +        }
> +    }
> +
> +    public void testReadingListProvider() throws Exception {
> +        for (int i = 0; i < mTests.size(); i++) {

nit: I'd rather use an iterator/for-each here.

@@ +96,5 @@
> +
> +            try {
> +                ContentUris.parseId(mProvider.insert(ReadingListItems.CONTENT_URI, b));
> +                // If we get to here, the flawed insertion succeeded. Fail the test.
> +                mAsserter.ok(false, "Insertion did not succeed with " + colName + " = null", "");

nit: ==

@@ +97,5 @@
> +            try {
> +                ContentUris.parseId(mProvider.insert(ReadingListItems.CONTENT_URI, b));
> +                // If we get to here, the flawed insertion succeeded. Fail the test.
> +                mAsserter.ok(false, "Insertion did not succeed with " + colName + " = null", "");
> +            } catch (Exception e) {}

Catch the specific exception that insert should be throwing, not everything.

@@ +183,5 @@
> +        @Override
> +        public void test() throws Exception {
> +            ContentValues original = createFillerReadingListItem();
> +            long id = ContentUris.parseId(mProvider.insert(ReadingListItems.CONTENT_URI, original));
> +            Cursor c = getItemById(id);

Do this as the last thing before the try because if we throw on something else, we don't close the cursor.

@@ +199,5 @@
> +                updates.put(ReadingListItems.TITLE, original.getAsString(ReadingListItems.TITLE) + "CHANGED");
> +                updates.put(ReadingListItems.URL, original.getAsString(ReadingListItems.URL) + "/more/stuff");
> +                updates.put(ReadingListItems.EXCERPT, original.getAsString(ReadingListItems.EXCERPT) + "CHANGED");
> +
> +                updated = mProvider.update(ReadingListItems.CONTENT_URI, updates,

You should verify that the updates are in the DB (though verify that originalDate is the same and dateModified is not) - you don't need to remove the whole test case.

Perhaps I misread this before (and if so, sorry!)?

@@ +241,5 @@
> +        private int testUpdateWithNullCol(long id, String colName) throws Exception {
> +            ContentValues updates = new ContentValues();
> +            updates.putNull(colName);
> +
> +            int updated = 0;

Why initialize this to the same value we assert? Wouldn't we want to fail in that case (as update does not throw - see below)?

@@ +246,5 @@
> +            try {
> +                updated = mProvider.update(ReadingListItems.CONTENT_URI, updates,
> +                                           ReadingListItems._ID + " = ?",
> +                                           new String[] { String.valueOf(id) });
> +            } catch (Exception e) { }

The documentation says that ContentProvider.update does not throw - is this catch necessary? If so, catch the specific exception you need to catch, not all of them.

@@ +282,5 @@
> +            try {
> +                while (c.moveToNext()) {
> +                    final String url = c.getString(c.getColumnIndex(ReadingListItems.URL));
> +                    mAsserter.ok(urls.contains(url), "Bulk inserted item with url = " + url + " was found in the DB", "");
> +                    urls.remove(url);

nit: Explain why we remove.

@@ +292,5 @@
> +
> +        @Override
> +        public void test() throws Exception {
> +            testBulkInsert();
> +            ensureEmptyDatabase();

Unnecessary - we do this before every test starts.

@@ +312,5 @@
> +            mResolver.notifyChangeList.clear();
> +
> +            // Insert
> +            final ContentValues h = createFillerReadingListItem();
> +

nit: ws.

@@ +350,5 @@
> +            final ContentValues[] hs = { createFillerReadingListItem(),
> +                                         createFillerReadingListItem(),
> +                                         createFillerReadingListItem() };
> +
> +            mResolver.notifyChangeList.clear();

nit: Move this directly under the comment so it's apparent it should be consistent.

@@ +401,5 @@
> +     * @param cursor over the row to be checked
> +     * @param values to be checked
> +     */
> +    private void assertRowEqualsContentValues(Cursor cursorWithActual, ContentValues expectedValues, boolean compareDateModified) {
> +        final String[] COLUMNS = {  ReadingListItems.TITLE,

nit: I *think* this will be constructed each time this method is run - I would make it a class variable.

@@ +436,5 @@
> +    }
> +
> +    private ContentValues createFillerReadingListItem() throws Exception {
> +        Random rand = new Random();
> +        return fillContentValues("Example", "http://example.com/?num=" + rand.nextInt(), "foo bar");

^_^

@@ +439,5 @@
> +        Random rand = new Random();
> +        return fillContentValues("Example", "http://example.com/?num=" + rand.nextInt(), "foo bar");
> +    }
> +
> +    private Cursor getItemById(Uri uri, long id, String[] projection) throws Exception {

Specify the specific Exception this throws, here and everywhere.

@@ +456,5 @@
> +    }
> +
> +    private long insertAnItemWithAssertion() throws Exception {
> +        ContentValues v = createFillerReadingListItem();
> +        long id = ContentUris.parseId(mProvider.insert(ReadingListItems.CONTENT_URI, v));

Before you use these, it'd be cool if you asserted that they were tested (i.e. set globals after each test is run).

Comment 41

[Sola Ogunsakin [:sola]]

Attachment: Part 2: ReadingListProvider tests

Comment 42

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8383307 [details] [diff] [review]
Part 2: ReadingListProvider tests

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

r+ w/ nits.

::: mobile/android/base/tests/testReadingListProvider.java
@@ +33,5 @@
> +                                    ReadingListItems.EXCERPT,
> +                                    ReadingListItems.LENGTH,
> +                                    ReadingListItems.DATE_CREATED };
> +
> +    // Indicates whether insertion have been tested.

nit: Explain that this is so we can use ContentProvider.insert, rather than inserting by hand.

@@ +34,5 @@
> +                                    ReadingListItems.LENGTH,
> +                                    ReadingListItems.DATE_CREATED };
> +
> +    // Indicates whether insertion have been tested.
> +    private boolean mInsertionsTested = false;

nit: mInsertTested or mContentProviderInsertTested (since it's the `insert` method).

@@ +73,5 @@
> +
> +    /**
> +     * Verify that we can insert a reading list item into the DB.
> +     * Depends on TestInsertItems because it uses the content provider's
> +     * insert method.

It depends on itself? That doesn't make much sense - remove that comment.

@@ +187,5 @@
> +
> +    /**
> +     * Verify that we can update reading list items.
> +     * Depends on TestInsertItems because it uses the content provider's
> +     * insert method.

On second thought, let's take out these dependency comments and document it in code (while we could leave in the comments, it's best not to duplicate information). Throw if `mInsertionsTested == false` before the insertion, they'll get the message if they reorder the tests.

@@ +232,5 @@
> +
> +                // DATE_CREATED and LENGTH should equal old values since they weren't updated.
> +                expectedValues.put(ReadingListItems.DATE_CREATED, originalDateCreated);
> +                expectedValues.put(ReadingListItems.LENGTH, original.getAsString(ReadingListItems.LENGTH));
> +            final boolean compareDateModified = false;

nit: indendation (I personally would leave out the temp var, but it's up to you. I usually prefer something like `assertRowEqualsContentValues(c, expectedValues, /* compareDateModified */ false);` but I haven't seen that used in our codebase so I'm not sure if it's okay. :P

@@ +275,5 @@
> +            int updated = mProvider.update(ReadingListItems.CONTENT_URI, updates,
> +                                           ReadingListItems._ID + " = ?",
> +                                           new String[] { String.valueOf(id) });
> +
> +            mAsserter.is(updated, 0, "Should not be able to update item with " + colName + " = null ");

nit: ==

@@ +308,5 @@
> +            Cursor c = mProvider.query(ReadingListItems.CONTENT_URI, null, null, null, null, null);
> +            try {
> +                while (c.moveToNext()) {
> +                    final String url = c.getString(c.getColumnIndex(ReadingListItems.URL));
> +                    mAsserter.ok(urls.contains(url), "Bulk inserted item with url = " + url + " was found in the DB", "");

nit: ==

@@ +338,5 @@
> +        @Override
> +        public void test() {
> +            final String CONTENT_URI = ReadingListItems.CONTENT_URI.toString();
> +
> +            mResolver.notifyChangeList.clear();

nit: Move under Insert comment below.

@@ +480,5 @@
> +    }
> +
> +    private long insertAnItemWithAssertion() {
> +        if (!mInsertionsTested) {
> +            fail("Insertions have not been tested yet.");

`mAsserter.ok(false, ...)`

Comment 43

[Sola Ogunsakin [:sola]]

Attachment: Part 1: Make ReadingListProvider

Comment 44

[Sola Ogunsakin [:sola]]

Attachment: Part 2: ReadingListProvider tests

Comment 45

[Sola Ogunsakin [:sola]]

Attachment: Part 1: Make ReadingListProvider

oops didn't qref

Comment 46

[Ryan VanderMeulen [:RyanVM]]

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

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ff556fe51be8
https://hg.mozilla.org/mozilla-central/rev/b77f7a5ba31e