decomtaminate and clean up nsIMenuBar, nsIMenu, nsIMenuItem

RESOLVED FIXED

Status

()

Core
Widget: Cocoa
P1
normal
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Josh Aas, Assigned: Josh Aas)

Tracking

Trunk
x86
Mac OS X
Points:
---
Bug Flags:
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 12 obsolete attachments)

(Assignee)

Description

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

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

10 years ago
Created attachment 321154 [details] [diff] [review]
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(-)
(Assignee)

Comment 2

10 years ago
Created attachment 321703 [details] [diff] [review]
fix v0.6

Compiles and runs, puts up menus correctly, but more work to do. More cleanup and memory mgmt work.
Attachment #321154 - Attachment is obsolete: true
(Assignee)

Comment 3

10 years ago
Created attachment 321724 [details] [diff] [review]
fix v0.7

No longer hemorrhages memory, still more cleanup and memory work to do though.
Attachment #321703 - Attachment is obsolete: true
(Assignee)

Comment 4

10 years ago
Created attachment 321824 [details] [diff] [review]
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(-)
Attachment #321724 - Attachment is obsolete: true
(Assignee)

Comment 5

10 years ago
Created attachment 321861 [details] [diff] [review]
fix v1.0

34 files changed, 1098 insertions(+), 1861 deletions(-)
Attachment #321824 - Attachment is obsolete: true
Attachment #321861 - Flags: review?(bent.mozilla)
(Assignee)

Comment 6

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

Updated

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

Updated

10 years ago
Flags: wanted1.9.1? → wanted1.9.1+
(Assignee)

Comment 8

10 years ago
Created attachment 324758 [details] [diff] [review]
fix v1.1

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)
(Assignee)

Updated

10 years ago
Attachment #324758 - Flags: review?(bent.mozilla)
(Assignee)

Comment 9

10 years ago
Created attachment 325055 [details] [diff] [review]
fix v1.2

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)
(Assignee)

Updated

10 years ago
Attachment #325055 - Flags: review?(bent.mozilla)

Comment 10

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

Comment 12

10 years ago
Created attachment 325307 [details] [diff] [review]
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).
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.
(Assignee)

Comment 15

10 years ago
Created attachment 326240 [details] [diff] [review]
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.
Attachment #325307 - Attachment is obsolete: true
Attachment #325307 - Flags: superreview?(roc)
(Assignee)

Comment 16

10 years ago
Created attachment 326788 [details] [diff] [review]
fix v1.5 (includes basic tests)
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?
(Assignee)

Comment 18

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

Comment 19

10 years ago
landed on 1.9.1 trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 20

10 years ago
backed out due to leaks and test failures
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 21

10 years ago
Created attachment 327075 [details] [diff] [review]
fix v1.6

Includes 2 bustage fixes for Windows, still need to fix leaks and unit tests.
Attachment #326788 - Attachment is obsolete: true
(Assignee)

Comment 22

10 years ago
Created attachment 327198 [details] [diff] [review]
fix v1.7

Fixes memory leaks, still have to investigate unit test failures.
Attachment #327075 - Attachment is obsolete: true
(Assignee)

Comment 23

10 years ago
Created attachment 327238 [details] [diff] [review]
fix v1.8

Fixes unit tests, checked in. Unit test failures were the result of a Makefile mistake.
Attachment #327198 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 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.
(Assignee)

Comment 26

9 years ago
(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.