Closed Bug 1363016 Opened 7 years ago Closed 4 years ago

Implement contextMenus.create WebExtension API method on android

Categories

(WebExtensions :: Android, enhancement, P5)

enhancement

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1595822

People

(Reporter: shatur, Assigned: shatur, Mentored)

References

Details

(Whiteboard: [triaged])

Attachments

(1 file)

Creates a new context menu item. The doc can be found at [1].

[1]. https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus/create
Blocks: 1269062
Priority: -- → P5
Whiteboard: [triaged]
To implement this method we can use legacy NativeWindow.contextmenus.add() method to add items to contextMenu. While investigating it I found that NativeWindow.contextmenu does not support checkbox and radio-buttons type items for now. 

So, we need to add that functionality to NativeWindows. I will file another bug where we can work on that.
Assignee: nobody → tushar.saini1285
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Hey Matthew,

Currently on fennec we do not support contextMenus of type checkbox, radio-button and separator. We can only add normal type context menu. I am working on adding that support to, but it seems there are some blockers.

So, I was thinking of moving forward with implementing this part (ie, adding normal type context menu) and onClicked eventListener and when we will be able to resolve above issue, then we can add those here too. Can you or any other from web-extension team please tell me if it will be alright?

Thanks!!
Flags: needinfo?(matthewjwein)
Matt might not be responding as quick anymore. I'm ni Luca who can be a bit more help from the WebExtension side. But yes, implementing a limited contextMenu support first sounds great to me! Thanks Tushar.
Flags: needinfo?(matthewjwein) → needinfo?(lgreco)
Hi :shatur,
Implementing an initial (even if limited) version of the contextMenu API sounds great to me too!

I took an initial look at the changes prototyped in the attached mozreview request and here is an initial round of feedback:

- I think that it would be better to keep the API namespaces and the file names (e.g. the JSON schema and the JS implementation) consistent to the ones that we are currently providing on Firefox Desktop (e.g. to make it easier to run an existent Firefox Desktop add-on on Firefox for Android), and in Bug 1333403 we have renamed the API namespace to `browser.menus` and turned `browser.contextMenus` into an alias of the new API namespace, which has also introduced support for importing a API namespace into another one, using the "$import" property in the menu JSON schema file as it is currently done in the schema file registered at browser level.

- we should keep track of the menu items created using the `browser.menus.create` API method and unregister all of them automatically when the related extension is shutting down (by providing the onShutdown lifecycle method in the API class, http://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/browser/components/extensions/ext-menus.js#628-638)

- we should introduces a small abstraction (e.g. similar to the way we are abstracting it in the Firefox Desktop version of this API, http://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/browser/components/extensions/ext-menus.js#366) which keep track of the properties associated to the created native context menu, forward the events to the listeners subscribed from the API implementation (e.g. when the native context menu item has been clicked), etc.

- in this set of patches that provides the `browser.menus.create` API method, we should also try to provide at least the `remove` and `removeAll` methods.

- ensure that we raise useful errors when an addon is trying to create a context menu item which is not currently supported on Android (e.g. by raising an helpful error message)

- like for the other UI elements implemented on Android (e.g. the pageAction and the browserAction), we are going to need to provide some methods to allow us to assert the status of the UI element and to fake a user interaction on it
(e.g. the BrowserAction and PageAction implementation provides `PageAction/BrowserAction.isShown(...)` and `PageAction/BrowserAction.synthetizeClick(...)`, http://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/mobile/android/modules/BrowserActions.jsm#109-128 and http://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/mobile/android/modules/PageActions.jsm#72-81)
Flags: needinfo?(lgreco)
Mentor: lgreco
Hey Luca!

Sorry, not to get back to you soon. I was little busy last week.

I am facing some problem, I am not able to add any type of context-menus. I think there is some problem due to aliasing or something else due to which create method in ext-menus.js is not reached. The error I am getting on webIDE is :

>> Error: Not implemented                        ExtensionCommon.jsm:536:11

I have debugged this error in past. I've tried various things, but I am not able to solve it this time. I've updated the patch with my new WIP patch. Can you please help me, to understand how web-extension is loaded (ie, steps followed) or how aliasing works. Or if there is any documentation I can refer to?

Thanks!!
Flags: needinfo?(lgreco)
I took a brief look at the updated patch and here is some pointers related to what I think that is still missing to get the API correctly registered in Fennec:

- in the Firefox Desktop version of the menus APIs, the implementation is split into two API module files:

  ext-menus.js (http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-menus.js), that is the part of the API implementation which runs in the main process, where the context menus are actually created and managed inside the Firefox Desktop internals

  ext-c-menus.js (http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-c-menus.js), that is the part of the API implementation which runs in the extension process (which can be the main process if the oop webextensions are not enabled, currently it is enabled by default only on Windows) and it provides the actual API properties that the extension code is going to call.

Even if Fennec is currently always running in non e10s mode (in single process mode) we should follow the same conventions (e.g. so that the API is, at least from an architecture point of view, compatible with the oop extension if they are going to be enabled on Fennec at some point).

In the Fennec WebExtensions internals, the tabs API is currently the only one that have its API implementation modules splitted into the ext-tabs.js and ext-c-tabs.js files.

Going back to the menus API, in Firefox Desktop we register the ext-c-menus.js API module here: http://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/browser/components/extensions/ext-c-browser.js#25-33

The analogous place in Fennec is the ext-c-android.js file:

- http://searchfox.org/mozilla-central/source/mobile/android/components/extensions/ext-c-android.js

The create API method should be defined in the ext-c-menus.js file (as it is in the Firefox Desktop version of this API, http://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/browser/components/extensions/ext-c-menus.js#115-117) and it is going to call the API method with the same name defined by ext-menus.js in the main process
using the context.childManager.callParentAsyncFunction helper (e.g. here is where this is done in the Firefox Desktop implementation: http://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/browser/components/extensions/ext-c-menus.js#123-125)

The following is a wiki page that provides some additional information about how an "oop-compatible API implementation" works internally: 

- https://wiki.mozilla.org/WebExtensions/Implementing_APIs_out-of-process

(But be aware that this wiki page can be out of sync with the implementation, and so always double-check it by looking at the code, which are always the best docs :-), or by discussing with me or one of the other developers which are working on the WebExtensions internals, we can help to put some light on the things that are not clear enough or that have been changes in the meantime).
Flags: needinfo?(lgreco)
Product: Toolkit → WebExtensions
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.