Closed Bug 405512 Opened 12 years ago Closed 12 years ago

deCOMtaminate nsIMenuRollup

Categories

(Core :: Widget, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: roc, Assigned: karunasagark)

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 2 obsolete files)

nsIMenuRollup is ugly. Probably we should just export nsXULPopupManager.h so it can be used by widget code. Instead of returning an nsISupportsArray, it could just take an nsTArray* to fill in.
Whiteboard: [good first bug]
on second thoughs, leave nsIMenuRollup in place, but instead of using IDL, make it a C++ .h file instead. It should still be an XPCOM interface though (inherit from nsISupports, have an IID).
you mean, nsIMenuRollup.idl file is to be removed, instead have nsIMenuRollup.h file - declaring an interface (abstract class), inheriting from nsISupport and have an IID?? If thats right, the only change is that, we removed .idl file. But why??
It's simpler because we don't have to generate a .h file during the build. IDEs understand it better. We don't have to worry that it might be used from script.

Also we can change the signatures of the methods to things that aren't expressible in IDL. For example methods that always succeed can return void instead of nsresult. GetSubmenuWidgetChain can take an nsTArray<nsIWidget*>* parameter (an array to fill in) instead of messing with nsISupportsArray.
i've done the changes (removed the .idl and instead now have .h file for nsIMenuRollup). The .h file is the one generated by xpidl on the original .idl file. Do we make any changes here - the nsTArray change??
Yes please. You can probably clean up the generated .h file by removing unused macro definitions etc too.
Attached patch deCOMtaminate nsIMenuRollup (obsolete) — Splinter Review
Attachment #296118 - Flags: review?(roc)
Both of those methods never return a failure code so you might as well just make them return void. (Declare them "virtual void".)

+				nsTArray<nsIWidget*> widgetChain;
+				menuRollup->GetSubmenuWidgetChain(&widgetChain);

Might as well declare nsAutoTArray<nsIWidget*,5> widgetChain.

+					nsCOMPtr<nsIWidget> widget = widgetChain[i];

You don't need to take an owning reference here. Just use nsIWidget*

+					if ( widget ) 

You don't need to test for non-null, GetSubmenuWidgetChain should just be documented to never append a null pointer.

Similar comments apply to the other implementations.
Attached patch deCOMtaminate nsIMenuRollup (obsolete) — Splinter Review
Attachment #296118 - Attachment is obsolete: true
Attachment #296936 - Flags: superreview?(roc)
Attachment #296936 - Flags: review?(roc)
Attachment #296118 - Flags: review?(roc)
+    // null pointers are not appended to the list of open popups
+    if (widget)

Don't null-check the widget. We already assert that it's non-null.

+/* Use this macro when declaring classes that implement this interface. */
+#define NS_DECL_NSIMENUROLLUP \
+  virtual void GetSubmenuWidgetChain(nsTArray<nsIWidget*> *_retval); \
+  virtual void AdjustPopupsOnWindowChange(void); 

Take this macro out and stop using it in nsXULPopupManager.h

+#define NS_IMENUROLLUP_IID \
+  {0x10c69225, 0x2c5a, 0x4948, \
+    { 0x86, 0x90, 0x0b, 0xc5, 0x89, 0xd1, 0x45, 0xb4 }}
+

We're changing the interface a lot so you need to change the IID here

+		nsAutoTArray<nsIWidget*, 5> widgetChain;
+                menuRollup->GetSubmenuWidgetChain(&widgetChain);

Fix indent

+                    GdkWindow* currWindow =
+                    (GdkWindow*) widget->GetNativeData(NS_NATIVE_WINDOW);

Indent the second line

+          PRUint32 count = widgetChain.Length();

These variables are probably not needed, just put widgetChain.Length() in each for loop
updated the IID for nsIMenuRollup. Also, in nsAutoTArray<nsIWidget*, 5>, whats the reason for 5?? From the ctor code for nsAutoTArray, it constructs a buffer of such a size, so probably having an AutoTArray is advantageous over just TArray?
Attachment #296936 - Attachment is obsolete: true
Attachment #297148 - Flags: superreview?(roc)
Attachment #297148 - Flags: review?(roc)
Attachment #296936 - Flags: superreview?(roc)
Attachment #296936 - Flags: review?(roc)
Right, it constructs a buffer with 5 elements on the stack, so we don't have to do an expensive heap allocation. The number 5 is arbitrary, we want it to be big enough to hold all the items we'd normally ever need. In this case we need one item for each open menu (e.g., View ... Character Encoding ... More Encodings ... East European would need 4 elements). I don't think we'll see more than 5 in any sanely designed UI! If we do, nsAutoTArray will still work, it will just have to allocate on the heap.
Comment on attachment 297148 [details] [diff] [review]
deCOMtaminate nsIMenuRollup

