Closed Bug 1363843 Opened 7 years ago Closed 7 years ago

Remove Java Addons support from Fennec, Part 2

Categories

(Firefox for Android Graveyard :: General, enhancement)

55 Branch
All
Android
enhancement
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

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

References

Details

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

Attachments

(1 file, 2 obsolete files)

Assignee: jh+bugzilla → nobody
Mentor: jh+bugzilla
Whiteboard: [good first bug][lang=js]
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)
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)
Here is the link to my patch: https://reviewboard-hg.mozilla.org/gecko/rev/330ce264b26d
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.
I have added another one with the correct info and additional patches to make it complete.
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.
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)
Attachment #8872822 - Attachment is obsolete: true
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.
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)
Attachment #8872868 - Attachment is obsolete: true
Attachment #8873230 - Flags: review?(jh+bugzilla)
Thank you for the explanation!
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+
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
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+
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/2f1c4c291038
Remove Java Addons support from Fennec, Part 2 r=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+.
https://hg.mozilla.org/mozilla-central/rev/2f1c4c291038
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: