Remove Java Addons support from Fennec, Part 2

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: JanH, Assigned: dev.varuniyer, Mentored)

Tracking

55 Branch
Firefox 55
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Updated

2 years ago
Assignee: jh+bugzilla → nobody
Mentor: jh+bugzilla
Whiteboard: [good first bug][lang=js]
(Assignee)

Comment 2

2 years ago
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.
Flags: needinfo?(jh+bugzilla)
(Reporter)

Comment 3

2 years ago
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).
Flags: needinfo?(jh+bugzilla)
(Assignee)

Comment 4

2 years ago
Here is the link to my patch: https://reviewboard-hg.mozilla.org/gecko/rev/330ce264b26d
(Assignee)

Comment 5

2 years ago
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.
(Assignee)

Comment 6

2 years ago
I have added another one with the correct info and additional patches to make it complete.
(Assignee)

Comment 7

2 years ago
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 hidden (mozreview-request)
(Assignee)

Comment 10

2 years ago
This is just to notify you to get rid of my old patch with the missing bug number and incomplete fixes.
Flags: needinfo?(jh+bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8872822 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Comment 12

2 years ago
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 hidden (mozreview-request)
(Reporter)

Comment 14

2 years ago
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.
Flags: needinfo?(jh+bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8872868 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8873230 - Flags: review?(jh+bugzilla)
(Assignee)

Comment 16

2 years ago
Thank you for the explanation!
(Reporter)

Comment 17

2 years ago
mozreview-review
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.
Attachment #8873230 - Flags: review?(jh+bugzilla) → review+
(Reporter)

Comment 18

2 years ago
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.
Assignee: nobody → dev.varuniyer
(Assignee)

Comment 19

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

https://reviewboard.mozilla.org/r/144680/#review149070
Attachment #8873230 - Flags: review+

Comment 20

2 years ago
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/2f1c4c291038
Remove Java Addons support from Fennec, Part 2 r=JanH
(Reporter)

Comment 21

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2f1c4c291038
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.