1014338 - Rework distribution processing to allow for delayed initialization

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

Spinning this off from Bug 1013024.

Comment 1

[Richard Newman [:rnewman]]

Attachment: Pre: add ThreadUtils.assertNotOnUiThread.

Comment 2

[Richard Newman [:rnewman]]

Attachment: Part 0: move Distribution class into its own package.

Comment 3

[Richard Newman [:rnewman]]

Attachment: Part 1: rework distribution handling.

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 4

[Richard Newman [:rnewman]]

Comment on attachment 8426707 [details] [diff] [review]
Pre: add ThreadUtils.assertNotOnUiThread.

Carrying forward Margaret's review from Bug 1013024.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 8426708 [details] [diff] [review]
Part 0: move Distribution class into its own package.

Carrying forward Margaret's review from Bug 1013024.

Comment 6

[Richard Newman [:rnewman]]

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.

Comment 7

[Richard Newman [:rnewman]]

Attachment: Part 2: add delayed distro initialization hook. v1

Comment 8

[Richard Newman [:rnewman]]

Attachment: Part 2: add delayed distro initialization hook. v2

Now with restored imports.

Comment 9

[:Margaret Leibovic]

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.

Comment 10

[:Margaret Leibovic]

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?

Comment 11

[Richard Newman [:rnewman]]

   https://hg.mozilla.org/integration/fx-team/rev/207ea035d63e
   https://hg.mozilla.org/integration/fx-team/rev/90150f97a6de
   https://hg.mozilla.org/integration/fx-team/rev/5054df38fc14
   https://hg.mozilla.org/integration/fx-team/rev/74d77419ad61

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/207ea035d63e
https://hg.mozilla.org/mozilla-central/rev/90150f97a6de
https://hg.mozilla.org/mozilla-central/rev/5054df38fc14
https://hg.mozilla.org/mozilla-central/rev/74d77419ad61

Comment 13

[Richard Newman [:rnewman]]

Follow-up to correct comment.

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

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/498dc4b32c2c

Comment 15

[Richard Newman [:rnewman]]

Tested by uploading to Play. Even the dramatically delayed intent when installing over LTE works fine.

Comment 16

[Richard Newman [:rnewman]]

Oops. That was meant for Bug 1124492.