280992 - Remove nsIFrameLoader / nsIFrameLoaderOwner

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Brian Ryner (not reading)]

Neither of these interfaces really need to exist...

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 173315 [details] [diff] [review]
patch

r+sr=jst

Comment 3

[Brian Ryner (not reading)]

checked in

Comment 4

[Boris Zbarsky [:bzbarsky]]

Er... I thought the plan was to make these scriptable so we can fix the
completely bogus behavior of XUL iframes and browsers (see bug 265086 comment 6).

Removing these effectively means we will never be able to reasonably
drag-and-drop tabs, for example (not without reinstating something like these
interfaces).

Either that, or I need to start subclassing nsXULElement and having subclasses
claim to be eFRAME_ELEMENT... and cast to nsGenericHTMLFrameElement, which seems
very unlikely.

So unless this is being a measurable perf hit, I'd really like us to revert this
change.

Comment 5

[Brian Ryner (not reading)]

(In reply to comment #4)
> Removing these effectively means we will never be able to reasonably
> drag-and-drop tabs, for example (not without reinstating something like these
> interfaces).

Can you elaborate on that?  It's not really mentioned in bug 265086.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Can you elaborate on that?  It's not really mentioned in bug 265086.

Sure.  We don't want to lose the docshell over a drag operation, and if we
remove and reinsert the node (which is the most sensible way to do a drag even
within a single window, and the only way across windows), and the docshell is
only held by the nsSubDocumentFrame, then it dies when the frame is destroyed.

I was thinking about this some more, and we could probably get away without
nsIFrameLoader if we stored an nsFrameLoader in the boxobject (instead of in the
XBL binding).  We'd still need a way of telling that things could have a
frameloader gotten off them, though.

Comment 7

[Brian Ryner (not reading)]

Attachment: reinstate these as scriptable interfaces

Hopefully this works for what Boris wants to use it for.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173708 [details] [diff] [review]
reinstate these as scriptable interfaces

This will work, though eventually I will need a way to create a frame loader
from XBL and set the content node on it...  I'll need to change
nsIFrameLoaderOwner then too, probably (to get the URI), so I can just add a
way to set the content node then.

Comment 9

[Brian Ryner (not reading)]

checked in.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Er... I'm going to reopen this bug, sorry.  Both patches involved are wrong (and
I need to learn to read more carefully), and as a result XUL iframes (eg the
prefs dialog in seamonkey, DOM Inspector, etc, are broken in today's builds). 
Requesting blocking as a result.

Detailed review comments on both patches coming up.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173315 [details] [diff] [review]
patch

>Index: content/html/content/src/nsGenericHTMLElement.cpp
>@@ -3481,13 +3457,11 @@ nsGenericHTMLFrameElement::SetDocument(n
>-    mFrameLoader->Destroy();

I think we want to keep calling Destroy() here...  That method does cleanup
that needs to happen when the node is removed from the document (in particular
calling SetSubDocumentFor, SetFrameElementInternal, nulling out the
mOwnerContent weak ptr (with this change that can end up dangling!), etc.

>Index: layout/generic/nsFrameFrame.cpp
>-      // ... remember that we own this frame loader...
>-      mOwnsFrameLoader = PR_TRUE;

This change is also wrong.  With this change, not only will the frame miss
calling Destroy() on the frame loader, but it will never change its URI (so
changing the "src" of a XUL iframe doesn't work anymore, hence the prefs and
DOMI bustage).

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173708 [details] [diff] [review]
reinstate these as scriptable interfaces

> nsresult
> nsFrameLoader::LoadFrame()

This needs to become NS_IMETHODIMP.  Same for GetDocShell.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for those issues

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 173774 [details] [diff] [review]
Patch for those issues

r+sr=jst, sorry for not remembering we'd need this to be scriptable on my
initial r+sr :(

Comment 15

[Scott MacGregor]

Also see Bug #281521 which I think bz's patch is going to fix. 

Comment 16

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is there a reason the patch checked in at 2005-02-07 22:55 might have caused
about a 0.5% Tp increase on btek?

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hrm, never mind -- some sort of time zone issue.