838343 - Convert HTMLFrameElement to WebIDL

Status: RESOLVED FIXED

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

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch 1 - renaming nsHTMLFrameElement to HTMLFrameElement

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Still waiting for green on try.

Comment 3

[Andrea Marchesini [:baku]]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

We clearly have terrible test coverage.  :(

As far as I can tell, this needs to implement MozFrameLoaderOwner like XULElement does and also needs a WebIDL equivalent for nsIDOMMozBrowserFrame and nsIMozBrowserFrame.

Justin, it looks like nsIDOMMozBrowserFrame stuff should just be exposed to untrusted web content and nsIMozBrowserFrame should be [ChromeOnly], right?

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

small indentation problem

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 7

[Boris Zbarsky [:bzbarsky]]

Might be worth putting GetContentDocument and GetContentWindow (and anything else shared with <iframe>, in fact) on the generic frame element, so <iframe> can just pick them up from there too.

Comment 8

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=0a1e9cb05950 green on try

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 711248 [details] [diff] [review]
part 1 - renaming

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

::: content/html/content/src/nsHTMLFrameElement.cpp
@@ +2,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * 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/. */
>  
> +#include "HTMLFrameElement.h"

Is this really needed here? I'd remove it (or leave it in the .cpp) if we can.

Comment 10

[Peter Van der Beken [:peterv]]

Comment on attachment 711250 [details] [diff] [review]
part 2 - webidl

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

::: dom/bindings/Bindings.conf
@@ +407,5 @@
>  },
>  
> +'HTMLFrameElement': {
> +    'hasInstanceInterface': 'nsIDOMHTMLFrameElement',
> +},

You can now drop this (bug 838269 made it obsolete).

::: dom/webidl/HTMLFrameElement.webidl
@@ +9,5 @@
> + * Opera Software ASA. You are granted a license to use, reproduce
> + * and create derivative works of this document.
> + */
> +
> +// http://www.whatwg.org/specs/web-apps/current-work/#dom-frameelement

I think this should be http://www.whatwg.org/specs/web-apps/current-work/#htmlframeelement.

@@ +16,5 @@
> +           attribute DOMString scrolling;
> +           attribute DOMString src;
> +           attribute DOMString frameBorder;
> +           attribute DOMString longDesc;
> +           attribute boolean noResize;

All of these need SetterThrows, no?

@@ +23,5 @@
> +  [GetterThrows]
> +  readonly attribute WindowProxy? contentWindow;
> +
> +  [TreatNullAs=EmptyString] attribute DOMString marginHeight;
> +  [TreatNullAs=EmptyString] attribute DOMString marginWidth;

SetterThrows?

Comment 11

[Andrea Marchesini [:baku]]

(In reply to Peter Van der Beken [:peterv] from comment #9)
> Comment on attachment 711248 [details] [diff] [review]
> part 1 - renaming
> 
> Review of attachment 711248 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/nsHTMLFrameElement.cpp
> @@ +2,5 @@
> >  /* This Source Code Form is subject to the terms of the Mozilla Public
> >   * 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/. */
> >  
> > +#include "HTMLFrameElement.h"
> 
> Is this really needed here? I'd remove it (or leave it in the .cpp) if we
> can.

It's needed. This is the cpp file that defines the class defined by HTMLFrameElement.h
Maybe I can change it to #include "mozilla/dom/HTMLFrameElement.h"

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 14

[Jacek Caban]

https://hg.mozilla.org/integration/mozilla-inbound/rev/abba7ec43f00
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b7b42fdfd57

Comment 15

[Peter Van der Beken [:peterv]]

(In reply to Andrea Marchesini (:baku) from comment #11)
> (In reply to Peter Van der Beken [:peterv] from comment #9)
> > Comment on attachment 711248 [details] [diff] [review]
> > > +#include "HTMLFrameElement.h"
> > 
> > Is this really needed here? I'd remove it (or leave it in the .cpp) if we
> > can.
> 
> It's needed. This is the cpp file that defines the class defined by
> HTMLFrameElement.h

Ugh, that was badly quoted. It was supposed to be about |#include "mozilla/Util.h"|.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> +#include "HTMLFrameElement.h"

Shouldn't that be mozilla/dom/HTMLFrameElement.h ?

Comment 17

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/6b7b42fdfd57
https://hg.mozilla.org/mozilla-central/rev/abba7ec43f00