Closed Bug 1291383 Opened 5 years ago Closed 5 years ago

[geckoview] Make GeckoProfile Fennec distribution agnostic

Categories

(GeckoView :: General, defect)

defect
Not set
normal

Tracking

(firefox51 fixed)

RESOLVED FIXED
Tracking Status
firefox51 --- fixed

People

(Reporter: nalexander, Assigned: jchen)

References

Details

Attachments

(3 files)

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.
jchen: I think this is the second thing to address, tied with Bug 1291384.
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Blocks: 1291385
Add dispatch() methods to EventDispatcher that allow Java code to
dispatch events to Bundle listeners (currently either UI or background
thread listeners).
Attachment #8786913 - Flags: review?(s.kaspari)
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.
Attachment #8786915 - Flags: review?(nalexander)
Move Distribution initialization (on profile directory creation) from
GeckoProfile to GeckoApplication, by listening to the Profile:Create
event.
Attachment #8786917 - Flags: review?(nalexander)
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.
Attachment #8786913 - Flags: feedback+
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?
Attachment #8786915 - Flags: review?(nalexander) → review+
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.
Attachment #8786917 - Flags: review?(nalexander)
Attachment #8786917 - Flags: review+
Attachment #8786917 - Flags: feedback?(mozilla)
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.
Attachment #8786917 - Flags: feedback?(mozilla) → feedback+
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?
Attachment #8786913 - Flags: review?(s.kaspari) → review+
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?
(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.
Pushed by nchen@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/6edd9a11c06d
https://hg.mozilla.org/mozilla-central/rev/555ab8465b07
https://hg.mozilla.org/mozilla-central/rev/2c8ce211041f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 51 → ---
You need to log in before you can comment on or make changes to this bug.