964447 - Create specific update window for HomeProvider storage

Status: RESOLVED FIXED

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

Description

[:Margaret Leibovic]

We want to create a notification to let add-ons know when they should sync their data, and then only allow updating during a window of time after that (or log warning messages if an add-on is updating at the wrong time).

Comment 1

[:Margaret Leibovic]

Attachment: Add sync notification APIs to HomeProvider

I decided that it would be best to use the update timer manager after all, and just register one timer, rather than one per dataset, to avoid potentially ending up with a bunch of timers.

This API still allows add-ons to register arbitrary update intervals, but we won't ever sync more frequently than once per hour.

And I should add a testcase for this to testHomeProvider, but I'll need to do some hack to make the update happen more frequently (perhaps factor out SYNC_INTERVAL_SECS into a pref).

Comment 2

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

Comment on attachment 8366467 [details] [diff] [review]
Add sync notification APIs to HomeProvider

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

::: mobile/android/app/mobile.js
@@ +827,5 @@
>  pref("browser.webapps.apkFactoryUrl", "http://dapk.net/application.apk");
>  #endif
> +
> +pref("home.sync.wifi-only", true);
> +

nit: extra newline at EOF.

::: mobile/android/modules/HomeProvider.jsm
@@ +24,2 @@
>  
> +const SYNC_INTERVAL_SECS = 3600;

nit: SYNC_CHECK_INTERVAL_SECS

Syncing does not necessary happen when the timer fires.

@@ +24,5 @@
>  
> +const SYNC_INTERVAL_SECS = 3600;
> +
> +const PREF_STORAGE_LAST_SYNC_TIME = "home.storage.lastSyncTime.%ID%";
> +const PREF_SYNC_WIFI_ONLY = "home.sync.wifi-only";

We should probably prompt the user on this one - file a followup?

