918884 - Implement a method to get the DOM element that owns a docshell

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Paul Rouget [:paul]]

With the DOM, we can do window.frameElement. It would be convenient to have a similar method from the docshell.

Here is how we do in JavaScript for the devtools: http://mxr.mozilla.org/mozilla-aurora/source/toolkit/devtools/LayoutHelpers.jsm#370

Comment 1

[Paul Rouget [:paul]]

And for the devtools, this needs to cross the mozbrowser and mozapp iframes.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So for a toplevel content docshell, should this return null or the <xul:browser> in question?

Comment 3

[Paul Rouget [:paul]]

The <xul:browser> if that's possible.

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: getcontainer-wip.patch

This patch implements a GetContainer method to return the result of GetFrameElementInternal, and adds a test for iframes and objects in devtools.  Let me know how many things I've done wrong :).  A couple of questions:

1) Right now, the test fails for the top level window (it is returning the window object instead of the xul element).  The others work, returning the proper [object HTMLIFrameElement] or [object HTMLObjectElement].  Any ideas for how to return the <xul:browser> as in pointed out in Comment 3?  Here is the error message: 
     > ./mach mochitest-browser browser/devtools/inspector/test/browser_inspector_frames.js
     > Container element for main window is xul:browser - Got [object XrayWrapper [object Window]], expected [object XULElement]
2) Should I move the test somewhere else (into dom/tests somewhere)?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 808794 [details] [diff] [review]
getcontainer-wip.patch

Looks great!

This should probably actually be a readonly attribute rather than a method - not sure why I suggested otherwise earlier. Should just be a matter of changing the IDL accordingly.

You'll need to add a security check (as in the other methods, see also bug 775868) lest this be abused by content code.

The comment in the IDL should probably be improved. You could describe it as "this window's frame element" without mentioning the docShell, since that actually isn't involved here, and you should probably also mention that this completely ignores all chrome/content or mozbrowser boundaries, rather than mentioning the specific example of the "top level content docshell".

bz may have additional feedback or concerns.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(In reply to Brian Grinstead [:bgrins] from comment #4)

Oh sorry, missed this comment.

> 1) Right now, the test fails for the top level window (it is returning the
> window object instead of the xul element).  The others work, returning the
> proper [object HTMLIFrameElement] or [object HTMLObjectElement].  Any ideas
> for how to return the <xul:browser> as in pointed out in Comment 3?  Here is
> the error message: 
>      > ./mach mochitest-browser
> browser/devtools/inspector/test/browser_inspector_frames.js
>      > Container element for main window is xul:browser - Got [object
> XrayWrapper [object Window]], expected [object XULElement]

Hmm, interesting. I'm not sure why this would be, offhand, maybe bz knows.

> 2) Should I move the test somewhere else (into dom/tests somewhere)?

The existing test is good, but there should probably also be a test that is devtools-agnostic somewhere under dom (e.g. dom/tests/browser/), which only tests the getter without any dependencies on devtools.

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #6)

OK, thanks for the feedback, I will rebuild a patch with the changes.

> (In reply to Brian Grinstead [:bgrins] from comment #4)
> 
> Oh sorry, missed this comment.
> 
> > 1) Right now, the test fails for the top level window (it is returning the
> > window object instead of the xul element).  The others work, returning the
> > proper [object HTMLIFrameElement] or [object HTMLObjectElement].  Any ideas
> > for how to return the <xul:browser> as in pointed out in Comment 3?  Here is
> > the error message: 
> >      > ./mach mochitest-browser
> > browser/devtools/inspector/test/browser_inspector_frames.js
> >      > Container element for main window is xul:browser - Got [object
> > XrayWrapper [object Window]], expected [object XULElement]
> 
> Hmm, interesting. I'm not sure why this would be, offhand, maybe bz knows.
> 

Oops, nevermind about question (1) - this was caused by a typo on my part.

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: getcontainer-wip1.patch

* Made GetContainerElement a readonly attribute rather than a getter.
* Updated comment in idl (may need to be more clear still)
* Added dom/tests/browser/browser_frame_element.js test to remove the dependency on devtools in testing.  This test is all green.  If there are any other elements worth testing (that can be a frame element) I can add them to the test.

I may actually remove the devtools tests here and instead modify the case in Bug 917448.

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 808816 [details] [diff] [review]
getcontainer-wip1.patch

> bz may have additional feedback or concerns.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 808816 [details] [diff] [review]
getcontainer-wip1.patch

>+  CallQueryInterface(element, aResult);

This will crash with a null-deref if you call this on a toplevel window.

Better to do:

  element.forget(aResult);

f+=me with that.

Comment 11

[Brian Grinstead [:bgrins]]

Attachment: getcontainer.patch

Expanded the test case to include an iframe with the mozbrowser attribute set.  Changed to `element.forget(aResult);` as suggested by bz.
 
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=dd8a633e5e12.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 809231 [details] [diff] [review]
getcontainer.patch

r=me

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab3635da717e

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ab3635da717e