Open Bug 479895 Opened 13 years ago Updated 6 years ago

Create a XUL element subclass for browser/iframe/editor

Categories

(Toolkit :: XUL Widgets, defect)

x86
macOS
defect
Not set
normal

Tracking

()

People

(Reporter: enndeakin, Unassigned)

Details

Attachments

(1 file)

Attached patch implement thisSplinter Review
Split off of bug 350264, this adds a subclass for the three xul frame types. This deprecates the use of nsIContainerBoxObject.
Attachment #363787 - Flags: review?(neil)
Comment on attachment 363787 [details] [diff] [review]
implement this

>+   nsCOMPtr<nsPIDOMWindow> win(do_GetInterface(docShell));
>+   if (!win)
>+     return NS_OK;
>+
>+   NS_ASSERTION(win->IsOuterWindow(),
>+                "Uh, this window should always be an outer window!");
>+   return CallQueryInterface(win, aResult); 
Nit: trailing space. But what I really wanted to mention was that nsPIDOMWindow inherits from nsIDOMWindow, so you don't need to CallQI here. In fact, if it wasn't for the outer window test, I'd use CallGI instead.

>+NS_IMETHODIMP nsXULFrameElement::GetContentDocument(nsIDOMDocument** aResult)
>+{
>+  nsCOMPtr<nsIDOMWindow> window;
>+  GetContentWindow(getter_AddRefs(window));
>+  if (window)
>+    window->GetDocument(aResult);
The problem I have here is that to get the window you have to go via the docShell or the document, and you can get from the docShell to the document without going near the window. In fact, if you don't want to refactor the code, I'd move this back into the xbl (property name="contentDocument" onget="return this.contentWindow && this.contentWindow.document;" perhaps).

>+  return nsnull;
This is wrong ;-) [Also you fail to clear the result if there is no window]

>+interface nsIDOMXULFrameElement : nsIDOMXULElement
This name might be confusing. How about XULContainerElement?

>-  <binding id="browser" extends="xul:browser">
>-    <implementation type="application/javascript" implements="nsIAccessibleProvider, nsIObserver, nsIDOMEventListener">
>+  <binding id="browser" extends="chrome://global/content/bindings/general.xml#iframe">
>+    <implementation type="application/javascript" implements="nsIObserver, nsIDOMEventListener">
>       <property name="accessibleType" readonly="true">
>         <getter>
>           <![CDATA[
>             return Components.interfaces.nsIAccessibleProvider.OuterDoc;
>           ]]>
>         </getter>
>       </property>
This can go too.

>+      <field name="_docShell"/>
Um... unset?

>+          if (!("contentWindow" in this))
>+            return null;
What does this achieve? Shouldn't it at least be an instanceof test?

>+          return this.contentWindow.QueryInterface(Components.interfaces.nsIInterfaceRequestor).
>+                   getInterface(Components.interfaces.nsIWebNavigation);
The problem here for chrome is that when this.contentWindow is unsafe and therefore wrapped in something or other, and so the docshell/webnav object is also returned in a wrapper. (I forgot why, ask mrbkap again.)
Attachment #363787 - Flags: review?(neil) → review-
> The problem I have here is that to get the window you have to go via the
> docShell or the document, and you can get from the docShell to the document
> without going near the window. In fact, if you don't want to refactor the code,
> I'd move this back into the xbl (property name="contentDocument" onget="return
> this.contentWindow && this.contentWindow.document;" perhaps).

But isn't that js code just the same thing?

> >+interface nsIDOMXULFrameElement : nsIDOMXULElement
> This name might be confusing. How about XULContainerElement?

There already is an nsIDOMXULContainerElement ;)

> The problem here for chrome is that when this.contentWindow is unsafe and
> therefore wrapped in something or other, and so the docshell/webnav object is
> also returned in a wrapper. (I forgot why, ask mrbkap again.)

I asked but wasn't sure if you two came to a conclusion on this.
(In reply to comment #2)
> > The problem I have here is that to get the window you have to go via the
> > docShell or the document, and you can get from the docShell to the document
> > without going near the window. In fact, if you don't want to refactor the code,
> > I'd move this back into the xbl (property name="contentDocument" onget="return
> > this.contentWindow && this.contentWindow.document;" perhaps).
> But isn't that js code just the same thing?
Yes, but my point is that it you are going to implement contentDocument in C++ then I'd prefer a better implementation than a transliteration of the JS.

> > >+interface nsIDOMXULFrameElement : nsIDOMXULElement
> > This name might be confusing. How about XULContainerElement?
> There already is an nsIDOMXULContainerElement ;)
D'oh!

> > The problem here for chrome is that when this.contentWindow is unsafe and
> > therefore wrapped in something or other, and so the docshell/webnav object is
> > also returned in a wrapper. (I forgot why, ask mrbkap again.)
> I asked but wasn't sure if you two came to a conclusion on this.
I forget, and I don't have scrollback any more...
Assignee: enndeakin → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.