1291383 - [geckoview] Make GeckoProfile Fennec distribution agnostic

Status: RESOLVED FIXED

(GeckoView :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

Right now, Gecko profile creation interacts with Fennec (and Gecko)
distributions in two non-trivial ways: it triggers adding default
bookmarks and it tries to download and process distribution files.
Neither of these are appropriate for GeckoView consuming applications.

This ticket tracks adding a way for GeckoView consumers to witness the
profile life-cycle events and trigger whatever behaviour is
appropriate.  Usually, one would use a globally available message bus
(or dependency injection) to achieve this, but we don't really have
that right now.  We could add some such mechanism, or we could vector
through the existing GeckoInterface to add callbacks at the
appropriate points, etc.

The commit at https://reviewboard.mozilla.org/r/60464/ just removes
the functionality in question.

Comment 1

[Nick Alexander :nalexander [he/him]]

jchen: I think this is the second thing to address, tied with Bug 1291384.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Let Java code dispatch events through EventDispatcher (v1)

Add dispatch() methods to EventDispatcher that allow Java code to
dispatch events to Bundle listeners (currently either UI or background
thread listeners).

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Dispatch Profile:Create event when creating profile directory (v1)

Dispatch a Profile:Create event when creating a profile directory, so we can
use it to initialize distributions. The actual distribution code will be moved
in a later patch.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Move Distribution initialization out of GeckoProfile (v1)

Move Distribution initialization (on profile directory creation) from
GeckoProfile to GeckoApplication, by listening to the Profile:Create
event.

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8786913 [details] [diff] [review]
Let Java code dispatch events through EventDispatcher (v1)

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

lgtm.  This seems very generally valuable (App-wide message bus!); worth a post to mobile-firefox-dev?

Oops, didn't realize you flagged sebastian.  He should bless this as well.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/EventDispatcher.java
@@ +270,5 @@
> +        if (message == null) {
> +            throw new IllegalArgumentException("Null message");
> +        }
> +
> +        // First try native listeners.

nit: there doesn't seem to be a second.  I think this comment is stale and can be removed.

Comment 6

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8786915 [details] [diff] [review]
Dispatch Profile:Create event when creating profile directory (v1)

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

This patch is fine *if the existing logic is correct*.  I cannot convince myself that it is!  You probably know this code best: why won't we see Profile:Create twice?

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8786917 [details] [diff] [review]
Move Distribution initialization out of GeckoProfile (v1)

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

This is fine by me.  Flagging mkaply for feedback, since he should know about Distribution-impacting startup changes.

Comment 8

[Mike Kaply [:mkaply]]

Comment on attachment 8786917 [details] [diff] [review]
Move Distribution initialization out of GeckoProfile (v1)

This looks fine to me.

Once it has landed, ping me and I can do some local tests with partner distributions to make sure everything works as expected.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8786913 [details] [diff] [review]
Let Java code dispatch events through EventDispatcher (v1)

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

Awesome, we finally have an event bus! :)

Is this something we can use more broadly (like Android UI components notifying each other about things) or does it make sense to look into using some standard libraries (Otto or Greenrobot EventBus) for that?

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

I didn't look at the other patches - but I remember that some of the distribution/profile/foo code required things to happen in a certain order. Does the event code change this? Are things still sequentially or are they happening in parallel?

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Sebastian Kaspari (:sebastian) from comment #9)
> Comment on attachment 8786913 [details] [diff] [review]
> Let Java code dispatch events through EventDispatcher (v1)
> 
> Review of attachment 8786913 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Awesome, we finally have an event bus! :)
> 
> Is this something we can use more broadly (like Android UI components
> notifying each other about things) or does it make sense to look into using
> some standard libraries (Otto or Greenrobot EventBus) for that?

Yeah I think we can use it more broadly, if the use case makes sense (e.g. if you want to broadcast a state change message asynchronously).

(In reply to Sebastian Kaspari (:sebastian) from comment #10)
> I didn't look at the other patches - but I remember that some of the
> distribution/profile/foo code required things to happen in a certain order.
> Does the event code change this? Are things still sequentially or are they
> happening in parallel?

It should be more or less the same as before. The only difference I can think of is that Distribution.addOnDistributionReadyCallback is now called asynchronously from the background thread instead of synchronously from whichever thread is creating a profile directory. I don't think that makes a difference though. Obviously we should still do some rigorous testing of distributions after this lands.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6edd9a11c06d
Let Java code dispatch events through EventDispatcher; r=sebastian
https://hg.mozilla.org/integration/mozilla-inbound/rev/555ab8465b07
Dispatch Profile:Create event when creating profile directory; r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c8ce211041f
Move Distribution initialization out of GeckoProfile; r=nalexander

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6edd9a11c06d
https://hg.mozilla.org/mozilla-central/rev/555ab8465b07
https://hg.mozilla.org/mozilla-central/rev/2c8ce211041f