1265442 - [FlyWeb] Move FlyWeb UI implementation for Fennec to system add-on

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Justin D'Arcangelo [:justindarc]]

One pre-requisite for landing the FlyWeb feature on m-c is having all user-facing UI implemented as a system add-on. This bug is for tracking the implementation of the FlyWeb UI for Fennec.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

I'm reasonably sure bug 1260213 will need to be fixed before the system add-on architecture will work on Fennec.

Comment 2

[Justin D'Arcangelo [:justindarc]]

Started development of this outside of the repo since we may want to also distribute this separately via addons.mozilla.org:

https://github.com/flyweb/flyweb-fennec-extension

Comment 3

[Justin D'Arcangelo [:justindarc]]

Attachment: WIP bug1265442.patch

Comment 4

[Justin D'Arcangelo [:justindarc]]

Attachment: WIP bug1265442.patch

Comment 5

[Justin D'Arcangelo [:justindarc]]

Attachment: WIP bug1265442.patch

Comment 6

[Justin D'Arcangelo [:justindarc]]

Attachment: WIP bug1265442.patch

Comment 7

[Justin D'Arcangelo [:justindarc]]

Attachment: bug1265442.patch

Margaret, here is my patch for the "larch" (FlyWeb) branch that reverts the built-in UI implementation in favor of a UI implemented as a system add-on. Since this is the first system add-on for Fennec, I had to implement some logic for extracting the add-on from the APK to the dataDir so it could be discovered and installed automatically. My familiarity with the Fennec codebase is near-zero, so I expect that I'm probably doing some things wrong and look forward to your review comments to help correct them :-)

Mike, I flagged you for feedback since you pointed out some things in the desktop version of the add-on code that I was unaware of and thought you might also have some helpful input. Thanks!

Also, to clarify, this patch is intended to be applied to the *larch* branch, not m-c, which is why you'll see a bunch of stuff removed from the old UI implementation. However, a separate, "official" patch for m-c will be derived from this one after it lands on "larch".

Comment 8

[Justin D'Arcangelo [:justindarc]]

Attachment: bug1265442.patch

Removed some unneeded logging that I forgot to revert from the last version of the patch.

Comment 9

[Justin D'Arcangelo [:justindarc]]

Attachment: bug1265442.patch

Updating the patch one last time because I noticed we were incorrectly showing system add-ons in "about:addons".

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8755974 [details] [diff] [review]
bug1265442.patch

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

::: mobile/android/extensions/flyweb/bootstrap.js
@@ +62,5 @@
> +      domWindow.removeEventListener("UIReady", onLoad, false);
> +      loadIntoWindow(domWindow);
> +    }, false);
> +  },
> + 

Nit - trailing ws

@@ +68,5 @@
> +  onWindowTitleChange: function(aWindow, aTitle) {}
> +};
> +
> +function loadIntoWindow(aWindow) {
> +  menuID = aWindow.NativeWindow.menu.add({

I'm not a Fennec hacker, so I really can't say for certain - is there only ever one "window" in this context? Because if not, menuID is going to be overwritten, so removal might not work as you'd expect (unless menuID is guaranteed to be the same from window to window).

::: mobile/android/extensions/flyweb/content/aboutFlyWeb.js
@@ +52,5 @@
> +window.addEventListener("DOMContentLoaded", () => {
> +  let list = document.getElementById("flyweb-list");
> +  list.addEventListener("click", (evt) => {
> +    let serviceId = evt.target.closest("[data-service-id]").getAttribute("data-service-id");
> +    

Nit: trailing ws

Comment 11

[:Margaret Leibovic]

Comment on attachment 8755974 [details] [diff] [review]
bug1265442.patch

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

Overall, I'm happy with the direction of this system add-on, an about page seems like a good approach.

However, I'm concerned about the logic to copy files from the APK. I do not think this current approach is the right way to do this. Why do we even need to be copying files out of the APK? And why do we need to do this in Java? How do desktop system add-ons work, and why can't we just do whatever desktop is doing?

You should also get review from a build peer on the build parts, glandium should be able to help you with that.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ +775,5 @@
>          }
>  
> +        // Copy the bundled system add-ons from the APK to the data directory.
> +        try {
> +            copyFeaturesFromAPK();

This is not a good place for this... this method gets called every time the app's main activity is created, which happens a lot. Does this even need to happen more than once? Is the concern about handling updates to the add-on code?

I think a better place for this would probably be somewhere like GeckoProfile.

@@ +3058,5 @@
>  
> +    /**
> +     * Copies the /assets/features folder out of the APK and into the app's data directory.
> +     */
> +    private void copyFeaturesFromAPK() throws IOException {

This method performs disk I/O on the main thread (and where it's currently being called, during startup!). You should make sure you're doing this on a background thread.

@@ +3114,5 @@
> +     * @return null if the parents could not be created.
> +     */
> +    private File getDataFile(final String name) {
> +        final Context context = getApplicationContext();
> +        final String dataDir = context.getApplicationInfo().dataDir;

Why do you need to use the application context here? You should be able to just use the activity context (i.e. call `getApplicationInfo()` directly.

::: mobile/android/extensions/flyweb/bootstrap.js
@@ +68,5 @@
> +  onWindowTitleChange: function(aWindow, aTitle) {}
> +};
> +
> +function loadIntoWindow(aWindow) {
> +  menuID = aWindow.NativeWindow.menu.add({

Yes, there is only ever one main XUL window, although that may change in the future. However, if that day comes, a lot of things will probably need fixing, since this assumption is spread throughout our code.

Comment 12

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8755974 [details] [diff] [review]
bug1265442.patch

Flagging :glandium for feedback on the methodology for extracting system add-ons from the APK. I'm working on a revised patch to move the file copy process to a background thread and only perform the operation when the app package is updated, but wanted to check and make sure nothing else was fatally wrong before going too far. Thanks!

Comment 13

[Justin D'Arcangelo [:justindarc]]

Attachment: bug1265442.patch

Margaret: Here's an updated version of the patch that puts the file copy operations in a background thread and also only performs them on a new install or an updated build. I've also flagged :glandium for feedback on this patch as you suggested.

Also, the method for checking when to re-extract bundled system add-ons was proposed as a solution to my question on mobile-firefox-dev: https://mail.mozilla.org/pipermail/mobile-firefox-dev/2016-May/002088.html

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8758896 [details] [diff] [review]
bug1265442.patch

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

It seems to me it would be desirable for the addons manager to be able to just use files from apk:assets/features directly, but I can understand that you don't want to block on that.

::: python/mozbuild/mozbuild/action/package_fennec_apk.py
@@ +56,5 @@
>  
> +    for features_dir in features_dirs:
> +        finder = FileFinder(features_dir, find_executables=False)
> +        for p, f in finder.find('**'):
> +            add(mozpath.join('assets', 'features', p), f, False)

If you put the files in assets/features in the package, why not just put them there directly? (in mobile/android/extensions/flyweb/moz.build)

Comment 15

[Justin D'Arcangelo [:justindarc]]

(In reply to Mike Hommey [:glandium] from comment #14)

> ::: python/mozbuild/mozbuild/action/package_fennec_apk.py
> @@ +56,5 @@
> >  
> > +    for features_dir in features_dirs:
> > +        finder = FileFinder(features_dir, find_executables=False)
> > +        for p, f in finder.find('**'):
> > +            add(mozpath.join('assets', 'features', p), f, False)
> 
> If you put the files in assets/features in the package, why not just put
> them there directly? (in mobile/android/extensions/flyweb/moz.build)

Can you clarify on how to do this? I didn't think it was possible to do this directly from `moz.build` because `package_fennec_apk.py` seems to only pull certain files/folders from the staging area to be zipped up in the APK. Since we never packaged a `features` folder in the APK before, it would previously exclude it from the APK without this change.

Comment 16

[Justin D'Arcangelo [:justindarc]]

https://hg.mozilla.org/projects/larch/rev/a9283215c0028b76ec0d2e2689ddc3ad626cb4a7
Bug 1265442 - [FlyWeb] Move FlyWeb UI implementation for Fennec to system add-on

Comment 17

[Justin D'Arcangelo [:justindarc]]

Landed this on "larch". Will extract a fresh patch against m-c in Bug 1272102 for final review:

https://bugzilla.mozilla.org/show_bug.cgi?id=1272102

Comment 18

[Mike Hommey [:glandium]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #15)
> (In reply to Mike Hommey [:glandium] from comment #14)
> 
> > ::: python/mozbuild/mozbuild/action/package_fennec_apk.py
> > @@ +56,5 @@
> > >  
> > > +    for features_dir in features_dirs:
> > > +        finder = FileFinder(features_dir, find_executables=False)
> > > +        for p, f in finder.find('**'):
> > > +            add(mozpath.join('assets', 'features', p), f, False)
> > 
> > If you put the files in assets/features in the package, why not just put
> > them there directly? (in mobile/android/extensions/flyweb/moz.build)
> 
> Can you clarify on how to do this? I didn't think it was possible to do this
> directly from `moz.build` because `package_fennec_apk.py` seems to only pull
> certain files/folders from the staging area to be zipped up in the APK.
> Since we never packaged a `features` folder in the APK before, it would
> previously exclude it from the APK without this change.

AFAICT, it puts everything under assets in the apk. So as long as you put features under assets/features in the first place, it should pick it from there.