578849 - Make the context-menu API e10s-compatible

Status: RESOLVED FIXED

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

Description

[Drew Willcoxon :adw]

The context-menu API needs to be made e10s-compatible.  The goal of this bug is to redesign the API (where appropriate) and make the subsequent implementation changes to support the new API.  The goal is not to convert context-menu's implementation to use e10s; that will come later.

Comment 1

[Drew Willcoxon :adw]

Attachment: proposal 1

I'm not really happy with this, but I'm less happy with other ideas I've heard and thought of.

Comment 2

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

Comment on attachment 470129 [details]
proposal 1

>The `setupItem` global function is available in item content scripts.  It takes
>a single options object with the following properties.  [Is there a better name
>than "setupItem"?]

Hmm, good question.  One issue with "setupItem" is that the verb "set up" comprises two words (the single word "setup" is a noun), so naming conventions would have us capitalize "up", i.e. setUpItem.  But that is a fairly comPliCated name.

Another possibility would be to have users define the context and onClick functions as global functions that take an "id" parameter that can be any of the IDs of items that are using this content script, with the functions then branching as appropriate based on the argument they are passed:

    function context(id) {
      switch(id) {
        case "page-info":
          ...
        case "remove-image":
          ...
      }
    }
    function onClick(id) { ... }

For the common case of addons that add a single item, no branching based on the value of the "id" parameter would be necessary, of course.


>id {string}
>  The ID of an item created in the jetpack script.  This is the ID of the item
>  to set up.

Can you explain a bit more about the reasoning behind the "id" option?  My first thought (as demonstrated in the suggestion above) is that it is intended to enable addons to create multiple items using the same content script, but none of the examples show an addon creating multiple items, and having items share content scripts might be a bit wonky, since the interactions between items sharing content scripts might not be clear to users, and the first item to load a content script would have to cache setupItem options for items whose IDs it doesn't recognize in anticipation of those items being created in the future, i.e.:

  jetpack:

    contextMenu.add(contextMenu.Item({
      id: "page-info",
      label: "Page Info",
      onClick: function (item, url) {
        showPageInfoWindow(url);
      },
      contentScriptURL: self.data.url("content.js")
    }));

    // The content script is loaded, which caused setupItem to get called
    // twice, once with ID "page-info" and once with ID "remove-image".
    // The Page Info item must then cache the setupItem options for the Remove
    // Image item.

    contextMenu.add(contextMenu.Item({
      id: "remove-image",
      label: "Remove Image",
      context: contextMenu.SelectorContext("img")
      contentScriptURL: self.data.url("content.js")
    }));

    // The content script is reused.  The Remove Image item must now retrieve
    // its cached setupItem options from the Page Info item.

  content:

    setupItem({
      id: "page-info",
      onClick: function (contextDesc) {
        return contextDesc.document.URL;
      }
    });

    setupItem({
      id: "remove-image",
      onClick: function (contextDesc) {
        var imgNode = contextDesc.node;
        imgNode.parentNode.removeChild(imgNode);
      }
    });


