Closed Bug 1180358 Opened 4 years ago Closed 4 years ago

Add the b2gdroid sub-product

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 43
Tracking Status
firefox42 --- affected
firefox43 --- fixed

People

(Reporter: fabrice, Assigned: fabrice)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Attached patch b2gdroid-product.patch v1 (obsolete) — Splinter Review
This adds a new mobile/android/b2gdroid product, with the MOZ_B2GDROID variable.
I'm not especially happy how that turned out - I guess it would be better to make less changes to mobile/android/base.

This patch produces a working apk that can be used as a homescreen, but the gecko parts to bring in the b2g/ and gaia parts will be done in another patch.
Assignee: nobody → fabrice
Attachment #8629549 - Flags: feedback?(nalexander)
Attached patch b2gdroid-product.patch v2 (obsolete) — Splinter Review
I guess not breaking fennec builds is expected ;)
Attachment #8629549 - Attachment is obsolete: true
Attachment #8629549 - Flags: feedback?(nalexander)
Attachment #8629613 - Flags: feedback?(nalexander)
Blocks: 1180461
Attachment #8629613 - Attachment is obsolete: true
Attachment #8629613 - Flags: feedback?(nalexander)
Attachment #8629759 - Flags: review?(nalexander)
Comment on attachment 8629759 [details] [diff] [review]
b2gdroid-product.patch

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

Fabrice, others: sorry that this is kind of a meta review.

There are a lot of implications to this approach.  I understand many
of the Java-side implications but few of the Mozilla build product and
Gecko implications.  I'm CCing glandium to fill some of those in.  I
think release engineering will have opinions too.

My opinion is that we shouldn't try to make a modified Fennec, we
should make a GeckoView consumer.  If we agree on that approach,
there's less urgency to land b2gdroid App-layer things into the tree
up front: the focus should be on building out the GeckoView-layer
interfaces that the App needs and the underlying Gecko features.  The
App can proceed outside of the tree as we work out GeckoView kinks and
we can avoid a thousand l10n, packaging, and testing pains.

My actionable suggestions for moving this forward:

* Land whatever platform things are needed to produce the altered Gecko libraries b2gdroid needs.
* Co-ordinate with release engineering to add a single new b2gdroid automation build, with the required Gecko mozconfig changes and an unmodified Fennec (and GeckoView).
* Base b2gdroid off of https://github.com/ncalexan/geckobrowser-gradle and the produced GeckoView-with-b2gdroid-Gecko.
* Develop b2gdroid out-of-tree while we figure out how to land a new GeckoView-consuming product into the tree.

If we absolutely must land in tree, I think we should consider landing
a straight copy of a working Gradle project and building with Gradle
directly.  This is not actually that hard -- I have (had) try builds
that do this at https://bugzilla.mozilla.org/show_bug.cgi?id=1119520.
(There are many reasons I haven't pushed this for Fennec, the most
significant being that the risk of hard-to-identify regressions is
high.  b2gdroid has ~0 users so such regressions are unlikely to be
catastrophic.)

Notes on b2gdroid

* There's no reason to repeat the path choices of Fennec, so:

I see you choose org.mozilla.b2gdroid for the Android package name.
Take org.mozilla.b2gdroid for the Java package name, too, please.

Use

mobile/android/b2gdroid/src/main/AndroidManifest.xml.in
and
mobile/android/b2gdroid/src/main/java/org/mozilla/b2gdroid/Launcher.java
and
mobile/android/b2gdroid/src/main/res/layout/launcher.xml

please.  (This is the Android standard, and would be exactly what you
would have if we follow my GeckoView-and-Gradle approach.)

* This looks a lot more like a GeckoView *consumer* than a modified Fennec.

For example, the app layout has a GeckoView widget, and the main
activity does not inherit from *either* GeckoApp or BrowserApp.  This
is fantastic, since making that code serve two masters is a bad idea.

The branch point is mobile/android/{app,b2gdroid}.  The current "app"
really means "fennec", and perhaps it could evolve to that name.

The fact that too much of Fennec is built in base/ right now can
evolve.  Over time, we can extract geckoview/ and move some of the
Fennec-specific pieces to app/ (or fennec/).

This argues for mobile/android/b2gdroid to not be a new product, but
instead an opt-in part of the mobile/android build.

* Opting in to Fennec's l10n process is sad-making.

