decomtaminate and clean up nsIMenuBar, nsIMenu, nsIMenuItem

RESOLVED FIXED

Status

()

defect
P1
normal
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: jaas, Assigned: jaas)

Tracking

Trunk
x86
macOS
Points:
---
Bug Flags:
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 12 obsolete attachments)

Assignee

Description

11 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

11 years ago
Status: NEW → ASSIGNED
Assignee

Comment 1

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

Comment 2

11 years ago
Posted 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
Assignee

Comment 3

11 years ago
Posted 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
Assignee

Comment 4

11 years ago
Posted 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
Assignee

Comment 5

11 years ago
Posted 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)
Assignee

Comment 6

11 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

11 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

11 years ago
Flags: wanted1.9.1? → wanted1.9.1+
Assignee

Comment 8

11 years ago
Posted 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)
Assignee

Updated

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

Comment 9

11 years ago
Posted 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)
Assignee

Updated

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

Comment 10

11 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

11 years ago
Posted 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.
Assignee

Comment 15

11 years ago
Posted 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)
Assignee

Comment 16

11 years ago
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

11 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

11 years ago
landed on 1.9.1 trunk
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee

Comment 20

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

Comment 21

11 years ago
Posted 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
Assignee

Comment 22

11 years ago
Posted patch fix v1.7 (obsolete) — Splinter Review
Fixes memory leaks, still have to investigate unit test failures.
Attachment #327075 - Attachment is obsolete: true
Assignee

Comment 23

11 years ago
Posted 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
Assignee

Updated

11 years ago
Status: REOPENED → RESOLVED
Closed: 11 years ago11 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

10 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.