>[This part isn't related to e10s, but it's a change I've been wanting to make.]

Yup, makes sense!


>URLContext(url)
>  This context occurs when the menu is invoked on a page whose URL matches the
>  given URL.  (`url` is a string?  Match pattern?  Regexp?)

This could use the same match syntax as Page Mods.


>"Page Info" item:
>
>  jetpack:
>
>    contextMenu.add(contextMenu.Item({
>      id: "page-info",
>      label: "Page Info",
>      onClick: function (item, url) {
>        showPageInfoWindow(url);
>      },

Since the item should be the "this" object in the onClick function, it shouldn't be necessary to pass it as the first argument to the onClick function.


>Content script sandboxes are "blank slates": there are no globals other than
>setupItem.  (This will make it difficult to impossible for people to use libs
>like jQuery, and make the API in this regard inconsistent with panel and
>page-mods (I think?)...)

Yeah.  I don't see how we can make the API, as currently constituted, consistent with Panel and Page Mods, which both load content scripts that each access a single page and whose lifetimes are similar to those of the pages they access, whereas this API makes Context Menu load content scripts that each access potentially many different pages and whose lifetimes are those of the items, not the pages.

An alternative API that would be similar to Panel and Page Mods and would allow the loading of jQuery would be to load content scripts only after an item has been clicked and inject the window, document, and node objects into the global scope of the symbiont in which the content scripts are loaded.  The Page Info example would then become:

  jetpack:

    contextMenu.add(contextMenu.Item({
      label: "Page Info",
      contentScriptURL: self.data.url("content.js"),
      onMessage: function (url) {
        showPageInfoWindow(url);
      },
    }));

  content:

    item.sendMessage(document.URL);

The API could still allow users to specify an onClick handler instead of contentScriptURL/onMessage for cases that don't require access to the content process.  The only limitation this would impose is that it would no longer be possible to specify a function context.  Which would suck.

But perhaps there is another solution for that, like a special expressionContext option that accepts an expression to be evaluated in the content process and whose truthiness determines whether or not to show the item?  I'm not sure whether a simple expression is sufficiently powerful, but it's certainly the only way I've used a context function to date, and it is how the Download Images example uses a context function (i.e. it simply returns |!!contextDesc.document.querySelector("img")|).

Alternately, the regular "context" option could take an ExpressionContext constructor that specifies the expression, making the Download Images example look like this:

  jetpack:

    contextMenu.add(contextMenu.Item({
      label: "Download Images",
      context: contextMenu.ExpressionContext('!!document.querySelector("img")'),
      onMessage: function (imgSrcArray) {
        downloadImages(imgSrcArray);
      },
      contentScriptURL: self.data.url("content.js")
    }));

  content:

    var imgNodes = document.querySelectorAll("img");
    var imgSrcArray = imgNodes.map(function (node) node.src);
    item.sendMessage(imgSrcArray);

We could even make jQuery available to expression contexts.

I don't know whether or not this alternative API is better, but it's something to think about.

Either way, this is the right general direction, we just need to insert the devils into (or extract the devils from?) the details.

Comment 3

[Drew Willcoxon :adw]

(In reply to comment #2)
> Hmm, good question.  One issue with "setupItem" is that the verb "set up"
> comprises two words (the single word "setup" is a noun), so naming conventions
> would have us capitalize "up", i.e. setUpItem.  But that is a fairly
> comPliCated name.

initItem?

> Can you explain a bit more about the reasoning behind the "id" option?

Hmm, I think I should explain the whole cache thing.  Here's how I envision what happens when the user invokes the context menu:

The context menu originates in the content process.  The content process runtime checks its cache to see whether any menu items should be shown given the current context.  This cache contains info about all the items added by all consumers of context-menu.  It contains info about items added by one add-on and info about items added by another add-on.  For those items that should be added, it grabs their labels and onClick handlers, which are also stored in the cache entries, so it can build the XUL DOM nodes and add them to the menu.

Basically such a cache avoids the problem of the content process's having to call into the jetpack process when the menu is shown, asking "which menu items should I show now?"  (Instead, "tell me upfront so I don't have to ask you later.")  That was one of bsmedberg's concerns.

Now, some parts of a menu item are initialized in the jetpack script (label, declarative context) and other parts in the content script (function contexts, content onClick handlers).  The content process runtime needs to be able to map all these parts to the same menu item, so that when you create an item in the jetpack process and send it across to the content process, the content process knows which function context and content onClick handler to associate with it.  If all parts of the item could be created in either jetpack or content, then an ID wouldn't be necessary.  Practically speaking though, I don't see how you could not need a content script, since you need to do something based on the page when the user clicks your item.

> and the first item to load a content script would have to cache setupItem
> options for items whose IDs it doesn't recognize in anticipation of those
> items being created in the future, i.e.:

It's true that if multiple items share the same content script, the same item info would be cached multiple times by the content process runtime, but is that really a problem?  The implementation either wouldn't re-cache an entry it's already cached, or it would just overwrite the old entry.  Alternatively, a single content script could be specified per the module singleton, not per item, but of course that wouldn't allow people to use multiple content scripts at all (and so is a worse idea I think).

> Yeah.  I don't see how we can make the API, as currently constituted,
> consistent with Panel and Page Mods, which both load content scripts that each
> access a single page and whose lifetimes are similar to those of the pages they
> access, whereas this API makes Context Menu load content scripts that each
> access potentially many different pages and whose lifetimes are those of the
> items, not the pages.

Along these lines were my thoughts on whether context-menu should be folded into page-mods or behave like page-mods.  How about this?

Item({
  id: "foo",
  include: ["http://example.com/*"],
  context: SelectorContext("img"),
  contentScriptURL: [self.data.url("content.js"), self.data.url("jQuery.js")],
  // ...
});

Here, content scripts behave just like content scripts in page mods: they're loaded when the item (page-mod) is created (right?) and when a page is loaded that matches the `include`; and window, document, etc. of the matching pages are injected into the scope of their symbionts, so jQueries can be used transparently.

Function contexts can still live in the content scripts.

There are a few things I like about `include`:  1) It forces people to recognize that the content context menu is for content, for pages, not for tasks unrelated to content.  (Although a match pattern of "*", i.e., all pages, must be legal too.)  2) It plants the idea that you can modify the context menu depending on what page the user is viewing, and some interesting use cases arise from that idea.  3) It's consistent with page-mods where that consistency makes sense.

One thing I don't like is that it treats what I had called the "URLContext" separately from the other Contexts, but maybe it is worth considering separately.  Anyway, I don't think that's a big deal.

Setting aside for now the details of how the jetpack and content scripts communicate, am I missing any obvious problems?  I'm probably missing some obvious problems.

Comment 4

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

> It's true that if multiple items share the same content script, the same item
> info would be cached multiple times by the content process runtime, but is that
> really a problem?  The implementation either wouldn't re-cache an entry it's
> already cached, or it would just overwrite the old entry.

Hmm, as I think about this more, I find it difficult to reason about it, but here's my best shot...

So, there are content scripts and content symbionts, and there isn't a one-to-one relationship between them, since you can load multiple content scripts into the same symbiont (f.e. jQuery and your own script that sets up/initializes a context menu item).

This also implies that you can load overlapping but not identical sets of content scripts for two items, i.e.:

  main.js:

    contextMenu.add(contextMenu.Item({
      id: "page-info",
      label: "Page Info",
      onClick: function (item, url) {
        showPageInfoWindow(url);
      },
      contentScriptURL: self.data.url("content.js")
    }));
  
    contextMenu.add(contextMenu.Item({
      id: "remove-image",
      label: "Remove Image",
      context: contextMenu.SelectorContext("img")
      contentScriptURL: [self.data.url("jQuery.js"),
                         self.data.url("content.js")]
    }));

The fact that the two items share a script suggests that it's possible for them to interact in that script, f.e. providing the Page Info item with a list of images removed by the Remove Image item:

  content.js:

    let pageInfoItem = {
      id: "page-info",
      onClick: function (contextDesc) {
        return { url: contextDesc.document.URL,
                 imagesRemoved: removeImageItem.imagesRemoved };
      }
    };
    initItem(pageInfoItem);
  
    let removeImageItem = {
      imagesRemoved: [],
      id: "remove-image",
      onClick: function (contextDesc) {
        var imgNode = $(contextDesc.node);
        this.imagesRemoved.push(imgNode.attr("src"));
        imgNode.remove();
      }
    };
    initItem(removeImageItem);

In order for that to work, the two items would have to be loaded into the same content symbiont.  But that would mean the symbiont would load the union of the sets of content scripts specified by items.  And it wouldn't necessarily load them in the order a particular item expected.  For example, in the above example, the symbiont would load content.js first (because of Page Info), then jQuery.js (because of Remove Item), even though Remove Image specified those scripts in the reverse order.  Which seems like it could cause all sorts of trouble, since it breaks a pretty fundamental expectation of developers about the way scripts load.

The alternative is to load each item's scripts in their own symbiont, with initMenu() simply ignoring invokations for items with IDs other than its own (no caching required).  And it is fairly simple to explain as well.  But it might confound developers who think that since they specify the same script twice, it should load only once, and its code should be able to interact the way code in a single script generally can.

Perhaps I'm reasoning about this wrong, but my general sense is that optimizing the API for script sharing creates a variety of thorny interface/documentation issues that are tricky to resolve or whose resolution makes the interface/documentation more complex.


> Alternatively, a
> single content script could be specified per the module singleton, not per
> item, but of course that wouldn't allow people to use multiple content scripts
> at all (and so is a worse idea I think).

Yeah, that seems pretty limiting.


> Along these lines were my thoughts on whether context-menu should be folded
> into page-mods or behave like page-mods.  How about this?
> 
> Item({
>   id: "foo",
>   include: ["http://example.com/*"],
>   context: SelectorContext("img"),
>   contentScriptURL: [self.data.url("content.js"), self.data.url("jQuery.js")],
>   // ...
> });
> 
> Here, content scripts behave just like content scripts in page mods: they're
> loaded when the item (page-mod) is created (right?) and when a page is loaded
> that matches the `include`; and window, document, etc. of the matching pages
> are injected into the scope of their symbionts, so jQueries can be used
> transparently.

Content scripts for a page mod aren't actually loaded when the page mod is created, only when a page is loaded that matches the `include` for an already-created page mod.  So a page mod that is created after a page has already been loaded will not modify that page (until the page is reloaded, anyway).

I expect that the common case will be for an addon to create a page mod on startup and keep it around for the life of the session, however, which means this limitation will have little practical impact.

Overall, this plan seems reasonable.  I'm a bit concerned about the cost of loading the content scripts on page load, before the context menu has been invoked.  We have to do that for page mods, which want to modify the page being loaded, but ideally we'd delay loading context menu items until actual menu invokation.

Nevertheless, that is mostly an implementation detail that we could fix later (modulo developers coming to rely on the early loading behavior).


> There are a few things I like about `include`:  1) It forces people to
> recognize that the content context menu is for content, for pages, not for
> tasks unrelated to content.  (Although a match pattern of "*", i.e., all pages,
> must be legal too.)  2) It plants the idea that you can modify the context menu
> depending on what page the user is viewing, and some interesting use cases
> arise from that idea.  3) It's consistent with page-mods where that consistency
> makes sense.
> 
> One thing I don't like is that it treats what I had called the "URLContext"
> separately from the other Contexts, but maybe it is worth considering
> separately.  Anyway, I don't think that's a big deal.

