Closed Bug 1014338 Opened 6 years ago Closed 6 years ago

Rework distribution processing to allow for delayed initialization

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 32

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

Attachments

(4 files, 1 obsolete file)

Spinning this off from Bug 1013024.
No longer blocks: 1014273
This patch cleans up some of the distribution loading code, and provides a hook
point for callers to hang code that needs to run once the distribution has been
loaded.
Comment on attachment 8426707 [details] [diff] [review]
Pre: add ThreadUtils.assertNotOnUiThread.

Carrying forward Margaret's review from Bug 1013024.
Attachment #8426707 - Flags: review+
Comment on attachment 8426708 [details] [diff] [review]
Part 0: move Distribution class into its own package.

Carrying forward Margaret's review from Bug 1013024.
Attachment #8426708 - Flags: review+
Comment on attachment 8426709 [details] [diff] [review]
Part 1: rework distribution handling.

This is a reasonable amount of cleanup and reworking that I was doing as part of Bug 1013024, and as a prerequisite for the fixes in Bug 1013684 and friends. Figured it deserved its own patch, so here it is.
Attachment #8426709 - Flags: review?(margaret.leibovic)
Attachment #8426712 - Flags: review?(margaret.leibovic)
Now with restored imports.
Attachment #8426718 - Flags: review?(margaret.leibovic)
Attachment #8426712 - Attachment is obsolete: true
Attachment #8426712 - Flags: review?(margaret.leibovic)
Blocks: 1012462
Comment on attachment 8426709 [details] [diff] [review]
Part 1: rework distribution handling.

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

LGTM.

::: mobile/android/base/distribution/Distribution.java
@@ +253,5 @@
> +
> +        // We try the APK, then the system directory.
> +        final boolean distributionSet =
> +                checkAPKDistribution() ||
> +                checkSystemDistribution();

I like how your pulled these into helper methods.
Attachment #8426709 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8426718 [details] [diff] [review]
Part 2: add delayed distro initialization hook. v2

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

::: mobile/android/base/distribution/Distribution.java
@@ +281,5 @@
>  
>          this.state = distributionSet ? STATE_SET : STATE_NONE;
>          settings.edit().putInt(keyName, this.state).commit();
> +
> +        runReadyQueue();

I was thinking it might make more sense to put one runReadyQueue() call after doInit() is called, but I realized it has multiple call sites, so this way is probably better, so that these calls are kept close together.

@@ +465,5 @@
>      }
> +
> +    /**
> +     * The provided <code>Runnable</code> will be executed after the distribution
> +     * is ready, or discarded if the distribution has already been processed.

Add a comment to let consumers know this Runnable will always be run on the background thread?
Attachment #8426718 - Flags: review?(margaret.leibovic) → review+
Tested by uploading to Play. Even the dramatically delayed intent when installing over LTE works fine.
Oops. That was meant for Bug 1124492.
You need to log in before you can comment on or make changes to this bug.