1096669 - Periodic update last synced value in the main FxA status screen

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Android Sync, defect)

Description

[Vivek Balakrishnan[:vivek]]

This is a follow up to bug 966103

"last synced value" should be updated once a minute when account status activity is in foreground.

Comment 1

[Vivek Balakrishnan[:vivek]]

Attachment: 1096669.patch

Last synced value updated using scheduled future

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8529834 [details] [diff] [review]
1096669.patch

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

I'm confident this will work, but I'd like to do it using the Handler if possible.  If you investigated this and there's a reason not to, let me know.

::: mobile/android/base/fxa/activities/FxAccountStatusFragment.java
@@ +62,5 @@
>    // still around to service it, and since we're not updating any UI, we'll just
>    // schedule the sync as usual. See also comment below about garbage
>    // collection.
>    private static final long DELAY_IN_MILLISECONDS_BEFORE_REQUESTING_SYNC = 5 * 1000;
> +  private static final long LAST_SYNCED_TIME_UPDATE_INTERVAL_IN_MINUTES = 1;

Let's use millis throughout.  It's standard in this part of the codebase.

@@ +107,5 @@
>    // Member variable so that re-posting pushes back the already posted instance.
>    // This Runnable references the fxAccount above, but it is not specific to a
>    // single account. (That is, it does not capture a single account instance.)
>    protected Runnable requestSyncRunnable;
> +  protected Runnable lastSyncedTimeUpdateRunnable =  new Runnable() {

nit: final.

@@ +512,5 @@
> +    // Schedule to periodically update last synced time.
> +    lastSyncedTimeUpdateFuture = LAST_SYNCED_UPDATE_SCHEDULER.scheduleWithFixedDelay(
> +        new Runnable() {
> +          @Override
> +          public void run() {

There's a way to post to the UI thread Android Handler that does the scheduling and the cancellation of an existing Runnable that simplifies this:

http://developer.android.com/reference/android/os/Handler.html#postDelayed%28java.lang.Runnable,%20long%29

This will work, I'm confident; but the Handler way is more Android-y and less Java-y.

Comment 3

[Vivek Balakrishnan[:vivek]]

Attachment: 1096669.patch

Last synced time updated periodically using Handlers

Comment 4

[Vivek Balakrishnan[:vivek]]

Attachment: 1096669.patch

Minor nit corrected

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8533847 [details] [diff] [review]
1096669.patch

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

::: mobile/android/base/fxa/activities/FxAccountStatusFragment.java
@@ +422,5 @@
>        throw new IllegalStateException(e);
>      }
>  
>      handler = new Handler(); // Attached to current (assumed to be UI) thread.
> +    lastSyncedTimeUpdateHandler = new Handler();

Use the existing handler.  Under the hood, these will do the same thing.  The important thing is being on the UI thread, since you'll update the UI.

@@ +551,5 @@
> +
> +    if (lastSyncedTimeUpdateRunnable != null) {
> +      lastSyncedTimeUpdateHandler.removeCallbacks(lastSyncedTimeUpdateRunnable);
> +    }
> +    lastSyncedTimeUpdateRunnable = new LastSyncTimeUpdateRunnable();

This requires null checking and allocates a new runnable each time.  (It's not a lot of memory churn, but it's the principle of the thing.)  Use the existing pattern, where you lazily initialize the runnable and then keep posting the same one.  That very conveniently pushes any existing runnable backwards.

Comment 6

[Vivek Balakrishnan[:vivek]]

Attachment: 1096669.patch

Review comments addressed.

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8534677 [details] [diff] [review]
1096669.patch

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

Solid!

Seems like we should do something similar in the RemoteTabsHomePanel, so I filed Bug 1112325.  It's a little tricky 'cuz the view is populated by an underlying ListAdapter implementation [1].  I'm not sure we want the ListAdapter to invalidate its contents every minute; we really just want a UI-only update every minute.

[1] http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/RemoteTabsExpandableListAdapter.java#126

::: mobile/android/base/fxa/activities/FxAccountStatusFragment.java
@@ +443,5 @@
>      super.onPause();
>      FxAccountSyncStatusHelper.getInstance().stopObserving(syncStatusDelegate);
> +
> +    // Focus lost, remove scheduled update if any.
> +    if (lastSyncedTimeUpdateRunnable != null) {

Nice, I was wondering if you would consider this.  I should have known better!

Comment 8

[Nick Alexander :nalexander [he/him]]

vivek: thanks for your work on this!  I CCed you on Bug 1063742, which is a bigger chunk of work that you might be interested in.

ni to me to land this.

Comment 9

[Nick Alexander :nalexander [he/him]]

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

Comment 10

[Nick Alexander :nalexander [he/him]]

Sorry this took so long to land.  Works perfectly on my device.  Bravo!

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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