Karen Rudnitski [:kar] Reporter
	Description • 10 years ago

I created a FxA on my Nexus 5 (fennec instance). I then signed-in to my desktop for the first time to attach it to the same FxA. Upon first sync, my Fennec thumbnails were replaced by my higher-frecency desktop thumbnails.

This shouldn't happen. Of course my desktop will be more heavily used with a certain set of use cases (ie work). This shouldn't corrupt my mobile experience where I use browsing mainly for recreation / non-business stuff. 

I thought we had talked about preserving the thumbnails based on Firefox instance instead of replacing them?

I don't know who owns this area or what flag or component to use, so I have just started here hoping it will get to the right spot for discussion.

	Richard Newman [:rnewman]
	Comment 1 • 10 years ago

This is something I neglected to spin out from Bug 1003608 Comment 3, so thanks for filing, Karen!

See also Bug 934030.

The core issue here is that Top Sites can't/doesn't distinguish between history created on this device versus elsewhere. The data simply isn't in the database.

That means that Sync from a desktop will typically stomp all over your mobile habits.

We can't really fix Sync to track per-device history, but we can fix our local data, which will be fine 95% of the time.

This will involve a small schema change, some work either in BrowserProvider or in Sync to make sure the right kinds of counts are changed, and a tweak to the Top Sites algorithm to match whatever expectations we have.

	Brad Lassey [:blassey] (use needinfo?)
	Comment 2 • 10 years ago

filter on [mass-p5]

	Bill Maggs (bmaggs)
	Comment 3 • 10 years ago

Does P5 mean that in reality it will never happen? It often does in my engineering groups. Just wondering. I assume you guys don't want Top Sites on Fennec to not reflect what you really visit on your phone, quite different than desktop browsing history. I would point out I have never noticed Top Sites on my Fennec reflecting anything except my Fennec top sites.

	Aaron Train [:aaronmt]
	Comment 4 • 10 years ago

No, it's an indication that we should triage this at some point.

	Richard Newman [:rnewman]
	Comment 5 • 9 years ago

Bug 1157553 Comment 6 has some relevant thinking.

	Richard Newman [:rnewman]
	Comment 6 • 9 years ago

Work:

* Schema changes and migration in BrowserProvider.
* Logic changes to frecency. See Firefox for iOS.
* Top sites adjustments to match. Perhaps domain grouping.
* Adjusting Sync to write visits into Fennec correctly and upload local visits.
* Migrating the existing Sync visits system.
* Tests.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 7 • 9 years ago

(In reply to Richard Newman [:rnewman] from comment #6)
> * Logic changes to frecency. See Firefox for iOS.

Can you point me to the algorithm on iOS?

> * Top sites adjustments to match.

I assume this comes for free with the schema & algorithm update.

> Perhaps domain grouping.

Let's keep the scope down – there is another bug open for this somewhere.

> * Tests.

;)

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 8 • 9 years ago

Richard, where does Sync write into the history database?

	Richard Newman [:rnewman]
	Comment 9 • 9 years ago