Yup, these are all good points, and I agree that it isn't a big deal either way.  You could have a URLContext that takes a page-mod-style match pattern (which does permit "*", by the way), or you could have an "include" property, and either way you could still load and treat the content scripts the same way page-mod does.


> Setting aside for now the details of how the jetpack and content scripts
> communicate, am I missing any obvious problems?  I'm probably missing some
> obvious problems.

Except for the issues raised by script sharing, I don't see any obvious problems.

Comment 5

[Drew Willcoxon :adw]

Thanks Myk.

I guess I don't understand content symbionts.  Your description makes it sound like there will be actual content symbiont objects or machines in the content process, each of which knows which content scripts it's running and which objects in the jetpack process it's hooked up to, rather than everything running in a shared content process runtime, which is what I've been assuming.

Looking at the sketch you posted to the group again now, I see that's the case.  OK.  So that's why item IDs and my caching model didn't make sense to you: they aren't needed in such a world.

I see the light!

> Perhaps I'm reasoning about this wrong, but my general sense is that optimizing
> the API for script sharing creates a variety of thorny interface/documentation
> issues that are tricky to resolve or whose resolution makes the
> interface/documentation more complex.

OK.  Separate content script files per menu item it is.  If we find a way later to support setting up multiple items per script, great.

> Overall, this plan seems reasonable.  I'm a bit concerned about the cost of
> loading the content scripts on page load, before the context menu has been
> invoked.  We have to do that for page mods, which want to modify the page being
> loaded, but ideally we'd delay loading context menu items until actual menu
> invokation.

