modules should be able to support other applications besides Firefox



9 years ago
8 years ago


(Reporter: adw, Unassigned)



Firefox Tracking Flags

(Not tracked)




9 years ago
Currently the context menu module supports only Firefox.  That's because it makes assumptions about the kinds of contexts that exist in the application:  For example, matching a CSS selector against the node in content the user right-clicked to open the context menu makes sense for Firefox, but maybe not for other apps.  The JEP [1] was also written assuming Firefox -- or at least a browser.

But there's no reason other apps with context menus can't be supported too.  We just need to let them define the kinds of contexts that make sense for them.  The BrowserWindow class in context-menu.js encapsulates context handling -- BrowserWindow(), get contextObj(), and isAnyContextCurrent() are the relevant parts.  Other apps will either have to replace BrowserWindow, or better yet there should be some mechanism for plugging in support per app.

From the point of view of consumers, probably they should get the right context menu simply by requiring the module.  They shouldn't have to sniff the app themselves.

See bug 548590 for the initial context menu implementation.

Note that this is also the case with tab-browser.js--it assumes very specific things about Firefox that should be abstracted away somehow.

Comment 2

9 years ago
Atul reminded me that the browserManager object is also relevant, since it tracks windows of type "navigator:browser".  Ideally the module should be refactored so that everything app-specific is in one place.  I don't think that should be too hard to do.
It'd be ideal if it wasn't necessary to modify the Jetpack core to add this support. Otherwise, supporting other apps becomes much harder from the standpoint of packaging, deployment and maintenance. Eg, getting Ubiquity working on Songbird was easy... provided we could patch the core Ubiquity files.

Now that we have a module system, we should be able to do things like this... modularly!

One approach might be to provide a way for modules to override other modules. That'd could look something like: The browser-specific part of the context menu module is split out into a dependent module, that other packages could override, possibly specified in their package.json.
Yeah! Narwhal's various platform engines do this exact thing--they don't have to provide any specific metadata in package.json to do so. It's easier to show with diagrams than it is to explain:



            context-menu.js  <-- A TB-specific impl.

            context-menu.js  <-- A Firefox-specific impl.

require("ui/context-menu") contains the logic common to all apps, and it calls require("ui/implementation/context-menu") to get at the module that provides app-specific implementation details, which is assumed to export a common interface.  Then depending on whether "firefox" or "thunderbird" is cited as a dependency for one's addon, the appropriate package is included and everything "just works" when require() is called.

Narwhal actually goes further with this and provides a default "stub" implementation module for APIs, which is "overridden" by packages that have their lib directories higher in the module search path. It's clever, though I'm not sure how elegant it actually is, simply because I have a hard time explaining it to people. :P
Note that if we do go the route of Narwhal here, it's likely that this bug will be blocked by bug 556582.

Comment 6

9 years ago
That sounds like a fine solution, Atul.  Not sure I follow the last paragraph about Narwhal though -- there's a "stub" package alongside "thunderbird" and "firefox"?  Would its implementations ever do anything useful and not just, say, throw a "not implemented" exception?  Why not build the stub into the "mother" module (ui/context-menu in this case) if require("implementation") fails?
Yeah, I think that's fine--I believe Narwhal uses its technique to provide defaults where they make more sense, e.g. a default SHA1 module that's pure JS, allowing for particular engines to override it if they have special non-standard hooks into optimized implementations (for instance, narwhal's XULRunner implementation would override the default with an implementation that delegated out the work to an XPCOM component).

In other words, don't worry about the last paragraph about Narwhal--I don't think it really makes much sense in our case.  We will probably need to formalize a "module not found" exception or perhaps add a "require.has()" method that tests to see if a module exists, though--I suppose that is the only cost, whereas just adding a stub module means that no additional exception-handling logic needs to be added to the code that imports the module. If that makes any sense whatsoever.
morphing this to cover all firefox-specific modules, since the approach will likely be the same, and this is referenced in several modules now.
Summary: Context menu module should support other applications besides Firefox → modules should be able to support other applications besides Firefox
The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Version: Trunk → unspecified
Priority: -- → P2
I know eventually (soon) that we want to support Fennec in the SDK, but are any other applications planned for support still?

This bug's been dead for a year. I'm tempted to close this out if no one convinces me otherwise in the next week. :)
Hearing no objections... I'm closing this bug.

Feel free to override me if you want, or more preferably file new bugs for supporting specific applications (and/or specific modules) which can have actual actionable progress. :)
Severity: normal → enhancement
Last Resolved: 8 years ago
Resolution: --- → WONTFIX
Hi Wes, we have exactly that issue that we have to figure out how solve. I already propose to Mossop to have a brainstorming about that, we'll probably open a new bug (or reopen that one, I dunno).

You need to log in before you can comment on or make changes to this bug.