494651 - Add support for menus in Jetpack

Status: RESOLVED FIXED

(Mozilla Labs :: Jetpack Prototype, defect, P1)

Description

[Abimanyu Raja]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4) Gecko/20090423 Firefox/3.5b4
Build Identifier: 

We need to have simple APIs to append menu items to the context menu as well as menus in the menu bar. Later, there should also be API to create your menus and sub-menus.

Reproducible: Always

Comment 1

[Abimanyu Raja]

Here's a very buggy patch. I just wanted to put it up so that we can discuss the API. What works so far:

jetpack.menuBar.find("tools").append({label:"Jetpack this page!"})
//you can .find any of the main menus in the menu bar - file, edit, view, history, tools, help

jetpack.menuBar.find("File").append({label:"Say Hi!", command: function(){alert('hi')})
//this ^ should work but doesn't; crashes firefox for some reason

As for context menus, they don't display properly yet - 

jetpack.contextMenu.append({label:"testingurcontext"});

Comment 2

[Abimanyu Raja]

Attachment: Initial patch

Comment 3

[Aza Raskin [:aza]]

Hey Abi,

Looking great. You should start by writing a JEP so that we can discuss what the API looks like. (You can be the proud new champion of JEP 13 :)

https://wiki.mozilla.org/Labs/Jetpack/JEPs

Then we'll work through the code.

Comment 6

[Drew Willcoxon :adw]

Attachment: WIP v1

Messy, but much functionality is there.

Comment 7

[Drew Willcoxon :adw]

Attachment: WIP v2

Saving my work.  Not much progress since last time, but it now mostly cleans up my myriad event listeners in a nice way.  No progress on debugging really annoying secure membrane issues, still testing in a chrome HTML doc.  Aside from polish and correctness I won't work too much more on this for an initial review.  I'll file followup bugs that will add the remaining functionality of the JEP.

Comment 8

[Aza Raskin [:aza]]

Around what level of functionality do we have here?

Comment 9

[Drew Willcoxon :adw]

Attachment: for review v1

This contains some dumps and XXXs but is maybe OK for first review.  I don't expect it to be the final patch, but I don't expect large changes either.  There are some differences from the JEP.

Briefly, the idea is that you have a stack of transforms per menu per feature.  When the XUL popup is shown, the transforms are pushed, and when the popup is hidden, the transforms are popped (undone).  The reasoning for this is context menus:  The browser's content context menu works this way basically.  There's one XUL popup that's modified per invocation, instead of an entirely new popup per invocation.  I mimic that, and since I've already built the machinery, extend it to all menus.

The patch supports:

- content context menus per CSS selector, e.g., menu.context.page.on("img")...
- menu bar menus
- popup menus on arbitrary nodes in content and chrome
- beforeShow and beforeHide methods on menus
- icons in menuitems
- misc stuff

It doesn't support:

- context menus on feature content like panels, slidebars, etc.
- the chrome context menus
- key shortcuts, mnemonics
- radio groups, checkbox menuitems
- some scheme for "Jetpack-friendly" menuitem IDs

I still have problems with DOM vis-a-vis secure membranes (see bug 517803), so I haven't been able to test in a real feature.  I've been using an HTML file in chromespace to test, and it's included.

Comment 10

[Aza Raskin [:aza]]

So to double check, this patch does not yet include

Comment 11

[Aza Raskin [:aza]]

So to double check, this patch does not yet include the ability to add a context menu to an item added by a Jetpack to the status bar? If so, that seems like the big ability we are missing to release with.

Also, Atul has mentioned that we will be moving a way from flexible membranes because of the addition of COWs. Atul, can you comment here and what that means for debugging of both this and the Toolbar addition?

Comment 12

[Drew Willcoxon :adw]

(In reply to comment #11)
> So to double check, this patch does not yet include the ability to add a
> context menu to an item added by a Jetpack to the status bar?

That's right, but you can set a "popup" menu to open on an item in the status bar when that item is left-clicked.  (Untested in feature content but should work.)  The reason I haven't added context menus for feature content is that I can't test it given the membrane issues I described.

The proper solution I think is to add a popup to the browser's mainPopupSet and set this popup as the contextmenu attribute of the status bar's iframe (and slidebar's browser, panel's iframe or whatever it uses, etc.).  This popup would then be accessible via jetpack.menu.context.

Actually now that I think about it, I could implement Menu.prototype.contextOn(node), which would associate a single context menu with a single node by setting node's contextmenu attribute to the popup.  That should work, but it doesn't allow for the broad CSS selector style (jetpack.menu.context.on("#foo")).

Comment 13

[Drew Willcoxon :adw]

Attachment: for review v2

Adds:

- Menu.prototype.contextOn(node) to attach a single context menu to a single node in chrome or feature content.
- Gets the ball rolling on chrome context menus.  The browser's toolbar context menu is available at jetpack.menu.context.chrome.  Should be a simple matter to add other menus.
- Menu.prototype.replace()

Comment 14

[Aza Raskin [:aza]]

Woot!

So for jetpack.menu.context.chrome, you are saying that this works for toolbars (temporarily) and that eventually it will work for all types of chrome-based UI (sidebars, slidebars, etc.)?

Comment 15

[Drew Willcoxon :adw]

Jetpack slidebars, panels, statusbar items, and all other "feature content" I do not include in "chrome", because they aren't really; they're content (as in chrome vs. content) inside of iframes and browsers.  Feature content context menus will be accessible from jetpack.menu.context.  As I mention I haven't built this namespace because I can't test it with my membrane problems.

So far in jetpack.menu.context.chrome I've added support for only the browser's toolbar context menu.  Other chrome context menus need to be supported, like the bookmarks toolbar menu, tab strip menu, menus in sidebars, etc.

(In the current architecture these menus have to be added one-by-one, which is fairly simple but kind of a pain.  It also means that any chrome context menus added by extensions won't be accessible.  I'd therefore like to change the architecture so that popups are listened for after the fact instead of added beforehand, but I'm not sure yet how to reliably determine whether an arbitrary popup is a chrome popup.)

Comment 16

[Drew Willcoxon :adw]

Maybe s/chrome/browser/, so:

jetpack.menu.context: Context menus on my feature's content.
jetpack.menu.context.page: The page's context menu.
jetpack.menu.context.browser: The various context menus of the browser's main window.

That's the distinction I'm trying to make, not necessarily chrome vs. content.  What do you think?

Comment 17

[Aza Raskin [:aza]]

I think I see now. I wonder if jetpack.menu.me, jetpack.menu.page, and jetapck.menu browser works better?

Atul is confirmed that he will be stripping out the flexible membrane (although not the binary component). Sync up with him for what this entails.

Comment 18

[Drew Willcoxon :adw]

Attachment: for review v3

(In reply to comment #17)
> I think I see now. I wonder if jetpack.menu.me, jetpack.menu.page, and
> jetapck.menu browser works better?

My first thought was, I like it.  But Fx 3.7 will replace the menubar with two menu buttons, "Page" and "Browser".  Using those names now to mean context menus will cause trouble down the road.  Also I think it's useful to distinguish between context and menubar menus, because they behave a little differently.  (Need to update the JEP about that.)

* Retains jetpack.menu.context but s/chrome/browser/ (comment 16).
* All browser context menus are supported due to changes I said I wanted to make in comment 15.
* Added Transforms.prototype.clear().
* Correctness fixes.

Comment 19

[Drew Willcoxon :adw]

JEP updated.  Before release will need to update again with notes about what's not yet implemented.
https://wiki.mozilla.org/Labs/Jetpack/JEP/14

Comment 20

[Drew Willcoxon :adw]

Attachment: for review v4

Ed, will you be able to review this?  I know it's a small beast of a patch.  Aside from changes due to review, this should be ready to go...  It now works in real features since Atul removed flexible membranes.

Supports context menus in slidebars and status items.  Also refactored using closures to hide (most) private data of exposed objects.  Misc smaller fixes.

Comment 21

[Drew Willcoxon :adw]

Comment on attachment 407313 [details] [diff] [review]
for review v4

Ugh, it doesn't take into account the hidden window or multiple windows, since I was developing in a chrome HTML file.  New patch coming.

Comment 22

[Drew Willcoxon :adw]

Attachment: for review v5

Works with hidden window, multiple windows.

Comment 23

[Atul Varma [:atul]]

Thanks.  Have you tried running this against the latest trunk, with the memory leak detection?  With your patch applied, I'm getting some strange errors reported, most of them due to memory leaks, but a few due to undefined variables as well.

Comment 24

[Atul Varma [:atul]]

Also, are there any automated tests you can write for this?

Comment 25

[Drew Willcoxon :adw]

By the memory leak detection do you mean the profiler server you wrote?  How do I use it?  Could you paste the undefined var errors?  I don't see any here.

Yeah, it needs tests.  There's a lot to the API, so it needs a lot of tests.  Time is a problem, I need to work on other things, but I'll see what I can do.

Comment 26

[Atul Varma [:atul]]

Ah, don't worry about it dogen.  I'll try to write tests for it, it's a good way for me to learn the code anyhow.

Comment 27

[Atul Varma [:atul]]

By the way, I'm working on your patch at this mercurial patch queue:

  http://hg.mozilla.org/users/avarma_mozilla.com/jetpack-patches/

The title of the patch is "menus-494651.patch".

Comment 28

[Drew Willcoxon :adw]

Oh.  What are you doing to my baby? :)

Is dogen your profiler tool?  Do you mean writing tests for dogen, or for the menu code?

Comment 29

[Drew Willcoxon :adw]

Oh I think you meant "dog" instead of "dogen".  BTW, I've made a few small modifications to the patch, a little code cleanup, no functional changes, as well as adding MemoryTrackers.  I didn't upload it yet because I expected other changes after review and was going to batch them together.

Comment 30

[Atul Varma [:atul]]

Oops, sorry, I already added MemoryTrackers... I'm getting reports of leaks here.

Applying the latest patch from http://hg.mozilla.org/users/avarma_mozilla.com/jetpack-patches/file/164671a33021/menus-494651.patch:

"""
$ python manage.py test -f JetpackTests.test-menu
---> pavement.test
*** Registering components in: nsJetpackAudio
[message]: Now running tests.
[message]: Found 19 test suites in external files.
[message]: JetpackTests.test-menu.js succeeded
[ERROR]  : Memory differences: {"UrlFactory":1,"JetpackContext":1,"Transforms":11,"Menu":8,"TransformsStack":11,"Unloader":10,"BrowserWatcher":7,"ContextMenuDomain":3,"ContextMenuSet":3}
           chrome://jetpack/content/js/test.js:L69
[message]: 0 out of 1 tests successful ( 1 failed ).
Tests failed: 1
Tests succeeded: 0
host-5-148:jetpack varmaa$ 
"""

Any ideas?  Do you get these on your end?

Comment 31

[Drew Willcoxon :adw]

(In reply to comment #30)
> Do you get these on your end?

Indeed.  Looking now.  (BTW, can we turn on JS 1.8 for tests?)

Comment 32

[Drew Willcoxon :adw]

I'm looking at test.js -- is the feature context that test-menu.js runs in unloaded after the test completes (and before memory leaks are determined)?  I attach a lot of unloaders to the feature context.

Comment 33

[David Dahl :ddahl]

(In reply to comment #30)

> [message]: Now running tests.
> [message]: Found 19 test suites in external files.
> [message]: JetpackTests.test-menu.js succeeded
> [ERROR]  : Memory differences:
> {"UrlFactory":1,"JetpackContext":1,"Transforms":11,"Menu":8,"TransformsStack":11,"Unloader":10,"BrowserWatcher":7,"ContextMenuDomain":3,"ContextMenuSet":3}
>            chrome://jetpack/content/js/test.js:L69
> [message]: 0 out of 1 tests successful ( 1 failed ).
> Tests failed: 1
> Tests succeeded: 0
> host-5-148:jetpack varmaa$ 
> """
> 
> Any ideas?  Do you get these on your end?

Atul:

I am getting a very similar error on the toolbar patch as well:

https://bugzilla.mozilla.org/show_bug.cgi?id=507258

Comment 34

[Drew Willcoxon :adw]

(In reply to comment #32)
> I'm looking at test.js -- is the feature context that test-menu.js runs in
> unloaded after the test completes (and before memory leaks are determined)?

Ah, I should have been looking at test-jetpacks.js.  Looks like my code has some cycles then...

Also I noticed that test-jetpacks.js runs all the files in tests/jetpacks, even my Emacs autocomplete *~ files, so I filed bug 524019.

Comment 35

[Drew Willcoxon :adw]

Attachment: for review v6

MemoryTracking + test failures on leaks is a nice tool!  Thanks Atul.  I had two problems in the Unloader class: not deleting the object's reference to the function passed in to Unloader(), and not properly removing the event listener added in _onEvent().  This patch passes the smoke tests you added; I'll work on more tests later unless you get to it first.

Are you reviewing this now or should I ask Ed again?

Comment 36

[Drew Willcoxon :adw]

Attachment: for review v7

Fixes leaks.  Uses patch in bug 524411.  Fixed bug that occurred when multiple features modify same menu.

Comment 37

[Drew Willcoxon :adw]

Attachment: v8

Has an automated test that covers maybe 70% of the API.

Comment 38

[Atul Varma [:atul]]

Comment on attachment 408561 [details] [diff] [review]
v8

Thanks for the tests! You can go ahead and push this.

Comment 39

[Drew Willcoxon :adw]

Attachment: v9

Atul, when I run the test suite from about:jetpack -- with or without my patch -- most of the time there are many memory errors that don't arise when running the suite from the command line.  With my patch there are usually memory errors from the menu tests, too.  Do you see that or know why it would be?  Race between GC and test code?  nsIDOMWindowUtils.garbageCollect() doesn't mention anything.

Found a way to lazy-load jetpack.menu, at the expense of a little more code in status-bar-panel.js and slidebar.js.  Added jetpack smoke test in addition to the unit tests from the last patch.

I'll push this tomorrow to give you a chance to look over the changes if you want or answer my question.

Comment 40

[Drew Willcoxon :adw]

Attachment: v9.1

Comment 41

[Drew Willcoxon :adw]

Attachment: v10

Polished Menu.popupOn() and menus inited with functions.  Forgot to mention earlier that jetpack.menu is now future'd.

Comment 42

[Drew Willcoxon :adw]

http://hg.mozilla.org/labs/jetpack/rev/cb9060ea23cd