Permit a sync at least once per launch of the browser

RESOLVED INCOMPLETE

Status

()

Firefox for Android
Android Sync
P3
normal
RESOLVED INCOMPLETE
4 years ago
9 days ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
This bug is to clear our rate limit each time Fennec chooses to. I'm probably gonna call that "in BrowserApp.onCreate".
(Assignee)

Comment 1

4 years ago
Created attachment 8355439 [details] [diff] [review]
Part 2: permit routine syncs each time BrowserApp launches.
(Assignee)

Comment 2

4 years ago
Created attachment 8355440 [details] [diff] [review]
Part 1: add SyncAccounts.permitRoutineSyncs.
(Assignee)

Comment 3

4 years ago
These two patches are untested, but they build.
Comment on attachment 8355439 [details] [diff] [review]
Part 2: permit routine syncs each time BrowserApp launches.

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

Let's pave the way to making Sync easier for Fennec devs to interact with.  I suggest breaking this out into a named method, adding a JavaDoc comment explaining that there's a rate limiting mechanism and that Fennec can ask to override that regime, and keeping the Runnable around so it can be posted to the background thread repeatedly.
Attachment #8355439 - Flags: feedback+
Comment on attachment 8355440 [details] [diff] [review]
Part 1: add SyncAccounts.permitRoutineSyncs.

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

nits and a substantial quibble.

::: mobile/android/base/sync/setup/SyncAccounts.java
@@ +323,5 @@
> +   * Allow a caller to remove soft rate limits for all Sync accounts. This
> +   * allows for ordinary syncs -- as would be triggered by launching the browser
> +   * -- to proceed unencumbered, resulting in a more up-to-date user experience.
> +   */
> +  public static void permitRoutineSyncs(Context context) {

This seems backwards.  Routine syncs are the ones that happen all the time, triggered by Android, and should be rate limited.  It's *non*-routine syncs -- Fennec just started, I want fresh tabs, I just sent a tab -- that should have the door opened.

@@ +354,5 @@
> +      }
> +    }
> +  }
> +
> +  private static SharedPreferences getAccountSharedPreferences(Context context, AccountManager accountManager, Account account) throws NoSuchAlgorithmException, UnsupportedEncodingException, CredentialException {

fly by: this exists in Utils.  I'd prefer it *not* exist in Utils, since TestUtils has problems when run from Eclipse.  So I'd like it if these getAccountSharedPreferences(*) thinks were not duplicated and not in Utils.

@@ +384,1 @@
>    public static void backgroundSetSyncAutomatically(final Account account, final boolean syncAutomatically) {

nit: can we remove this background* thing?  Our style is not to do background explicitly like this.

::: mobile/android/base/sync/syncadapter/SyncAdapter.java
@@ +75,5 @@
>    public synchronized long getEarliestNextSync() {
>      return accountSharedPreferences.getLong(SyncConfiguration.PREF_EARLIEST_NEXT_SYNC, 0);
>    }
>  
> +  public synchronized void setEarliestNextSync(long next, boolean dueToBackoff) {

nit: not a huge fan of hard-to-interpret flags.  How about an enum, or a second explanatory method?
Attachment #8355440 - Flags: feedback+
(Assignee)

Comment 6

4 years ago
(In reply to Nick Alexander :nalexander from comment #5)

> > +   * Allow a caller to remove soft rate limits for all Sync accounts. This
> > +   * allows for ordinary syncs -- as would be triggered by launching the browser
> > +   * -- to proceed unencumbered, resulting in a more up-to-date user experience.
> > +   */
> > +  public static void permitRoutineSyncs(Context context) {
> 
> This seems backwards.  Routine syncs are the ones that happen all the time,
> triggered by Android, and should be rate limited.  It's *non*-routine syncs
> -- Fennec just started, I want fresh tabs, I just sent a tab -- that should
> have the door opened.

Depends on your definition of "routine".

I'm using "routine" as "not forced", where forced syncs are ones caused by invoking CR.requestSync.

We get routine sync events all the time -- every 30 seconds during most browsing.

Sync's rate limiting does not permit them to cause a sync to occur.

What this method does is to permit the next routine sync to occur (for each Account).

We're actually not going to trigger a sync immediately on startup; we'll wait until the routine sync that's triggered shortly after the browser touches BrowserProvider. Not touching the DB immediately on first page load just bought us an 11% talos win, so my approach is to remove the barrier to the next routine sync, not to trigger a non-routine sync. Similar results, better perf.

(We'll trigger a non-routine sync in onPause, which will be a different bug. In that case, the DB will be open, the browser will be going into the background, and you'll probably have data to upload for your other clients to see.)


> fly by: this exists in Utils.  I'd prefer it *not* exist in Utils, since
> TestUtils has problems when run from Eclipse.  So I'd like it if these
> getAccountSharedPreferences(*) thinks were not duplicated and not in Utils.

I read that as "please delete the version in Utils".


> > +  public synchronized void setEarliestNextSync(long next, boolean dueToBackoff) {
> 
> nit: not a huge fan of hard-to-interpret flags.  How about an enum, or a
> second explanatory method?

Sure.
(Assignee)

Comment 7

4 years ago
Urgency for this is dramatically reduced now that scheduling works better.
Priority: -- → P3
(Assignee)

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → INCOMPLETE

Updated

9 days ago
Product: Android Background Services → Firefox for Android
You need to log in before you can comment on or make changes to this bug.