667957 - Context menu items don't appear when there's no DOM, but they should

Status: RESOLVED FIXED

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

Description

[Drew Willcoxon :adw]

There's no way to add context menu items to standalone non-DOM files.  For example, if you navigate to http://www.mozilla.org/images/template/dino.png, your menu items don't appear in the context menu, no matter whether you click on the image or outside it in the page.  Same for an ogg file.  (Interestingly, text files aren't affected.)  You should be able to add context menu items to these types of files.

The problem seems to be that DOMContentLoaded (naturally) isn't fired when there's no DOM.  context-menu creates workers on DOMContentLoaded, so no DOMContentLoaded no worker, and no worker no menu items.  I played around with observing "content-document-global-created" instead, and at least that appears to be fired for all kinds of pages.  This reminds me that the lifetime of workers, like when they're created, is not part of the API, but it really should be.

This bug was reported on the mailing list:

http://groups.google.com/group/mozilla-labs-jetpack/t/fe9b3051cf5da6e7

Comment 1

[Drew Willcoxon :adw]

Attachment: patch

This uses the content-document-global-created notification [1] rather than DOMContentLoaded.  When document.readyState != "loading", it creates a worker for that document's window.  When readyState == "interactive", the document's DOM is ready, according to my testing, and not all sub-resources have finished loading, according to [2], which sounds a lot like DOMContentLoaded.  So workers with this patch should be created at basically the same time that workers are currently created.  I can't actually use DOMContentLoaded by waiting until it's fired because images and the like don't cause it to be fired.

There are a couple of small, semi-related changes:

* Rename browserManager.windows to browserManager.browserWins to help cut
  down on potential confusion between browser chrome windows and content
  windows.

* Replace my own inner window ID getter with window-utils.getInnerId.  I
  didn't realize it existed.

[1] https://developer.mozilla.org/en/Observer_Notifications#Documents
[2] http://www.whatwg.org/specs/web-apps/current-work/multipage/dom.html#dom-document-readystate

Comment 2

[Drew Willcoxon :adw]

Comment on attachment 542655 [details] [diff] [review]
patch

My patch for bug 669475 bitrots this, so canceling review request for now.

Comment 3

[Drew Willcoxon :adw]

Attachment: patch 2

Updated for bug 669475's landing.

Comment 4

[Drew Willcoxon :adw]

Attachment: patch 3

Bug 672152 is related, and it touches the same code as this bug, so I've included its fix in this patch too.  Now browserManager is the sole object responsible for handling content windows.  That responsibility used to be awkwardly split between it and BrowserWindow.  This patch also includes a test for the aformentioned bug.

Comment 5

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

Comment on attachment 546870 [details] [diff] [review]
patch 3

+  // Stores the given content window with the manager and registers it with each
+  // top-level item's worker registry.
+  _registerContentWin: function BM__registerContentWin(win) {
+    let innerID = winUtils.getInnerId(win);
+    if (innerID in this.contentWins)
+      return;

Nit: it'd be useful to describe the rationale for this check.  When I first saw it, I was concerned that it was papering over an unknown issue.  It wasn't until I read further through the patch that I discovered it is hear to handle a known issue in a simple fashion.


Also, this patch fails to apply because of a recent change.  To fix the conflict, I just added api-utils/ to two require() calls in the patch, i.e. (as a hand-crafted patch to your patch):

    init: function BM_init() {
-     require("unload").ensure(this);
+     require("api-utils/unload").ensure(this);
--    let windowTracker = new (require("window-utils").WindowTracker)(this);
+-    let windowTracker = new (require("api-utils/window-utils").WindowTracker)(this);

r=myk

Comment 6

[Drew Willcoxon :adw]

https://github.com/mozilla/addon-sdk/commit/50c5c876e130520cf9ae8d8508346eefcfe74320