1363843 - Remove Java Addons support from Fennec, Part 2

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Jan Henning [:JanH]]

+++ This bug was initially created as a clone of Bug #1361418 +++

https://dxr.mozilla.org/mozilla-central/rev/b21b974d60d3075ae24f6fb1bae75d0f122f28fc/mobile/android/chrome/content/browser.js#2251 can be removed as well

Comment 1

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

The block at http://searchfox.org/mozilla-central/rev/7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/mobile/android/chrome/content/browser.js#2251-2264 should go.  Those look like they're not invoked, but please double-check.

Comment 2

[Varun Iyer]

I would like to take up this bug. This would be my first bug so help getting started would be great. I looked at the file, and it seems to me that the add function in the menu needs to be deleted as well as the two lines: options.type = "Menu:Add";
      options.id = this._menuId;

Please let me know if I am correct and if I can get started.

Comment 3

[Jan Henning [:JanH]]

Hello and welcome,

to get started, have a look at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build for how to build Firefox for Android, and http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html / http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html for how to submit patches.

As for the code that needs removing, you can use https://dxr.mozilla.org/ or https://searchfox.org/ to search our whole codebase. If you do a search for loadDex (https://dxr.mozilla.org/mozilla-central/search?q=loadDex&redirect=false), you can see that nobody else is calling these two methods, so it is safe to remove them.

menu.add on the other hand *is* used by us (see e.g. https://dxr.mozilla.org/mozilla-central/search?q=nativewindow.menu&redirect=false) and this is also what add-ons are using to add additional menu entries, so this shouldn't be removed.

If you've got any further questions, feel free to ask them either here, or in #mobile on IRC (https://wiki.mozilla.org/IRC).

Comment 4

[Varun Iyer]

Here is the link to my patch: https://reviewboard-hg.mozilla.org/gecko/rev/330ce264b26d

Comment 5

[Varun Iyer]

Sorry I did not realize I was supposed to put the bug number in the commit message. I hope you are okay with the patch as is.

Comment 6

[Varun Iyer]

I have added another one with the correct info and additional patches to make it complete.

Comment 7

[Varun Iyer]

Feel free to scrap my first, less complete patch. I hope this one is right. It has the bug number and correct description to match this bug.

Comment 8

[Varun Iyer]

Attachment: Bug 1363843 - Remove Java Addons support from Fennec, Part 2

Review commit: https://reviewboard.mozilla.org/r/144340/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144340/

Comment 9

[Varun Iyer]

This is the new one: https://reviewboard-hg.mozilla.org/gecko/rev/2e918231be96

Comment 10

[Varun Iyer]

This is just to notify you to get rid of my old patch with the missing bug number and incomplete fixes.

Comment 11

[Varun Iyer]

Attachment: Bug 1363843 - Remove Java Addons support from Fennec, Part 2

Review commit: https://reviewboard.mozilla.org/r/144376/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144376/

Comment 12

[Varun Iyer]

Hi, I was wondering if you could please look at my commits so far and tell me what I else is needed to disable addons.

Comment 13

[Varun Iyer]

Comment on attachment 8872868 [details]
Bug 1363843 - Remove Java Addons support from Fennec, Part 2

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144376/diff/1-2/

Comment 14

[Jan Henning [:JanH]]

Sure, no problem.

So, to sort it all out:
- If you want to completely abort a patch, you can look at the Review Summary in Mozreview and use Close -> Discarded, but I think you've already figured that one out.
- For updating a patch however don't discard the old one and then upload a new one, as that makes it more difficult to see what has actually changed. Instead, edit your commit locally and then use "hg commit --amend" (or git commit --amend if you're using git) to update the changeset and then push it again to review. Mozreview will then figure out that you're submitting an update for an already existing patch and proceed accordingly.
- Don't use needinfo for getting a patch reviewed; instead, you can specifically ask for review. Either include the IRC nickname of the person you want to ask in the commit message (e.g. "Bug 1363843 - Remove Java Addons support from Fennec, Part 2. r?janh", or you can go to the "Review Summary" in mozreview and then enter the name in the "Reviewers" colummn - just click on the crayon to edit it.

As for the patch itself, I fear you went a bit overboard with it. This bug is *not* about removing add-ons support in general - just clearing up some remnants of the former support for Java-based add-ons on Android.

To give a little context: Firefox on Android is made up of two parts
- the browser engine ("Gecko"), which is written in C/C++/JavaScript/Rust and shared with Firefox on desktop computers
- the user interface, which is a more or less normal Android app written in Java.
The two parts mostly communicate by sending messages to each other. From JavaScript in Gecko to Android this is e.g. done by calling GlobalEventDispatcher.sendRequest(), which is what you can see in the code linked from the first comment.

In addition to normal JavaScript-based Firefox add-ons, until recently on Android we also supported add-ons written in Java. Gecko could then load such addons by using NativeWindow.loadDex() (https://dxr.mozilla.org/mozilla-central/rev/b21b974d60d3075ae24f6fb1bae75d0f122f28fc/mobile/android/chrome/content/browser.js#2251), which in turn would send a message ("Dex:Load") to the Android app side of Firefox to handle the rest. The code for that has already been removed however, i.e. on the Java-/Android app-side of Firefox nobody is listening for "Dex:Load"/"Dex:Unload" messages any more.

Therefore, these two functions (NativeWindow.loadDex() and NativeWindow.unloadDex()) are no longer necessary and can be removed, which is all that this bug is about.

Comment 15

[Varun Iyer]

Attachment: Bug 1363843 - Remove Java Addons support from Fennec, Part 2

Review commit: https://reviewboard.mozilla.org/r/144680/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144680/

Comment 16

[Varun Iyer]

Thank you for the explanation!

Comment 17

[Jan Henning [:JanH]]

Comment on attachment 8873230 [details]
Bug 1363843 - Remove Java Addons support from Fennec, Part 2

https://reviewboard.mozilla.org/r/144680/#review148918

Looks fine.

One small tip for the future: Don't necessarily just copy the bug title for the commit message. Instead, make it a short description of what your patch is actually doing.
In this case it's okay, though, as it's a small patch and the bug title is descriptive enough for our purposes.

Comment 18

[Jan Henning [:JanH]]

I've also started a test run for your patch, you can follow its progress here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c16837badec

If everything turns out okay, you can then set the "checkin-needed" keyword on this bug to get this landed. Thanks and feel free to tackle your next bug whenever you like.

Comment 19

[Varun Iyer]

Comment on attachment 8873230 [details]
Bug 1363843 - Remove Java Addons support from Fennec, Part 2

https://reviewboard.mozilla.org/r/144680/#review149070

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f1c4c291038
Remove Java Addons support from Fennec, Part 2 r=JanH

Comment 21

[Jan Henning [:JanH]]

Landed it for you - next time, just edit this bug and add "checkin-needed" in the "Keyword" field in the "Tracking" section.

Also, in this case there's no point in giving your own patch a r+.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2f1c4c291038