433952 - decomtaminate and clean up nsIMenuBar, nsIMenu, nsIMenuItem

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[Josh Aas]

We should decomtaminate nsIMenuBar, nsIMenu, nsIMenuItem and clean them up in general while we're at it. That stuff doesn't need to be xpcom-based and it is a mess.

Comment 1

[Josh Aas]

Attachment: fix v0.5

This is a huge amount of the work, it compiles but it won't work. Backing up here.

26 files changed, 962 insertions(+), 1692 deletions(-)

Comment 2

[Josh Aas]

Attachment: fix v0.6

Compiles and runs, puts up menus correctly, but more work to do. More cleanup and memory mgmt work.

Comment 3

[Josh Aas]

Attachment: fix v0.7

No longer hemorrhages memory, still more cleanup and memory work to do though.

Comment 4

[Josh Aas]

Attachment: fix v0.8

Further memory usage reductions, fixed major bug, cross-platform build fixes, general cleanup. More auditing to do, cleanup.

34 files changed, 1062 insertions(+), 1799 deletions(-)

Comment 5

[Josh Aas]

Attachment: fix v1.0

34 files changed, 1098 insertions(+), 1861 deletions(-)

Comment 6

[Josh Aas]

There is probably more stuff that could be cleaned up, like using a newer hash table type, but I think we should go with this for now. We can get more stuff in followup. This works well (afaict) and is a huge improvement over our old code.

Comment 7

[Damon Sicore (:damons)]

Adding this to the 1.9.1 triage queue by marking this wanted1.9.1?.  If this should be a blocker, please mark accordingly.

Comment 8

[Josh Aas]

Attachment: fix v1.1

Improved patch, includes fixes from a big review by Ben Turner. Thanks Ben!

Comment 9

[Josh Aas]

Attachment: fix v1.2

A bit more cleanup of string functions, thanks Nick.

Comment 10

[Nick Kreeger]

Comment on attachment 325055 [details] [diff] [review]
fix v1.2

Looks good to me, r+. It would be nice to see some-form of ownership documentation as bent had mentioned. But other than that, I only had a few little nits from my read through:


+    NSMenuItem* newMenuItem = [[[NSMenuItem alloc] initWithTitle:[nativeMenu title] action:NULL keyEquivalent:@""] autorelease];
+    [mRootMenu addItem:newMenuItem];
+    [newMenuItem setSubmenu:nativeMenu];

Would it make more sense to use a |[newMenuItem release]| call after the you added the menuitem? I only mention this for our discussion about auto-release pools.



It would be nice to know inside some of these classes which class the virtual function definitions came from. For example:
  
  class nsCocoaWindow:  {
   ...
   // nsIWidget:
   NS_IMETHODIMP SetMenuBar(nsIMenuBar *aMenuBar);
   ...
  }
  
I only mention this because I'm use to and like the easy |NS_DECL_FOO| definitions in class declarations. 


Low priority super-minor nits:
+    NS_IMETHOD SetMenuBar(void* aMenuBar) = 0;
+  NS_IMETHOD CreateNativeMenuBar(nsIWidget* aParent, nsIContent* aMenuBarNode)=0;
...
+  nsIContent*               Content() { return mContent; }
+  void*             NativeData()     {return (void*)mRootMenu;}
+  nsMenuObjectTypeX MenuObjectType() {return eMenuBarObjectType;}

It would be super nice if the spacing was consistent with the rest of the style of the patch.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 325055 [details] [diff] [review]
fix v1.2

I think this looks good.

Comment 12

[Josh Aas]

Attachment: fix v1.3

I think this is ready to go. We don't have a way to automate tests for this code (yet?), I don't think we need them now. This patch shouldn't change any behavior (it does not fix buggy behavior).

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #12)
> I think this is ready to go. We don't have a way to automate tests for this
> code (yet?), I don't think we need them now. This patch shouldn't change any
> behavior (it does not fix buggy behavior).

Checking that is exactly why tests are useful. If our expectations about code were always correct, we wouldn't have any bugs :-).

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

   // Obtain the menubar for a window
-  nsIMenuBar GetMenuBar();
+  nsMenuBarX GetMenuBar();

Who calls this?

+  nsHashtable        mObserverTable;       // stores observers for content change notification

At some point this should change to nsTHashtable. Could be followup.

Although, you might want to investigate just storing the observer as a node property using GetProperty/SetProperty and get rid of this hashtable.

