Closed Bug 168117 Opened 17 years ago Closed 11 years ago
Need to implement ns
IWidget::Set Parent on the Mac
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.
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?
Comment on attachment 119235 [details] [diff] [review] Implement SetParent for carbon and cocoa widgets r=pink
Attachment #119235 - Flags: review?(pinkerton) → review+
Attachment #119235 - Flags: superreview?(bryner) → superreview+
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
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.
Status: NEW → RESOLVED
Closed: 11 years ago
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.
Comment on attachment 343993 [details] [diff] [review] rediffed against m-c, carbon dropped This lacks obj-c exception guards in "nsChildView::SetParent".
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.
I noticed only after uploading that the patch had some unnecessary formatting churn in it, so this takes that out.
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:firstname.lastname@example.org/ 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.
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.
builds are at https://build.mozilla.org/tryserver-builds/2008-10-21_09:email@example.com/ 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: superreview?(roc) → superreview+
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 ago → 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.