(In reply to Michael Comella (:mcomella) from comment #8)
> Richard, where does Sync write into the history database?

mobile/android/base/sync/repositories/android/AndroidBrowserHistoryRepositorySession.java


(In reply to Michael Comella (:mcomella) from comment #7)

> Can you point me to the algorithm on iOS?

https://github.com/mozilla/firefox-ios/blob/master/Storage/SQL/SQLiteHistory.swift#L360

The main differences here versus Android:

* Visits are stored in a separate table and joined into history.
* We calculate frecency by looking separately at your local and remote visits, dramatically upweighting the local ones.
* We group results by domain, so it's more complicated.

We plan to revisit this in the next few months to pre-calculate part or all of the frecency score and introduce some more concepts from desktop, such as using the visit types to contribute differing amounts to the frecency calculation:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/Frecency_algorithm


Something else you'll care about is how we track duplicate visits:

https://github.com/mozilla/firefox-ios/blob/master/Storage/SQL/SQLiteHistory.swift#L595

The Sync record format doesn't uniquely identify visits, nor can it express deletion, so we have to be careful to make visits collide when possible.

	Richard Newman [:rnewman]
	Comment 10 • 9 years ago

I figured it was worth outlining some more concrete steps here.

1. (a) Define a visits table in the main browser DB. Begin tracking local visits with a visit type and a timestamp using this table. You'll need to touch recording visits, frecency queries, clearing of history, and just about everything else in LocalBrowserDB.

Note that you won't need to track changes or deletion per-visit: Sync always uploads all of the visits for a history record when it changes. You will need to mark the history item as modified when you add a visit.

At this point browsing should work but Sync won't -- we haven't yet addressed how to merge incoming visits.

You can land this in a branch, or keep rebasing the patch. (I don't recommend landing this towards the end of a cycle, because you will need to do performance work.)


1.5. Visit count -> visit. You'll need to _synthesize_ visits from Fennec's existing visit counts as part of migration. Sync has some code to do this as part of the extender. If Sync isn't set up you can mark all of these visits as local; otherwise, you have little choice but to guess.


2. Migrate visits from Sync into the visits table. This is what we were discussing tonight. We already have JSON blobs in the extension DB for users who have a Sync account and are syncing history; we'll want to parse these out and insert them all as remote visits.

Once this is done, again, Sync won't work, but we'll be a step closer. You can drop the history extension DB at this point.


3. Sync. Now we need to shift the visit insertion/merging logic out of Sync's extender into Fennec. The iOS codebase has a lot of this logic built into the SQL layer (INSERT OR IGNORE); I suggest you use it. You'll reach a point where you can insert a history record and all of its visits and have the right thing happen, with local visits remaining local and new remote visits being inserted.

Once this is done, Sync will be working again.


4. Performance. At this point you might find that top sites calculations are too slow with large volumes of data (18,000 history items and 36,000 visits, as my iOS profile has). We can't regress too much, so you'll have a choice:

(a) Keep the existing frecency algorithm, and materialize the visit counts that it needs. This gets us parity with the existing implementation but with individual visits, with a small UPDATE cost on visit insertion. We can generalize to local versus remote later.

(b) Extend the existing frecency algorithm to match desktop, complete with materialized scores. This would put Android a step ahead of iOS, and buys us both accuracy and performance, but it's more work.

(c) Cache top sites, just as we do on iOS: Bug 1211984.


5. Top Sites. Once we have the data we need, you can start to improve the top sites query to use local-aware frecency. You laid the groundwork for this in (1) and (4), and you can use iOS as some inspiration.

Note that desktop and iOS both coalesce history items into domain tiles (Bug 1184582). Doing so is *another* extension, so I suggest punting this to a follow-up bug if you choose to take this step.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 11 • 9 years ago

Bug 1046709 - (WIP) Schema changes.

This is just to check we all agree on the schema - the migration and removal of
the old fields still needs to happen.

The schema closely mirrors iOS':
  History: https://github.com/mozilla/firefox-ios/blob/e077523cc0581c0b3939350243158a03a721e8a3/Storage/SQL/BrowserTable.swift#L141
  Visits: https://github.com/mozilla/firefox-ios/blob/e077523cc0581c0b3939350243158a03a721e8a3/Storage/SQL/BrowserTable.swift#L266

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 12 • 9 years ago

Comment on attachment 8686925 [details]
MozReview Request: Bug 1046709 - (WIP) Schema changes.

Richard, is this your expectation for the new schema?

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 13 • 9 years ago

Comment on attachment 8686925 [details]
MozReview Request: Bug 1046709 - (WIP) Schema changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25105/diff/1-2/

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 14 • 9 years ago

Comment on attachment 8686925 [details]
MozReview Request: Bug 1046709 - (WIP) Schema changes.

Revert to the date_modified from iOS' separate local/server_modified, as discussed on irc.

	Richard Newman [:rnewman]
	Comment 15 • 9 years ago

Sorry for not getting to this yet, Michael; tomorrow, I hope!

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 16 • 9 years ago

Comment on attachment 8686925 [details]
MozReview Request: Bug 1046709 - (WIP) Schema changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25105/diff/2-3/

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 17 • 9 years ago

Bug 1046709 - (WIP) Separate visits from history.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 18 • 9 years ago

Comment on attachment 8686925 [details]
MozReview Request: Bug 1046709 - (WIP) Schema changes.

>https://reviewboard.mozilla.org/r/25105/

	:Margaret Leibovic
	Comment 19 • 8 years ago

Maybe you can talk to rnewman about this next week when we're all together in SF. I think it would be good to make forward progress on this so that we can address the outstanding issue of desktop sites overwhelming your top sites after you set up sync on a new profile.

	:Grisha Kruglov Assignee
	Comment 20 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/40953/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40953/

	:Grisha Kruglov Assignee
	Comment 21 • 8 years ago

piggy backs onto what history tracking is doing

Review commit: https://reviewboard.mozilla.org/r/40955/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40955/

	:Grisha Kruglov Assignee
	Comment 22 • 8 years ago

Comment on attachment 8731955 [details]
MozReview Request: Bug 1046709 - Part 1: schema migration, data migration, test db r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40953/diff/1-2/

	:Grisha Kruglov Assignee
	Comment 23 • 8 years ago

Comment on attachment 8731956 [details]
MozReview Request: Bug 1046709 - Part 2: CRUD for Visits - query/insert/delete; tests. r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40955/diff/1-2/

	:Grisha Kruglov Assignee
	Comment 24 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/41177/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41177/

	:Grisha Kruglov Assignee
	Comment 25 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/41179/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41179/

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 26 • 8 years ago

Comment on attachment 8686925 [details]
MozReview Request: Bug 1046709 - (WIP) Schema changes.

>https://reviewboard.mozilla.org/r/25105/

	:Grisha Kruglov Assignee
	Comment 27 • 8 years ago

Comment on attachment 8732442 [details]
MozReview Request: Bug 1046709 - Part 3: Synthesize visits when importing history from Android r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41179/diff/1-2/

	:Grisha Kruglov Assignee
	Comment 28 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/41557/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41557/

	:Grisha Kruglov Assignee
	Comment 29 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/42617/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42617/

	:Grisha Kruglov Assignee
	Comment 30 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/42619/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42619/

	:Grisha Kruglov Assignee
	Comment 31 • 8 years ago

Comment on attachment 8731955 [details]
MozReview Request: Bug 1046709 - Part 1: schema migration, data migration, test db r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40953/diff/2-3/

	:Grisha Kruglov Assignee
	Comment 32 • 8 years ago

Comment on attachment 8731956 [details]
MozReview Request: Bug 1046709 - Part 2: CRUD for Visits - query/insert/delete; tests. r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40955/diff/2-3/

	:Grisha Kruglov Assignee
	Comment 33 • 8 years ago

Comment on attachment 8732441 [details]
MozReview Request: Bug 1046709 - Part 3: insert/remove visit record when user visits a page or removes history item(s) r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41177/diff/1-2/

	:Grisha Kruglov Assignee
	Comment 34 • 8 years ago

Comment on attachment 8732442 [details]
MozReview Request: Bug 1046709 - Part 3: Synthesize visits when importing history from Android r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41179/diff/2-3/

	:Grisha Kruglov Assignee
	Comment 35 • 8 years ago

Comment on attachment 8733108 [details]
MozReview Request: Bug 1046709 - Part 4: Sync changes r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41557/diff/1-2/

	:Grisha Kruglov Assignee
	Comment 36 • 8 years ago

My apologies for the mess that is the commit history here, but the squashed diff isn't too bad to look at. The work so far covers local visit tracking and sync.
As far as I can tell, other than potential cleanup and whatever might arise from feedback, work that is still left is: better testing of this stuff (right now it's kind of a lip service...), and the top sites calculations.

	:Grisha Kruglov Assignee
	Comment 37 • 8 years ago

Another thing is history deletions. Due to "delete cascaded" visits will be removed, but not right away - iirc, history records are first "cleaned up" (leaving only guid intact, as setting the deleted flag), and are only actually deleted in batches once they've been marked as deleted for about 20 days. I imagine this time buffer exists for sync's sake?

Leanest solution would be to manually remove visit records (by guid) for now.

	Richard Newman [:rnewman]
	Comment 38 • 8 years ago

(In reply to :Grisha Kruglov from comment #37)
> Another thing is history deletions. Due to "delete cascaded" visits will be
> removed, but not right away - iirc, history records are first "cleaned up"
> (leaving only guid intact, as setting the deleted flag), and are only
> actually deleted in batches once they've been marked as deleted for about 20
> days. I imagine this time buffer exists for sync's sake?

Some background:

Sync's current history object format[0] doesn't have a way to model deleted history visits.

That is: once a visit has escaped into the wild (synced to the server and down to another device) it can only be removed from other clients by deleting the history for a URL in its entirety by uploading a deleted=true record.

That's why we don't track deletion of individual visits, even though some Firefoxes support fine-grained clearing of history (desktop's "forget" feature). We track deletion of history items as a whole.

We do need to be able to sync deleted history items, and so that's why we keep the sentinels in the database for a while. Sync will clean them up after they're uploaded, or they'll expire if you don't have Sync configured. (It's easier than finding out if you're using Sync.)


> Leanest solution would be to manually remove visit records (by guid) for now.

Yes. There might be _a lot_ of visits in the database after a deletion: consider the case of Clear History. When we mark a history item as deleted, we should delete all of its visits. When we mark all history items as deleted, we should drop all of the related visits.


[0] <http://docs.services.mozilla.com/sync/objectformats.html>

	:Grisha Kruglov Assignee
	Comment 39 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/42677/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42677/

	Nick Alexander :nalexander [he/him]
	Comment 40 • 8 years ago

https://reviewboard.mozilla.org/r/40951/#review39581

Pass 1 down.  The significant points:

1) reworking patch series to read linearly.

2) lifting visit insertion to the domain layer, so that the SQL layer is "pure".

3) ensuring that History.VISITS is always COUNT(Visits.GUID, ...) as appropriate.  (No need to use the count in this patch series.)

4) Moving tests to unit tests, for simplicity and speed.

Great patch series, Grisha!

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:158
(Diff revision 5)
> +                Visits._ID + " INTEGER PRIMARY KEY AUTOINCREMENT," +
> +                Visits.HISTORY_GUID + " TEXT NOT NULL," +
> +                Visits.VISIT_TYPE + " INTEGER NOT NULL DEFAULT 1," +
> +                Visits.DATE_VISITED + " INTEGER NOT NULL, " +
> +                Visits.IS_LOCAL + " INTEGER NOT NULL DEFAULT 1, " +
> +

nit: kill line.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:163
(Diff revision 5)
> +
> +                "FOREIGN KEY (" + Visits.HISTORY_GUID + ") REFERENCES " +
> +                TABLE_HISTORY + "(" + History.GUID + ") ON DELETE CASCADE" +
> +                ");");
> +
> +        // TODO index on... depends on frecency calculations?

nit: either include a bug number reference, or drop this.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:488
(Diff revision 5)
>                  oldTabsDB.close();
>              }
>          }
>      }
>  
> +    private void copyHistoryExtensionDataToVisitsTable(SQLiteDatabase historyExtensionDb, SQLiteDatabase db) {

Annotate with `@NonNull`.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:493
(Diff revision 5)
> +    private void copyHistoryExtensionDataToVisitsTable(SQLiteDatabase historyExtensionDb, SQLiteDatabase db) {
> +        String historyExtensionTable = "HistoryExtension";
> +        String columnGuid = "guid";
> +        String columnVisits = "visits";
> +
> +        try (

Sadly, try with resources is API 19+.  So, 2020.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:499
(Diff revision 5)
> +                Cursor cursor = historyExtensionDb.query(historyExtensionTable,
> +                        new String[] {columnGuid, columnVisits},
> +                        null, null, null, null, null
> +                )
> +        ) {
> +            // empty cursor, nothing to copy

nit: prefer complete sentences.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:535
(Diff revision 5)
> +
> +                    cv.put(Visits.DATE_VISITED, convertDateFromHistoryExtensionsDBToLocalFormat(
> +                            (Long) visit.get("date")));
> +                    cv.put(Visits.VISIT_TYPE, (Long) visit.get("type"));
> +                    cv.put(Visits.HISTORY_GUID, guid);
> +                    cv.put(Visits.IS_LOCAL, 0);

nit: worth an explanatory comment.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:536
(Diff revision 5)
> +                    cv.put(Visits.DATE_VISITED, convertDateFromHistoryExtensionsDBToLocalFormat(
> +                            (Long) visit.get("date")));
> +                    cv.put(Visits.VISIT_TYPE, (Long) visit.get("type"));
> +                    cv.put(Visits.HISTORY_GUID, guid);
> +                    cv.put(Visits.IS_LOCAL, 0);
> +                    db.insert(Visits.TABLE_NAME, null, cv);

Bulk-insert if you can.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:539
(Diff revision 5)
> +                    cv.put(Visits.HISTORY_GUID, guid);
> +                    cv.put(Visits.IS_LOCAL, 0);
> +                    db.insert(Visits.TABLE_NAME, null, cv);
> +                }
> +
> +                // "synthesize" visits which fennec "recorded" locally,

nit: explain how this arises.  I.e, "Sync at t0, browse at t1, migrate ..." and Sync never populates the new visits.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:575
(Diff revision 5)
> +            if (!cursor.moveToFirst()) {
> +                Log.e(LOGTAG, "Expected to get history visit count with guid but failed: " + guid);
> +                return 0;
> +            }
> +
> +            knownVisits = cursor.getInt(cursor.getColumnIndex(History.VISITS));

nit: `getColumnIndexOrThrow`.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:580
(Diff revision 5)
> +            knownVisits = cursor.getInt(cursor.getColumnIndex(History.VISITS));
> +        }
> +
> +        int visitsToSynthesize = knownVisits - baseNumberOfVisits;
> +
> +        // TODO or should we throw an IllegalArgumentException here..?

Yeah, do this first :)

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:606
(Diff revision 5)
> +        return fakeVisits;
> +    }
> +
> +    private void insertSynthesizedVisits(SQLiteDatabase db, ContentValues[] visits) {
> +        debug("Inserting " + visits.length + " synthesized visits");
> +        for (ContentValues visit: visits) {

nit: bulk insert.

House style is "visit : visits".

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1414
(Diff revision 5)
>  
>                  case 30:
>                      upgradeDatabaseFrom29to30(db);
>                      break;
> +
> +                case 31:

TODO: add a (small) v30 with no history visits from Sync, and ensure it updates.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:448
(Diff revision 5)
>                  deleteUnusedImages(uri);
>                  break;
>              }
>  
> +            case VISITS:
> +                trace("Deleting visits: " + uri);

Specifically test removing a History GUID, and observing that the record is marked DELETED and the visits are purged.

Verify that visiting a History GUID that is DELETED marks the GUID not deleted (?) and adds a new visit.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:534
(Diff revision 5)
>                  break;
>              }
>  
> +            case VISITS: {
> +                trace("Insert on VISITS: " + uri);
> +                id = insertVisit(uri, values);

Assert that inserting a visit with an unknown GUID fails.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:641
(Diff revision 5)
>              case HISTORY: {
>                  debug("Updating history: " + uri);
>                  if (shouldUpdateOrInsert(uri)) {
>                      updated = updateOrInsertHistory(uri, values, selection, selectionArgs);
> +
> +                    debug("Recording a visit: " + uri);

Better to lift visit insertion to the domain layer, not the SQL layer.  I'd like to write SQL unit tests that would replace/update without inserting additional records.

A little investigation revealed `mUpdateHistoryUriWithProfile`, which is exactly when we want to insert a new Visit record.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:643
(Diff revision 5)
>                  if (shouldUpdateOrInsert(uri)) {
>                      updated = updateOrInsertHistory(uri, values, selection, selectionArgs);
> +
> +                    debug("Recording a visit: " + uri);
> +                    updated += insertVisit(uri, values);
> +

nit: no trailing newlines, please.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1365
(Diff revision 5)
>          final UpdateOperation[] ops = { UpdateOperation.ASSIGN, UpdateOperation.EXPRESSION };
>  
>          return DBUtils.updateArrays(db, TABLE_HISTORY, valuesAndVisits, ops, selection, selectionArgs);
>      }
>  
> +    private long insertVisit(Uri uri, ContentValues values) {

grisha and I discussed this; pursuant to the comment about SQL level vs. domain level, we expect this to be simpler because there will only be inserting a visit for a known history item.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/receivers/FxAccountDeletedService.java
(Diff revision 5)
>        }
>      } catch (Exception e) {
>        Logger.warn(LOG_TAG, "Got exception deleting the Firefox Sync clients database; ignoring.", e);
>      }
>  
> -    // Clear Firefox Sync history data table.

Let's keep this.  We'd like to get to a world with a complete client set attached to all visits; in this world, it doesn't make sense to attach unknown clients to history visits.  Let's wipe them if they're not local.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserHistoryDataAccessor.java:50
(Diff revision 5)
> -            .get(AndroidBrowserHistoryRepositorySession.KEY_DATE);
>          if (visitDate > mostRecent) {
>            mostRecent = visitDate;
>          }
>        }
> -      // Fennec stores milliseconds. The rest of Sync works in microseconds.
> +      cv.put(BrowserContract.History.DATE_LAST_VISITED, VisitsHelper.dateFromSyncToFennec(mostRecent));

nit: keep this note.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserHistoryDataAccessor.java:80
(Diff revision 5)
> -    dataExtender.delete(oldGUID);
> -    dataExtender.store(newGUID, rec.visits);
> -    super.update(oldGUID, newRecord);
> -  }
>  
> -  @Override
> +    VisitsHelper.insertNewVisitsFromRecord(context.getContentResolver(), rec);