+  nsCOMPtr<nsIDOMNode> domNode(do_QueryInterface(aContent));
+  nsCOMPtr<nsIDOMDocument> domDoc;
+  domNode->GetOwnerDocument(getter_AddRefs(domDoc));
+  nsCOMPtr<nsIDocument> doc(do_QueryInterface(domDoc));

Why this little dance instead of just aContent->GetOwnerDoc()?

+  doc->AddMutationObserver(this);

Do you need to observe the whole document, why not just the menu node?

+  mMenuArray.AppendElement(aMenu); // owner
+
+  NSMenu* nativeMenu = (NSMenu*)aMenu->NativeData();
+  nsIContent* menuContent = aMenu->Content();
+  if (menuContent->GetChildCount() > 0 &&
+      !nsMenuUtilsX::NodeIsHiddenOrCollapsed(menuContent)) {
+    NSMenuItem* newMenuItem = [[[NSMenuItem alloc] initWithTitle:[nativeMenu title] action:NULL keyEquivalent:@""] autorelease];
+    [mRootMenu addItem:newMenuItem];

I'm confused. Because of the "if", menu indices don't necessarily correspond between mMenuArray and mRootMenu. But later on in RemoveMenu, we assume they do.

+  void              HideItem(nsIDOMDocument* inDoc, const nsAString & inID, nsIContent** outHiddenNode);

Just return the hidden node directly?

+// All menu objects subclass this.
+class nsMenuObjectX

Do you want to describe here how these objects are managed? Who owns them, who frees them?

+  nsMenuX* menu = new nsMenuX();
+  if (!menu)

Use nsAutoPtr here, and just .forget() it at the end.

I think it's worth spending some time to write tests for this before it lands. I guess we'll need some code to traverse the native menus; perhaps we could have a method that serializes the native menu state into some string format that the tests can then analyze to make sure we built the right thing.

Comment 15

[Josh Aas]

Attachment: fix v1.4

Addresses roc's comments. I don't want to rewrite the hash table stuff now, nor do I want to mess with the observation code at the moment. That stuff should be looked at but after this gets in, I'm not really modifying either part here. Tests on the way.

Comment 16

[Josh Aas]

Attachment: fix v1.5 (includes basic tests)

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can you answer this?
> Do you need to observe the whole document, why not just the menu node?

+    // We remove the application menu from consideration for the top-level menu

Why? Wouldn't it be potentially useful to be able to activate stuff in it?

Could you add some tests that dynamically add and remove items, and hide and show items, and verify that the right things happen when trying to activate items before, at, and after the hidden/shown/added/removed items? And test disabled items, assuming that ActivateNativeMenuItemAt won't be able to activate a disabled item?

Comment 18

[Josh Aas]

(In reply to comment #17)
> Can you answer this?
> > Do you need to observe the whole document, why not just the menu node?

We can just observe the menu node, but I don't want to change that now. This patch is already too big, my original goal was just to decomtaminate and clean up. That and the hash table stuff are on my to-do list for the future of this code.

> +    // We remove the application menu from consideration for the top-level
> menu
> 
> Why? Wouldn't it be potentially useful to be able to activate stuff in it?

I did that because that way the testing stuff only applies to what you make in the XUL - otherwise the off by 1 thing is sort of arbitrary. Like what happens if/when we add other standard menus?

Comment 19

[Josh Aas]

landed on 1.9.1 trunk

Comment 20

[Josh Aas]

backed out due to leaks and test failures

Comment 21

[Josh Aas]

Attachment: fix v1.6

Includes 2 bustage fixes for Windows, still need to fix leaks and unit tests.

Comment 22

[Josh Aas]

Attachment: fix v1.7

Fixes memory leaks, still have to investigate unit test failures.

Comment 23

[Josh Aas]

Attachment: fix v1.8

Fixes unit tests, checked in. Unit test failures were the result of a Makefile mistake.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Crash-stat shows few crashes @ http://hg.mozilla.org/releases/mozilla-1.9.1/annotate/d4681ff6dfb8/widget/src/cocoa/nsMenuX.mm#l901
I'd guess there is a null check missing.

Comment 26

[Josh Aas]

(In reply to comment #25)
> Crash-stat shows few crashes @
> http://hg.mozilla.org/releases/mozilla-1.9.1/annotate/d4681ff6dfb8/widget/src/cocoa/nsMenuX.mm#l901
> I'd guess there is a null check missing.

A sample crash address is "0x17da7bb", its probably not a null check that we're missing.