Add support for menus in Jetpack

RESOLVED FIXED in 0.6

Status

P1
normal
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: abimanyuraja, Assigned: adw)

Tracking

Dependency tree / graph

Details

(URL)

Attachments

(1 attachment, 13 obsolete attachments)

v10
81.33 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
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
(Reporter)

Updated

10 years ago
Version: unspecified → Trunk
(Reporter)

Comment 1

10 years ago
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"});
(Reporter)

Comment 2

10 years ago
Created attachment 379470 [details] [diff] [review]
Initial patch

Comment 3

10 years ago
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.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P2
Target Milestone: -- → 0.3

Updated

10 years ago
Depends on: 496421
Assignee: nobody → adw
Priority: P2 → P1
Target Milestone: 0.3 → 0.6
Duplicate of this bug: 506288
Duplicate of this bug: 496421
Created attachment 402124 [details] [diff] [review]
WIP v1

Messy, but much functionality is there.
Attachment #379470 - Attachment is obsolete: true
Created attachment 403858 [details] [diff] [review]
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.
Attachment #402124 - Attachment is obsolete: true

Comment 8

9 years ago
Around what level of functionality do we have here?
Created attachment 405922 [details] [diff] [review]
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.
Attachment #403858 - Attachment is obsolete: true
Attachment #405922 - Flags: review?(edilee)
So to double check, this patch does not yet include
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?
(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")).
Created attachment 406050 [details] [diff] [review]
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()
Attachment #405922 - Attachment is obsolete: true
Attachment #406050 - Flags: review?(edilee)
Attachment #405922 - Flags: review?(edilee)
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.)?
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.)
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?
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.
Created attachment 406475 [details] [diff] [review]
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.
Attachment #406050 - Attachment is obsolete: true
Attachment #406475 - Flags: review?(edilee)
Attachment #406050 - Flags: review?(edilee)
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
Created attachment 407313 [details] [diff] [review]
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.
Attachment #406475 - Attachment is obsolete: true
Attachment #407313 - Flags: review?(edilee)
Attachment #406475 - Flags: review?(edilee)
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.
Attachment #407313 - Flags: review?(edilee)
Created attachment 407488 [details] [diff] [review]
for review v5

Works with hidden window, multiple windows.
Attachment #407313 - Attachment is obsolete: true
Attachment #407488 - Flags: review?(edilee)
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.
Also, are there any automated tests you can write for this?
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.
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.
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".
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?
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.
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?
(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?)
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.
(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
(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.
Created attachment 407955 [details] [diff] [review]
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?
Attachment #407488 - Attachment is obsolete: true
Attachment #407488 - Flags: review?(edilee)
Depends on: 524411
Created attachment 408440 [details] [diff] [review]
for review v7

Fixes leaks.  Uses patch in bug 524411.  Fixed bug that occurred when multiple features modify same menu.
Attachment #407955 - Attachment is obsolete: true
Attachment #408440 - Flags: review?(avarma)
Created attachment 408561 [details] [diff] [review]
v8

Has an automated test that covers maybe 70% of the API.
Attachment #408440 - Attachment is obsolete: true
Attachment #408440 - Flags: review?(avarma)

Updated

9 years ago
Attachment #408561 - Flags: review+
Comment on attachment 408561 [details] [diff] [review]
v8

Thanks for the tests! You can go ahead and push this.
Created attachment 408790 [details] [diff] [review]
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.
Attachment #408561 - Attachment is obsolete: true
Created attachment 408793 [details] [diff] [review]
v9.1
Attachment #408790 - Attachment is obsolete: true
Created attachment 408991 [details] [diff] [review]
v10

Polished Menu.popupOn() and menus inited with functions.  Forgot to mention earlier that jetpack.menu is now future'd.
Attachment #408793 - Attachment is obsolete: true
http://hg.mozilla.org/labs/jetpack/rev/cb9060ea23cd
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Depends on: 526382
Depends on: 527283
Depends on: 527951
You need to log in before you can comment on or make changes to this bug.