Closed Bug 433952 Opened 12 years ago Closed 12 years ago

decomtaminate and clean up nsIMenuBar, nsIMenu, nsIMenuItem

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: jaas, Assigned: jaas)

References

Details

Attachments

(1 file, 12 obsolete files)

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.
Status: NEW → ASSIGNED
Attached patch fix v0.5 (obsolete) — Splinter Review
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(-)
Attached patch fix v0.6 (obsolete) — Splinter Review
Compiles and runs, puts up menus correctly, but more work to do. More cleanup and memory mgmt work.
Attachment #321154 - Attachment is obsolete: true
Attached patch fix v0.7 (obsolete) — Splinter Review
No longer hemorrhages memory, still more cleanup and memory work to do though.
Attachment #321703 - Attachment is obsolete: true
Attached patch fix v0.8 (obsolete) — Splinter Review
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(-)
Attachment #321724 - Attachment is obsolete: true
Attached patch fix v1.0 (obsolete) — Splinter Review
34 files changed, 1098 insertions(+), 1861 deletions(-)
Attachment #321824 - Attachment is obsolete: true
Attachment #321861 - Flags: review?(bent.mozilla)
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.
Attachment #321861 - Flags: review?(nick.kreeger)
Adding this to the 1.9.1 triage queue by marking this wanted1.9.1?.  If this should be a blocker, please mark accordingly.
Flags: wanted1.9.1?
Priority: -- → P1
Flags: wanted1.9.1? → wanted1.9.1+
Attached patch fix v1.1 (obsolete) — Splinter Review
Improved patch, includes fixes from a big review by Ben Turner. Thanks Ben!
Attachment #321861 - Attachment is obsolete: true
Attachment #324758 - Flags: review?(nick.kreeger)
Attachment #321861 - Flags: review?(nick.kreeger)
Attachment #321861 - Flags: review?(bent.mozilla)
Attachment #324758 - Flags: review?(bent.mozilla)
Attached patch fix v1.2 (obsolete) — Splinter Review
A bit more cleanup of string functions, thanks Nick.
Attachment #324758 - Attachment is obsolete: true
Attachment #325055 - Flags: review?(nick.kreeger)
Attachment #324758 - Flags: review?(nick.kreeger)
Attachment #324758 - Flags: review?(bent.mozilla)
Attachment #325055 - Flags: review?(bent.mozilla)
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.
Attachment #325055 - Flags: review?(nick.kreeger) → review+
Comment on attachment 325055 [details] [diff] [review]
fix v1.2

I think this looks good.
Attachment #325055 - Flags: review?(bent.mozilla) → review+
Attached patch fix v1.3 (obsolete) — Splinter Review
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).
Attachment #325055 - Attachment is obsolete: true
Attachment #325307 - Flags: superreview?(roc)
(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 :-).
   // 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.
Attached patch fix v1.4 (obsolete) — Splinter Review
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.
Attachment #325307 - Attachment is obsolete: true
Attachment #325307 - Flags: superreview?(roc)
Attached patch fix v1.5 (includes basic tests) (obsolete) — Splinter Review
Attachment #326240 - Attachment is obsolete: true
Attachment #326788 - Flags: superreview?(roc)
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?
(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?
Attachment #326788 - Flags: superreview?(roc) → superreview+
landed on 1.9.1 trunk
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
backed out due to leaks and test failures
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch fix v1.6 (obsolete) — Splinter Review
Includes 2 bustage fixes for Windows, still need to fix leaks and unit tests.
Attachment #326788 - Attachment is obsolete: true
Attached patch fix v1.7 (obsolete) — Splinter Review
Fixes memory leaks, still have to investigate unit test failures.
Attachment #327075 - Attachment is obsolete: true
Attached patch fix v1.8Splinter Review
Fixes unit tests, checked in. Unit test failures were the result of a Makefile mistake.
Attachment #327198 - Attachment is obsolete: true
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Duplicate of this bug: 440946
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.
(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.
You need to log in before you can comment on or make changes to this bug.