Could we reasonably b2gdroid to en-US for now, and hope that we can
catch Android l10n up in the future?  If not, we will be forced to
cargo-cult a bunch of really dated l10n and packaging choices forward,
making it even harder to address them.

We absolutely should not use mobile/android/base/strings.xml.in for
b2gdroid strings.  I will do the work to make sure we can use
GeckoView as a proper library and develop b2gdroid as a proper
application.

* I can't judge how modified the underlying Gecko is.

I believe there are some significant modifications for b2g-style
Gecko, especially around xpconnect and the process model.  But those
integrations do not change the Gecko <-> GeckoView messaging channel
and integration.

This argues for mobile/android/b2gdroid to be a new product that
produces its own geckolibs.aar and geckoview.aar.  And then the Java
front-end consumes them.

* There's a lot of branding and configuration cruft here.

I can understand having channel branding, but we should pare down to
one mozconfig with no apk splits at all: produce only Nightly Android
11+ builds.

* Testing this in automation is going to be rough.

There's an argument in favour of duplicating the Fennec product: hey,
testing!  In reality, I don't think that approach will work at all.  I
think it smarter to actually invest in some GeckoView Android testing,
and then in some actual b2gdroid product testing, rather than try to
bend the existing Fennec testing to a new situation.

* nits:

** confvars.sh makes little sense:
*** The stumbler shouldn't be on.
*** Firefox Accounts profiles shouldn't be on.
*** The tab queue shouldn't be on.
*** Install tracking shouldn't be on.
*** Native devices should probably not be on.
*** ... etc

** The Android manifest should be trimmed:
*** No shared ID!
*** There should be no mention of accounts;
*** The existing providers don't make sense for a GeckoView consumer (presumably Gecko-level things manage bookmarks, pinning, history, etc).
Attachment #8629759 - Flags: review?(nalexander)
snorp, gps, glandium, blassey, mfinkle: I want to make sure nobody is cut out of this discussion.

glandium: can you comment on the general product/sub-product approach fabrice is proposing?  You know much more about packaging, etc then I do.
Flags: needinfo?(mh+mozilla)
(In reply to [:fabrice] NOT READING BUGMAIL UNTIL JULY 27 from comment #1)
> Created attachment 8629549 [details] [diff] [review]
> b2gdroid-product.patch v1
> 
> This adds a new mobile/android/b2gdroid product, with the MOZ_B2GDROID
> variable.
> I'm not especially happy how that turned out - I guess it would be better to
> make less changes to mobile/android/base.

Yes, but the changes are not so significant -- we can work around them in the existing build system, and if we can transition to a real GeckoView consumer they won't be at all necessary.

> This patch produces a working apk that can be used as a homescreen, but the
> gecko parts to bring in the b2g/ and gaia parts will be done in another
> patch.
I think comment 4 covers all I would have to say.
Flags: needinfo?(mh+mozilla)
I'll add that while I can understand that perfect is the enemy of done, there are cases like this one where when something new is added, going for the quick "done" is also setting us on the path of never fixing it to be better. There are rewards of making this a proper geckoview consumer that go beyond b2gdroid, and if b2gdroid is not made a geckoview consumer I'm afraid the situation for geckoview consumers is not going to change and that b2gdroid will never be one.
I'm not sure I can add much beyond comment #4 and comment #8. Although I will say that ~4 years ago, B2G started in more or less the same boat. Development was done out of the tree and they had to invent a lot of tooling around consuming mozilla-central externally. There was also a strong "not invented here" approach at first, with reluctance to adopt things like mach and use shell scripts instead. The implications are still felt today, as B2G has its own build system and tools that strongly overlap with those in mozilla-central but are maintained separately and thus constitute some duplication of effort. (It's worth noting that things have only managed to converge over time.) I make the argument that everyone would have been better off if everyone was under one "tent" and the new project could fix problems at the source instead of perpetually working around them and reinventing the wheel. This approach takes a lot more initial effort, but pays dividends in the long run. A rising tide lifts all ships. There is some balance where you want a new project to move fast and not be encumbered by existing procedure/overhead/complexity. If the project fails, you minimize wasted effort. But if it succeeds, then you need to keep investing into a divergent set of tools.

