Closed Bug 1014338 Opened 7 years ago Closed 7 years ago

Rework distribution processing to allow for delayed initialization

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

Attachments

(4 files, 1 obsolete file)

Spinning this off from Bug 1013024.
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.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.