Yes, I share this concern, but I reconciled it by considering that it's no worse than page mods, as you mention, and that it allows jQueries and function contexts.

The next issue is how the jetpack script and content script communicate: whether by send/postMessage like panel, or by what we've discussed earlier, where content onClick's and the function context's return values are meaningful.

Comment 6

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

(In reply to comment #5)
> The next issue is how the jetpack script and content script communicate:
> whether by send/postMessage like panel, or by what we've discussed earlier,
> where content onClick's and the function context's return values are
> meaningful.

One is more consistent with other APIs, while the other is more optimized for expected use cases.  Either one seems reasonable, and I haven't been able to come up with any good reasons for preferring one over the other.  Do you have any preference?

Comment 7

[Drew Willcoxon :adw]

Working on patch, no idea if good, looks like this:

Item({
  label: "Edit Image",
  include: ["*"],
  context: SelectorContext("img"),
  contentScript: '
    on("click", function (contextClickedNode) {
      postMessage(contextClickedNode.src);
    });
  ',
  onMessage: function (imgSrc) {
    editImage(imgSrc);
  }
});

Item({
  label: "Edit Mozilla Image",
  include: ["*"],
  contentScript: '
    on("context", function (contextClickedNode) {
      return contextClickedNode.localName == "img" &&
             /mozilla/.test(contextClickedNode.src);
    });
    on("click", function (contextClickedNode) {
      postMessage(contextClickedNode.src);
    });
  ',
  onMessage: function (imgSrc) {
    editImage(imgSrc);
  }
});

Looking at Jetpack's event impl, I see that the return values of event listeners are not meaningful, so relying on fn's return value in on("context", fn) might be weird.

Comment 8

[Drew Willcoxon :adw]

There is no onClick on the Item, because there is no legitimate use of the content context menu that does not need to access the page or its properties.  Clicks are handled in content, where, if the action to be performed cannot be handled in content, content data is collected and postMessage()ed to jetpack.

Comment 9

[Drew Willcoxon :adw]

Attachment: patch

Haven't updated the test yet (but things seem to work in my toy examples).  Will attach separate patch for that at a later date since it will be big.

Haven't switched over to a context object system mentioned in the attached proposal (e.g., PageContext(), SelectorContext(), etc. instead of strings meaning selector contexts, etc.).  I'd still like to do that, but I think it'd be better to focus on that in its own bug, which I'll file after this one lands.

Haven't implemented page-mod's `include` property as we had discussed.  I'd still like to do that too -- same goes here.  Right now items are "applied" to all pages, and workers are created for every pair of content-windows and items.

The updated context-menu.md and inline comments hopefully explain a lot...

Workers are created on DOMContentLoaded.

Content scripts only pertain to top-level items.  If you define a content script on a sub-item, it's ignored.  This means if you create a menu and want to be notified of clicks on sub-items, you have to define a content script on the menu that listens for clicks.  You can tell which sub-item was clicked because your click listener is passed that sub-item's `data`.  Originally this behavior was an accident, but I like it.  It's thrifty.  But maybe there are bad consequences I haven't thought of?

Comment 10

[Drew Willcoxon :adw]

(In reply to comment #9)
> Haven't updated the test yet (but things seem to work in my toy examples). 
> Will attach separate patch for that at a later date since it will be big.

Er, to be clear, I'll attach it to this bug, to land together with this patch.

Comment 11

[Drew Willcoxon :adw]

Attachment: test patch

Comment 12

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

Maybe my eyes are tired (or you are just awesome), but I don't see a single problem with these patches, not even a grammar nit, despite their large size.  Excellent!

One issue I do notice, however, is that some tests fail on Firefox tip.  Some tests fail on Firefox tip without the patches too, and it's cumbersome to compare the output from test runs with and without the patches, but it does look like there are additional failures with the patches that don't show up without them.

Which gives me mixed feelings, because on the one hand I want tests to pass on tip.  On the other hand, there is a separate major deliverable on the 0.9 plan to get tests passing on tip, and one could argue that it is the job of the owner of that deliverable to make that happen, while folks landing other patches continue to test against Firefox 4.0b6 (where all tests pass) until that happens.

Drew: perhaps you could take a quick look and see if there's an obvious reason why additional tests are failing on tip?  The first failure is an exception triggering a timeout, which definitely doesn't occur without the patches:

error: An exception occurred.
Traceback (most recent call last):
  File "resource://addon-kit-addon-kit-lib/context-menu.js", line 939, in CMP_handleEvent
    this.__proto__.handleEvent.call(this, event);
  File "resource://addon-kit-addon-kit-lib/context-menu.js", line 777, in Popup_handleEvent
    this.window.fireClick(clickedItem, contextObj, clickedItem.data);
  File "resource://addon-kit-addon-kit-lib/context-menu.js", line 646, in BW_fireClick
    let worker = this.workerReg.find(contextObj.window, item);
  File "resource://addon-kit-addon-kit-lib/context-menu.js", line 460, in WR_find
    let winKey = this._winKey(contentWin);
  File "resource://addon-kit-addon-kit-lib/context-menu.js", line 524, in WR__winKey
    return win.document.URL;
TypeError: win is null
error: TEST FAILED: test-context-menu.testContentContextArgs (timed out)

Comment 13

[Drew Willcoxon :adw]

Thanks Myk!  I've only run the test so far on OS X with 3.6 and the nightly, and isolated-ly at that, where it passes.  It's definitely my responsibility to make sure it passes before landing (until such time that I give up and gladly force responsibility onto someone else), so I'll take a look.  The failure is actually kind of troubling because I think it may suggest a real bug in the patch.

Comment 14

[Drew Willcoxon :adw]

Myk, could you attach a console dump of the test suite run with the -v option?

I can't reproduce.  On OS X with 3.6 and the nightly, I see similar context-menu errors about "win is null".  test-tabs closes a tab inside an onReady handler triggered by that very tab, which apparently is bad, because it causes the defaultView of this tab's document inside my DOMContentLoaded listener to be null -- the doc has no window.  (Wrapping the close inside a timeout of 0ms fixes the errors.)  While I don't understand how this could cause your error, your error is consistent with a document having no window.

On Linux with the nightly, naturally I get an entirely different set of errors, but no context-menu errors.

One thing I didn't understand until just now is why the context menu code is running at all during other tests.  The patch doesn't remove the DOMContentLoaded listener on the tabbrowser after the module is unloaded -- that's a bug, it should -- but fixing that didn't change anything.  Then I realized that the test harness probably still has an instance of the module loaded somewhere.  But, the context-menu test only uses sandboxed loaders that it destroys after use -- except in one place, where the test nakedly requires context-menu to check that the host app is Firefox.  When I remove that, the test stops dumping my debugging info after it finishes.

Maybe this is one cause of tests clobbering each other.  Lots of our APIs add event listeners to windows and things like that.  Maybe we should require tests to always used sandbox loaders, or modify the test harness to use them automatically if possible.  (But on the other hand, tests clobbering each other often reveals real bugs...)

Comment 15

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

Attachment: console log for Linux test run

Here's the output from |cfx test -b ~/Applications/firefox-trunk/firefox -F context-menu -v > ~/578849.log 2>&1| in packages/addon-kit/ on Linux (Ubuntu 9.10) with the latest nightly trunk build of Firefox and the patches applied.

Note that I'm only running context-menu tests, so these failures are not due to interactions with the tests for other modules (although, as you note, there may well be additional failures from such interactions).

Comment 16

[Drew Willcoxon :adw]

Oh snap, I wonder if it's because of bug 383930, which landed in August.  A null doc.popupNode would definitely cause that error.

Comment 17

[Drew Willcoxon :adw]

From bug 383930 comment 3:

> This patch adds popup.triggerNode which is valid while the popup is open.
> document.popupNode may still be and is cleared when the popup is closed to
> prevent the leak in bug 552341. Embedding cases might still leak if they set
> popupNode, but I don't think that is much of an issue.

That would do it.  The failure happens after an item in the menu is clicked and the popup is closed.

And for some reason I can now reproduce in Linux after not being able to.  Without the e10s patch applied, there are context-menu errors on the nightly consistent with bug 383930's change.  But I still can't reproduce on OS X...

Comment 18

[Drew Willcoxon :adw]

Attachment: patch 2

The only substantive changes this makes from the previous is that BrowserWindow.prototype.destroy removes the DOMContentLoaded listener on the gBrowser, as it should, and BrowserWindow.prototype.handleEvent guards against a null event.target.defaultView in the DOMContentLoaded listener to prevent the failures mentioned in comment 14.

Comment 19

[Drew Willcoxon :adw]

Attachment: popupNode fixes

These are the changes to accomodate bug 383930.  For me this fixes the error you reported, Myk.  Attaching as a separate patch to hopefully make them easier to review.  Or if you want to fix this in a separate bug that's fine with me too.

Basically this patch stores popupNode (via contextObj) in the BrowserWindow object when the top-level context menu first opens, rather than assuming document.popupNode exists all the time.

Comment 20

[Drew Willcoxon :adw]

Attachment: test patch 2

Only change from the previous patch is that require("context-menu") is no longer used to check if the app supports context-menu to avoid keeping an instance of the module around when other tests run.

Comment 21

[Drew Willcoxon :adw]

Attachment: popupNode fixes 2

(In reply to comment #19)
> Basically this patch stores popupNode (via contextObj) in the BrowserWindow
> object when the top-level context menu first opens, rather than assuming
> document.popupNode exists all the time.

Actually since e10s means that contextObj ({ node, document, window }) is no longer part of the API, there's no reason to use contextObj internally.  So this patch just keeps track of popupNode, which is the node part of contextObj.  document and window can be derived from popupNode when needed.

Comment 22

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

Comment on attachment 482215 [details] [diff] [review]
popupNode fixes 2

This looks great and does indeed fix those tests.  Roxors, r=myk!

Comment 23

[Drew Willcoxon :adw]

http://hg.mozilla.org/labs/jetpack-sdk/rev/3118da53761a
http://hg.mozilla.org/labs/jetpack-sdk/rev/b24549829f09
http://hg.mozilla.org/labs/jetpack-sdk/rev/7d1c7f1a212e

Comment 24

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