548590 - Implement a page context menu module

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Drew Willcoxon :adw]

Proposed here:
https://wiki.mozilla.org/Labs/Jetpack/Reboot/JEP/112

Discussed here:
http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/83cb9e8645831371

Comment 1

[Drew Willcoxon :adw]

Atul, what's the status on the separation of modules into chrome- and non-chrome-privileged?  I know we're ignoring security for now, but are we still writing modules as if the barrier exists?  The context menu module accesses content pages and chrome windows.  I really don't want to surprise you again like I did with the prototype's menu stuff.

Comment 2

[Atul Varma [:atul]]

Thanks for asking--do you think you could upload the latest version of your work (even if it's totally incomplete) as an attachment so I can take a look and comment on any potential issues that might come up during securitization?

Comment 3

[Drew Willcoxon :adw]

Attachment: WIP 1

The chrome stuff so far is in WindowHelper (and BrowserWatcher, which will be replaced by your WindowTracker).  It gets the menupopup of the chrome window's context menu, adds popup{showing,hiding} event handlers, inspects the content document via popupNode, and creates and modifies elements in the chrome window DOM.

Comment 4

[Drew Willcoxon :adw]

Does anybody object to this?

* If the number of items added by all Jetpack extensions is less
  than N, show the items in the top-level menu.  Otherwise hide
  them in a "Jetpack" submenu.  (N = 10 or 15, it could even be a
  pref.)

* Sort the items according to the user's locale.

* A menu separator separates Jetpack items (or the overflow
  submenu) from non-Jetpack items.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #4)
> Does anybody object to this?

It all makes sense to me!

Comment 6

[Drew Willcoxon :adw]

Attachment: WIP 2

Atul, this should be ready for review later this week but I wanted to get your feedback on my approach first.  I'll try to summarize.

When the module is loaded, it adds a popupshowing listener to the context menu popups of all current windows and all new windows that are opened.  That seems wasteful if the module is never requir()ed, so I think I'll add it the first time an item is added to the menu instead.

add() adds items to the menu, and the popupshowing listener determines whether to hide items or show them based on the current context.

Access to chrome windows and context documents is baked into the module.  If that's not acceptable, maybe we can work in later releases to tease out lower-level modules, since it's implementation that doesn't affect the high-level API.  If that's the case, we should probably assign someone the work of writing chrome window and content document modules that allow event listeners, element creation, etc.

Since the notion of a context menu acting on page content is specific to Firefox, is there any way to not load the module if the app is not Firefox?  Or should it throw errors when its methods are called?  If we want to extend it to other apps in later releases, we can work to do that.

This patch creates a new package called "ui".  (Is a "jetpack-" prefix really necessary?)

-----

To do:

* More tests, but I think a large majority are done.
* Implement menu separators.
* Implement sorting of top-level items by user's locale.
* Incorporate Atul's window helper module.
* Code polish, cleanup.

Comment 7

[Drew Willcoxon :adw]

I'm going to break out the Collection class into jetpack-core/collection so other people can use it and move context-menu into a new "browser" package, rather than "ui".

Comment 8

[Myk Melez [:myk] [@mykmelez]]

As I've been reviewing APIs, I recently noticed that there's some inconsistency between ways of exporting functionality, with some modules (like this context menu one) exporting individual methods and properties (and getters/setters and iterators, where applicable) while others export an explicit singleton object that contains their methods/properties, i.e.:

  let contextMenu = require("context-menu");

But:

  let storage = require("storage").storage;
  let tabs = require("tabs").tabs;

I prefer the style used in this context menu module, because it is simpler, but it seems to run afoul of the CommonJS convention to export objects with methods/properties rather than the methods/properties themselves.

The last time I spoke to Kevin Dangoor about it, he convinced me that we should follow the CommonJS convention (which I then added to the design guidelines <https://wiki.mozilla.org/Labs/Jetpack/Design_Guidelines>), although I don't remember the specific reasoning now.  cc:ing him for his thoughts.

Comment 9

[Drew Willcoxon :adw]

(In reply to comment #8)
> I prefer the style used in this context menu module, because it is simpler, but
> it seems to run afoul of the CommonJS convention to export objects with
> methods/properties rather than the methods/properties themselves.

Hmm, but the context menu module does export an object.  This object has add() and remove() and so on defined on it.  It's the object you get when you require() the module.  Another layer of indirection seems unnecessary.

The simple storage case is different.  There, the data store itself is an object, and to avoid polluting it with the properties of the module, it hangs off the object exported by the module.  That frees us up to define properties on the module if we want, like require("storage").sync(), require("storage").quota, etc.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #9)
> Hmm, but the context menu module does export an object.  This object has add()
> and remove() and so on defined on it.  It's the object you get when you
> require() the module.  Another layer of indirection seems unnecessary.

I agree.  I only brought it up because I recalled a conversation with Kevin that seemed to suggest this is problematic in the CommonJS context, although I might have misinterpreted what he was saying.


> The simple storage case is different.

Mmm, indeed.  And tabs is more akin to context menu than to storage, so I expect we can implement that the way we implement this:

  let contextMenu = require("contextMenu");
  // contextMenu now has the API's methods/properties.
  
  let tabs = require("tabs");
  // tabs now has the API's methods/properties/getters/setters/iterator.

Kevin: any issues with this approach?

Comment 11

[Drew Willcoxon :adw]

(In reply to comment #7)
> I'm going to break out the Collection class into jetpack-core/collection so
> other people can use it

bug 559169

Comment 12

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #6)
> Since the notion of a context menu acting on page content is specific to
> Firefox, is there any way to not load the module if the app is not Firefox?
> Or should it throw errors when its methods are called?  If we want to extend
> it to other apps in later releases, we can work to do that.

The xul-app module <http://hg.mozilla.org/users/avarma_mozilla.com/atul-packages/file/tip/packages/misc/lib/xul-app.js> helps figure out what application a module is running in, which this code could use to make that determination.

That's the easy part, of course, the hard part being what to do about it.  It seems to me like the ideal solution would be for the module to throw an exception and fail to load upon being required if being run in an application that it doesn't support.  I'd like to hear Atul's thoughts on the matter, though.

Comment 13

[Atul Varma [:atul]]

Another approach to the conundrum mentioned in comment 10 may be to have module namespaces called "ui" and "ui/browser", such that we have:

var contextMenu = require("ui").contextMenu;
var tabs = require("ui/browser").tabs;

This way "ui" could basically be a module that just imports other modules and attaches their exports to itself, e.g. the code of ui.js might be:

  exports.contextMenu = require("xul/context-menu").contextMenu;

And whatnot.

Also, I just posted to the Jetpack mailing list about something related to this:

http://groups.google.com/group/mozilla-labs-jetpack/msg/d0621d5ce95b2d19

Should we continue the discussion there so more folks (who don't know about the existence of this bug) can get involved?

Comment 14

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #13)
> Should we continue the discussion there so more folks (who don't know about the
> existence of this bug) can get involved?

Sure. Let's just make sure to prioritize finding an approach that works in the short term for this module so we don't block it from landing in 0.3, even if the long term solution turns out to be something different after additional discussion.

Comment 15

[Atul Varma [:atul]]

For sure, that makes sense!

Comment 16

[Drew Willcoxon :adw]

Attachment: patch v1

Ready for browser review.  Some Jetpacky things still need to be worked out: using Atul's window-utils, deciding the package this belongs to, what to do if this is loaded or its test is run by a non-Firefox app, getting bug 559169 reviewed and landed, and possibly pulling out some helpers here into their own module.

Comment 17

[Drew Willcoxon :adw]

Comment on attachment 439180 [details] [diff] [review]
patch v1

Myk, this is ready for API review too (and anything else you want to look at), with the to-dos noted in comment 16.

Comment 18

[Dietrich Ayala (:dietrich)]

Comment on attachment 439180 [details] [diff] [review]
patch v1


>+// The ID of the menu separator that separates standard context menu items from
>+// our user items.
>+const SEPARATOR_ID = "jetpack-context-menu-separator";

why id over class?

>+// The label of the overflow sub-<menu>.
>+const OVERFLOW_MENU_LABEL = "Jetpack";

add a TODO to localize this?

>+// The ID of the overflow sub-<menu>.
>+const OVERFLOW_MENU_ID = "jetpack-content-menu-overflow-menu";
>+
>+// The ID of the overflow submenu's <menupopup>.
>+const OVERFLOW_POPUP_ID = "jetpack-content-menu-overflow-popup";

ditto id vs class

>+/**
>+ * Creates a simple menu item.
>+ *
>+ * @params options
>+ *         @prop label
>+ *               The item's label.
>+ *         @prop data
>+ *               An optional arbitrary string to associate with the item.

Brief use-case? also, do you enforce strings only, or just limited to scalar values? are objects stringified, or exception thrown?

>+ *         @prop onCommand
>+ *               An optional function that will be called when the item is
>+ *               clicked.  It is called as onCommand(contextObj, data).
>+ *               contextObj is an object describing the context in which the
>+ *               context menu was invoked.  data is the item's data property.

Can you describe what contextObj looks like?

>+ *         @prop context
>+ *               If the item is added to the top-level context menu, this
>+ *               specifies the context under which the item will appear.  If
>+ *               undefined, the page context is assumed.  Ignored if the item is
>+ *               contained in a submenu.

Can you enumerate the possible values for context in the comment?

>+  this.toString = function () {
>+    return '[object Item "' + self.label + '"]';
>+  };

the self const seems unnecessary, could use options.label?

>+}
>+
>+/**
>+ * Creates an item that expands into a submenu.
>+ *
>+ * @params options
>+ *         @prop label
>+ *               The item's label.
>+ *         @prop items
>+ *               An array of items that the submenu will contain.
>+ *         @prop onCommand
>+ *               An optional function that will be called when any of the item's
>+ *               descendants is clicked. (The commands of descendants are
>+ *               invoked first, in a bottom-up, bubbling manner.)  It is called
>+ *               as onCommand(contextObj, data).  contextObj is an object
>+ *               describing the context in which the context menu was invoked.
>+ *               data is the data property of the item that was selected or
>+ *               undefined if the selected item has no data.
>+ *         @prop context
>+ *               If the item is added to the top-level context menu, this
>+ *               specifies the context under which the item will appear.  If
>+ *               undefined, the page context is assumed.  Ignored if the item is
>+ *               contained in a submenu.

ditto per the Item comment.

>+ */
>+function Menu(options) {
>+  const self = this;

re self, ditto per the Item comment.

>+  // TODO: Add setters for these.  Updating label and items would require
>+  // finding this menus's DOM element updating it as well.
>+  this.__defineGetter__("label", function () options.label);
>+  this.__defineGetter__("items", function () (options.items || []).slice(0));
>+  this.__defineGetter__("onCommand", function () options.onCommand);

hum, maybe have a setter for the element-creator to pass the dom element to?

>+/**
>+ * Creates a menu separator.
>+ *
>+ * @params options
>+ *         @prop context
>+ *               If the item is added to the top-level context menu, this
>+ *               specifies the context under which the item will appear.  If
>+ *               undefined, the page context is assumed.  Ignored if the item is
>+ *               contained in a submenu.
>+ */

ditto re context comment

>+// Returns true if val is an array.
>+function isArray(val) {

>+function makePublicConstructor(privateCtor) {

>+function validateOptions(options, requirements) {

per IRC, ideally these things would move to a separate utils package. please file a bug or add a TODO for this.

>+// Keeps track of all browser windows.
>+let windowManager = {

hrm, this and the BrowserWatcher stuff that isn't in window-utils should probably be made module-agnostic and be in it's own browser-namespaced package. it's going to be reproduced all over otherwise. anything that needs to augment/track UI or do work across all current and future windows will need to use this same pattern. hm, looking further, this even includes the BrowserWindow bit.

no action now, but we should figure it out once this gets landed and you've converted over to window-utils.

>+// is a <menupopup> that represents the popup in the DOM, and window is the
>+// BrowserWindow containing the popup.  The popup is responsible for creating
>+// and adding items to it and handling command events.

what's "it" in that last sentence?

>+  browserWatcher.onClose = function (doc, window) {
>+    windowManager.unregisterWindow(window);
>+  };
>+})();

from BrowserWatcher definition to here all seems like it should be grouped with the windowWatcher stuff. maybe move all that stuff down to the bottom with this?

<still need to go through the popup and test code>

Comment 19

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 439180 [details] [diff] [review]
patch v1

This API implementation looks great and is true to the context menu API we'd previously specified.  design-review+!

There are two issues that came up as I was reading through the code that we should consider, however:

1. naming of onCommand property

The "command" event is conventional within Mozilla code, but it isn't used on the web, where "click" and variants of "activate" are more common.

For example, the DOM 3 events spec <http://www.w3.org/TR/2003/NOTE-DOM-Level-3-Events-20031107/DOM3-Events.html> defines the "DOMActivate" event as occurring when "an element is activated, for instance, using a mouse device, a keyboard device, or a voice command,".

Wikipedia's DOM events page <http://en.wikipedia.org/wiki/DOM_events> similarly describes it as "Similar to XUL command event. Fires when an element is activated, for instance, through a mouse click or a keypress."

And IE supports an equivalent "onactivate" event.

However, this message from the W3C WebApps WG to the CSS WG <http://lists.w3.org/Archives/Public/www-dom/2010JanMar/0004.html> makes a good case for deprecating "DOMActivate" in favor of "click", which is broadly understood on the web to mean "activate via mouse, keyboard, or other device."

And the latest draft of the DOM 3 Events Specification <http://dev.w3.org//2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html> does exactly that, deprecating "DOMActivate" and encouraging implementers to use "click" instead.

Code comments in the patch also support the ubiquity of "click", describing Item.onCommand as "an optional function that will be called when the item is clicked" and Menu.onCommand as "an optional function that will be called when any of the item's descendants is clicked."

Therefore, it seems like we should rename this property "onClick".


2. passing data vs. instance

onCommand handlers are currently passed a context object and the value of the data property for the item that was clicked.  It occurs to me that the API would be both simpler and more flexible if they were passed the instance of the Item object rather than the value of its data property.

The API would be simpler because, for the common case in which an addon's items' labels are unique, the addon wouldn't have to supply data to distinguish between them, just as the "value" attribute of HTML <option> elements is optional when their labels are sufficient to identify them.

And it would be more flexible because, if we later add other properties to Item that handlers want to access (or if handlers want to access existing Item properties for some use case we haven't anticipated), they could easily do so without our having to add parameters to the handler function calls.

It isn't strictly necessary to do this to support those use cases, since addons can implement instance retrieval themselves by caching instances in a collection indexed by the values of their data properties and then looking them up in their handler functions.  But it does seem like a useful optimization.

Comment 20

[Drew Willcoxon :adw]

(In reply to comment #18)
> >+// The ID of the menu separator that separates standard context menu items from
> >+// our user items.
> >+const SEPARATOR_ID = "jetpack-context-menu-separator";
> 
> why id over class?

There's only one of these separators per context menu.  Or maybe I don't get what you mean?

> >+// The ID of the overflow sub-<menu>.
> >+const OVERFLOW_MENU_ID = "jetpack-content-menu-overflow-menu";
> >+
> >+// The ID of the overflow submenu's <menupopup>.
> >+const OVERFLOW_POPUP_ID = "jetpack-content-menu-overflow-popup";
> 
> ditto id vs class

Only one of each of these too.

> >+/**
> >+ * Creates a simple menu item.
> >+ *
> >+ * @params options
> >+ *         @prop label
> >+ *               The item's label.
> >+ *         @prop data
> >+ *               An optional arbitrary string to associate with the item.
> 
> Brief use-case? also, do you enforce strings only, or just limited to scalar
> values? are objects stringified, or exception thrown?

Yeah, hmm... it's no problem to flesh out the comments of all these exported functions, but since they will be documented in two places -- the JEP and the pretty user-facing docs -- do you think it's worth repeating in the code?

> >+  // TODO: Add setters for these.  Updating label and items would require
> >+  // finding this menus's DOM element updating it as well.
> >+  this.__defineGetter__("label", function () options.label);
> >+  this.__defineGetter__("items", function () (options.items || []).slice(0));
> >+  this.__defineGetter__("onCommand", function () options.onCommand);
> 
> hum, maybe have a setter for the element-creator to pass the dom element to?

A setter defined on the item wouldn't work, since we don't want clients to be able call it.  I'm thinking that it'll have to start a search at the ContextMenuPopup and work down into sub-popups, looking through all the itemWrappers to find the right element.

> >+// Returns true if val is an array.
> >+function isArray(val) {
> 
> >+function makePublicConstructor(privateCtor) {
> 
> >+function validateOptions(options, requirements) {
> 
> per IRC, ideally these things would move to a separate utils package. please
> file a bug or add a TODO for this.

bug 559859

> >+// Keeps track of all browser windows.
> >+let windowManager = {
> 
> hrm, this and the BrowserWatcher stuff that isn't in window-utils should
> probably be made module-agnostic and be in it's own browser-namespaced package.
> it's going to be reproduced all over otherwise. anything that needs to
> augment/track UI or do work across all current and future windows will need to
> use this same pattern. hm, looking further, this even includes the
> BrowserWindow bit.

window-utils does everything BrowserWatcher does and more, so I think we're OK there.   window-utils lets people implement the pattern you describe.  windowManager sounds like it could be module-agnostic but it's actually pretty specific to this module.  It's just a window-utils consumer (or will be once I update the patch).  Similar for BrowserWindow.

> from BrowserWatcher definition to here all seems like it should be grouped with
> the windowWatcher stuff. maybe move all that stuff down to the bottom with
> this?

BrowserWatcher will be replaced with WindowTracker from window-utils.  Actually it might still exist, since WindowTracker tracks all windows, not just browser windows, but it will become a simple consumer of WindowTracker.  A diagram of who will be calling whom might look like:

WindowTracker (window-utils) -> BrowserWatcher (probably renamed to BrowserTracker) -> windowManager (maybe renamed to browserManager)

Comment 21

[Drew Willcoxon :adw]

(In reply to comment #19)
Thanks Myk, great ideas.  I'll update the patch and JEP.

data vs. instance will be a little tricky in the bubble-to-Menu case, since the Menu won't have a handy reference to the clicked Item unless the Item is a child.  I can either start a search at the Menu, looking through all descendants, to find the Item that matches the element, or I can just stick a JS property on each element that refers to the Item.  Probably will do the latter, but it makes me worry about the potential for memory leaks.

Comment 22

[Dietrich Ayala (:dietrich)]

(In reply to comment #20)
> There's only one of these separators per context menu.  Or maybe I don't get
> what you mean?

nm, my misunderstanding.

> > Brief use-case? also, do you enforce strings only, or just limited to scalar
> > values? are objects stringified, or exception thrown?
> 
> Yeah, hmm... it's no problem to flesh out the comments of all these exported
> functions, but since they will be documented in two places -- the JEP and the
> pretty user-facing docs -- do you think it's worth repeating in the code?

the allowable types, yes. the use-case is fine in the docs.

> > >+  // TODO: Add setters for these.  Updating label and items would require
> > >+  // finding this menus's DOM element updating it as well.
> > >+  this.__defineGetter__("label", function () options.label);
> > >+  this.__defineGetter__("items", function () (options.items || []).slice(0));
> > >+  this.__defineGetter__("onCommand", function () options.onCommand);
> > 
> > hum, maybe have a setter for the element-creator to pass the dom element to?
> 
> A setter defined on the item wouldn't work, since we don't want clients to be
> able call it.  I'm thinking that it'll have to start a search at the
> ContextMenuPopup and work down into sub-popups, looking through all the
> itemWrappers to find the right element.

that could mean lots of redundant and synchronous work... maybe hash of objects -> elements in this scope, but that's a little dodgy. doesn't matter yet, i guess.

> window-utils does everything BrowserWatcher does and more, so I think we're OK
> there.   window-utils lets people implement the pattern you describe. 
> windowManager sounds like it could be module-agnostic but it's actually pretty
> specific to this module.  It's just a window-utils consumer (or will be once I
> update the patch).  Similar for BrowserWindow.

ok, i'll look at it again once you make the conversion. building out the other modules will flex this stuff more, so we can figure it out then.

> > from BrowserWatcher definition to here all seems like it should be grouped with
> > the windowWatcher stuff. maybe move all that stuff down to the bottom with
> > this?
> 
> BrowserWatcher will be replaced with WindowTracker from window-utils.

[...]

oh, i was just making nit about where in the file these things were :) went <window code> <popup code> <more window code>.

Comment 23

[Drew Willcoxon :adw]

Atul, what package should this belong to?  Comment 13 sounds tentative, but should we do that?  And what should happen in non-Firefox apps, at least for 0.3?

Comment 24

[Drew Willcoxon :adw]

Attachment: patch v2

Addresses comment 18, comment 19, and comment 22.  Also uses window-utils and includes docs.  Except for addressing future review comments and finalizing the module's package, this should be ready to go.

(In reply to comment #22)
> that could mean lots of redundant and synchronous work... maybe hash of objects
> -> elements in this scope, but that's a little dodgy. doesn't matter yet, i
> guess.

Yeah, I've thought about that too.  Actually these patches are already doing linear searches per popup through lists of itemWrappers, e.g. to find the right DOM element to remove given an item in the popup.  That could be optimized too.

Comment 25

[Drew Willcoxon :adw]

Attachment: patch v2.1

Removes an unnecessary conditional in Popup.handleEvent, doesn't attach an Item to each DOM element but instead stores the last-clicked Item (Popup.handleEvent), and makes the page context a little smarter by excluding selected text (BrowserWindow._isPageContextCurrent).

Comment 26

[Dietrich Ayala (:dietrich)]

some minor doc comments:

>diff --git a/packages/browser/docs/context-menu.md b/packages/browser/docs/context-menu.md
>new file mode 100644
>--- /dev/null
>+++ b/packages/browser/docs/context-menu.md
>@@ -0,0 +1,287 @@
>+The `context-menu` module provides access to the browser's page context menu.

beginner's-mind nit: s/provides access/allows you to add items/

imo should do s/item/menu item/ throughout the docs too.

>+      was invoked.  See Examining Contexts below for details.  <em>item</em>
>+      is the item itself.

can you internal-link to Examining Contexts? ditto for the Specifying Contexts references.

[update: saw you filed a bug about this]

>+<tt>contextMenu.**add**(*item*)</tt>
>+
>+Adds an item to the context menu.  *item* is an <tt>Item</tt> or <tt>Menu</tt>.
>+<tt>Separator</tt>s can't be added to the top-level menu.

please note the behavior if it's a separator

>+<tt>contextMenu.**remove**(*item*)</tt>
>+
>+Removes an item from the context menu.  *item* must have been previously added.

please note the behavior if it's an item that doesn't exist

Comment 27

[Dietrich Ayala (:dietrich)]

Comment on attachment 439989 [details] [diff] [review]
patch v2.1

>+// TODO: Make this a user-controllable pref.  The test will have to be updated.
>+const OVERFLOW_THRESHOLD = 10;

file bug

>+BrowserWindow.prototype = {
>+
>+  // The current context description object.
>+  get contextObj() {
>+    let node = this.doc.popupNode;
>+    return {
>+      node: node,
>+      document: node ? node.ownerDocument : null,
>+      window: node ? node.ownerDocument.defaultView : null
>+    };
>+  },

under what circumstances would node be null?

>+// Register the browserManager to listen for window openings and closings.
>+// Do this only after setting prototypes and such above, because this will cause
>+// browserManager's onTrack to be called immediately if there are open windows.
>+let windowTracker = new (require("window-utils").WindowTracker)(browserManager);
>+require("unload").ensure(windowTracker);

i'd prefer that this be in an init() function for browserManager so that the dependency is clearer in the code.

>diff --git a/packages/browser/package.json b/packages/browser/package.json
>new file mode 100644
>--- /dev/null
>+++ b/packages/browser/package.json
>@@ -0,0 +1,12 @@
>+{
>+  "name": "browser",
>+  "description": "Jetpack's Firefox library.",
>+  "keywords": ["javascript", "engine", "platform", "firefox"],
>+  "author": "The Mozilla Foundation",
>+  "contributors": [
>+    "Drew Willcoxon <[e-mail]>"
>+  ],
>+  "version": "0.3pre",
>+  "license": "MPL 1.1/GPL 2.0/LGPL 2.1",
>+  "dependencies": ["jetpack-core"]
>+}

This feels weird. If the only part of the whole module that is browser-specific is the BrowserWindow object, then why is all of it in /browser?

Could you instead use the new xul-app module to no-op for non-Firefox apps for now?

(This doesn't need to block landing, we can shuffle locations later I guess)

r=me otherwise!

Comment 28

[Drew Willcoxon :adw]

(In reply to comment #27)
> >+const OVERFLOW_THRESHOLD = 10;
> 
> file bug

I actually went ahead and made it a pref, since it's easy.

> >+BrowserWindow.prototype = {
> >+
> >+  // The current context description object.
> >+  get contextObj() {
> >+    let node = this.doc.popupNode;
> >+    return {
> >+      node: node,
> >+      document: node ? node.ownerDocument : null,
> >+      window: node ? node.ownerDocument.defaultView : null
> >+    };
> >+  },
> 
> under what circumstances would node be null?

There were one or two times early during the unit test's development where popupNode was null after dispatching a contextmenu event to the page.  Pretty sure it was my error -- wasn't waiting for the page to load first, something like that.  It hasn't happened since the test started to take form, but I figure it's better to be safe.  I added a comment to that effect.

> i'd prefer that this be in an init() function for browserManager so that the
> dependency is clearer in the code.

Good idea.

> This feels weird. If the only part of the whole module that is browser-specific
> is the BrowserWindow object, then why is all of it in /browser?

I agree.  I'm thinking I'll change the package back to "ui", throw if the app isn't Firefox, and export test functions only if require("context-menu") doesn't throw.  Or I guess based on comment 13 rather than a new package a "module namespace", which I think means a "ui" subdirectory within jetpack-core/libs/.

Do I need Atul's review before landing?

Comment 29

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #28)
> Do I need Atul's review before landing?

You need review from some other Jetpack peer (i.e. Atul, Brian, or me) for just the Jetpack-specific parts (not the browser code parts, which Dietrich has already handled).  I've pung Atul asking if he can review; if he doesn't, I will.

Comment 30

[Atul Varma [:atul]]

Here's my comments... You can view a nicer version of them here:

  http://reviews.visophyte.org/r/439989/

on file: packages/browser/docs/context-menu.md line 3
> ## Constructors ##

These are great docs. Looks like we might want to file a bug to auto-generate
TOCs too, though, since some folks might want to just skip all the reference
info and go straight for the examples. This could go hand-in-hand with bug
560008.



on file: packages/browser/docs/context-menu.md line 32
>       was invoked.  See Examining Contexts below for details.  <em>item</em>

Wait, where is item in the method signature? You refer to it like it's an
argument, but it doesn't seem to be...


on file: packages/browser/lib/context-menu.js line 40
> const collection = require("collection");

For now, it might make sense to just use require("xul-app") to verify that
Firefox is being used at the beginning of this file. This way the check can be
done at import/require-time and raise a helpful error like "this module can
currently only be used with Firefox", which quickly lets developers know
what's going on, as opposed to getting more confusing behavior later in the
program's execution.

We can figure out a better solution later.



on file: packages/browser/lib/context-menu.js line 274
> // High-level Jetpack API constructors are supposed to be callable without using

Should we also document the limitation that 'instanceof' doesn't work here?



on file: packages/browser/lib/context-menu.js line 281
>     let obj = { constructor: PublicCtor, __proto__: privateCtor.prototype };

it might be useful to put a memory.track(obj, privateCtor.name) here--this way
anything that's created via the public constructor is automatically tracked
for leaks.



on file: packages/browser/lib/context-menu.js line 384
> // TODO: If other apps besides Firefox want to support the context menu in

Seems like if this is to be the interface that anything else plugs into, we
may want to store the "navigator:browser" string in here (or a method that
does the test) rather than in browserManager. Not a super high priority right
now, though.


on file: packages/browser/lib/context-menu.js line 799
>   // Does a binary search on elts, a NodeList, and returns the DOM element

This seems like the kind of intricate code that would be really good to unit
test, as it's easily-decoupled from the rest of the code and not too hard to
pass a mock/fake NodeList into.

I guess this would probably involve exporting the function (sans the closure,
which it seems independent of?) with a "_" at the beginning of the exported
name, since it's only being exported for purposes of unit testing.

Also, I'm curious how this code behaves if an item with targetLabel isn't
found.



on file: packages/browser/package.json line 3
>   "description": "Jetpack's Firefox library.",

I'm fine with this being Firefox-only for now, but we should probably ping
Andrew Sutherland about this at some point so he can figure out how easy it
might be to refactor/port this to support thunderbird and/or other XULrunner
apps.


Finally, I noticed when running 'cfx test -a firefox -x 5' that the "tracked memory objects in testing sandbox" count increases almost every iteration; it should stay stable at 2 after every run. This suggests that the sandbox objects are being leaked, but I'm not sure why--it could just be that weird bug we talked about last week, especially since the # of tracked objects doesn't *reliably* increase from one iteration to another.

Comment 31

[Drew Willcoxon :adw]

Attachment: patch v3

This patch address comment 30.  It also moves context-menu to jetpack-core, since as we discussed in today's meeting, it's not yet clear what kinds of packages or modules namespaces we'll need, given that we don't have a lot of modules yet.

(In reply to comment #28)
> I actually went ahead and made it a pref, since it's easy.

Should have mentioned that I asked Myk about it, and for now we'll use a "jetpack.PACKAGE.MODULE.PREF" form for prefs service preference names.

(In reply to comment #30)
> These are great docs. Looks like we might want to file a bug to auto-generate
> TOCs too, though, since some folks might want to just skip all the reference
> info and go straight for the examples. This could go hand-in-hand with bug
> 560008.

Thanks, filed bug 560735.

> For now, it might make sense to just use require("xul-app") to verify that
> Firefox is being used at the beginning of this file. This way the check can be
> done at import/require-time and raise a helpful error like "this module can
> currently only be used with Firefox", which quickly lets developers know
> what's going on, as opposed to getting more confusing behavior later in the
> program's execution.
> 
> We can figure out a better solution later.

That's what this patch does.  A dummy test.pass() test is run if the app isn't Firefox.

> Should we also document the limitation that 'instanceof' doesn't work here?

Noted.

> it might be useful to put a memory.track(obj, privateCtor.name) here--this way
> anything that's created via the public constructor is automatically tracked
> for leaks.

Done.

> on file: packages/browser/lib/context-menu.js line 384
> > // TODO: If other apps besides Firefox want to support the context menu in
> 
> Seems like if this is to be the interface that anything else plugs into, we
> may want to store the "navigator:browser" string in here (or a method that
> does the test) rather than in browserManager. Not a super high priority right
> now, though.

Not sure I understand how the window type factors in.

> This seems like the kind of intricate code that would be really good to unit
> test, as it's easily-decoupled from the rest of the code and not too hard to
> pass a mock/fake NodeList into.
> 
> I guess this would probably involve exporting the function (sans the closure,
> which it seems independent of?) with a "_" at the beginning of the exported
> name, since it's only being exported for purposes of unit testing.

Done.

> Also, I'm curious how this code behaves if an item with targetLabel isn't
> found.

Like the comment says, it returns the item before which to insert the new item.  So if the target isn't found, it returns the item that would have come after the target, were the target in the list.  If the target should be the last item, returns null.  I expanded the comment a little on that last point.

> on file: packages/browser/package.json line 3
> >   "description": "Jetpack's Firefox library.",
> 
> I'm fine with this being Firefox-only for now, but we should probably ping
> Andrew Sutherland about this at some point so he can figure out how easy it
> might be to refactor/port this to support thunderbird and/or other XULrunner
> apps.

Filed bug 560716.  It should be fairly easy.

> Finally, I noticed when running 'cfx test -a firefox -x 5' that the "tracked
> memory objects in testing sandbox" count increases almost every iteration; it
> should stay stable at 2 after every run.

Thanks!  This was because of a bug in the test.  (I accidentally created two TestHelpers in one of the tests and didn't free one of them.)

Comment 32

[Atul Varma [:atul]]

Comment on attachment 440386 [details] [diff] [review]
patch v3

Awesome, thanks!

The window type factors in just because different XUL apps (firefox, thunderbird, etc) hold their tab-browsers in windows that have different types; e.g. selecting 'navigator:browser' windows won't work on thunderbird, because their tab browsers are found in 'mail:3pane' windows. So it seems like either browserManager will also need to be provided on a per-app basis, or we should refactor things later a bit. I think the window type issue might also be helped by the fixing of bug 560620, perhaps.

Comment 33

[Drew Willcoxon :adw]

Oh right, yeah.  I forgot that browserManager sniffs for window type. :\  I'll update bug 560716.

Pushed:
http://hg.mozilla.org/labs/jetpack-sdk/rev/4fac73963042

A small followup after I forgot to update the overflow threshold preference name when going back to jetpack-core:
http://hg.mozilla.org/labs/jetpack-sdk/rev/8e7baeaff465

Comment 34

[Myk Melez [:myk] [@mykmelez]]

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".