I expect this'll also address part of Bug 1127451. See fields: https://github.com/mozilla-services/readinglist/wiki/API-Design-proposal

I'll build on this to continue to adjust the schema, but uploading for early feedback. You can't remove a constraint with ALTER TABLE, so the only option is migrate-drop-rename.

Now with nulled-out GUIDs.

This introduces a slightly different approach to fixing the current-create problem, and also hooks in the schema change.

Just noticed that we have an index on a unique column. No need.

More fleshing out.

Comment on attachment 8560615 [details] [diff] [review] Part 1: use _id instead of guid when referring to reading list items. v2 Review of attachment 8560615 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/db/BrowserDatabaseHelper.java @@ +705,1 @@ > try { Update this comment: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserDatabaseHelper.java?force=1#690 @@ +824,5 @@ > + ReadingListItems.URL + ")"); > + db.execSQL("CREATE UNIQUE INDEX reading_list_guid ON " + TABLE_READING_LIST + "(" + > + ReadingListItems.GUID + ")"); > + db.execSQL("CREATE INDEX reading_list_content_status ON " + TABLE_READING_LIST + "(" + > + ReadingListItems.CONTENT_STATUS + ")"); Does the same issue as bug 1128523 apply here? Do we need to do all this if we went through upgradeDatabaseFrom17to18? ::: mobile/android/base/tests/testReadingListProvider.java @@ -272,5 @@ > updates.put(ReadingListItems.TITLE, b.getAsString(ReadingListItems.TITLE) + "CHANGED"); > final int updated = mProvider.update(ReadingListItems.CONTENT_URI, updates, > - ReadingListItems._ID + " = ?", > - new String[] { String.valueOf(INVALID_ID) }); > - mAsserter.is(updated, 0, "Should not be able to update item with an invalid GUID"); Heh, this test didn't even use the GUID column here.

Comment on attachment 8560662 [details] [diff] [review] Part 2: adjust schema upgrading process for reading list DB. v1 Review of attachment 8560662 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/db/BrowserDatabaseHelper.java @@ +35,2 @@ > private static final String LOGTAG = "GeckoBrowserDBHelper"; > + Nit: whitespace. @@ +797,5 @@ > } > } > > private void upgradeDatabaseFrom22to23(SQLiteDatabase db) { > + if (didCreateCurrentReadingListTable) { Oh, you dealt with this here :)

Comment on attachment 8560823 [details] [diff] [review] Part 2: adjust schema upgrading process for reading list DB. v3 I'll have a newer version later today. Thanks for the early feedback!

Comment on attachment 8560823 [details] [diff] [review] Part 2: adjust schema upgrading process for reading list DB. v3 Review of attachment 8560823 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/db/BrowserDatabaseHelper.java @@ +856,5 @@ > + "NULL, modified, created, content_status, " + > + "CASE read WHEN 1 THEN % ELSE NULL END, " + > + "%" + > + " FROM " + TABLE_READING_LIST, > + new String[] {thisDevice, thisDevice}); Can we use ContentProviderTest to make a simple test for this?

This is the first two patches squashed together. This patch does several things: * Uses _id instead of guid when referring to reading list items, allowing the guid column to be null. * Reworks schema upgrading. * Completely revises the reading list schema itself. * Fixes the tests. * Cleans up how we do deletion: if an item hasn't yet been synced, it's simply deleted immediately. We can do this because the server allocates GUIDs. Notably it does *not* yet track the metadata needed for syncing. I'll write that as a separate patch for clarity, and then fold them down before landing.

It turns out we were using bookmark columns here, which was wrong. We also need to specify ADDED_ON etc.

Fixed a couple of things.

This adjusts some sync-related ADDED_ON handling.

This is a WIP for the small schema extension for syncing itself, just FYI. I'm working on tests for state transitions, and the minor other changes needed to track these states.