This looks good.

Now you probably want to find something more interesting to work on :-)
Attachment #297148 - Flags: superreview?(roc)
Attachment #297148 - Flags: superreview+
Attachment #297148 - Flags: review?(roc)
Attachment #297148 - Flags: review+
Assignee: nobody → karunasagark
OS: Mac OS X → All
Hardware: PC → All
Status: NEW → ASSIGNED
Comment on attachment 297148 [details] [diff] [review]
deCOMtaminate nsIMenuRollup

The patch removes nsIMenuRollup.idl and instead declares it in nsIMenuRollup.h - its still an XPCOM interface (IID updated). Also, it replaces nsISupportsArray with nsTArray or nsAutoTArray where appropriate.

Reward: clearness of code (IDE understands better), flexible method signatures

Risk: nsIMenuRollup is no longer scriptable (https://bugzilla.mozilla.org/show_bug.cgi?id=405512#c3)
Attachment #297148 - Flags: approval1.9?
(In reply to comment #13)
> (From update of attachment 297148 [details] [diff] [review])
> The patch removes nsIMenuRollup.idl and instead declares it in nsIMenuRollup.h
> - its still an XPCOM interface (IID updated). Also, it replaces
> nsISupportsArray with nsTArray or nsAutoTArray where appropriate.
> 
> Reward: clearness of code (IDE understands better), flexible method signatures
> 
> Risk: nsIMenuRollup is no longer scriptable
> (https://bugzilla.mozilla.org/show_bug.cgi?id=405512#c3)
> 

Is this a problem for extensions?  Shaver/Finkle/Resig?

I'll put it on the list of potential updating problems, but based on some code searches, I don't think it will be a problem.
Comment on attachment 297148 [details] [diff] [review]
deCOMtaminate nsIMenuRollup

a+ for 1.9 - thanks everyone!
Attachment #297148 - Flags: approval1.9? → approval1.9+
Checking in layout/xul/base/public/nsXULPopupManager.h;
/cvsroot/mozilla/layout/xul/base/public/nsXULPopupManager.h,v  <--  nsXULPopupManager.h
new revision: 1.21; previous revision: 1.20
done
Checking in layout/xul/base/src/nsXULPopupManager.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsXULPopupManager.cpp,v  <--  nsXULPopupManager.cpp
new revision: 1.42; previous revision: 1.41
done
Checking in widget/public/Makefile.in;
/cvsroot/mozilla/widget/public/Makefile.in,v  <--  Makefile.in
new revision: 1.117; previous revision: 1.116
done
RCS file: /cvsroot/mozilla/widget/public/nsIMenuRollup.h,v
done
Checking in widget/public/nsIMenuRollup.h;
/cvsroot/mozilla/widget/public/nsIMenuRollup.h,v  <--  nsIMenuRollup.h
initial revision: 1.1
done
Removing widget/public/nsIMenuRollup.idl;
/cvsroot/mozilla/widget/public/nsIMenuRollup.idl,v  <--  nsIMenuRollup.idl
new revision: delete; previous revision: 1.3
done
Checking in widget/src/beos/nsWindow.cpp;
/cvsroot/mozilla/widget/src/beos/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.140; previous revision: 1.139
done
Checking in widget/src/cocoa/nsChildView.mm;
/cvsroot/mozilla/widget/src/cocoa/nsChildView.mm,v  <--  nsChildView.mm
new revision: 1.297; previous revision: 1.296
done
Checking in widget/src/gtk2/nsWindow.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.252; previous revision: 1.251
done
Checking in widget/src/os2/nsWindow.cpp;
/cvsroot/mozilla/widget/src/os2/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.226; previous revision: 1.225
done
Checking in widget/src/windows/nsWindow.cpp;
/cvsroot/mozilla/widget/src/windows/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 3.721; previous revision: 3.720
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
You need to log in before you can comment on or make changes to this bug.