There's a test that needs to be written here to ensure that content reconciled history records get the right (the union of the?) history visits.

That is, if {guid1, url} and {guid2, url} are both seen, we want to write {guid2, url, union of visits}.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserHistoryDataAccessor.java
(Diff revision 5)
> -    dataExtender.store(newGUID, rec.visits);
> -    super.update(oldGUID, newRecord);
> -  }
>  
> -  @Override
> +    VisitsHelper.insertNewVisitsFromRecord(context.getContentResolver(), rec);
> -  public int purgeGuid(String guid) {

Consider a Post: commit, inlining `purgeGuid` entirely.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserHistoryDataAccessor.java:114
(Diff revision 5)
>        cvs[index] = getContentValues(record);
>        index += 1;
>      }
>  
>      // First update the history records.
>      int inserted = context.getContentResolver().bulkInsert(getUri(), cvs);

These records will all be inserted with VISITS = 1.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserHistoryDataAccessor.java:124
(Diff revision 5)
>                     inserted + " records but expected " +
>                     size     + " records; continuing to update visits.");
>      }
> -    // Then update the history visits.
> -    dataExtender.bulkInsert(records);
> +
> +    for (Record record : records) {
> +      VisitsHelper.insertNewVisitsFromRecord(context.getContentResolver(), (HistoryRecord) record);

We need to make sure here that VISITS is correct.

This might look like another patch to replace History.VISITS with a count from the Visits table.  This leads naturally into splitting the awesomebar search into local and remote parts :)

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/VisitsHelper.java:1
(Diff revision 5)
> +package org.mozilla.gecko.sync.repositories.android;

nit: license header.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/VisitsHelper.java:15
(Diff revision 5)
> +import org.mozilla.gecko.db.BrowserContract;
> +import org.mozilla.gecko.sync.repositories.domain.HistoryRecord;
> +
> +import java.util.ArrayList;
> +
> +public class VisitsHelper {

nit: always class comment, explaining how this fits into the ecosystem.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/VisitsHelper.java:28
(Diff revision 5)
> +     * @param cr <code>ContentResolver</code> used to <code>bulkInsert</code> visits
> +     * @param guid History GUID to use when inserting visit records
> +     * @param visits <code>JSONArray</code> list of (date, type) tuples for visits
> +     * @return number of visits inserted
> +     */
> +    public static int bulkInsert(@NonNull ContentResolver cr, @NonNull String guid, @NonNull JSONArray visits) {

In general, better to pass `Cursor` instances than `ContentResolver` instances.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/VisitsHelper.java:37
(Diff revision 5)
> +        }
> +        return cr.bulkInsert(BrowserContractHelpers.VISITS_CONTENT_URI, visitsToStore);
> +    }
> +
> +    /**
> +     * Maps all visits for a given history GUID to an array of JSONObjects with "date" and "type" keys

nit: full sentence.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/VisitsHelper.java:53
(Diff revision 5)
> +                        BrowserContractHelpers.VISITS_CONTENT_URI,
> +                        new String[]{BrowserContract.Visits.VISIT_TYPE, BrowserContract.Visits.DATE_VISITED},
> +                        BrowserContract.Visits.HISTORY_GUID + " = ?",
> +                        new String[]{guid}, null)
> +        ) {
> +            if (cursor != null) {

Prefer early return through-out.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/VisitsHelper.java:77
(Diff revision 5)
> +     *
> +     * @param cr <code>ContentResolver</code> which will be used to <code>query</code> and <code>bulkInsert</code>
> +     * @param record <code>HistoryRecord</code> from which to insert items.
> +     * @return number of records inserted
> +     */
> +    public static int insertNewVisitsFromRecord(@NonNull ContentResolver cr, @NonNull HistoryRecord record) {

It's worth noting in the docstring that we're merging sets, and that we can't make any assumptions about incoming visit timestamps being monotonically increasing, or any assumptionsa bout incoming visit timestamps being larger (or smaller) than locally collected visits.  (Sync devices can come and go with visit timestamps from different "eras".)

Let's try to `bulkInsert` here, ignoring conflicts.  This might require making `(GUID, DATE)` unique.

Well worth unit testing this method in isolation to ensure we ignore, add, etc.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/VisitsHelper.java:109
(Diff revision 5)
> +            }
> +        }
> +
> +        // build up a list of all visits that we need to insert
> +        ArrayList<ContentValues> visitsToInsert = new ArrayList<>();
> +        JSONObject visit;

