844169 - Move HTMLIFrameElement to WebIDL

Status: RESOLVED FIXED

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

Comment 1

[(no longer active)]

I have the patches for this bug, but I need to know the answers to two questions:

1. Our allowfullscreen XPCOM property <http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/html/nsIDOMHTMLIFrameElement.idl#41> has the wrong case (should be allowFullscreen).  Should I just rename the XPCOM property and update the code/tests that depend on it?

2. nsHTMLIFrameElement implements nsIDOMGetSVGDocument which provides the getSVGDocument interface.  As far as I can tell, this interface maps to the GetSVGDocument interface in the SVG spec <http://www.w3.org/TR/SVG/struct.html#InterfaceGetSVGDocument>.  Should I just add that interface and say "HTMLIFrameElement implements GetSVGDocument;"?  The HTML spec doesn't mention this interface at all.

Comment 2

[(no longer active)]

(In reply to :Ehsan Akhgari from comment #1)
> 2. nsHTMLIFrameElement implements nsIDOMGetSVGDocument which provides the
> getSVGDocument interface.  As far as I can tell, this interface maps to the
> GetSVGDocument interface in the SVG spec
> <http://www.w3.org/TR/SVG/struct.html#InterfaceGetSVGDocument>.  Should I
> just add that interface and say "HTMLIFrameElement implements
> GetSVGDocument;"?  The HTML spec doesn't mention this interface at all.

(This was implemented in bug 287465.)

Comment 3

[(no longer active)]

(And if I need to add the GetSVGDocument Web IDL interface, should I mark it as [NoInterfaceObject]?  If so, where can I file a bug against the SVG spec to add that change?)

Comment 4

[(no longer active)]

Another question, http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/test_iframe_sandbox_general.html?force=1#94 seems to check that the default sandbox property returns an empty string.  With my patches, it returns something which converts to an empty string (== "" returns true but === "" doesn't).  I'm not sure what that means...

Comment 5

[(no longer active)]

Attachment: Part 1: Rename nsHTMLIFrameElement to mozilla::dom::HTMLIFrameElement

Comment 6

[(no longer active)]

Attachment: Part 2: Move HTMLIFrameElement to Web IDL bindings

Comment 7

[(no longer active)]

Ah, there is also the nsIDOMMozBrowserFrame which we probably need to support in the Web IDL binding.  Should mozbrowser be exposed to content or just to chrome?

Comment 8

[(no longer active)]

(In reply to :Ehsan Akhgari from comment #7)
> Ah, there is also the nsIDOMMozBrowserFrame which we probably need to
> support in the Web IDL binding.  Should mozbrowser be exposed to content or
> just to chrome?

(See also bug 844462 -- we forgot to handle this for HTMLFrameElement too.)

Comment 9

[(no longer active)]

(In reply to :Ehsan Akhgari from comment #7)
> Ah, there is also the nsIDOMMozBrowserFrame which we probably need to
> support in the Web IDL binding.  Should mozbrowser be exposed to content or
> just to chrome?

Justin might know what we should do here too...

Comment 10

[Boris Zbarsky [:bzbarsky]]

I believe the claim was that mozbrowser only needs to be exposed to chrome.  Justin?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Should I just rename the XPCOM property and update the code/tests that depend on it?

Chris?  What happened here is that we implemented the spec proposal, and then the spec got bikeshedded to a change of case, iirc.  What's not clear to me is how stable the post-bikeshedding state is.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Should I just add that interface and say "HTMLIFrameElement implements GetSVGDocument;"?

For <object> and <embed> I just put that on a partial interface.

Worth checking what the SVG spec is actually doing here.  The problem is that I can't find a non-obsolete version in actual readable form and heycam is on vacation.  :(

Comment 13

[Boris Zbarsky [:bzbarsky]]

Bugs against the SVG spec presumably go to [e-mail], cc heycam and jwatt.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> I'm not sure what that means...

In the XPCOM binding, .sandbox returns a string.

In the WebIDL binding it returns an object with a stringifier.  Being an object, it's not === to any string, but if you use == the stringifier is invoked to produce a string, and in this case that returns "".

What we should be doing here, I'm not sure.  The spec for .sandbox was insane last I checked it, though hixie is more or less ignoring my feedback to that effect.  For now it might be best to keep the XPCOM behavior and file a bug on the discrepancy with the current spec draft or something.

Comment 15

[Justin Lebar (not reading bugmail)]

(In reply to Boris Zbarsky (:bz) from comment #10)
> I believe the claim was that mozbrowser only needs to be exposed to chrome. 
> Justin?

I may have said the wrong thing before; I'm not sure anymore.  Is the question whether we expect the following code to work in content

  var iframe = document.createElement('iframe');
  iframe.mozbrowser = true;
  document.body.appendChild(iframe);

?  I guess it would be nice if that worked.  But we can also use setAttribute (right?).  If we're going to break the idiom above, we should ensure that Gaia isn't using it (grepping for 'mozbrowser' should be sufficient).  I guess our tests aren't using it, since they aren't all orange.

Comment 16

[(no longer active)]

(In reply to comment #15)
> (In reply to Boris Zbarsky (:bz) from comment #10)
> > I believe the claim was that mozbrowser only needs to be exposed to chrome. 
> > Justin?
> 
> I may have said the wrong thing before; I'm not sure anymore.  Is the question
> whether we expect the following code to work in content
> 
>   var iframe = document.createElement('iframe');
>   iframe.mozbrowser = true;
>   document.body.appendChild(iframe);

Well, that, and:

  var supportsMozbrowser = "mozbrowser" in iframe;
  // should supportsMozbrowser be true here?

> ?  I guess it would be nice if that worked.  But we can also use setAttribute
> (right?).

Yeah, setAttribute will continue to work regardless.

>  If we're going to break the idiom above, we should ensure that Gaia
> isn't using it (grepping for 'mozbrowser' should be sufficient).  I guess our
> tests aren't using it, since they aren't all orange.

Hmm, is that enough?  Don't we support mozbrowser as an API which others can use to write browser apps?  (Well, I'm not sure if there is a "spec" for mozbrowser and how it's supposed to work.)

Comment 17

[(no longer active)]

(In reply to comment #14)
> > I'm not sure what that means...
> 
> In the XPCOM binding, .sandbox returns a string.
> 
> In the WebIDL binding it returns an object with a stringifier.  Being an
> object, it's not === to any string, but if you use == the stringifier is
> invoked to produce a string, and in this case that returns "".

I see.  Actually that should have been obvious, not sure why I got confused.

> What we should be doing here, I'm not sure.  The spec for .sandbox was insane
> last I checked it, though hixie is more or less ignoring my feedback to that
> effect.  For now it might be best to keep the XPCOM behavior and file a bug on
> the discrepancy with the current spec draft or something.

Keeping the XPCOM behavior means making it a string property, right?

I'm not sure what discrepancy you're talking about with regards to the spec draft...

Comment 18

[(no longer active)]

(In reply to comment #12)
> > Should I just add that interface and say "HTMLIFrameElement implements GetSVGDocument;"?
> 
> For <object> and <embed> I just put that on a partial interface.
> 
> Worth checking what the SVG spec is actually doing here.  The problem is that I
> can't find a non-obsolete version in actual readable form and heycam is on
> vacation.  :(

So I guess I can just put it on a partial interface for now, especially since that's what you've already done, and we might as well be consistent in our not-knowing-what-to-do.  :-)

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Keeping the XPCOM behavior means making it a string property, right?

Yes.

> I'm not sure what discrepancy you're talking about with regards to the spec draft...

The one we'd have wrt the spec if we leave it a string property.

Comment 20

[Justin Lebar (not reading bugmail)]

> Hmm, is that enough?  Don't we support mozbrowser as an API which others can use to write browser 
> apps?  (Well, I'm not sure if there is a "spec" for mozbrowser and how it's supposed to work.)

We do, but such apps are "privileged", which means we have to review them.  Given that, plus the fact that the API is essentially un-documented and also going to change significantly going forward, I'm not particularly worried about breaking apps.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 717473 [details] [diff] [review]
Part 1: Rename nsHTMLIFrameElement to mozilla::dom::HTMLIFrameElement

r=me

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

It seems kind of silly to me that the HTMLIFrameElement.allowFullscreen IDL attribute reflects the allowfullscreen content attribute which differs in spelling (capitalization of 'f'), but it looks like that's the convention used elsewhere.

Anne: Is just to clarify, it looks like camelCase is the convention for IDL attributes, and all lower case is the convention for content attributes?

Comment 23

[Anne (:annevk)]

Yes. It's all over HTML, e.g. crossorigin vs crossOrigin. And fullscreen is a single word in the unprefixed world.

Comment 24

[(no longer active)]

Comment on attachment 717473 [details] [diff] [review]
Part 1: Rename nsHTMLIFrameElement to mozilla::dom::HTMLIFrameElement

https://hg.mozilla.org/integration/mozilla-inbound/rev/62243742ec76

Comment 25

[(no longer active)]

(In reply to Boris Zbarsky (:bz) from comment #19)
> > I'm not sure what discrepancy you're talking about with regards to the spec draft...
> 
> The one we'd have wrt the spec if we leave it a string property.

Ah, I read that as you asking me to file a spec bug for some reason!  Filed bug 845057.

(In reply to Justin Lebar [:jlebar] from comment #20)
> > Hmm, is that enough?  Don't we support mozbrowser as an API which others can use to write browser 
> > apps?  (Well, I'm not sure if there is a "spec" for mozbrowser and how it's supposed to work.)
> 
> We do, but such apps are "privileged", which means we have to review them. 
> Given that, plus the fact that the API is essentially un-documented and also
> going to change significantly going forward, I'm not particularly worried
> about breaking apps.

OK, sounds good.  Thanks!

(We still need Chris to weigh in on allowFullscreen...)

Comment 26

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to :Ehsan Akhgari from comment #1)
> I have the patches for this bug, but I need to know the answers to two
> questions:
> 
> 1. Our allowfullscreen XPCOM property
> <http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/html/
> nsIDOMHTMLIFrameElement.idl#41> has the wrong case (should be
> allowFullscreen).  Should I just rename the XPCOM property and update the
> code/tests that depend on it?

Yes, please just rename the XPCOM property and update any tests/code that depend on that.

Comment 27

[(no longer active)]

Attachment: Part 2: Rename nsIDOMHTMLIFrameElement.allowfullscreen to allowFullscreen

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/62243742ec76

Comment 29

[(no longer active)]

Comment on attachment 718195 [details] [diff] [review]
Part 2: Rename nsIDOMHTMLIFrameElement.allowfullscreen to allowFullscreen

https://hg.mozilla.org/integration/mozilla-inbound/rev/91d78336d372

Comment 30

[(no longer active)]

Attachment: Part 3: Move HTMLIFrameElement to Web IDL bindings

Comment 31

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=3110358f4d26

Comment 32

[(no longer active)]

Comment on attachment 718219 [details] [diff] [review]
Part 3: Move HTMLIFrameElement to Web IDL bindings

This still needs more work...

Comment 33

[Boris Zbarsky [:bzbarsky]]

Do any websites use .allowfullscreen in practice?  If so, we may want both names alongside each other....

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/91d78336d372

Comment 35

[Anne (:annevk)]

I should have said this here: WebKit's code does not appear to have an IDL attribute for allowfullscreen so impact is likely minimal on that front.

Comment 36

[(no longer active)]

So, I was writing a patch to make sure that iframe.mozbrowser is only accessed with chrome privileges, which requires a whole bunch of stuff to be wrapped in SpecialPowers.wrap, and I hit bug 845379, which makes this impossible to fix.

Comment 37

[(no longer active)]

Attachment: Part 3: Make sure that all callers of HTMLIFrameElement.mozbrowser have chrome privileges

This is the patch I was talking about.

Comment 38

[Justin Lebar (not reading bugmail)]

> So, I was writing a patch to make sure that iframe.mozbrowser is only accessed with chrome 
> privileges, which requires a whole bunch of stuff to be wrapped in SpecialPowers.wrap,

Is setAttribute insufficient for some reason?

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 718471 [details] [diff] [review]
Part 3: Make sure that all callers of HTMLIFrameElement.mozbrowser have chrome privileges

r=me

Comment 40

[(no longer active)]

(In reply to Justin Lebar [:jlebar] from comment #38)
> > So, I was writing a patch to make sure that iframe.mozbrowser is only accessed with chrome 
> > privileges, which requires a whole bunch of stuff to be wrapped in SpecialPowers.wrap,
> 
> Is setAttribute insufficient for some reason?

No, I'm just retarded and didn't think of it in time.  :)

Comment 41

[(no longer active)]

Comment on attachment 718471 [details] [diff] [review]
Part 3: Make sure that all callers of HTMLIFrameElement.mozbrowser have chrome privileges

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

Comment 42

[(no longer active)]

So I did a try push on the final patch which I will attach in a sec, and here's the results: <https://tbpl.mozilla.org/?tree=Try&rev=02af205fbc9c>  There's a number of more test failures which are not obvious to me, and I'm afraid I won't have time in the near future to debug them.  It would be great if somebody else can take this over.  Sorry for dropping the ball here. :(

Comment 43

[(no longer active)]

Attachment: Part 4: Move HTMLIFrameElement to Web IDL bindings (WIP)

Comment 44

[(no longer active)]

Oh, one other thing that I just realized I forgot to do was to check gaia for .mozbrowser accesses...

Comment 45

[Ryan VanderMeulen [:RyanVM]]

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

Comment 46

[Boris Zbarsky [:bzbarsky]]

So the failures there seem to be made up of test_focus_disabled.html which was just busted and has since been backed out and various mozbrowser stuff that's likely to be due to nsIMozBrowserFrame not being implemented in the new setup.

Comment 47

[Boris Zbarsky [:bzbarsky]]

In Gaia, apps/browser/js/browser.js has a .mozbrowser set.

Should I just make that attribute non-chromeonly but conditioned on the "dom.mozBrowserFramesEnabled" pref or something else that would only expose it where it should be exposed?

For that matter, where should the nsIMozBrowserFrame stuff go?  Should that be chromeonly, or do untrusted things need it?  I suppose I could just query for all the uses in m-c and gaia if the answer is not known offhand...

Comment 48

[Boris Zbarsky [:bzbarsky]]

For what it's worth, as far as I can tell the only QI to nsIMozBrowserFrame in JS code is in a jsm.... at least in m-c and gaia.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Confirmed that just throwing appManifestURL on HTMLIFrameElement fixes those oranges from that try run that I could reproduce locally.  Justin, do I need to expose any of the rest of nsIMozBrowserFrame to script?

Comment 50

[(no longer active)]

(In reply to comment #49)
> Confirmed that just throwing appManifestURL on HTMLIFrameElement fixes those
> oranges from that try run that I could reproduce locally.  Justin, do I need to
> expose any of the rest of nsIMozBrowserFrame to script?

Gah, I did see that interface but then I said to myself it doesn't have DOM in its name, so perhaps it's not meant to be exposed to js.  Sorry for not fact checking that idea.

Comment 51

[Boris Zbarsky [:bzbarsky]]

Well, it's not exposed by default.  You have to explicitly QI to nsIMozBrowserFrame to get those members right now.  [ChromeOnly] solves that problem, of course.

Comment 52

[Justin Lebar (not reading bugmail)]

> Justin, do I need to expose any of the rest of nsIMozBrowserFrame to script?

To Chrome script?  If the tests pass, you likely don't need to.  So long as it's easy to add another attribute (seems so), I don't mind if we expose as little as we can get away with.

Comment 53

[Boris Zbarsky [:bzbarsky]]

OK.  At this point this seems to be all green except for one thing: every single b2g test is red with this:

  INFO -    File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/marionette/marionette.py", line 290, in _handle_error
10:41:55    ERROR -      raise JavascriptException(message=message, status=status, stacktrace=stacktrace)
10:41:55    ERROR -  marionette.errors.JavascriptException: TypeError: currentAppFrame(...) is null at: app://system.gaiamobile.org/js/wrapper.js line: 17
10:41:56    ERROR - Return code: 1

I tried making mozbrowser not be ChromeOnly, but that doesn't change anything.

I also tried running b2g mochitests locally in a desktop b2g build.  That seems to work fine....

Comment 54

[Justin Lebar (not reading bugmail)]

That error is a bit odd to be happening in isolation.  Perhaps another error is hiding earlier up in the log?  We're pretty good about making them hard to find.

Comment 55

[Boris Zbarsky [:bzbarsky]]

I didn't see anything obvious, but I'm not quite sure what to be looking for.  There's a log at https://tbpl.mozilla.org/php/getParsedLog.php?id=20188914&tree=Try&full=1 if you're willing to take a look too...

So I tried pushing with HTMLIFrameElement still on XPCOM bindings but WebIDL quickstubs installed for the HTMLIFrameElement interface, all interfaces other than that one disabled in classinfo, and everything except appManifestURL nsIMozBrowserFrame marked noscript, to mimic as closely as I can the actual behavior with WebIDL.  That run is green...

Comment 56

[Justin Lebar (not reading bugmail)]

I don't see anything either.  :-/

Comment 57

[Boris Zbarsky [:bzbarsky]]

OK, I believe I know why this fails now.

The b2g code is firing events from a jsm on a content iframe, then has event listeners in the content that end up throwing.  Right now the DOM error reporter sees those exceptions, sees that there is an XPCCallContext on the stack (from the XPCWN xray) and suppresses the exception.

With this patch, WebIDL xrays have no XPCCallContext, so the exception is actually reported from the event listener and the test harness sees an uncaught exception and kills the test run.  This all happens during startup of the emulator, so it affects every single test.

Comment 58

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated WIP

Comment 59

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4: Move HTMLIFrameElement to Web IDL bindings;

Comment 60

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 727755 [details] [diff] [review]
Part 4: Move HTMLIFrameElement to Web IDL bindings;

Review of attachment 727755 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/HTMLIFrameElement.h
@@ +9,5 @@
>  #include "nsGenericHTMLFrameElement.h"
>  #include "nsIDOMHTMLIFrameElement.h"
>  #include "nsIDOMGetSVGDocument.h"
>  
> +class nsXULElement;

Why?

@@ +67,5 @@
> +  void SetSrc(const nsAString& aSrc, ErrorResult& aError)
> +  {
> +    SetHTMLAttr(nsGkAtoms::src, aSrc, aError);
> +  }
> +  void GetName(DOMString& aName)

const, and some more

@@ +77,5 @@
> +    SetHTMLAttr(nsGkAtoms::name, aName, aError);
> +  }
> +  nsDOMSettableTokenList* Sandbox()
> +  {
> +    return GetTokenList(nsGkAtoms::sandbox);

// We're implementing sandbox as a string for now, see bug 845057.

Drop this for now?

@@ +79,5 @@
> +  nsDOMSettableTokenList* Sandbox()
> +  {
> +    return GetTokenList(nsGkAtoms::sandbox);
> +  }
> +  bool AllowFullscreen()

const

@@ +131,5 @@
> +    SetHTMLAttr(nsGkAtoms::frameborder, aFrameBorder, aError);
> +  }
> +  void GetLongDesc(DOMString& aLongDesc)
> +  {
> +    GetHTMLAttr(nsGkAtoms::longdesc, aLongDesc);

This is a URI attribute currently.

@@ +157,5 @@
> +  nsIDocument* GetSVGDocument()
> +  {
> +    return GetContentDocument();
> +  }
> +  bool Mozbrowser()

const

@@ +165,5 @@
> +  void SetMozbrowser(bool aAllow, ErrorResult& aError)
> +  {
> +    SetHTMLBoolAttr(nsGkAtoms::mozbrowser, aAllow, aError);
> +  }
> +  using nsGenericHTMLFrameElement::SetMozbrowser;

Why?

@@ +173,5 @@
>  protected:
>    virtual void GetItemValueText(nsAString& text);
>    virtual void SetItemValueText(const nsAString& text);
> +
> +  virtual JSObject* WrapNode(JSContext *aCx, JSObject *aScope) MOZ_OVERRIDE;

* to the left

::: content/html/content/src/nsGenericHTMLFrameElement.h
@@ +5,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#ifndef nsGenericHTMLFrameElement_h
> +#define nsGenericHTMLFrameElement_h

Heh.

Comment 61

[Boris Zbarsky [:bzbarsky]]

> Why?

Not needed anymore. Removed.

> const, and some more

Can't do, because it makes the call from the binding ambiguous.

> Drop this for now?

Er, should be a GetHTMLAttr.  Good catch.

> const

Done for AllowFullscreen, Mozbrowser.

> This is a URI attribute currently.

Good catch, fixed.

> Why?

Because otherwise I get warnings about hidden overloaded virtual functions.

> * to the left

Done.

Comment 62

[Boris Zbarsky [:bzbarsky]]

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

Comment 63

[Ed Morley [:emorley]]

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