Comment on attachment 8563687 [details] [diff] [review] Part 1: change schema for reading list. v3 Review of attachment 8563687 [details] [diff] [review]: ----------------------------------------------------------------- This is generally looking good, but I want to look at it more closely when I'm feeling less tired. I'll finish reviewing by tomorrow morning. ::: mobile/android/base/db/LocalReadingListAccessor.java @@ +15,2 @@ > > public class LocalReadingListAccessor implements ReadingListAccessor { Really love that this logic is in its own class now. @@ +67,5 @@ > @Override > public boolean isReadingListItem(ContentResolver cr, String uri) { > final Cursor c = cr.query(mReadingListUriWithProfile, > + new String[] { ReadingListItems._ID }, > + ReadingListItems.URL + " = ? OR " + ReadingListItems.RESOLVED_URL + " = ?", This makes me a bit worried because our UI code currently has no concept of "resolved url". Right now we use this code to show an "add" or "remove" button in reader mode: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/ReadingListHelper.java#210 Do we also need to take the resolved url into account when displaying items in the list? I suppose we should only show resolved urls in the list. And I also suppose that we would only ever show the resolved url in reader mode (need to resolve to get to the content!). This seems fine for now, but let's keep an eye on this and think through what we're going to do in the UI.

(In reply to :Margaret Leibovic from comment #18) > This is generally looking good, but I want to look at it more closely when > I'm feeling less tired. I'll finish reviewing by tomorrow morning. Much appreciated. > This makes me a bit worried because our UI code currently has no concept of > "resolved url". Yeah, I'll be fixing this across the board a little later. It'll affect the fetcher, the UI, and syncing -- essentially everywhere we touch a URL needs to decide whether to use both, or one in particular (usually only when writing). On the plus side, it means reading list items won't show "http://t.co/12314", but instead will resolve to -- and thus show -- "http://nytimes.com/arts/...".

Saving my work.

(In reply to Richard Newman [:rnewman] from comment #19) > > This makes me a bit worried because our UI code currently has no concept of > > "resolved url". > > Yeah, I'll be fixing this across the board a little later. It'll affect the > fetcher, the UI, and syncing -- essentially everywhere we touch a URL needs > to decide whether to use both, or one in particular (usually only when > writing). > > On the plus side, it means reading list items won't show > "http://t.co/12314", but instead will resolve to -- and thus show -- > "http://nytimes.com/arts/...". I don't think this is actually an issue now that we have the content fetching logic, since I believe the Readability logic just updates the url field. But I may be wrong. In any case, we definitely need to think through how we want this to all work in an ideal world (and then start fixing things).

Comment on attachment 8563687 [details] [diff] [review] Part 1: change schema for reading list. v3 Review of attachment 8563687 [details] [diff] [review]: ----------------------------------------------------------------- This is generally looking good, but I want to look at it more closely when I'm feeling less tired. I'll finish reviewing by tomorrow morning. ::: mobile/android/base/db/LocalReadingListAccessor.java @@ +15,2 @@ > > public class LocalReadingListAccessor implements ReadingListAccessor { Really love that this logic is in its own class now. @@ +67,5 @@ > @Override > public boolean isReadingListItem(ContentResolver cr, String uri) { > final Cursor c = cr.query(mReadingListUriWithProfile, > + new String[] { ReadingListItems._ID }, > + ReadingListItems.URL + " = ? OR " + ReadingListItems.RESOLVED_URL + " = ?", This makes me a bit worried because our UI code currently has no concept of "resolved url". Right now we use this code to show an "add" or "remove" button in reader mode: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/ReadingListHelper.java#210 Do we also need to take the resolved url into account when displaying items in the list? I suppose we should only show resolved urls in the list. And I also suppose that we would only ever show the resolved url in reader mode (need to resolve to get to the content!). This seems fine for now, but let's keep an eye on this and think through what we're going to do in the UI. ::: mobile/android/base/db/ReadingListProvider.java @@ +148,5 @@ > + final String whereNullGUID = ReadingListItems._ID + " = " + id + " AND " + ReadingListItems.GUID + " IS NULL"; > + final String whereNotNullGUID = ReadingListItems._ID + " = " + id + " AND " + ReadingListItems.GUID + " IS NOT NULL"; > + > + total += db.delete(TABLE_READING_LIST, whereNullGUID, null); > + total += updateItems(uri, DELETED_VALUES, whereNotNullGUID, null); When would both of these things happen? Isn't _ID a unique column? ::: mobile/android/base/db/SharedBrowserDatabaseProvider.java @@ +114,5 @@ > + protected String getDeletedItemSelection(long earlierThan) { > + if (earlierThan == -1L) { > + return BrowserContract.SyncColumns.IS_DELETED + " = 1"; > + } > + return BrowserContract.SyncColumns.IS_DELETED + " = 1 AND " + BrowserContract.SyncColumns.DATE_MODIFIED + " <= " + earlierThan; We already import SyncColumns, you can drop the BrowserContract bit.

Stupid splinter re-posted some of my comments, ignore those.

(In reply to :Margaret Leibovic from comment #21) > I don't think this is actually an issue now that we have the content > fetching logic, since I believe the Readability logic just updates the url > field. But I may be wrong. Yeah, this should only be relevant for items that are added before resolution (e.g., via Add to Firefox). If you've loaded a page in Reader Mode, then tapped Add to List, then sure, it'll have already been resolved, and we just put the same URL in both fields. If we do a background fetch, then we don't change the existing URL field -- we put the final URL and title in the resolved_ fields.

(In reply to :Margaret Leibovic from comment #22) > > + total += db.delete(TABLE_READING_LIST, whereNullGUID, null); > > + total += updateItems(uri, DELETED_VALUES, whereNotNullGUID, null); > > When would both of these things happen? Isn't _ID a unique column? They wouldn't both happen; either could happen, and we don't know without trying. I guess I could check for success in the first before performing the second pass. (I might also switch this to explicitly check the sync state == NEW in a later patch; this one only has the GUID to work with.)

This is the approach we just discussed on IRC; using UPDATE to change flags in the DB in one shot, rather than having to either separate every flag into its own column, or having to issue two queries each time (SELECT then UPDATE).

Spotted when I came to use this that I'd omitted some `static`s.

I think I found a solution. Not yet tested.

/r/3955 - Bug 1130461 - Part 0: remove redundant comment. /r/3957 - Bug 1130461 - Part 1: change schema for reading list. r=margaret /r/3959 - Bug 1130461 - Part 2: correct value setting in AddToReadingList. r=margaret /r/3961 - Bug 1130461 - Part 4: implement in-place computed UPDATE in DBUtils. /r/3963 - Bug 1130461 - Part 3: track enough information to allow RL syncing. Pull down these commits: hg pull review -r 054af2e52943950d255fed98db85d913dc89b70a

Comment on attachment 8563102 [details] [diff] [review] Part 2: correct value setting in AddToReadingList. v1 Switching to ReviewBoard.

Comment on attachment 8565778 [details] [diff] [review] Part 4: implement in-place computed UPDATE in DBUtils. v3 Switching to ReviewBoard.

Comment on attachment 8565813 [details] MozReview Request: bz://1130461/rnewman Sorry for the churn, Margaret.

Comment on attachment 8565813 [details] MozReview Request: bz://1130461/rnewman /r/3955 - Bug 1130461 - Part 0: remove redundant comment. /r/3957 - Bug 1130461 - Part 1: change schema for reading list. r=margaret /r/3959 - Bug 1130461 - Part 2: correct value setting in AddToReadingList. r=margaret /r/3961 - Bug 1130461 - Part 4: implement in-place computed UPDATE in DBUtils. /r/3963 - Bug 1130461 - Part 3: track enough information to allow RL syncing. Pull down these commits: hg pull review -r 1ece12fcdf533b5b83ed36ddae5ccc3aa9edda00

Comment on attachment 8565813 [details] MozReview Request: bz://1130461/rnewman /r/3955 - Bug 1130461 - Part 0: remove redundant comment. /r/3957 - Bug 1130461 - Part 1: change schema for reading list. r=margaret /r/3959 - Bug 1130461 - Part 2: correct value setting in AddToReadingList. r=margaret /r/3961 - Bug 1130461 - Part 4: implement in-place computed UPDATE in DBUtils. /r/3963 - Bug 1130461 - Part 3: track enough information to allow RL syncing. /r/3995 - Bug 1130461 - Part 5: implement shutdown() for SharedBrowserDatabaseProvider. Pull down these commits: hg pull review -r cbf7f58d685074a56db30c5a7f787e8fdc5ad68c

Comment on attachment 8565813 [details] MozReview Request: bz://1130461/rnewman /r/3955 - Bug 1130461 - Part 0: remove redundant comment. /r/3957 - Bug 1130461 - Part 1: change schema for reading list. r=margaret /r/3959 - Bug 1130461 - Part 2: correct value setting in AddToReadingList. r=margaret /r/3961 - Bug 1130461 - Part 4: implement in-place computed UPDATE in DBUtils. /r/3963 - Bug 1130461 - Part 3: track enough information to allow RL syncing. /r/3995 - Bug 1130461 - Part 5: implement shutdown() for SharedBrowserDatabaseProvider. /r/4009 - Bug 1130461 - Part 6: migrate url and title to resolved_url and resolved_title if content_status is FETCHED. /r/4011 - Bug 1130461 - Part 7: set resolved title and URL in ReadingListHelper if content_status is FETCHED. Pull down these commits: hg pull review -r 72f7401b222017f29f0fea3a208cd0c78d7e006c

https://reviewboard.mozilla.org/r/3955/#review3215 Ship It!

https://reviewboard.mozilla.org/r/3957/#review3205 Ship It! ::: mobile/android/base/db/ReadingListProvider.java (Diff revision 2) > + // TODO: mark for syncing based on the fields in `values`. Make sure there are bugs filed for any outstanding TODO items.

https://reviewboard.mozilla.org/r/3959/#review3217 Ship It!

https://reviewboard.mozilla.org/r/3961/#review3211 Ship It! ::: mobile/android/base/db/DBUtils.java (Diff revision 3) > + public static int updateArrays(SQLiteDatabase db, String table, ContentValues[] values, UpdateOperation[] ops, String whereClause, String[] whereArgs) { Can we write a unit test for this? I want to be able to trust that it "just works".

https://reviewboard.mozilla.org/r/3963/#review3209 Ship It! ::: mobile/android/base/db/ReadingListAccessor.java (Diff revision 4) > + void updateContent(ContentResolver cr, long itemID, String resolvedTitle, String resolvedURL, String excerpt); With this new method, do we still need updateReadingListItem(cr, values)? ::: mobile/android/base/tests/testReadingListProvider.java (Diff revision 4) > + // TODO: CONTENT_STATUS on deletion? This sould probably be part of deleting the content from the cache, which will happen when we delete the item. We could reset the CONTENT_STATUS to UNFETCHED once we delete the content from the cache. ::: mobile/android/base/tests/testReadingListProvider.java (Diff revision 4) > + private class TestStateSequencing extends TestCase { This test gives me more confidence in our logic. I'm happy that you wrote this.

https://reviewboard.mozilla.org/r/3995/#review3219 Ship It!

https://reviewboard.mozilla.org/r/4009/#review3221 Ship It!

https://reviewboard.mozilla.org/r/4011/#review3213 Ship It! ::: mobile/android/base/ReadingListHelper.java (Diff revision 1) > + if (message.has("resolved_title")) { Will there be another bug to update this on the JS side?

Comment on attachment 8565813 [details] MozReview Request: bz://1130461/rnewman https://reviewboard.mozilla.org/r/3953/#review3223 Ship It!

There is a lot of logic in these patches, but overall it looks reasonable to me, and the tests give me a lot more confidence. I think we should ask QA to verify some migrations manually (I wonder if we can even get some very old builds to test the migration path through the bookmarks migration).

(In reply to :Margaret Leibovic from comment #41) Thanks for the thorough review! > With this new method, do we still need updateReadingListItem(cr, values)? Yes, unless we amend ReadingListHelper. I might do that in a follow-up. > This sould probably be part of deleting the content from the cache, which > will happen when we delete the item. We could reset the CONTENT_STATUS to > UNFETCHED once we delete the content from the cache. Yeah, I left code notes for that; this is a reminder (sans bug number) to do the same for tests! > > + if (message.has("resolved_title")) { > > Will there be another bug to update this on the JS side? Yes. It's not urgent. > I think we should ask QA to verify some migrations manually (I wonder if we > can even get some very old builds to test the migration path through the > bookmarks migration). I manually verified migration from the previous schema version, and everything seemed to work. Will do so again for the full bundle before I land it. More verification always welcome! > > + public static int updateArrays(SQLiteDatabase db, String table, ContentValues[] values, UpdateOperation[] ops, String whereClause, String[] whereArgs) { > > Can we write a unit test for this? I want to be able to trust that it "just > works". Yeah, it's on my list. Will do so before it lands.

First landing: DBUtils change, with a test. Test passes locally, so I'm going to skip the lengthy try run; we can back out if needed.

I wondered why I was hitting the network block, and assumed a test harness issue, but it was too repeatable to be the harness. Turns out that adding to RL -> ContentObserver -> background fetch of the page! My bad. This patch is the simplest thing to avoid that: a volatile boolean in the ReadingListHelper to simply not trigger the background fetch. We flip that before running RL tests, which should entirely avoid the problem (until we need to write tests for background fetches). Trying to do this on the Gecko side ran me into a maze of which-messages-do-I-need-to-send that was too much for my Saturday morning brain, so this is the result.

Woohoo green try!

I landed parts as deps, so no need to [leave open].

https://hg.mozilla.org/integration/fx-team/rev/861d28fc5194 https://hg.mozilla.org/integration/fx-team/rev/1352ce4c6916 https://hg.mozilla.org/integration/fx-team/rev/73d9b63d4df1

https://hg.mozilla.org/mozilla-central/rev/861d28fc5194 https://hg.mozilla.org/mozilla-central/rev/1352ce4c6916 https://hg.mozilla.org/mozilla-central/rev/73d9b63d4df1

Can you nominate this for uplift to 38? It blocks the uplift of bug 1135900. Thanks!

(In reply to Liz Henry :lizzard from comment #57) > Can you nominate this for uplift to 38? It blocks the uplift of bug 1135900. > Thanks! This has extended bake requirements, and is in support of syncing, so I'll nom this when Bug 1117830 is done, landed, and tested (or in the unlikely event that another DB migration needs to be uplifted). I'd like to avoid database migration churn on 38 if we need fixes before landing Bug 1117830. I hope that bug will land late this week or early next. Leaving ni open for that.

https://hg.mozilla.org/mozilla-aurora/rev/2a568085f042 https://hg.mozilla.org/mozilla-aurora/rev/dfe8f55e047a https://hg.mozilla.org/mozilla-aurora/rev/bfe706982340