Richard Newman [:rnewman] Assignee
	Description • 11 years ago

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

	Richard Newman [:rnewman] Assignee
	Comment 3 • 11 years ago

These two patches are untested, but they build.

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 4 • 11 years ago

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.

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 5 • 11 years ago

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?

	Richard Newman [:rnewman] Assignee
	Comment 6 • 11 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.

	Richard Newman [:rnewman] Assignee
	Comment 7 • 11 years ago

Urgency for this is dramatically reduced now that scheduling works better.