nit: prefer to move references closer to definition.  There's no savings to declaring this and reusing it.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/VisitsHelper.java:158
(Diff revision 5)
> +        }
> +        return date / 1000;
> +    }
> +
> +    /**
> +     *

nit: description.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/VisitsHelper.java:178
(Diff revision 5)
> +        visit.put(SYNC_DATE_KEY, date);
> +        visits.add(visit);
> +    }
> +
> +    @SuppressWarnings("unchecked")
> +    private static JSONObject normalizeVisit(JSONObject visit) {

nit: `ensureVisitDateIs ...`.

::: mobile/android/tests/background/junit3/src/org/mozilla/gecko/sync/repositories/android/VisitsHelperTest.java:1
(Diff revision 5)
> +package org.mozilla.gecko.sync.repositories.android;

nit: throughout, license headers.  Tests have CC, code is MPL.

::: mobile/android/tests/background/junit3/src/org/mozilla/gecko/sync/repositories/android/VisitsHelperTest.java:9
(Diff revision 5)
> +import android.test.AndroidTestCase;
> +
> +import org.json.simple.JSONObject;
> +import org.mozilla.gecko.db.BrowserContract;
> +
> +public class VisitsHelperTest extends AndroidTestCase {

Make this a unit test, following the model of https://bugzilla.mozilla.org/show_bug.cgi?id=1260478.

	:Grisha Kruglov Assignee
	Comment 41 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/43899/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43899/

	:Grisha Kruglov Assignee
	Comment 42 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/43901/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43901/

	:Grisha Kruglov Assignee
	Comment 43 • 8 years ago

Comment on attachment 8731955 [details]
MozReview Request: Bug 1046709 - Part 1: schema migration, data migration, test db r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40953/diff/3-4/

	:Grisha Kruglov Assignee
	Comment 44 • 8 years ago

Comment on attachment 8731956 [details]
MozReview Request: Bug 1046709 - Part 2: CRUD for Visits - query/insert/delete; tests. r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40955/diff/3-4/

	:Grisha Kruglov Assignee
	Comment 45 • 8 years ago

Comment on attachment 8732441 [details]
MozReview Request: Bug 1046709 - Part 3: insert/remove visit record when user visits a page or removes history item(s) r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41177/diff/2-3/

	:Grisha Kruglov Assignee
	Comment 46 • 8 years ago

Comment on attachment 8732442 [details]
MozReview Request: Bug 1046709 - Part 3: Synthesize visits when importing history from Android r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41179/diff/3-4/

	:Grisha Kruglov Assignee
	Comment 47 • 8 years ago

Comment on attachment 8733108 [details]
MozReview Request: Bug 1046709 - Part 4: Sync changes r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41557/diff/2-3/

	:Grisha Kruglov Assignee
	Comment 48 • 8 years ago

Comment on attachment 8735168 [details]
MozReview Request: Bug 1046709 - Part 6: Remove remote visits when FxAccount is deleted r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42617/diff/1-2/

	:Grisha Kruglov Assignee
	Comment 49 • 8 years ago

Comment on attachment 8735169 [details]
MozReview Request: Bug 1046709 - Part 5: Delete history db extensions related stuff r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42619/diff/1-2/

	:Grisha Kruglov Assignee
	Comment 50 • 8 years ago

Comment on attachment 8735304 [details]
MozReview Request: Bug 1046709 - Post: remove dead code r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42677/diff/1-2/

	Nick Alexander :nalexander [he/him]
	Comment 51 • 8 years ago

Comment on attachment 8731956 [details]
MozReview Request: Bug 1046709 - Part 2: CRUD for Visits - query/insert/delete; tests. r=nalexander,rnewman

https://reviewboard.mozilla.org/r/40955/#review40489

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserHistoryDataAccessor.java:69
(Diff revision 4)
>  
>      Logger.debug(LOG_TAG, "Storing record " + record.guid);
>      Uri newRecordUri = super.insert(record);
>  
>      Logger.debug(LOG_TAG, "Storing visits for " + record.guid);
> -    VisitsHelper.bulkInsert(context.getContentResolver(), rec.guid, rec.visits);
> +    VisitsHelper.bulkInsertRemoteVisits(context.getContentResolver(), rec.guid, rec.visits, false);

I find it very surprising that you `insert` with no timestamp conversion, but you `update` with a timestamp conversion.

Why is this not symmetric?

	Nick Alexander :nalexander [he/him]
	Comment 52 • 8 years ago

Comment on attachment 8732441 [details]
MozReview Request: Bug 1046709 - Part 3: insert/remove visit record when user visits a page or removes history item(s) r=nalexander,rnewman

https://reviewboard.mozilla.org/r/41177/#review40491

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:681
(Diff revision 3)
> -                  History.URL + " = ?",
> -                  new String[] { uri });
> +                whereClause,
> +                new String[]{uri});
> +
> +        // We need to insert a visit now, but first we need to look up GUID for this history record.
> +        // Possible error scenarios here mean that we might end up with a discrepancy between History.VISITS
> +        // count and count(Visits) for a given History.GUID. Hopefully it won't happen though.

Wait, we fully expect this to happen in the future, right?  I.e., this only doesn't happen now because Sync doesn't try to sync the number of visits across devices, and each client keeps its own tally.

As we start to drop old visits, this will not be an error condition.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1494
(Diff revision 3)
>              cursor.close();
>          }
>      }
>  
> +    /**
> +     * Utility method used by AndroidImport to insert visit data for history records that were just imported.

nit: is this for one history record (== one URL)?  Or for all URLs?

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1512
(Diff revision 3)
> +        ArrayList<ContentValues> visitsToInsert = new ArrayList<>(upperBoundForNumberOfVisits);
> +
> +        // If for any reason we fail to obtain history GUID for a tuple we're processing,
> +        // let's just ignore it. It's possible that the "best-effort" history import
> +        // did not fully succeed, so we could be missing some of the records.
> +        for (ContentValues historyVisitsInformation : historyVisitsToSynthesize) {

Hmm, it's for all URLs.  This looks like you slurp the entire system browser DB into memory.  Can we do this URL-by-URL to avoid that?

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1527
(Diff revision 3)
> +                }
> +                if (!cursor.moveToFirst()) {
> +                    continue;
> +                }
> +                historyGUID = cursor.getString(
> +                        cursor.getColumnIndex(History.GUID)

This is shared across queries, is it not?  Extract this out.

::: mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java:119
(Diff revision 3)
>  
>          flushBatchOperations();
>      }
>  
>      public void mergeHistory() {
> +        ArrayList<ContentValues> visitSynthesisCache = new ArrayList<>();

nit: `synthesizedVisits`?

::: mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java:145
(Diff revision 3)
>                      byte[] data = cursor.getBlob(faviconCol);
>                      mDB.updateHistoryInBatch(mCr, mOperations, url, title, date, visits);
>                      if (data != null) {
>                          mDB.updateFaviconInBatch(mCr, mOperations, url, null, null, data);
>                      }
> +                    ContentValues visitData = new ContentValues();

Memory concerns aside, perhaps follow suit and do `mDB.insertHistoryVisitsInBatch`?

	Nick Alexander :nalexander [he/him]
	Comment 53 • 8 years ago

Comment on attachment 8732442 [details]
MozReview Request: Bug 1046709 - Part 3: Synthesize visits when importing history from Android r=nalexander,rnewman

https://reviewboard.mozilla.org/r/41179/#review40499

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:160
(Diff revision 4)
>                  Visits.VISIT_TYPE + " INTEGER NOT NULL DEFAULT 1," +
>                  Visits.DATE_VISITED + " INTEGER NOT NULL, " +
>                  Visits.IS_LOCAL + " INTEGER NOT NULL DEFAULT 1, " +
>  
>                  "FOREIGN KEY (" + Visits.HISTORY_GUID + ") REFERENCES " +
> -                TABLE_HISTORY + "(" + History.GUID + ") ON DELETE CASCADE" +
> +                TABLE_HISTORY + "(" + History.GUID + ") ON DELETE CASCADE ON UPDATE CASCADE" +

Sneaky.  I wonder how Richard feels about such things.

	Nick Alexander :nalexander [he/him]
	Comment 54 • 8 years ago

Comment on attachment 8733108 [details]
MozReview Request: Bug 1046709 - Part 4: Sync changes r=nalexander,rnewman

https://reviewboard.mozilla.org/r/41557/#review40501

Rubber stamp.

	Nick Alexander :nalexander [he/him]
	Comment 55 • 8 years ago

Comment on attachment 8735168 [details]
MozReview Request: Bug 1046709 - Part 6: Remove remote visits when FxAccount is deleted r=nalexander,rnewman

https://reviewboard.mozilla.org/r/42617/#review40503

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserHistoryRepositorySession.java:85
(Diff revision 2)
>    }
>  
>    private Record addVisitsToRecord(Record record) throws NullCursorException {
>      Logger.debug(LOG_TAG, "Adding visits for GUID " + record.guid);
>  
> -    ((HistoryRecord) record).visits = VisitsHelper.getHistoryVisitsForGUID(
> +    // Sync is an object store, so what we attach here will replace what's already present on sync's servers.

nit: "on the Sync server".

Try to differentiate between "Sync" the product, capitalized, and the action "to sync" or "syncing", not capitalized.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserHistoryRepositorySession.java:86
(Diff revision 2)
>  
>    private Record addVisitsToRecord(Record record) throws NullCursorException {
>      Logger.debug(LOG_TAG, "Adding visits for GUID " + record.guid);
>  
> -    ((HistoryRecord) record).visits = VisitsHelper.getHistoryVisitsForGUID(
> -            dbHelper.context.getContentResolver(), record.guid);
> +    // Sync is an object store, so what we attach here will replace what's already present on sync's servers.
> +    // We upload 20 most recent visits for each history record. See Bug 1164660 on why just 20.

nit: summarize why, at this time, this choice:

```
// We must upload a recent subset of visits for each history record for space reasons; we chose 20 to be conservative.  See Bug 1164660 for details.
```

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/VisitsHelper.java:61
(Diff revision 2)
>          }
>          try {
>              cursor.moveToFirst();
>              while (!cursor.isAfterLast()) {
>                  insertTupleIntoVisitsUnchecked(visits,
> -                        cursor.getInt(cursor.getColumnIndex(BrowserContract.Visits.VISIT_TYPE)),
> +                        cursor.getInt(cursor.getColumnIndex(Visits.VISIT_TYPE)),

Extract out of the loop.

	Nick Alexander :nalexander [he/him]
	Comment 56 • 8 years ago

Comment on attachment 8735169 [details]
MozReview Request: Bug 1046709 - Part 5: Delete history db extensions related stuff r=nalexander,rnewman

https://reviewboard.mozilla.org/r/42619/#review40505

Stamp!

	Nick Alexander :nalexander [he/him]
	Comment 57 • 8 years ago

Comment on attachment 8735304 [details]
MozReview Request: Bug 1046709 - Post: remove dead code r=nalexander,rnewman

https://reviewboard.mozilla.org/r/42677/#review40507

Stamp!

	Nick Alexander :nalexander [he/him]
	Comment 58 • 8 years ago

Comment on attachment 8737334 [details]
MozReview Request: Bug 1046709 - If visits are missing from sync hist record, insert a fake one r=nalexander

https://reviewboard.mozilla.org/r/43899/#review40509

OK.

	Nick Alexander :nalexander [he/him]
	Comment 59 • 8 years ago

Comment on attachment 8737335 [details]
MozReview Request: Bug 1046709 - Remove all remote visits when sync account is deleted r=nalexander

https://reviewboard.mozilla.org/r/43901/#review40511

lgtm.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/receivers/FxAccountDeletedService.java:94
(Diff revision 1)
>  
> +    // Delete visits we have received from Sync (anything marked as "remote").
> +    // Note that we do not adjust "visits" counter on history records themselves.
> +    int visitsDeleted = context.getContentResolver().delete(
> +            BrowserContract.Visits.CONTENT_URI,
> +            BrowserContract.Visits.IS_LOCAL + " = ?",

No need for a parameter here, but it's your call.

	Nick Alexander :nalexander [he/him]
	Comment 60 • 8 years ago

grisha: I added some small nits and comments.  Only one real concern, with Import from Android.  Next task, tests.  Next pass, rewrite history and have rnewman review.  Looking good!

	:Grisha Kruglov Assignee
	Comment 61 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d0b63600126

	:Grisha Kruglov Assignee
	Comment 62 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dce8ece55d98

	:Grisha Kruglov Assignee
	Comment 63 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe7ddbad0ca3

	:Grisha Kruglov Assignee
	Comment 64 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=545b302b48a9

	:Grisha Kruglov Assignee
	Comment 65 • 8 years ago

Comment on attachment 8731955 [details]
MozReview Request: Bug 1046709 - Part 1: schema migration, data migration, test db r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40953/diff/4-5/

	:Grisha Kruglov Assignee
	Comment 66 • 8 years ago

Comment on attachment 8731956 [details]
MozReview Request: Bug 1046709 - Part 2: CRUD for Visits - query/insert/delete; tests. r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40955/diff/4-5/

	:Grisha Kruglov Assignee
	Comment 67 • 8 years ago

Comment on attachment 8732441 [details]
MozReview Request: Bug 1046709 - Part 3: insert/remove visit record when user visits a page or removes history item(s) r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41177/diff/3-4/

	:Grisha Kruglov Assignee
	Comment 68 • 8 years ago

Comment on attachment 8732442 [details]
MozReview Request: Bug 1046709 - Part 3: Synthesize visits when importing history from Android r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41179/diff/4-5/

	:Grisha Kruglov Assignee
	Comment 69 • 8 years ago

Comment on attachment 8733108 [details]
MozReview Request: Bug 1046709 - Part 4: Sync changes r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41557/diff/3-4/

	:Grisha Kruglov Assignee
	Comment 70 • 8 years ago

Comment on attachment 8735168 [details]
MozReview Request: Bug 1046709 - Part 6: Remove remote visits when FxAccount is deleted r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42617/diff/2-3/

	:Grisha Kruglov Assignee
	Comment 71 • 8 years ago

Comment on attachment 8735169 [details]
MozReview Request: Bug 1046709 - Part 5: Delete history db extensions related stuff r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42619/diff/2-3/

	:Grisha Kruglov Assignee
	Comment 72 • 8 years ago

Comment on attachment 8735304 [details]
MozReview Request: Bug 1046709 - Post: remove dead code r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42677/diff/2-3/

	:Grisha Kruglov Assignee
	Comment 73 • 8 years ago

Comment on attachment 8731956 [details]
MozReview Request: Bug 1046709 - Part 2: CRUD for Visits - query/insert/delete; tests. r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40955/diff/5-6/

	:Grisha Kruglov Assignee
	Comment 74 • 8 years ago

Comment on attachment 8732441 [details]
MozReview Request: Bug 1046709 - Part 3: insert/remove visit record when user visits a page or removes history item(s) r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41177/diff/4-5/

	:Grisha Kruglov Assignee
	Comment 75 • 8 years ago

Comment on attachment 8732442 [details]
MozReview Request: Bug 1046709 - Part 3: Synthesize visits when importing history from Android r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41179/diff/5-6/

	:Grisha Kruglov Assignee
	Comment 76 • 8 years ago

Comment on attachment 8733108 [details]
MozReview Request: Bug 1046709 - Part 4: Sync changes r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41557/diff/4-5/

	:Grisha Kruglov Assignee
	Comment 77 • 8 years ago

Comment on attachment 8735168 [details]
MozReview Request: Bug 1046709 - Part 6: Remove remote visits when FxAccount is deleted r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42617/diff/3-4/

	:Grisha Kruglov Assignee
	Comment 78 • 8 years ago

Comment on attachment 8735169 [details]
MozReview Request: Bug 1046709 - Part 5: Delete history db extensions related stuff r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42619/diff/3-4/

	:Grisha Kruglov Assignee
	Comment 79 • 8 years ago

Comment on attachment 8735304 [details]
MozReview Request: Bug 1046709 - Post: remove dead code r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42677/diff/3-4/

	Richard Newman [:rnewman]
	Comment 80 • 8 years ago

Comment on attachment 8731955 [details]
MozReview Request: Bug 1046709 - Part 1: schema migration, data migration, test db r=nalexander,rnewman

https://reviewboard.mozilla.org/r/40953/#review41839

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:156
(Diff revision 5)
> +    private void createVisitsTable(SQLiteDatabase db) {
> +        debug("Creating " + TABLE_VISITS + " talbe");
> +        db.execSQL("CREATE TABLE " + TABLE_VISITS + "(" +
> +                Visits._ID + " INTEGER PRIMARY KEY AUTOINCREMENT," +
> +                Visits.HISTORY_GUID + " TEXT NOT NULL," +
> +                Visits.VISIT_TYPE + " INTEGER NOT NULL DEFAULT 1," +

`TINYINT` when you know they're going to be… well, tiny.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:491
(Diff revision 5)
> +    private void copyHistoryExtensionDataToVisitsTable(SQLiteDatabase historyExtensionDb, SQLiteDatabase db) {
> +        String historyExtensionTable = "HistoryExtension";
> +        String columnGuid = "guid";
> +        String columnVisits = "visits";
> +
> +        Cursor cursor = historyExtensionDb.query(historyExtensionTable,

`final` everywhere you can.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:507
(Diff revision 5)
> +
> +            String guid;
> +            JSONArray visitsInHistoryExtensionDB;
> +            JSONObject visit;
> +            while (!cursor.isAfterLast()) {
> +                guid = cursor.getString(cursor.getColumnIndex(columnGuid));

Extract the indices once, outside the loop, and use `getColumnIndexOrThrow`.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:515
(Diff revision 5)
> +                if (visitsInHistoryExtensionDB == null) {
> +                    continue;
> +                }
> +
> +                // We need to ensure that history table has a record with GUID in question, otherwise
> +                // we might violate foreign key constraints.

Can you just use `INSERT OR IGNORE`, or find another solution to this? There might be ten thousand rows here, and running a query for each one might make this migration take minutes.

Another solution might be to use `PRAGMA defer_foreign_keys`, insert everything, then drop any visits that don't match history before committing the transaction.

You should load-test this migration!

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:583
(Diff revision 5)
> +    }
> +
> +    private int getNumberOfVisitsToSynthesize(@NonNull SQLiteDatabase db, @NonNull String guid, int baseNumberOfVisits) {
> +        int knownVisits;
> +
> +        Cursor cursor = db.query(

`final`

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:636
(Diff revision 5)
> +        for (ContentValues visit : visits) {
> +            db.insert(Visits.TABLE_NAME, null, visit);
> +        }
> +    }
> +
> +    private Long convertDateFromHistoryExtensionsDBToLocalFormat(@NonNull Long date) {

You can't do this. You need to preserve microsecond timestamps for *upload*, and also to de-dupe on insertion. You should use microsecond timestamps for visits.

Reference: iOS does so.

https://github.com/mozilla/firefox-ios/blob/master/Providers/Profile.swift#L262

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1372
(Diff revision 5)
> +            if (cursor.getCount() == 0) {
> +                Log.d(LOGTAG, "No history records to synthesize visits for.");
> +                return;
> +            }
> +
> +            if (!cursor.moveToFirst()) {

Just use this instead of checking `getCount`. `getCount`, IIRC, is surprisingly expensive.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1382
(Diff revision 5)
> +            int guidCol = -1;
> +            int visitsCol = -1;
> +            int dateCol = -1;
> +            while (!cursor.isAfterLast()) {
> +                if (guidCol == -1) {
> +                    guidCol = cursor.getColumnIndex(History.GUID);

Just do these once outside the loop!

	Richard Newman [:rnewman]
	Comment 81 • 8 years ago

(In reply to Richard Newman [:rnewman] from comment #80)

> You can't do this. You need to preserve microsecond timestamps for *upload*,
> and also to de-dupe on insertion. You should use microsecond timestamps for
> visits.

To doubly re-emphasize this: other clients rely on us preserving data. For example, Client A uploads a visit at 1460168511206123. You flatten it to milliseconds: 1460168511206. You add a local visit: 1460168511333. You now upload a record with two visits:

1460168511206000
1460168511333000

The remote client now thinks it has *three* visits, because yours has a different timestamp:

1460168511206123
1460168511206000
1460168511333000

Oops!

	Richard Newman [:rnewman]
	Comment 82 • 8 years ago

Comment on attachment 8733108 [details]
MozReview Request: Bug 1046709 - Part 4: Sync changes r=nalexander,rnewman

https://reviewboard.mozilla.org/r/41557/#review42019

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserHistoryDataAccessor.java:89
(Diff revision 5)
> +   * @param newRecord new <code>HistoryRecord</code> to replace old one with, and insert visits from
> +   */
>    @Override
>    public void update(String oldGUID, Record newRecord) {
> +    // First, update existing history records with new values. This might involve changing history GUID,
> +    // and thanks to ON UPDATE CASCADE clause on Visits.HISTORY_GUID foreign key, visits will be "ported over"

Gosh, I hope we correctly enable foreign key support on all supported Android versions.

(FKs aren't on by default.)

Please make sure this is covered by a test.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserHistoryDataAccessor.java:145
(Diff revision 5)
>      }
> -    // Then update the history visits.
> -    dataExtender.bulkInsert(records);
> +
> +    for (Record record : records) {
> +      HistoryRecord rec = (HistoryRecord) record;
> +      // Default history "visits" count is 1 (ensured on insertion), so in case visits array is empty/missing,
> +      // we need to insert a fake visit to keep our counter and list of visits in sync.

I don't think this is correct.

Processing a record like this:

```
  guid: "foo",
  lastModified: 12345,
  visits: []
```

should not result in any visits being inserted into the database. It's entirely valid for a history item to exist but have no _known_ visits.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/VisitsHelper.java:44
(Diff revision 5)
> +
> +        if (visits.size() == 0) {
> +            return visitsToStore;
> +        }
> +
> +        for (int i = 0; i < visits.size(); i++) {

Lift out `visits.size()` into a `final` local at the top of this method.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/VisitsHelper.java:46
(Diff revision 5)
> +            return visitsToStore;
> +        }
> +
> +        for (int i = 0; i < visits.size(); i++) {
> +            visitsToStore[i] = getVisitContentValues(
> +                    guid, (JSONObject) visits.get(i), DEFAULT_IS_LOCAL_VALUE, convertDateToFennec);

Same comment about millis/micros conversion -- don't do it.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/VisitsHelper.java:63
(Diff revision 5)
> +     */
> +    public static JSONArray getRecentHistoryVisitsForGUID(@NonNull ContentProviderClient contentClient,
> +                                                          @NonNull String guid, int limit) throws RemoteException {
> +        JSONArray visits = new JSONArray();
> +
> +        Cursor cursor = contentClient.query(

`final`

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/VisitsHelper.java:65
(Diff revision 5)
> +                                                          @NonNull String guid, int limit) throws RemoteException {
> +        JSONArray visits = new JSONArray();
> +
> +        Cursor cursor = contentClient.query(
> +                visitsUriWithLimit(limit),
> +                new String[]{Visits.VISIT_TYPE, Visits.DATE_VISITED},

Spaces!

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/VisitsHelper.java:74
(Diff revision 5)
> +            return visits;
> +        }
> +        try {
> +            int dateVisitedCol = -1;
> +            int visitTypeCol = -1;
> +            if (cursor.moveToFirst()) {

You can move this check to the top of the `try` and early return.

	Richard Newman [:rnewman]
	Comment 83 • 8 years ago

Comment on attachment 8735168 [details]
MozReview Request: Bug 1046709 - Part 6: Remove remote visits when FxAccount is deleted r=nalexander,rnewman

https://reviewboard.mozilla.org/r/42617/#review42029

Why do this at all?

What we used to do was leave the Fennec visit count intact, but clear up the space we were taking up with the extension table.

With your change we actually lose data -- we throw away your remote history.

That's a big change, and I think it's wrong.

	Richard Newman [:rnewman]
	Comment 84 • 8 years ago

Comment on attachment 8732441 [details]
MozReview Request: Bug 1046709 - Part 3: insert/remove visit record when user visits a page or removes history item(s) r=nalexander,rnewman

https://reviewboard.mozilla.org/r/41177/#review42031

Don't run 4+ SQL queries over a ContentResolver every time the browser visits a page!

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:677
(Diff revision 5)
> -                  History.URL + " = ?",
> -                  new String[] { uri });
> +                whereClause,
> +                new String[]{uri});
> +
> +        // We need to insert a visit now, but first we need to look up GUID for this history record.
> +        String historyGUID = null;
> +        Cursor cursor = cr.query(History.CONTENT_URI, new String[]{History.GUID}, whereClause, whereArgs, null);

This is a really painful and ugly way to do this.

ContentProviders, and their over-engineered URI abstraction, exist to hide this kind of nonsense.

You're doing one roundtrip through the ContentResolver to get a GUID, then another to insert a visit, and separately we're calling `updateHistoryTitle`… which is yet another jump through hoops.

The right way to do this is for the front-end to tell the CP, at a particular contentURI: "I visited URL foobar.com/baz, with title ABC, at time 1234".

The CP is then responsible for:

* Creating a history row if one doesn't exist for that URL.
* Updating the title and bumping the modified time if a row did exist.
* Inserting a visit for the resulting GUID.
* … and anything else that we want to track when visits occur.

The caller here becomes a single stanza that doesn't need to concern itself with GUIDs.

The key to this is realizing that just because you call `cr.insert(...)` doesn't mean that a single SQL `INSERT` happens on the other end.

	:Grisha Kruglov Assignee
	Comment 85 • 8 years ago

Comment on attachment 8731955 [details]
MozReview Request: Bug 1046709 - Part 1: schema migration, data migration, test db r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40953/diff/5-6/

	:Grisha Kruglov Assignee
	Comment 86 • 8 years ago

Comment on attachment 8731956 [details]
MozReview Request: Bug 1046709 - Part 2: CRUD for Visits - query/insert/delete; tests. r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40955/diff/6-7/

	:Grisha Kruglov Assignee
	Comment 87 • 8 years ago

Comment on attachment 8732442 [details]
MozReview Request: Bug 1046709 - Part 3: Synthesize visits when importing history from Android r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41179/diff/6-7/

	:Grisha Kruglov Assignee
	Comment 88 • 8 years ago

Comment on attachment 8733108 [details]
MozReview Request: Bug 1046709 - Part 4: Sync changes r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41557/diff/5-6/

	:Grisha Kruglov Assignee
	Comment 89 • 8 years ago

Comment on attachment 8735169 [details]
MozReview Request: Bug 1046709 - Part 5: Delete history db extensions related stuff r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42619/diff/4-5/

	:Grisha Kruglov Assignee
	Comment 90 • 8 years ago

Comment on attachment 8735304 [details]
MozReview Request: Bug 1046709 - Post: remove dead code r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42677/diff/4-5/

	:Grisha Kruglov Assignee
	Comment 91 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a45a84842c28

	:Grisha Kruglov Assignee
	Comment 92 • 8 years ago

https://reviewboard.mozilla.org/r/41557/#review42019

> Gosh, I hope we correctly enable foreign key support on all supported Android versions.
> 
> (FKs aren't on by default.)
> 
> Please make sure this is covered by a test.

Wrote unit tests for cascaded updates of GUIDs and for deletions.

> I don't think this is correct.
> 
> Processing a record like this:
> 
> ```
>   guid: "foo",
>   lastModified: 12345,
>   visits: []
> ```
> 
> should not result in any visits being inserted into the database. It's entirely valid for a history item to exist but have no _known_ visits.

Removed any "faking" of visits - other than synthesis during import and migration.

> Spaces!

Will re-run checkstyle locally to catch these.

	:Grisha Kruglov Assignee
	Comment 93 • 8 years ago

https://reviewboard.mozilla.org/r/40953/#review41839

> Can you just use `INSERT OR IGNORE`, or find another solution to this? There might be ten thousand rows here, and running a query for each one might make this migration take minutes.
> 
> Another solution might be to use `PRAGMA defer_foreign_keys`, insert everything, then drop any visits that don't match history before committing the transaction.
> 
> You should load-test this migration!

Ended up going with insertWithOnConflict with CONFLICT_IGNORE.

> You can't do this. You need to preserve microsecond timestamps for *upload*, and also to de-dupe on insertion. You should use microsecond timestamps for visits.
> 
> Reference: iOS does so.
> 
> https://github.com/mozilla/firefox-ios/blob/master/Providers/Profile.swift#L262

Storing visit timestamps in microseconds now, and doing millisecond->microsecond conversion at two places: inserting visit as by-product of history update (with INCREMENT_VISITS), and when importing visits from AndroidImport.

	:Grisha Kruglov Assignee
	Comment 94 • 8 years ago

https://reviewboard.mozilla.org/r/41179/#review40499

> Sneaky.  I wonder how Richard feels about such things.

Cascading behaviour is covered by unit tests now, both for deletion and for GUID changes.

	Nick Alexander :nalexander [he/him]
	Comment 95 • 8 years ago

Comment on attachment 8731955 [details]
MozReview Request: Bug 1046709 - Part 1: schema migration, data migration, test db r=nalexander,rnewman

https://reviewboard.mozilla.org/r/40953/#review42697

Looking good!

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java:113
(Diff revision 6)
> +    @RobocopTarget
> +    public interface VisitsColumns {
> +        public static final String HISTORY_GUID = "history_guid";
> +        public static final String VISIT_TYPE = "visit_type";
> +        public static final String DATE_VISITED = "date";
> +        public static final String IS_LOCAL = "is_local";

Where are you defining what "is_local" means?  A class comment somewhere in the history provider?

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:486
(Diff revision 6)
>                  oldTabsDB.close();
>              }
>          }
>      }
>  
> +    private void copyHistoryExtensionDataToVisitsTable(SQLiteDatabase historyExtensionDb, SQLiteDatabase db) {

nit: comment that we once had a Sync-only database, and this is getting rid of it.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:531
(Diff revision 6)
> +                    db.insertWithOnConflict(Visits.TABLE_NAME, null, cv, SQLiteDatabase.CONFLICT_IGNORE);
> +                }
> +
> +                // We might have generated local visits before Sync had a chance to send them up.
> +                // Below we figure out how many visits we have locally that Sync isn't aware of, and we
> +                // "synthesize" them for insertion into Visits table. Synthesizes of a visit entails

nit: s/Synthesizes of a visit/Synthesizing a visit/

	Nick Alexander :nalexander [he/him]
	Comment 96 • 8 years ago

Comment on attachment 8731956 [details]
MozReview Request: Bug 1046709 - Part 2: CRUD for Visits - query/insert/delete; tests. r=nalexander,rnewman

https://reviewboard.mozilla.org/r/40955/#review42701

Great looking tests!

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:442
(Diff revision 7)
>                          new String[] { Long.toString(ContentUris.parseId(uri)) });
>                  // fall through
>              case HISTORY: {
>                  trace("Deleting history: " + uri);
>                  beginWrite(db);
> +                // Deletes from Sync will be actual DELETE statements, which will cascade delete relevant visits.

s/will be/are/.  Worth saying that deletes from Fennec mark records deleted?

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1384
(Diff revision 7)
> +            // Sync works in microseconds, so we store visit timestamps in microseconds as well.
> +            // History timestamps are in milliseconds.
> +            // This is the conversion point for locally generated visits.
> +            final long visitDate;
> +            if (values.containsKey(History.DATE_LAST_VISITED)) {
> +                visitDate = values.getAsLong(History.DATE_LAST_VISITED) * 1000;

Is there a reason to have the DB track microseconds?  I feel like Sync is in the wrong here; millis is more than enough, and agrees with what Fennec does locally.

Perhaps trying to avoid multiple Sync visits mapping to the same milli is an issue?

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1402
(Diff revision 7)
> +        } finally {
> +            cursor.close();
> +        }
> +
> +        if (visitValues.length == 1) {
> +            return insertVisit(uri, visitValues[0]);

We carefully ignore conflicts for one visit.  It's my belief that bulkInsert always ignores conflicts.  Can you confirm?  Is my understanding flawed?

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1674
(Diff revision 7)
> +            cursor.close();
> +        }
> +
> +        return db.delete(
> +                Visits.TABLE_NAME,
> +                DBUtils.computeSQLInClause(historyGUIDs.length, Visits.HISTORY_GUID),

This is limited, I think to 1000 parameters.  Is there a helper to do this more generally?  Can you warn and not crash in case you saw more than 1000?

::: mobile/android/tests/background/junit4/resources/robolectric.properties:3
(Diff revision 7)
>  sdk=21
>  constants=org.mozilla.gecko.BuildConfig
> +packageName=org.mozilla.gecko

Correct.  If you can push https://bugzilla.mozilla.org/show_bug.cgi?id=1260478 across the line I'd appreacite it :)

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderHistoryVisitsTest.java:47
(Diff revision 7)
> +        cursor.close();
> +
> +        ContentValues historyToInsert = new ContentValues();
> +        historyToInsert.put(History.URL, "https://www.eff.org");
> +        assertEquals(1, historyClient.update(
> +                historyTestUri.buildUpon()

nit: Consider lifting these to separate lines.  Reading this style of code is pretty hard.

	:Grisha Kruglov Assignee
	Comment 97 • 8 years ago

https://reviewboard.mozilla.org/r/40955/#review42705

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderHistoryVisitsTest.java:234
(Diff revision 7)
> +        assertEquals("testGUID3", cursor.getString(cursor.getColumnIndex(BrowserContract.Visits.HISTORY_GUID)));
> +        cursor.close();
> +    }
> +
> +    @Test
> +    public void testHistoryGUIDUpdate() throws Exception {

add comment

	Nick Alexander :nalexander [he/him]
	Comment 98 • 8 years ago

Comment on attachment 8732442 [details]
MozReview Request: Bug 1046709 - Part 3: Synthesize visits when importing history from Android r=nalexander,rnewman

https://reviewboard.mozilla.org/r/41179/#review42733

nice.

	Richard Newman [:rnewman]
	Comment 99 • 8 years ago

(In reply to Nick Alexander :nalexander from comment #96)

> Is there a reason to have the DB track microseconds?  I feel like Sync is in
> the wrong here; millis is more than enough, and agrees with what Fennec does
> locally.

Primarily: they need to round-trip, so we need to follow the finest granularity in use in the system. Otherwise we'll have visits breeding every time a URL is visited on two devices. I gave a concrete example of this to grisha directly.

Secondarily: we rely on uniqueness of visits by time. Millisecond is probably fine, but if we have microseconds -- and we do -- we should use them.


> > +        if (visitValues.length == 1) {
> > +            return insertVisit(uri, visitValues[0]);
> 
> We carefully ignore conflicts for one visit.  It's my belief that bulkInsert
> always ignores conflicts.  Can you confirm?  Is my understanding flawed?

bulkInsert itself delegates to the CP's implementation; the CP can choose what to do, and conflict behavior isn't specified. In our case indeed we should ignore conflicts.


> This is limited, I think to 1000 parameters.  Is there a helper to do this
> more generally?  Can you warn and not crash in case you saw more than 1000?

This is limited to SQLITE_MAX_VARIABLE_NUMBER, which defaults to 999. We use chunking helpers for this on iOS.

	Nick Alexander :nalexander [he/him]
	Comment 100 • 8 years ago

Comment on attachment 8733108 [details]
MozReview Request: Bug 1046709 - Part 4: Sync changes r=nalexander,rnewman

https://reviewboard.mozilla.org/r/41557/#review42737

Didn't look too closely here, but it looks okay to me.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserHistoryRepositorySession.java:87
(Diff revision 6)
>      Logger.debug(LOG_TAG, "Adding visits for GUID " + record.guid);
> -    HistoryRecord hist = (HistoryRecord) record;
> -    JSONArray visitsArray = getDataExtender().visitsForGUID(hist.guid);
> -    long missingRecords = hist.fennecVisitCount - visitsArray.size();
>  
> -    // Note that Fennec visit times are milliseconds, and we are working
> +    // Sync is an object store, so what we attach here will replace what's already present on the Sync servers.

nice.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/VisitsHelper.java:14
(Diff revision 6)
> +import android.database.Cursor;
> +import android.net.Uri;
> +import android.os.RemoteException;
> +import android.support.annotation.NonNull;
> +
> +import org.json.simple.JSONArray;

Just FYI: over time, we'd like to replace `org.json.simple` entirely.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1204559.  (This doesn't block this ticket, just a path for better services code over time.)

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/android/VisitsHelperTest.java:10
(Diff revision 6)
> +
> +import android.content.ContentProviderClient;
> +import android.content.ContentValues;
> +import android.net.Uri;
> +
> +import junit.framework.Assert;

This seems wrong.  I expect everything to be `org.junit`.

	Nick Alexander :nalexander [he/him]
	Comment 101 • 8 years ago

Comment on attachment 8735169 [details]
MozReview Request: Bug 1046709 - Part 5: Delete history db extensions related stuff r=nalexander,rnewman

https://reviewboard.mozilla.org/r/42619/#review42739

Stamp!

	Nick Alexander :nalexander [he/him]
	Comment 102 • 8 years ago

Comment on attachment 8735304 [details]
MozReview Request: Bug 1046709 - Post: remove dead code r=nalexander,rnewman

https://reviewboard.mozilla.org/r/42677/#review42741

If you want.

	:Grisha Kruglov Assignee
	Comment 103 • 8 years ago

Comment on attachment 8731955 [details]
MozReview Request: Bug 1046709 - Part 1: schema migration, data migration, test db r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40953/diff/6-7/

	:Grisha Kruglov Assignee
	Comment 104 • 8 years ago

Comment on attachment 8731956 [details]
MozReview Request: Bug 1046709 - Part 2: CRUD for Visits - query/insert/delete; tests. r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40955/diff/7-8/

	:Grisha Kruglov Assignee
	Comment 105 • 8 years ago

Comment on attachment 8732442 [details]
MozReview Request: Bug 1046709 - Part 3: Synthesize visits when importing history from Android r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41179/diff/7-8/

	:Grisha Kruglov Assignee
	Comment 106 • 8 years ago

Comment on attachment 8733108 [details]
MozReview Request: Bug 1046709 - Part 4: Sync changes r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41557/diff/6-7/

	:Grisha Kruglov Assignee
	Comment 107 • 8 years ago

Comment on attachment 8735169 [details]
MozReview Request: Bug 1046709 - Part 5: Delete history db extensions related stuff r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42619/diff/5-6/

	:Grisha Kruglov Assignee
	Comment 108 • 8 years ago

Comment on attachment 8735304 [details]
MozReview Request: Bug 1046709 - Post: remove dead code r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42677/diff/5-6/

	:Grisha Kruglov Assignee
	Comment 109 • 8 years ago

https://reviewboard.mozilla.org/r/40955/#review42701

> Is there a reason to have the DB track microseconds?  I feel like Sync is in the wrong here; millis is more than enough, and agrees with what Fennec does locally.
> 
> Perhaps trying to avoid multiple Sync visits mapping to the same milli is an issue?

Converting from microseconds->milliseconds will end up with strange things happening (it's also way more work). I'll quote Richard:

"""
To doubly re-emphasize this: other clients rely on us preserving data. For example, Client A uploads a visit at 1460168511206123. You flatten it to milliseconds: 1460168511206. You add a local visit: 1460168511333. You now upload a record with two visits:

1460168511206000
1460168511333000

The remote client now thinks it has *three* visits, because yours has a different timestamp:

1460168511206123
1460168511206000
1460168511333000

Oops!
"""

> We carefully ignore conflicts for one visit.  It's my belief that bulkInsert always ignores conflicts.  Can you confirm?  Is my understanding flawed?

bulkInsert will end up running the same code as insert, but in batch, so insertVisit in this case. Which will end up doing insertWithOnConflict and setting CONFLICT_IGNORE.

> This is limited, I think to 1000 parameters.  Is there a helper to do this more generally?  Can you warn and not crash in case you saw more than 1000?

Added code to chunk delete statements, and tests that exercise this. For example, for 1200 GUIDs, it'll run two DELETE statements.

	:Grisha Kruglov Assignee
	Comment 110 • 8 years ago

https://reviewboard.mozilla.org/r/41557/#review42737

> Just FYI: over time, we'd like to replace `org.json.simple` entirely.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1204559.  (This doesn't block this ticket, just a path for better services code over time.)

I'll do a pass over to convert things away from `org.json.simple` in a different ticket.

	:Grisha Kruglov Assignee
	Comment 111 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=affa3979a8c8

	:Grisha Kruglov Assignee
	Comment 112 • 8 years ago

Comment on attachment 8731955 [details]
MozReview Request: Bug 1046709 - Part 1: schema migration, data migration, test db r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40953/diff/7-8/

	:Grisha Kruglov Assignee
	Comment 113 • 8 years ago

Comment on attachment 8731956 [details]
MozReview Request: Bug 1046709 - Part 2: CRUD for Visits - query/insert/delete; tests. r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40955/diff/8-9/

	:Grisha Kruglov Assignee
	Comment 114 • 8 years ago

Comment on attachment 8732442 [details]
MozReview Request: Bug 1046709 - Part 3: Synthesize visits when importing history from Android r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41179/diff/8-9/

	:Grisha Kruglov Assignee
	Comment 115 • 8 years ago

Comment on attachment 8733108 [details]
MozReview Request: Bug 1046709 - Part 4: Sync changes r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41557/diff/7-8/

	:Grisha Kruglov Assignee
	Comment 116 • 8 years ago

Comment on attachment 8735169 [details]
MozReview Request: Bug 1046709 - Part 5: Delete history db extensions related stuff r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42619/diff/6-7/

	:Grisha Kruglov Assignee
	Comment 117 • 8 years ago

Comment on attachment 8735304 [details]
MozReview Request: Bug 1046709 - Post: remove dead code r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42677/diff/6-7/

	:Grisha Kruglov Assignee
	Comment 118 • 8 years ago

Comment on attachment 8731956 [details]
MozReview Request: Bug 1046709 - Part 2: CRUD for Visits - query/insert/delete; tests. r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40955/diff/9-10/

	:Grisha Kruglov Assignee
	Comment 119 • 8 years ago

Comment on attachment 8732442 [details]
MozReview Request: Bug 1046709 - Part 3: Synthesize visits when importing history from Android r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41179/diff/9-10/

	:Grisha Kruglov Assignee
	Comment 120 • 8 years ago

Comment on attachment 8733108 [details]
MozReview Request: Bug 1046709 - Part 4: Sync changes r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41557/diff/8-9/

	:Grisha Kruglov Assignee
	Comment 121 • 8 years ago

Comment on attachment 8735169 [details]
MozReview Request: Bug 1046709 - Part 5: Delete history db extensions related stuff r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42619/diff/7-8/

	:Grisha Kruglov Assignee
	Comment 122 • 8 years ago

Comment on attachment 8735304 [details]
MozReview Request: Bug 1046709 - Post: remove dead code r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42677/diff/7-8/

	:Grisha Kruglov Assignee
	Comment 123 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9e74212cce8

	:Grisha Kruglov Assignee
	Comment 124 • 8 years ago

Weights local visits much more heavily than remote visits.

Review commit: https://reviewboard.mozilla.org/r/46905/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46905/

	:Grisha Kruglov Assignee
	Comment 125 • 8 years ago

Comment on attachment 8731956 [details]
MozReview Request: Bug 1046709 - Part 2: CRUD for Visits - query/insert/delete; tests. r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40955/diff/10-11/

	:Grisha Kruglov Assignee
	Comment 126 • 8 years ago

Comment on attachment 8732442 [details]
MozReview Request: Bug 1046709 - Part 3: Synthesize visits when importing history from Android r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41179/diff/10-11/

	:Grisha Kruglov Assignee
	Comment 127 • 8 years ago

Comment on attachment 8733108 [details]
MozReview Request: Bug 1046709 - Part 4: Sync changes r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41557/diff/9-10/

	:Grisha Kruglov Assignee
	Comment 128 • 8 years ago

Comment on attachment 8735169 [details]
MozReview Request: Bug 1046709 - Part 5: Delete history db extensions related stuff r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42619/diff/8-9/

	:Grisha Kruglov Assignee
	Comment 129 • 8 years ago

Comment on attachment 8735304 [details]
MozReview Request: Bug 1046709 - Post: remove dead code r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42677/diff/8-9/

	:Grisha Kruglov Assignee
	Comment 130 • 8 years ago

Comment on attachment 8731955 [details]
MozReview Request: Bug 1046709 - Part 1: schema migration, data migration, test db r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40953/diff/8-9/

	:Grisha Kruglov Assignee
	Comment 131 • 8 years ago

Comment on attachment 8731956 [details]
MozReview Request: Bug 1046709 - Part 2: CRUD for Visits - query/insert/delete; tests. r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40955/diff/11-12/

	:Grisha Kruglov Assignee
	Comment 132 • 8 years ago

Comment on attachment 8732442 [details]
MozReview Request: Bug 1046709 - Part 3: Synthesize visits when importing history from Android r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41179/diff/11-12/

	:Grisha Kruglov Assignee
	Comment 133 • 8 years ago

Comment on attachment 8733108 [details]
MozReview Request: Bug 1046709 - Part 4: Sync changes r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41557/diff/10-11/

	:Grisha Kruglov Assignee
	Comment 134 • 8 years ago

Comment on attachment 8735169 [details]
MozReview Request: Bug 1046709 - Part 5: Delete history db extensions related stuff r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42619/diff/9-10/

	:Grisha Kruglov Assignee
	Comment 135 • 8 years ago

Comment on attachment 8735304 [details]
MozReview Request: Bug 1046709 - Post: remove dead code r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42677/diff/9-10/

	Richard Newman [:rnewman]
	Comment 136 • 8 years ago

Comment on attachment 8731955 [details]
MozReview Request: Bug 1046709 - Part 1: schema migration, data migration, test db r=nalexander,rnewman

https://reviewboard.mozilla.org/r/40953/#review44225

	Richard Newman [:rnewman]
	Comment 137 • 8 years ago

Comment on attachment 8731956 [details]
MozReview Request: Bug 1046709 - Part 2: CRUD for Visits - query/insert/delete; tests. r=nalexander,rnewman

https://reviewboard.mozilla.org/r/40955/#review44227

	Richard Newman [:rnewman]
	Comment 138 • 8 years ago

Comment on attachment 8732442 [details]
MozReview Request: Bug 1046709 - Part 3: Synthesize visits when importing history from Android r=nalexander,rnewman

https://reviewboard.mozilla.org/r/41179/#review44229

Rubberstamp.

	Richard Newman [:rnewman]
	Comment 139 • 8 years ago

Comment on attachment 8733108 [details]
MozReview Request: Bug 1046709 - Part 4: Sync changes r=nalexander,rnewman

https://reviewboard.mozilla.org/r/41557/#review44231

Rubberstamp.

	Richard Newman [:rnewman]
	Comment 140 • 8 years ago

Comment on attachment 8735169 [details]
MozReview Request: Bug 1046709 - Part 5: Delete history db extensions related stuff r=nalexander,rnewman

https://reviewboard.mozilla.org/r/42619/#review44233

	:Grisha Kruglov Assignee
	Comment 141 • 8 years ago

Comment on attachment 8731955 [details]
MozReview Request: Bug 1046709 - Part 1: schema migration, data migration, test db r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40953/diff/9-10/

	:Grisha Kruglov Assignee
	Comment 142 • 8 years ago

Comment on attachment 8731956 [details]
MozReview Request: Bug 1046709 - Part 2: CRUD for Visits - query/insert/delete; tests. r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40955/diff/12-13/

	:Grisha Kruglov Assignee
	Comment 143 • 8 years ago

Comment on attachment 8732442 [details]
MozReview Request: Bug 1046709 - Part 3: Synthesize visits when importing history from Android r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41179/diff/12-13/

	:Grisha Kruglov Assignee
	Comment 144 • 8 years ago

Comment on attachment 8733108 [details]
MozReview Request: Bug 1046709 - Part 4: Sync changes r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41557/diff/11-12/

	:Grisha Kruglov Assignee
	Comment 145 • 8 years ago

Comment on attachment 8735169 [details]
MozReview Request: Bug 1046709 - Part 5: Delete history db extensions related stuff r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42619/diff/10-11/

	:Grisha Kruglov Assignee
	Comment 146 • 8 years ago

Comment on attachment 8735304 [details]
MozReview Request: Bug 1046709 - Post: remove dead code r=nalexander,rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42677/diff/10-11/

	Pulsebot
	Comment 147 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/c3262228d62f
https://hg.mozilla.org/integration/fx-team/rev/851f85403d6e
https://hg.mozilla.org/integration/fx-team/rev/cf418a927ed8
https://hg.mozilla.org/integration/fx-team/rev/661901c98bea
https://hg.mozilla.org/integration/fx-team/rev/4639a73a29d3
https://hg.mozilla.org/integration/fx-team/rev/1daa36fd1395

	Carsten Book [:Tomcat]
	Comment 148 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/c3262228d62f
https://hg.mozilla.org/mozilla-central/rev/851f85403d6e
https://hg.mozilla.org/mozilla-central/rev/cf418a927ed8
https://hg.mozilla.org/mozilla-central/rev/661901c98bea
https://hg.mozilla.org/mozilla-central/rev/4639a73a29d3
https://hg.mozilla.org/mozilla-central/rev/1daa36fd1395

	u549602
	Comment 149 • 8 years ago

This issue was verified as fixed on the latest Aurora build (48.0a2  2016-04-28) with CLEAN profiles (both desktop and mobile) and a newly created sync account. 

By using an old sync account and an intense used desktop profile, the top sites on mobile were switched by the desktop ones. This issue was tried several times with the old desktop account that was synced over a fresh mobile account with the same result, top-sites form desktop replacing mobile remote visits.
 The same behavior can be observed on the latest Nightly build (49.0a1 2016-04-28)

Grisha, Margaret,Richard is this the desired behavior or should we call for a clearer separation approach when using old accounts?

	:Grisha Kruglov Assignee
	Comment 150 • 8 years ago

This bug tracks the data layer work: recording and syncing local visits properly in a way that will allow for a better top sites experience. There is a Bug 1265525 to track changes to queries powering Top Sites (which will produce user-visible changes), and a meta Bug 1265516 to track this effort in its entirety.

It's expected that there aren't any user-noticeable changes resulting from this Bug landing - it's a foundation piece.

High level overview of things to be on a lookout while testing these changes:
1) Data migrations run correctly when first upgrading for multiple scenarios:
- sync was never set up, no browsing history. no-op
- sync was never set up, some browsing history. visits synthesized from history as local
- sync was setup then disabled. visits are synthesized from history as remote
- sync is enabled. visits are migrated from the old sync history database, ensuring we don't port over visits for non-existent history records ("orphan visits")
1.2) Migrating very old Sync accounts with tons of visits. Should perform "well enough".
2) Browsing websites records visits correctly (as local)
3) Sync works correctly with locally recorded visits; incoming history data is correctly merged in, marked as remote
4) Android Import functionality works (history import from Android browser on pre-6.0 devices)
5) Clear history clears out recorded visits correctly
6) History expiration deletes visits
7) Old HistoryExtensionsDB file is removed after the migration
8) Disabling Sync shouldn't affect visits table

To test these access to browser.db and HistoryExtensionsDB is needed, in order to run SQL queries to ensure data is in correct state.

	Ioana Chiorean
	Comment 151 • 8 years ago

Ninu's QA work can be tracked here - https://wiki.mozilla.org/QA/Fennec/Prioritizatization_of_mobile_browsing_history_over_synced_desktop_history