1157553 - Sync history

Status: RESOLVED FIXED

(Firefox for iOS :: Sync, defect)

Comment 1

[Andre Klapper]

Hi Richard, 
unfortunately this report is not very useful because it does not describe the problem well. If you have time and can still reproduce the problem, please read https://developer.mozilla.org/en-US/docs/Mozilla/QA/Bug_writing_guidelines and add a more useful description to this report.

Comment 2

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

Andre - While true, comment 0 is not full of helpful descriptive text, I think this bug is a placeholder for Richard. It's likely that no one else but Richard will be taking this bug.

Comment 3

[Richard Newman [:rnewman]]

Pre: 3456084

Comment 4

[Richard Newman [:rnewman]]

https://github.com/mozilla/firefox-ios/commit/26bb5088ba94667753d9f0ebbe3d9127e1149b0f

Comment 5

[Richard Newman [:rnewman]]

(Sorry, that's "pre" stuff.)

Comment 6

[Richard Newman [:rnewman]]

Time to capture some notes about visits.

The Sync history record format[1] is "open-world":

1. the set of items is not comprehensive -- you might have visited URLs that are not on the server. That's true for two reasons: clients upload no more than 5,000 history items (my profile has over 100,000), and no more than 30 days' worth; and the server discards history items after 60 days[2].

2. the set of visits for each item is not comprehensive -- you might have visited a URL more times than the history record indicates. That's because desktop includes no more than 10 visits in a payload[3].


Changing these two assumptions would be impossible:

1. 100,000 history records is prohibitive to store or download for each user.

2. the max size of a Sync record isn't enough to store 4,000+ visits (apparently I visit Twitter and Facebook a lot).


As such, my neat formalism -- throw away remote visits and replace them every time we download a changed record -- can't be used.

Furthermore, we're not able to delete individual visits without extending the record format to include a notion of a deleted visit. That's possible, but not something to address right now.


As a result, we always need to do additive merging, and we need to carefully consider how deletion works -- it's not possible for us to tell other clients to throw away all visits but _these_, only to throw away all visits for a URL, and only if they've already synced and have the right GUID!


*shakes fist at Sync*


[1] <http://docs.services.mozilla.com/sync/objectformats.html#id4>
[2] <http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/engines/history.js#12>
[3] Bug 1164660.

Comment 7

[Richard Newman [:rnewman]]

Attachment: Pull req.

Time to get started on this.

One thing still needs to be done, which I'm about to file: we currently record all visits with type = 0 (unknown), which means we won't sync them.

N.B., the patch order makes much more sense when not viewed on GitHub.

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8607244 [details] [review]
Pull req.

Comments on GH up to part 5.  tl;dr: this looks pretty great.  I'm a big fan of commented SQL over abstraction.  Perhaps I delude myself.

This needs a moderate dose of test writing, which I will consider doing myself in exchange for money.

I think bnicholson or wesj should be involved in the visit tracking review.

Comment 9

[Richard Newman [:rnewman]]

Fortunately Wes reviewed the visit tracking stuff yesterday :D

Swipe-to-remove is cargo-culted from bookmarks, so doesn't really need review.

Comment 10

[Richard Newman [:rnewman]]

Still to do before I land this:

* Unhook the tabs trigger.

Still to do before I enable this:

* Make sure uploading of deletions occur. I think it doesn't.
* More tests and testing.
* Implement a very basic scheduler to run syncs.

Comment 11

[Richard Newman [:rnewman]]

Merged: 78668bd

Will file a bunch of follow-ups.