Closed Bug 168117 Opened 17 years ago Closed 11 years ago

Need to implement nsIWidget::SetParent on the Mac

Categories

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

PowerPC
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: kmcclusk, Assigned: zwol)

References

()

Details

(Keywords: top100, Whiteboard: whitebox)

Attachments

(1 file, 5 obsolete files)

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.
Keywords: nsbeta1, top100
Priority: -- → P1
Target Milestone: --- → mozilla1.3alpha
Whiteboard: whitebox
Blocks: 135263
-> sfraser
Assignee: dcone → sfraser
Target Milestone: mozilla1.3alpha → mozilla1.4beta
Do we have a testcase?
Status: NEW → ASSIGNED
Hardware: PC → Macintosh
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
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?
Attachment #119235 - Flags: superreview?(bryner)
Attachment #119235 - Flags: review?(pinkerton)
Comment on attachment 119235 [details] [diff] [review]
Implement SetParent for carbon and cocoa widgets

r=pink
Attachment #119235 - Flags: review?(pinkerton) → review+
nsbeta1+
Keywords: nsbeta1nsbeta1+
Attachment #119235 - Flags: superreview?(bryner) → superreview+
Is the patch still valid?
Component: GFX → Widget: Mac
Target Milestone: mozilla1.4beta → ---
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?
QA Contact: chrispetersen → mac
Inspection shows that this never landed. Over to default owner.
Assignee: sfraser_bugs → joshmoz
Status: ASSIGNED → NEW
Assignee: joshmoz → nobody
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).
Attachment #119235 - Attachment is obsolete: true
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.
Keywords: checkin-needed
Assignee: nobody → sfraser_bugs
http://hg.mozilla.org/mozilla-central/rev/210fea7af2bf
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
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.
Attachment #343993 - Flags: review?(smichaud)
Attachment #343993 - Flags: review?(joshmoz)
Comment on attachment 343993 [details] [diff] [review]
rediffed against m-c, carbon dropped

This lacks obj-c exception guards in "nsChildView::SetParent".
Backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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;
revised as requested.  I still can't even compile this, though.
Assignee: sfraser_bugs → zweinberg
Attachment #343993 - Attachment is obsolete: true
Attachment #344101 - Flags: review?(smichaud)
Attachment #343993 - Flags: review?(smichaud)
Attachment #343993 - Flags: review?(joshmoz)
I noticed only after uploading that the patch had some unnecessary formatting churn in it, so this takes that out.
Attachment #344101 - Attachment is obsolete: true
Attachment #344102 - Flags: review?(smichaud)
Attachment #344101 - Flags: review?(smichaud)
Comment on attachment 344102 [details] [diff] [review]
same code as 344101, cosmetic cleanups

Can you compile this one? :-)
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.
Assignee: zweinberg → sfraser_bugs
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.
(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:14-zweinberg@mozilla.com-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 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.
Attachment #344102 - Flags: review?(smichaud) → review+
Does this need sr or shall I just go ahead and tag it for checkin again?
Comment on attachment 344102 [details] [diff] [review]
same code as 344101, cosmetic cleanups

This still needs review from me and sr.
Attachment #344102 - Flags: review?(joshmoz)
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.
here's the requested changes, will kick off another try server build shortly.
Assignee: sfraser_bugs → zweinberg
Attachment #344102 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #344222 - Flags: review?(joshmoz)
Attachment #344102 - Flags: review?(joshmoz)
builds are at https://build.mozilla.org/tryserver-builds/2008-10-21_09:14-zweinberg@mozilla.com-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.
Attachment #344222 - Flags: review?(joshmoz) → review+
Attachment #344222 - Flags: superreview?(roc)
Attachment #344222 - Flags: superreview?(roc) → superreview+
Keywords: checkin-needed
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
Attachment #344222 - Attachment description: (rev 4) make sure parent native widget exists, more formatting → (rev 4) make sure parent native widget exists, more formatting [Checkin: Comment 31]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.