111230 - Use Cocoa for Widget instead of Carbon

Status: RESOLVED DUPLICATE of bug 326469

(Core :: Widget: Cocoa, defect)

Description

[Mike Pinkerton (not reading bugmail)]

I swear someone filed this bug already, but i guess not. Using as a place to
hang  patches for mozilla to use widget/src/cocoa in the place of widget/src/mac
in the mach-o build.

Comment 1

[Mike Pinkerton (not reading bugmail)]

Attachment: tree diffs to use cocoa

stuff i can't really check in but is needed. small tweaks.

Comment 2

[Zach Lipton [:zach]]

Why can't this be checked in? Let it be a build option with the default to --
widget=mac and --widget=cocca being an option. It wouldn't need review 
since it is not built by default (though the configure changes would need 
review from cls)

Comment 3

[David Hyatt]

Attachment: Patch to get widget compiling again for cocoa.

This patch gets the cocoa widgets compiling again.  nsRepeater needs to be
linked in, and timer stuff needs to be yanked from the widget factory.

Comment 4

[Mike Pinkerton (not reading bugmail)]

Attachment: updated changes that coexist with the rest of the tree and can land

these are the updated changes, ifdef'd appropriately so they can land in the
tree.

need r/sr.

Comment 5

[Mike Pinkerton (not reading bugmail)]

cc'ing seawood for the configure/makefile changes.

Comment 6

[David Hyatt]

I am able to use cocoaembed fine with double buffering enabled.  I 
suspect that isn't necessary any more.

Fix the COOCA typo that should be COCOA.

sr=hyatt

Comment 7

[hacker formerly known as [e-mail]]

Comment on attachment 66461 [details] [diff] [review]
updated changes that coexist with the rest of the tree and can land

r=cls

Comment 8

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 66461 [details] [diff] [review]
updated changes that coexist with the rest of the tree and can land

landed.

Comment 9