@@ +49,5 @@
>    deleteFromDataset:
>      "DELETE FROM items WHERE dataset_id = :dataset_id"
>  }
>  
> +function usingLAN() {

nit: isUsingLAN()

@@ +51,5 @@
>  }
>  
> +function usingLAN() {
> +  let network = Cc["@mozilla.org/network/network-link-service;1"].getService(Ci.nsINetworkLinkService);
> +  return (network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_WIFI || network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_ETHERNET);

nit: === (is that necessary for Gecko code?)

@@ +54,5 @@
> +  let network = Cc["@mozilla.org/network/network-link-service;1"].getService(Ci.nsINetworkLinkService);
> +  return (network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_WIFI || network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_ETHERNET);
> +}
> +
> +function nowInSeconds() {

nit: getNowInSeconds()

@@ +55,5 @@
> +  return (network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_WIFI || network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_ETHERNET);
> +}
> +
> +function nowInSeconds() {
> +  return Math.round(Date.now()/1000);

nit: ...(Date.now() / 1000);

@@ +58,5 @@
> +function nowInSeconds() {
> +  return Math.round(Date.now()/1000);
> +}
> +
> +function getLastSyncPref(datasetId) {

nit (that I feel less strongly about): getLastSyncPrefName

nit: datasetId -> datasetID

@@ +84,5 @@
> +
> +  /**
> +   * Broadcasts a "home-provider-sync" notification if it's an appropriate time to sync.
> +   *
> +  * @param datasetId Dataset to sync

nit: Indentation.

@@ +89,5 @@
> +   */
> +  requestSync: function(datasetId) {
> +    // Make sure it's a good time to sync.
> +    if (Services.prefs.getBoolPref(PREF_SYNC_WIFI_ONLY) && !usingLAN()) {
> +      Cu.reportError("HomeProvider: failed to sync because we're not on wifi");

nit: Technically, it's because we're not on a LAN - it would be good to specify that.

@@ +93,5 @@
> +      Cu.reportError("HomeProvider: failed to sync because we're not on wifi");
> +      return;
> +    }
> +
> +    Services.obs.notifyObservers(null, "home-provider-sync", datasetId);

Perhaps we should define "home-provider-sync" as an exported constant so add-on developers can specify the constant (rather than a String that they can typo without error output).

Also, would it be more efficient to include the datasetID or, at the least, an addon id, in the observer's topic string? That way we're not notifying every add-on for every dataset (at least, if that's how the observer service works).

@@ +101,5 @@
> +  /**
> +   * Specifies that a sync should be requested for the given dataset and update interval.
> +   *
> +   * @param datasetId Dataset to sync
> +   * @param interval Update interval in seconds (minimum 3600 seconds = 1 hour)

You may want to validate the input argument (and throw on error?) so add-on developers don't miss this comment.

@@ +107,5 @@
> +  addPeriodicSync: function(datasetId, interval) {
> +    gSyncIntervals[datasetId] = interval;
> +
> +    if (!gTimerRegistered) {
> +      gUpdateTimerManager.registerTimer("home-provider-sync-timer", this, SYNC_INTERVAL_SECS);

In my testing, registerTimer is equivalent to this is setTimeout, as opposed to setInterval. The docs [1] are ambiguous because the argument is called "interval", but it does not mention repeating, so you'll have to reset it by hand.

I'll paste-bin you my scratchpad code.

[1]: https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIUpdateTimerManager#registerTimer%28%29

@@ +113,5 @@
> +    }
> +  },
> +
> +  /**
> +   * Removes a periodic sync timer. 

nit: ws

@@ +119,5 @@
> +   * @param datasetId Dataset to sync.
> +   */
> +  removePeriodicSync: function(datasetId) {
> +    delete gSyncIntervals[datasetId];
> +    Services.prefs.clearUserPref(getLastSyncPref(datasetId));

Perhaps you want to unregister the timer if `Object.keys(gSyncIntervals).length === 0`?

Comment 3

[:Margaret Leibovic]

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

Thanks for the thorough review!

> @@ +24,5 @@
> >  
> > +const SYNC_INTERVAL_SECS = 3600;
> > +
> > +const PREF_STORAGE_LAST_SYNC_TIME = "home.storage.lastSyncTime.%ID%";
> > +const PREF_SYNC_WIFI_ONLY = "home.sync.wifi-only";
> 
> We should probably prompt the user on this one - file a followup?

Yeah, I think we can land this defaulted to false, but then have a user-visible setting for this. I'll file a follow-up for how to expose this to the user.

> @@ +51,5 @@
> >  }
> >  
> > +function usingLAN() {
> > +  let network = Cc["@mozilla.org/network/network-link-service;1"].getService(Ci.nsINetworkLinkService);
> > +  return (network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_WIFI || network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_ETHERNET);
> 
> nit: === (is that necessary for Gecko code?)

Not necessary for gecko code, but more rigorous. I think we normally default to using ==, but we can be more rigorous in instances like this.

> @@ +58,5 @@
> > +function nowInSeconds() {
> > +  return Math.round(Date.now()/1000);
> > +}
> > +
> > +function getLastSyncPref(datasetId) {

> nit: datasetId -> datasetID

Nope! We already hashed out this argument and settled on datasetId everywhere :P

> 
> @@ +89,5 @@
> > +   */
> > +  requestSync: function(datasetId) {
> > +    // Make sure it's a good time to sync.
> > +    if (Services.prefs.getBoolPref(PREF_SYNC_WIFI_ONLY) && !usingLAN()) {
> > +      Cu.reportError("HomeProvider: failed to sync because we're not on wifi");
> 
> nit: Technically, it's because we're not on a LAN - it would be good to
> specify that.

Technically I copied this code from SimpleServiceDiscovery.jsm, and it's kinda weird that we might have an Android device on ethernet... but you're right that we should update the terminology if we're checking for that. 

> @@ +93,5 @@
> > +      Cu.reportError("HomeProvider: failed to sync because we're not on wifi");
> > +      return;
> > +    }
> > +
> > +    Services.obs.notifyObservers(null, "home-provider-sync", datasetId);
> 
> Perhaps we should define "home-provider-sync" as an exported constant so
> add-on developers can specify the constant (rather than a String that they
> can typo without error output).

Good idea.

> Also, would it be more efficient to include the datasetID or, at the least,
> an addon id, in the observer's topic string? That way we're not notifying
> every add-on for every dataset (at least, if that's how the observer service
> works).

Also a good idea, but we'd need do have a well-documented way of how we construct that topic string. Maybe addPeriodicSync can return the topic string that the add-on should listen for, then we wouldn't even need to worry about publicizing the "home-provider-sync" topic.

> @@ +101,5 @@
> > +  /**
> > +   * Specifies that a sync should be requested for the given dataset and update interval.
> > +   *
> > +   * @param datasetId Dataset to sync
> > +   * @param interval Update interval in seconds (minimum 3600 seconds = 1 hour)
> 
> You may want to validate the input argument (and throw on error?) so add-on
> developers don't miss this comment.

Yeah, I was also thinking of changing the API to specify update interval in hours, since we actually only check once per hour.

> @@ +107,5 @@
> > +  addPeriodicSync: function(datasetId, interval) {
> > +    gSyncIntervals[datasetId] = interval;
> > +
> > +    if (!gTimerRegistered) {
> > +      gUpdateTimerManager.registerTimer("home-provider-sync-timer", this, SYNC_INTERVAL_SECS);
> 
> In my testing, registerTimer is equivalent to this is setTimeout, as opposed
> to setInterval. The docs [1] are ambiguous because the argument is called
> "interval", but it does not mention repeating, so you'll have to reset it by
> hand.
> 
> I'll paste-bin you my scratchpad code.
> 
> [1]:
> https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/
> nsIUpdateTimerManager#registerTimer%28%29

Weird... I'll look into that.

> @@ +119,5 @@
> > +   * @param datasetId Dataset to sync.
> > +   */
> > +  removePeriodicSync: function(datasetId) {
> > +    delete gSyncIntervals[datasetId];
> > +    Services.prefs.clearUserPref(getLastSyncPref(datasetId));
> 
> Perhaps you want to unregister the timer if
> `Object.keys(gSyncIntervals).length === 0`?

There is no API to unregister an update timer! See bug 964525.

Comment 4

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #3)

> > Also, would it be more efficient to include the datasetID or, at the least,
> > an addon id, in the observer's topic string? That way we're not notifying
> > every add-on for every dataset (at least, if that's how the observer service
> > works).
> 
> Also a good idea, but we'd need do have a well-documented way of how we
> construct that topic string. Maybe addPeriodicSync can return the topic
> string that the add-on should listen for, then we wouldn't even need to
> worry about publicizing the "home-provider-sync" topic.

Actually that wouldn't work, since the consumer would want to add an observer before requesting the sync.

Comment 5

[:Margaret Leibovic]

Attachment: Add sync notification APIs to HomeProvider (v2)

I believe this addresses all your comments. I filed bug 965606 about exposing the LAN-only pref to users.

Comment 6

[:Margaret Leibovic]

Comment on attachment 8367703 [details] [diff] [review]
Add sync notification APIs to HomeProvider (v2)

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

::: mobile/android/modules/HomeProvider.jsm
@@ +91,5 @@
> +
> +  /**
> +   * Broadcasts a "home-provider-sync" notification if it's an appropriate time to sync.
> +   *
> +  * @param datasetId Dataset to sync

Oops, missed this indentation fix. Fixed locally.

Comment 7

[:Margaret Leibovic]

Comment on attachment 8367703 [details] [diff] [review]
Add sync notification APIs to HomeProvider (v2)

Actually, I want to revise this.

I don't think we should broadcast the datasetId with the notifications, since an add-on could then learn all the dataset ids of all the other add-ons, making it easy to screw over someone else's data.

I think a better approach is to make custom notification topics for each dataset, but we can have a helper method to do that, so that an add-on can know what topic to observe.

Comment 8

[:Margaret Leibovic]

Attachment: Add sync notification APIs to HomeProvider (v3)

Comment 9

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

Comment on attachment 8368174 [details] [diff] [review]
Add sync notification APIs to HomeProvider (v3)

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

lgtm but I want to consider a few additions.

To mitigate accidental collisions, I think we make the datasetID inserted into the database a combination of addonID and the datasetID. Ideally, we get this addonID from the platform (i.e. which addon is calling this code) but since we've already discussed that this is not easy, it can be passed into the functions by the addon developer (perhaps stored once via HomeProvider.getStorage)? Then the datasetID is merged with the addonID to form the datasetID inserted into the DB and those used for notifications. That way, when an addon developer makes a datasetID like `items`, there are less possible issues.

Alternatively, we can enforce a datasetID convention and catch this on AMO reviews, but it seems like the combination would be a more comprehensive solution.

::: mobile/android/modules/HomeProvider.jsm
@@ +62,5 @@
> +  return Math.round(Date.now() / 1000);
> +}
> +
> +function getLastSyncPrefName(datasetId) {
> +  return PREF_STORAGE_LAST_SYNC_TIME.replace(/%ID%/, datasetId);

nit: Stupidly nitty, but it should be more performant to just append the datasetID because that's all we're doing anyway.

@@ +113,5 @@
> +   *
> +   * @param datasetId Dataset to sync
> +   * @param interval Update interval in seconds. The value of the "home.sync.checkIntervalSecs"
> +   *                 pref determines how frequently we check if this interval has passed, which
> +   *                 defaults to 3600 seconds (1 hour).

I want to note a slight hesitation to put the pref name and the time in the comments (as these values may change and we might forget to update the documentation), but since add-on developers need to see this, I can't think of a better solution.

We could define it in terms of constants we don't intend to change (which they then could look up), but that's probably more tedious than worthwhile.

@@ +115,5 @@
> +   * @param interval Update interval in seconds. The value of the "home.sync.checkIntervalSecs"
> +   *                 pref determines how frequently we check if this interval has passed, which
> +   *                 defaults to 3600 seconds (1 hour).
> +   */
> +  addPeriodicSync: function(datasetId, interval) {

Perhaps we should take a callback here and manage observing the events for the addons?

Comment 10

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #9)
> Comment on attachment 8368174 [details] [diff] [review]
> Add sync notification APIs to HomeProvider (v3)
> 
> Review of attachment 8368174 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm but I want to consider a few additions.
> 
> To mitigate accidental collisions, I think we make the datasetID inserted
> into the database a combination of addonID and the datasetID. Ideally, we
> get this addonID from the platform (i.e. which addon is calling this code)
> but since we've already discussed that this is not easy, it can be passed
> into the functions by the addon developer (perhaps stored once via
> HomeProvider.getStorage)? Then the datasetID is merged with the addonID to
> form the datasetID inserted into the DB and those used for notifications.
> That way, when an addon developer makes a datasetID like `items`, there are
> less possible issues.

I want to push back on this. It's up to the add-on developer to make sure their datasetId is unique. So the addon could just call its dataset "myaddonid-mydatasetid" or whatever. We're never going to be able to totally prevent collisions as long as we're just depending on things add-ons pass in.

> Alternatively, we can enforce a datasetID convention and catch this on AMO
> reviews, but it seems like the combination would be a more comprehensive
> solution.

I think we should just enforce a "make your datasetId unique" convention. Adding another parameter just seems like extra hassle that doesn't even necessarily fix the problem.

> ::: mobile/android/modules/HomeProvider.jsm
> @@ +62,5 @@
> > +  return Math.round(Date.now() / 1000);
> > +}
> > +
> > +function getLastSyncPrefName(datasetId) {
> > +  return PREF_STORAGE_LAST_SYNC_TIME.replace(/%ID%/, datasetId);
> 
> nit: Stupidly nitty, but it should be more performant to just append the
> datasetID because that's all we're doing anyway.

Sure, I was just copying this over from the update timer code, but you're right that sounds simpler.

> @@ +113,5 @@
> > +   *
> > +   * @param datasetId Dataset to sync
> > +   * @param interval Update interval in seconds. The value of the "home.sync.checkIntervalSecs"
> > +   *                 pref determines how frequently we check if this interval has passed, which
> > +   *                 defaults to 3600 seconds (1 hour).
> 
> I want to note a slight hesitation to put the pref name and the time in the
> comments (as these values may change and we might forget to update the
> documentation), but since add-on developers need to see this, I can't think
> of a better solution.

I think it's okay to mention the default value, since we're also mentioning the pref name, so we at least have a path to figure out what's going on if someone is confused by the wrong value.

> We could define it in terms of constants we don't intend to change (which
> they then could look up), but that's probably more tedious than worthwhile.

I intentionally chose a pref instead of a constant, since we'll want to be able to change this when running tests (we don't want to wait 1 hour while performing an automated test).

> @@ +115,5 @@
> > +   * @param interval Update interval in seconds. The value of the "home.sync.checkIntervalSecs"
> > +   *                 pref determines how frequently we check if this interval has passed, which
> > +   *                 defaults to 3600 seconds (1 hour).
> > +   */
> > +  addPeriodicSync: function(datasetId, interval) {
> 
> Perhaps we should take a callback here and manage observing the events for
> the addons?

Yeah, I've gone back and forth on that. The nice thing about the current model is that the add-on can call requestSync and addPeriodicSync separately, but have the same code handle the result. However, that may actually be kind of confusing in practice. I'll explore this callback idea.

I'll also look into adding some test coverage to testHomeProvider.

Comment 11

[:Margaret Leibovic]

Attachment: Add sync notification APIs to HomeProvider (v4)

Now with test coverage! I tried checking for the callback firing multiple times, but I think I ran up against minimum interval times that are baked into the update timer manager. I think there are ways for me to toggle around those prefs for tests, but I don't think it's important enough to try to investigate now. However, I did verify locally with a test add-on that the callback will fire multiple times (if you wait long enough).

I like the suggestion to add callbacks to the API instead of making add-on developers observe notifications. I also moved setting the last sync pref into notify, so that forced requestSync calls won't interfere with periodic sync timers. We can refine this in a follow-up if needed, but I think that setting the time in requestSync could lead to unexpected behavior, since they can be called with different callback functions.

Comment 12

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

Comment on attachment 8370453 [details] [diff] [review]
Add sync notification APIs to HomeProvider (v4)


>diff --git a/mobile/android/app/mobile.js b/mobile/android/app/mobile.js

>+// Whether or not to only broadcast home provider sync notifications to when the
>+// user is connected to a local network.
>+pref("home.sync.lanOnly", false);

It might technically be LAN, but the docs and UI will say "wifi". This is true with any similar feature too. We even have network prefs that say "wifi" but mean ethernet/LAN

>diff --git a/mobile/android/modules/HomeProvider.jsm b/mobile/android/modules/HomeProvider.jsm

>+const DB_PATH = OS.Path.join(OS.Constants.Path.profileDir, "home.sqlite");

I wonder what this costs since it's code that executes on import. Could this be lazy?

>+  requestSync: function(datasetId, callback) {

I assume we could pass a method bound to an object too. I don't want to only depend on anon functions for all interactions. Creating lots and lots of anon functions can't be good for memory.

Comment 13

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

(In reply to :Margaret Leibovic from comment #10)
> I want to push back on this. It's up to the add-on developer to make sure
> their datasetId is unique. So the addon could just call its dataset
> "myaddonid-mydatasetid" or whatever. We're never going to be able to totally
> prevent collisions as long as we're just depending on things add-ons pass in.

I prefer addonid-datasetid because it's likely to be safer and easier to enforce on AMO reviews (i.e. datasetid no longer has to be unique and there's only one addonid to check), though I didn't realize how it's needed for every `*Sync` call so I understand what a(n unexplicable) pain it would be as two args. Unique datasetids work for me!

> > We could define it in terms of constants we don't intend to change (which
> > they then could look up), but that's probably more tedious than worthwhile.
> 
> I intentionally chose a pref instead of a constant, since we'll want to be
> able to change this when running tests (we don't want to wait 1 hour while
> performing an automated test).

Sorry, I meant define the preference name in the comments in terms of constants so we say, "...this is throttled to the value of the PREF_SYNC_CHECK_INVERVAL_SECS preference". That name is less likely to change than the value but I don't entirely think it's worthwhile.

Comment 14

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

(In reply to Mark Finkle (:mfinkle) from comment #12)
> It might technically be LAN, but the docs and UI will say "wifi". This is
> true with any similar feature too. We even have network prefs that say
> "wifi" but mean ethernet/LAN

I'm down for using the term "wifi" for consistency reasons with a comment in the `isUsingWifi` function explaining this motivation.

Comment 15

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #14)
> (In reply to Mark Finkle (:mfinkle) from comment #12)
> > It might technically be LAN, but the docs and UI will say "wifi". This is
> > true with any similar feature too. We even have network prefs that say
> > "wifi" but mean ethernet/LAN
> 
> I'm down for using the term "wifi" for consistency reasons with a comment in
> the `isUsingWifi` function explaining this motivation.

So... rename things to say "wifi" instead of "LAN"?

I can also put DB_PATH inside a lazy getter, that's a good idea.

Comment 16

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

Comment on attachment 8370453 [details] [diff] [review]
Add sync notification APIs to HomeProvider (v4)

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

r+ with a private `HomeProvider.notify` function. We can do the test work in a followup.

::: mobile/android/base/tests/testHomeProvider.js
@@ +8,5 @@
>  Cu.import("resource://gre/modules/HomeProvider.jsm");
>  Cu.import("resource://gre/modules/osfile.jsm");
>  Cu.import("resource://gre/modules/Sqlite.jsm");
>  Cu.import("resource://gre/modules/Task.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");

nit: Alphabetize.

@@ +20,4 @@
>  const DB_PATH = OS.Path.join(OS.Constants.Path.profileDir, "home.sqlite");
>  
> +add_test(function test_request_sync() {
> +  HomeProvider.requestSync(TEST_DATASET_ID, function callback(datasetId) {

Test the boolean return value.

@@ +20,5 @@
>  const DB_PATH = OS.Path.join(OS.Constants.Path.profileDir, "home.sqlite");
>  
> +add_test(function test_request_sync() {
> +  HomeProvider.requestSync(TEST_DATASET_ID, function callback(datasetId) {
> +    do_check_eq(datasetId, TEST_DATASET_ID);

We probably want to test the LAN sync functionality.

nit: Not sure the best implementation, but it might be a good idea to ensure this doesn't wait for the global timer tick to run.

@@ +29,5 @@
> +add_test(function test_periodic_sync() {
> +  // Lower the check interval for testing purposes.
> +  Services.prefs.setIntPref(PREF_SYNC_CHECK_INTERVAL_SECS, TEST_INTERVAL_SECS);
> +
> +  HomeProvider.addPeriodicSync(TEST_DATASET_ID, TEST_INTERVAL_SECS, function callback(datasetId) {

nit: We should ensure this uses the timer (e.g. at least TEST_INTERVAL_SECS have passed before callback is called).

@@ +33,5 @@
> +  HomeProvider.addPeriodicSync(TEST_DATASET_ID, TEST_INTERVAL_SECS, function callback(datasetId) {
> +    do_check_eq(datasetId, TEST_DATASET_ID);
> +
> +    // Clean up before running the next test.
> +    Services.prefs.clearUserPref(PREF_SYNC_CHECK_INTERVAL_SECS);

Are these preferences saved after test execution? If so, if we're killed mid-test or an Exception is thrown (I assume `do_check_eq` does not throw an exception because `run_next_test` needs to be run), this doesn't get called. Can we put this in a `finally` clause?

@@ +34,5 @@
> +    do_check_eq(datasetId, TEST_DATASET_ID);
> +
> +    // Clean up before running the next test.
> +    Services.prefs.clearUserPref(PREF_SYNC_CHECK_INTERVAL_SECS);
> +    HomeProvider.removePeriodicSync(TEST_DATASET_ID);

We should test this functionality before using it to cleanup.

::: mobile/android/modules/HomeProvider.jsm
@@ +113,5 @@
> +   */
> +  addPeriodicSync: function(datasetId, interval, callback) {
> +    // Warn developers if they're expecting more frequent notifications that we allow.
> +    if (interval < gSyncCheckIntervalSecs) {
> +      Cu.reportError("HomeProvider: Sync notifications are throttled to " + gSyncCheckIntervalSecs + " seconds");

I realize we don't specify the addon-id here, which could be confusing when you get multiple of these.

Perhaps we can have an optional registerLogtag() function so that developers can differentiate their addons' logtag?

@@ +132,5 @@
> +   * Removes a periodic sync timer.
> +   *
> +   * @param datasetId Dataset to sync.
> +   */
> +  removePeriodicSync: function(datasetId) {

nit: Add a comment to the end of this function saying why we don't attempt to unregister the gUpdateTimerManager even if gSyncCallbacks is empty (just so aspiring refactorers don't waste time on it).

@@ +140,5 @@
> +
> +  /**
> +   * nsITimerCallback implementation. Checks to see if it's time to sync any registered datasets.
> +   */
> +  notify: function(timer) {

This is function is accessible by anyone - intentional? I think it'd be better if it was not exported.

@@ +147,5 @@
> +      try {
> +        lastSyncTime = Services.prefs.getIntPref(getLastSyncPrefName(datasetId));
> +      } catch(e) { }
> +
> +      let syncCallback = gSyncCallbacks[datasetId];

nit: Misleading name as an object. I'd prefer a destructuring assignment:

`let { interval: syncInterval, callback: syncCallback } = gSyncCallbacks[datasetId];`

@@ +151,5 @@
> +      let syncCallback = gSyncCallbacks[datasetId];
> +      if (lastSyncTime < getNowInSeconds() - syncCallback.interval) {
> +        let syncSuccess = this.requestSync(datasetId, syncCallback.callback);
> +        if (syncSuccess) {
> +          Services.prefs.setIntPref(getLastSyncPrefName(datasetId), getNowInSeconds());

nit: Cache getNowInSeconds() for here and above

Comment 17

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

(In reply to :Margaret Leibovic from comment #15)
> So... rename things to say "wifi" instead of "LAN"?

Yes, and add a comment saying why the code does not do the function's namesake. :)

Comment 18

[:Margaret Leibovic]

Attachment: Add sync APIs to HomeProvider (v5)

I addressed some of the test feedback, but yeah, we can continue to improve the test in a follow-up. I just want to get this landed!

Comment 19

[:Margaret Leibovic]

Comment on attachment 8371833 [details] [diff] [review]
Add sync APIs to HomeProvider (v5)

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

::: mobile/android/app/mobile.js
@@ +828,5 @@
> +
> +// Whether or not to only sync home provider data when the user is on wifi.
> +pref("home.sync.wifiOnly", false);
> +
> +// How frequently to check if we should broadcast home provider sync notifications.

Oops this comment is wrong now, fixing locally.

Comment 20

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

Comment on attachment 8371833 [details] [diff] [review]
Add sync APIs to HomeProvider (v5)

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

r+, assuming the nsiUpdateTimerManager callback works as noted below.

::: mobile/android/modules/HomeProvider.jsm
@@ +154,5 @@
> +      callback: callback
> +    };
> +
> +    if (!gTimerRegistered) {
> +      gUpdateTimerManager.registerTimer("home-provider-sync-timer", syncTimerCallback, gSyncCheckIntervalSecs);

Does this work? The documentation seems to imply it has to be an object with a function as a "notify" attribute: https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsITimerCallback

Comment 21

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

Don't forget to file a followup for any additional testing you think may be necessary (can be added to bug 963352 if you want).

Comment 22

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #20)
> Comment on attachment 8371833 [details] [diff] [review]
> Add sync APIs to HomeProvider (v5)
> 
> Review of attachment 8371833 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+, assuming the nsiUpdateTimerManager callback works as noted below.
> 
> ::: mobile/android/modules/HomeProvider.jsm
> @@ +154,5 @@
> > +      callback: callback
> > +    };
> > +
> > +    if (!gTimerRegistered) {
> > +      gUpdateTimerManager.registerTimer("home-provider-sync-timer", syncTimerCallback, gSyncCheckIntervalSecs);
> 
> Does this work? The documentation seems to imply it has to be an object with
> a function as a "notify" attribute:
> https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/
> nsITimerCallback

That documentation is probably really old. I saw gavin talking to you about exactly this on IRC yesterday - those objects used to be required, but we have the "function" marker now, so we don't need them anymore.

Comment 23

[:Margaret Leibovic]

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

Comment 24

[Carsten Book [:Tomcat]]

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