I guess what I want to say is I like the idea of developing this in tree and as a fully supported app. But I'm scared of the possibility that it will balloon into excessive requests for build and tools changes, which current maintainers may not have time or mandate to fix, thus jeopardizing the potential for b2gdroid's success because we're slowing you down. At this juncture, it's probably best to perform the least invasive changes to mozilla-central to enable b2gdroid to be developed mostly externally (read: as a GeckoView consumer) and we can reconsider the relationship later.
(In reply to Gregory Szorc [:gps] from comment #9)

> I guess what I want to say is I like the idea of developing this in tree and
> as a fully supported app. But I'm scared of the possibility that it will
> balloon into excessive requests for build and tools changes, which current
> maintainers may not have time or mandate to fix, thus jeopardizing the
> potential for b2gdroid's success because we're slowing you down. At this
> juncture, it's probably best to perform the least invasive changes to
> mozilla-central to enable b2gdroid to be developed mostly externally (read:
> as a GeckoView consumer) and we can reconsider the relationship later.

If we choose to develop this as a GeckoView consumer, that doesn't mean it can't live in the tree. I would argue it should still live in the tree, so that it can be developed in parallel with GeckoView, without needing to worry about keeping version dependencies in sync. It can also serve as a good regression test for GeckoView.
Mostly agree with comment 4, but with a stronger preference for getting this into the tree sooner rather than later to avoid too much divergence. Ideally we could use a common configuration for geckoview (i.e. the same geckolibs.aar and geckoview.aar as Fennec produces). Happy to work through how to get there if there are significant differences.
Hi all,

Thanks for you detailed comments! that's very helpful.

I don't understand fully what needs to be done to be a proper GeckoView consumer, but I'm willing to work with you guys to make that happen, in tree. What's sure is that we won't be using the same gecko as fennec since we need a bunch of b2g components & config (see bug 1181209).

Timeline wise, we want to be ready to ship in time for b2g 2.5, early in November. Given that we will have a long tail of things to fix after we land the base product, I'd really like the GeckoView part to be done in about a month.

Who's in?
fabrice: the try job above is a rather stripped down version of your work.  If all is good, it will produce a b2g-like Gecko, a b2gdroid omni.ja, and b2gdroid-specific geckolibs and geckoview AAR files.  The produced b2gdroid-geckoview has no changes from the Fennec-geckoview: all the changes are in the .so libraries and the produced omni.ja.

Locally, I can use the AAR files these patches produce, the b2gdroid branch at https://github.com/ncalexan/geckobrowser-gradle/tree/b2gdroid, and gradle to get a functioning org.mozilla.b2gdroid App on my device.  It seems to be working reasonably well, other than the frequent segfaults :)

I will check in on the try build tomorrow and update the branch so that you can try building it.  In the meantime, you can look at the partial reversions I pushed to try and see what you think.
Flags: needinfo?(fabrice)
fabrice: the try build succeeded and produced geckolibs and geckoview AAR files.  I've updated the b2gdroid branch of https://github.com/ncalexan/geckobrowser-gradle.  If you check it out and follow the README, you should end up with an org.mozilla.b2gdroid on your device.  Works decently here locally.

If you could confirm it's doing reasonable things -- enough to move forward with -- then we can start talking about building a b2gdroid APK in automation (without doing terrible things in mobile/android/base).
I just built from geckobrowser-gradle and this is working as expected. So +1 from me.

However, we will need to add b2gdroid specific java code that messages back & forth with gecko. In my prototype I used Messaging.jsm, GeckoEventListener and GeckoAppShell.sendEventToGecko(). How will that be possible with this setup?
Flags: needinfo?(fabrice)
(In reply to [:fabrice] Fabrice Desré from comment #17)
> I just built from geckobrowser-gradle and this is working as expected. So +1
> from me.
> 
> However, we will need to add b2gdroid specific java code that messages back
> & forth with gecko. In my prototype I used Messaging.jsm, GeckoEventListener
> and GeckoAppShell.sendEventToGecko(). How will that be possible with this
> setup?

I think Bug 1035420 adds the interface you want.  See https://github.com/mfinkle/geckobrowser/blob/master/src/com/starkravingfinkle/geckobrowser/MainActivity.java#L140 for an example.
https://hg.mozilla.org/mozilla-central/rev/936e69f7fedd
https://hg.mozilla.org/mozilla-central/rev/479b08b6995b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
You need to log in before you can comment on or make changes to this bug.