[Joel Nelson (don't send email)]

What is the status of this bug?  What still needs to be done before it can be
landed in the trunk?

Comment 10

[Hiro]

Cocoa was specified for Widget, and build did Firefox. 
Firefox doesn't work though build succeeded. 
I think that nsNativeAppSupportForCocoa.mm is as necessary also for toolkit as
suite. 

http://lxr.mozilla.org/mozilla/source/toolkit/xre/Makefile.in#116
http://lxr.mozilla.org/mozilla/source/xpfe/bootstrap/Makefile.in#324

Comment 11

[Josh Aas]

I just checked in a patch so that Firefox will run with cocoa widgets (appshell
fixes). No menu bar though. Taking this bug.

Comment 12

[Josh Aas]

Attachment: cocoa window code cleanup v1.0

This is code cleanup for the Cocoa widgets window files. Mostly it is code
removal (#if 0 stuff), but some new menu support. Once these files are cleaned
up future patches that do more will make more sense. We don't need this removed
code for reference since we have Carbon widgets to reference. The code I'm
removing is basically identical (it was copied).

No current projects use any of this code, and it pretty much doesn't work right
now anyway.

Comment 13

[Mike Pinkerton (not reading bugmail)]

can you put the code removal in one patch and the new changes in another? it's
impossible to tell the new stuff with all the noise.

should |mMenuBar| be a nsCOMPtr since we obviously own it?

Comment 14

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 198846 [details] [diff] [review]
cocoa window code cleanup v1.0

looks ok i guess, does the menu also have a strong ref to the window? is there
a cycle that the SetParent() is breaking?

r=pink

Comment 15

[Josh Aas]

the menubar holds a weak ref to its parent, so we're good

Comment 16

[Josh Aas]

Also, I modified the patch to use a comptr, it works fine. I'll just check that
in instead of the non-comptr in v1.0

Comment 17

[Simon Fraser [no longer active]]

Comment on attachment 198846 [details] [diff] [review]
cocoa window code cleanup v1.0

You might consider a little C++ class in nsCocoaWindow.mm to hold nsCOMPtrs, if
you find yourself having to manage more than 1 or 2 pointers by hand.

Comment 18

[Simon Fraser [no longer active]]

(In reply to comment #16)
> Also, I modified the patch to use a comptr, it works fine. I'll just check that
> in instead of the non-comptr in v1.0

This is gcc4 only. In gcc 3, ctors/dtors of of C++ member vars are not called.

Comment 19

[Simon Fraser [no longer active]]

(In reply to comment #18)
> (In reply to comment #16)
> > Also, I modified the patch to use a comptr, it works fine. I'll just check that
> > in instead of the non-comptr in v1.0
> 
> This is gcc4 only. In gcc 3, ctors/dtors of of C++ member vars are not called.

Sorry, I assumed you were talking about using an nsCOMPtr in an Obj-C class. In
a C++ class it will of course work fine.

Comment 20

[Josh Aas]

checked in

Comment 21

[Josh Aas]

Attachment: menu work, basic non-functional rewrite v1.0

This is a dump of most of my work so far on Cocoa menus. It is NPOB, and
doesn't work. However, it puts menus on the screen, has most of an event
system, and is a big step in the right direction.

I'm not going to defend anything in this patch (except for very high-level
design issues) since it is a major work in progress. I only want to land it so
that I don't have so much uncommitted code around and so that future patches
are more clear. There are tons of bugs in this, I know.

One reason that the diff looks weird is that I started with fresh copies of the
equivalent files from widget/src/carbon instead of editing the files in
widget/src/cocoa.

Comment 22

[Mike Pinkerton (not reading bugmail)]

+#if DEBUG
+// utility instance counter class
+class nsInstanceCounter
+{

should this be #DEBUG or #DEBUG_josh ?

+    MenuDelegate*               mMenuDelegate;

be explicit about memory ownership. I assume this is a weak ref?

+    NSMenu*                 mRootMenu;            // root menu, representing
entire menu bar.

same commcent, though i assume we own this? if not, who does? the OS?

+  [mMenuDelegate release];

oh, i guess this is a strong ref. It's not common to own your delegate, is it?
This should certainly be made explicit and commented on the API if it's required. 

+NS_METHOD nsMenuX::GetParent(nsISupports*& aParent)

shouldn't these be NS_IMETHODIMP ? I forget the differences.

+NS_METHOD nsMenuX::SetNativeData(void * aData)
+{
+  mMacMenu = (NSMenu*) aData;
+  return NS_OK;
+}

do we have to release |mMacMenu| here or do we not own it?

+NS_METHOD nsMenuX::AddMenuListener(nsIMenuListener * aMenuListener)
+{
+  mListener = aMenuListener; // strong ref
+  return NS_OK;
+}

if this is a strong ref, don't we have to first autorelease |mListener| to avoid
leaking? Also, should this be a list of listeners, not just a single one? The
name of the method (and the format of the subsequent remove call) indicates that
more than one is allowed.

+nsEventStatus nsMenuX::MenuConstruct(
+    const nsMenuEvent & aMenuEvent,
+    nsIWidget         * aParentWindow, 
+    void              * /* menuNode */,
+	  void              * aDocShell)

remove tabs.

+NS_METHOD nsMenuX::GetMenuContent(nsIContent ** aMenuContent)
+{
+  NS_ENSURE_ARG_POINTER(aMenuContent);
+  NS_IF_ADDREF(*aMenuContent = mMenuContent);
+	return NS_OK;
+}

same as above. also, i think this should be:

 NS_ENSURE_ARG_POINTER(*aMenuContent); 

+NSMenu* nsMenuX::NewMenuWithGeckoString(nsString& menuTitle)
+{
+  NSString* title = [NSString stringWithCharacters:(UniChar*)menuTitle.get()
length:menuTitle.Length()];
+  NSMenu* myMenu = [[NSMenu alloc] initWithTitle:title];
+  [myMenu setDelegate:mMenuDelegate];
+  
...
+  
+  return myMenu;
+}

Obj-C guidelines say that any object created by a function should be
autoreleased upon return. Either that, or this function should have a Create...
prefix to indicate that the object has already been retained. I'd say go the
latter route and rename the function.

+  nsCOMPtr<nsIMenuItem> pnsMenuItem = do_CreateInstance(kMenuItemCID);
+  if (pnsMenuItem) {

if we're going through the hassle of new code and removing spacing, remove the
saari's hungarian notation as well. Happens in a few places.

+    // Make nsMenu a child of parent nsMenu. The parent takes ownership
+    nsCOMPtr<nsISupports> supports2(do_QueryInterface(pnsMenu));
+	  pParentMenu->AddItem(supports2);

tabs

+// Determines how many menus are visible among the siblings that are before me.
+// It doesn't matter if I am visible. Note that this will always count the Apple
+// menu, since we always put it in there.

is this still true about the apple menu?

+static MenuRef getCarbonMenuRef(NSMenu* aMenu)

should be GetCarbonMenuRef(...)

+- (id)initWithGeckoMenu:(nsMenuX*)geckoMenu
+{
+  [super init];
+  mGeckoMenu = geckoMenu;
+  mHaveInstalledCarbonEvents = FALSE;
+  return self;
+}

use this pattern for init:

if ((self = [super init])) {
  ...
}
return self;

i know you said you have another patch that already relies on this landing. I
would sr- this until some of the above issues are pointed out, but i don't know
if you can even resubmit this patch w/out those other changes.

what do you want to do? 

Comment 23

[Josh Aas]

Thanks for the review. Much of what you pointed out is already in my next patch.
I'd like to get sr for the patch I posted, then I'll land it (with a few
additional fixes that aren't posted yet, many of which you commented on), and
follow up with yet another patch soon after. This will get all the files in
place and my coming patches will be easier to review.

Comment 24

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 199253 [details] [diff] [review]
menu work, basic non-functional rewrite v1.0

sr=pink after discussion comments with josh for his next round of this patch.

Comment 25

[Josh Aas]

landed updated patch and file name changes on trunk

Comment 26

[Josh Aas]

Apparently this got backed out due to Camino tbox orange. Please note in the bug
if that needs to be done so I don't get surprised scanning bonsai logs. I wonder
why this caused funniness in Camino...

Comment 27

[Simon Fraser [no longer active]]

(In reply to comment #26)
> Apparently this got backed out due to Camino tbox orange. Please note in the bug
> if that needs to be done so I don't get surprised scanning bonsai logs.

What needs to be done is that you need to stick around to watch tinderbox after
checking in  :)

> I wonder why this caused funniness in Camino...

I expect nothing; tinderbox is still orange.

Comment 28

[Josh Aas]

I took care tbox watching by talking to Mark. I did not simply check in and leave.

Comment 29

[Wayne Woods]

Josh, the code you checked in on 2005-10-07 (comment 20) couldn't have caused bug 325455, could it? Just guessing, but the timing is right.

Comment 30

[Josh Aas]

Wayne - none of this code is used in Firefox right now. This is all Cocoa widget code, we use Carbon widgets.

Comment 31

[Wayne Woods]

Sorry about that. I knew you were working towards Cocoa widgets in Fx, and thought maybe that patch had some underlying code that might have affected the active browser. Ah well, I'll look elsewhere :)

Comment 32

[Josh Aas]

This bug was created "to hang  patches for mozilla to use widget/src/cocoa in the place of widget/src/mac in the mach-o build."

From now on we should be filing individual bugs for problems with Cocoa widgets. Bug 326469 will track progress on the individual bugs standing in the way of making Cocoa widgets default for all Mac OS X products.

I am marking this a duplicate of 326469 because I don't want to give the impression that Cocoa widgets are ready to go.

*** This bug has been marked as a duplicate of 326469 ***