168117 - Need to implement nsIWidget::SetParent on the Mac

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Mac, defect, P1)

Description

[Kevin McCluskey (gone)]

this bug was broken off from bug 129034.

The platform specific widget library needs to support reparenting existing
nsIWidget's by implementing the nsIWidget::SetParent method. Until this is
Mozilla's print preview will sometimes display iframes and other elements which
require native widgets on the wrong page.

CC'ing peterl since plugin's also need this capability.

Comment 1

[Kevin McCluskey (gone)]

-> sfraser

Comment 2

[Simon Fraser [no longer active]]

Do we have a testcase?

Comment 3

[Kevin McCluskey (gone)]

The testcase in bug 129034 can be used. If you load the test and go into print
preview it will attempt to reparent the widget. 

http://bugzilla.mozilla.org/attachment.cgi?id=98057&action=view

Comment 4

[Simon Fraser [no longer active]]

How does SetParent play with AddChild/RemoveChild? I don't see the Windows code
updating the child list when SetParent is called. Is this a bug?

Comment 5

[Simon Fraser [no longer active]]

Attachment: Implement SetParent for Mac widgets

Comment 6

[Simon Fraser [no longer active]]

Attachment: Implement SetParent for carbon and cocoa widgets

Comment 7

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 119235 [details] [diff] [review]
Implement SetParent for carbon and cocoa widgets

r=pink

Comment 8

[Kevin McCluskey (gone)]

nsbeta1+

Comment 9

[Mats Palmgren (inactive)]

Is the patch still valid?

Comment 10

[Samuel Sidler (old account; do not CC)]

Did this just never land? As Mats asks, is this patch still valid? Simon, can you comment or close this bug, whichever is more relevant?

Comment 11

[Simon Fraser [no longer active]]

Inspection shows that this never landed. Over to default owner.

Comment 12

[Zack Weinberg (:zwol)]

Attachment: rediffed against m-c, carbon dropped

The patch here seems to have been forgotten for several years.  As it's on the critical path for bug 90268 (although only on Mac), it would be nice to land it ASAP.

The current widget/src/cocoa code looks to be unchanged since 2003, so I've rediffed against tip of m-c and dropped the widget/src/mac changes (since Carbon is dead).

Comment 13

[Zack Weinberg (:zwol)]

I am tagging this bug for checkin, on the assumption that the reviews from 2003 are still valid.  (The only difference between my new patch and Simon's original patch is I threw out the chunks that applied to code that no longer exists.)  Note however that I do not have a Mac so I cannot test this at all.

Comment 14

[Reed Loden [:reed]]

http://hg.mozilla.org/mozilla-central/rev/210fea7af2bf

Comment 15

[Josh Aas]

Comment on attachment 343993 [details] [diff] [review]
rediffed against m-c, carbon dropped

That patch was *way* too old, we need to re-review it.

Comment 16

[Josh Aas]

Comment on attachment 343993 [details] [diff] [review]
rediffed against m-c, carbon dropped

This lacks obj-c exception guards in "nsChildView::SetParent".

Comment 17

[Josh Aas]

Backed out.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 343993 [details] [diff] [review]
rediffed against m-c, carbon dropped

This is basically fine.

But (as Josh pointed out) you need to add the following ObjC exception
guards at the beginning and end:

  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
  NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;

And I think you should add the following before the kungFuDeathGrip:

  if (mOnDestroyCalled)
    return NS_OK;

Comment 19

[Zack Weinberg (:zwol)]

Attachment: adds exception guards and mOnDestroyCalled check

revised as requested.  I still can't even compile this, though.

Comment 20

[Zack Weinberg (:zwol)]

Attachment: same code as 344101, cosmetic cleanups

I noticed only after uploading that the patch had some unnecessary formatting churn in it, so this takes that out.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 344102 [details] [diff] [review]
same code as 344101, cosmetic cleanups

Can you compile this one? :-)

Comment 22

[Zack Weinberg (:zwol)]

Not personally, but I've been informed that it's now possible to use the try server for this sort of thing.  (I had been under the impression that with my level of access, I could only use the try server to test CVS patches, not moz-central patches.)  There's no label yet, but I think the cycle that started at 9:14:15 is this patch.

Comment 23

[Steven Michaud [:smichaud] (Retired)]

So you mean that you don't have a Mac to compile this on?  (And don't
mean that you tried and failed to compile this patch?)

By the way, you need to specify a "Patch level" of '1' when telling
the tryserver to process a Mercurial patch.

Comment 24

[Zack Weinberg (:zwol)]

(In reply to comment #23)
> So you mean that you don't have a Mac to compile this on?  (And don't
> mean that you tried and failed to compile this patch?)

Correct.  I only have Linux installations to hand.

I wouldn't post a patch that I had gotten compile errors on!

> By the way, you need to specify a "Patch level" of '1' when telling
> the tryserver to process a Mercurial patch.

Yes, I did.  Builds are now at

https://build.mozilla.org/tryserver-builds/2008-10-21_09:[e-mail]-mac-setparent/

so it looks like compilation went fine on all platforms, and I guess we'll find out what Talos thinks of them in a couple hours.

Comment 25

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 344102 [details] [diff] [review]
same code as 344101, cosmetic cleanups

This looks fine to me.

If the tryserver Talos builds indicate any problems (which I think is
highly unlikely), we can deal with that later.

Comment 26

[Zack Weinberg (:zwol)]

Does this need sr or shall I just go ahead and tag it for checkin again?

Comment 27

[Josh Aas]

Comment on attachment 344102 [details] [diff] [review]
same code as 344101, cosmetic cleanups

This still needs review from me and sr.

Comment 28

[Josh Aas]

Comment on attachment 344102 [details] [diff] [review]
same code as 344101, cosmetic cleanups

+  [mView removeFromSuperview];      // we hold a ref to mView, so this is OK

Kill the space between the code and this comment or put the comment above the code.

+  mParentView   = (NSView*)aNewParent->GetNativeData(NS_NATIVE_WIDGET);

I don't think there is any guarantee that this won't be nil no matter how unlikely. Please nil check it and return NS_ERROR_FAILURE if it is false. Perhaps you should do that check before the actual swap so that if it fails you leave things in a clean state.

Comment 29

[Zack Weinberg (:zwol)]

Attachment: (rev 4) make sure parent native widget exists, more formatting [Checkin: Comment 31]

here's the requested changes, will kick off another try server build shortly.

Comment 30

[Zack Weinberg (:zwol)]

builds are at https://build.mozilla.org/tryserver-builds/2008-10-21_09:[e-mail]-mac-setparent/

talos is green, but it would be nice if someone with a mac could try the test described in comment 3 on this build.

Comment 31

[Serge Gautherie (:sgautherie)]

Comment on attachment 344222 [details] [diff] [review]
(rev 4) make sure parent native widget exists, more formatting
[Checkin: Comment 31]

http://hg.mozilla.org/mozilla-central/rev/f226385376aa