Closed Bug 1437638 Opened 6 years ago Closed 6 years ago

Implement iframe as a child class of nsXULElement

Categories

(Core :: XUL, task, P5)

task

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: Gijs, Assigned: enndeakin)

References

Details

Attachments

(3 files, 6 obsolete files)

In bug 1403231 we simply added a "src" property to all XUL nodes, and it only really does something useful for image elements.

There are other simple-ish XUL elements around that are implemented in XBL and would benefit from moving into compiled code (that is, the actual implementation is sufficiently simple that as a custom element there'd be more overhead from it being a custom element than we'd gain). :bgrins and I were talking about whether it would be possible to come up with a scheme where these implementations would move directly into C++ without having the overhead of having those properties on every element.

Examples include:

<deck>
<iframe>
potentially "basecontrol"
<resizer>

etc. 

AIUI the current issue is that there's no inheritance on the C++ side - only XULElement - so making those properties not do anything on non-relevant elements is not trivial. This bug is about evaluating whether it's worth changing that for the purposes of implementing "simple" elements in C++ (instead of Custom Elements).
See Also: → 1436351
> without having the overhead of having those properties on every element

Mental overhead or runtime overhead?

There's basically no runtime overhead: they get installed once on the prototype and that's it.

I agree there is mental overhead.

Adding inheritance on the C++ side would not be too terrible work-wise, but would add a bit of overhead to either element creation or element wrapper creation.  You basically have two options.  Either you change nsXULElement::WrapNode to check the tag and call the right binding's Wrap() (so an if cascade in there) or you create C++ subclasses of nsXULElement and make sure that all the places that currently call the constructor call some function that returns the right subclass (with branching on the tag name at that point).
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #1)
> > without having the overhead of having those properties on every element
> 
> Mental overhead or runtime overhead?
> 
> There's basically no runtime overhead: they get installed once on the
> prototype and that's it.
> 
> I agree there is mental overhead.

Well, `selectedIndex` also exist on other things (<radio>, maybe <tabbox>, etc.) so I think it'd be confusing to have one implementation in C++ land specific to one consumer (<deck>) and then overriding them from XBL for other consumers. But maybe I'm overly paranoid in that respect. And then yes, mental overhead in terms of "why does this <box> have a `selectedIndex` property, and what does it even do?".

> Adding inheritance on the C++ side would not be too terrible work-wise, but
> would add a bit of overhead to either element creation or element wrapper
> creation.  You basically have two options.  Either you change
> nsXULElement::WrapNode to check the tag and call the right binding's Wrap()
> (so an if cascade in there) or you create C++ subclasses of nsXULElement and
> make sure that all the places that currently call the constructor call some
> function that returns the right subclass (with branching on the tag name at
> that point).

The overhead will almost certainly be less than the overhead of actually creating XBL bindings, I suppose, but then again we don't create that many <deck> bindings in browser.xul and we'd take the overhead for every XUL element... There's about 1300 non-anon-content ones of those in a blank browser window for me, about 2770 if I log the constructor from the commandline and just open + close Firefox with 1 window. That sounds pretty performance-sensitive.

Would it be possible to hide the properties from JS with webidl based on the element tag name, thus avoiding any constructor overhead? Or is that not clever (or maybe just impossible!) for other reasons?
Flags: needinfo?(bzbarsky)
> Would it be possible to hide the properties from JS with webidl based on the element tag name

Not easily.  The properties are defined on the prototype.  We could do something where we skip the proto lookup based on tagname, but that means making the element wrapper itself a proxy and that has its own performance costs.

Note that for HTML we already pay the cost of a branch on tag to create the right HTML element.  The way it's implemented in the parser is that we have a hashtable mapping tagnames to constructor functions (see the nsHtml5ElementName struct).  For createElement we end up doing a hashtable lookup on the tagname to find an nsHTMLTag id, then indexing into an array of constructor functions with that id.  I think doing something like that for XUL should be fine, fwiw, though obviously we can measure.
Flags: needinfo?(bzbarsky)
I tried an implementation of both a) subclassing nsXULElement and b) changing WrapNode for popup in bug 1446961.
See Also: → 1446961
Priority: -- → P5
Sounds like the main request in this bug is going to be handled in Bug 1446961. Neil, would implementing <iframe> be possible using the same mechanism from that bug? If so, what do you think about repurposing this one to do that?
Flags: needinfo?(enndeakin)
Flags: needinfo?(enndeakin)
Summary: Consider coming up with a scheme to move simple XUL elements into C++ → Implement iframe as a child class of nsXULElement
Attached patch Implement frame subclass (obsolete) — Splinter Review
OK, here is a patch to implement iframe (and browser/editor) that way.
Assignee: nobody → enndeakin
Attached patch Implement frame subclass (obsolete) — Splinter Review
Attachment #8971993 - Attachment is obsolete: true
Attached patch Implement frame subclass (obsolete) — Splinter Review
Attachment #8971995 - Attachment is obsolete: true
Looks like XULFrameElement is quite fitting as a base class for the bindings that currently use these properties.

When all of these binding have also been converted to Custom Elements, I wonder if we could have XULFrameElement implemented in JavaScript and used as a base class, since none of the implementations depend on C++-only internals.
(In reply to :Paolo Amadini from comment #9)
> Looks like XULFrameElement is quite fitting as a base class for the bindings
> that currently use these properties.
> 
> When all of these binding have also been converted to Custom Elements, I
> wonder if we could have XULFrameElement implemented in JavaScript and used
> as a base class, since none of the implementations depend on C++-only
> internals.

I don't think <xul:iframe> will need to be a Custom Element after this change - there's no behavior beyond these properties. Having iframe be a Custom Element (as prototyped in Bug 1411707) felt a bit wrong anyway.

I guess browser and editor will still need JS attached (although I haven't looked closely). We could always look into making those customized autonomous elements (something like <iframe is="moz-editor">).
Attached patch Implement frame subclass (obsolete) — Splinter Review
Now with all tests fixed.
Attachment #8971996 - Attachment is obsolete: true
This part moves the frame loader to the subclass, allowing various checks that currently occur on every element to be removed.
This alternate implementation replaces both of the other patches. Instead of a subclass, it moves iframe to use a custom element and makes browser/editor inherit it. This is simpler code and requires no C++ changes, but prevents the optimization from part 2 from being applicable.
(In reply to Neil Deakin from comment #13)
> Created attachment 8976016 [details] [diff] [review]
> Alternate patch using custom elements
> 
> This alternate implementation replaces both of the other patches. Instead of
> a subclass, it moves iframe to use a custom element and makes browser/editor
> inherit it. This is simpler code and requires no C++ changes, but prevents
> the optimization from part 2 from being applicable.

I like the idea and simplicity here but my only concern with this is that browser and editor would presumably then have both a XBL binding and a Custom Element attached to them. This is something DOM folks have seemed generally concerned about doing - we'd at least want to have them look closer to make sure that won't cause problems.

I'm guessing we could fully convert `editor` to Custom Element but that `browser` would be harder because of the three inherited bindings bound to the same tag name (https://bgrins.github.io/xbl-analysis/tree/#browser). That said, if we could figure out how to flatten those bindings together or otherwise handle `browser` as a Custom Element I'd be all for it. There's some prior discussion about it in Bug 1442058 and Bug 1441935.
I generally think the first approach is probably the way to go anyway.
Adds some additonal tests.
Attachment #8974088 - Attachment is obsolete: true
Attachment #8982389 - Flags: review?(paolo.mozmail)
Comment on attachment 8982389 [details] [diff] [review]
Part 1 - Implement frame subclass

webidl changes. I'm wondering if I should put these in chrome-webidl instead, but test_interfaces fails when running in an xbl context if I do that.
Attachment #8982389 - Flags: review?(bzbarsky)
> but test_interfaces fails when running in an xbl context if I do that.

Well... what behavior do you want?  This bit:

 [HTMLConstructor, Func="IsChromeOrXBL"]
 interface XULFrameElement : XULElement

says the constructor should be exposed to chrome and to XBL scopes.  Then this line in test_interfaces.js:

>+    {name: "XULFrameElement", insecureContext: true, xbl: true},

Says that the test should check that it _is_ exposed in xbl scopes and is _not_ exposed in a normal web page scope.

When you put it in chrome-webidl, did you leave that Func annotation?  What did you put in test_interface.js?  And more importantly, what would you expect the behavior to be in that case?
Flags: needinfo?(enndeakin)
Comment on attachment 8982389 [details] [diff] [review]
Part 1 - Implement frame subclass

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

The implementation bits look good to me, including aligning the subtle differences in how null and QueryInterface were handled in the different elements.

::: dom/xul/nsXULElement.cpp
@@ +177,5 @@
>        nodeInfo->Equals(nsGkAtoms::panel) ||
>        nodeInfo->Equals(nsGkAtoms::tooltip)) {
>      return NS_NewXULPopupElement(nodeInfo.forget());
>    }
> +  else if (nodeInfo->Equals(nsGkAtoms::iframe) ||

nit: "else" on the same line as the brace
Attachment #8982389 - Flags: review?(paolo.mozmail) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #18)
> > but test_interfaces fails when running in an xbl context if I do that.
> 
> Well... what behavior do you want?

I think I want it be exposed the same way that XULElement is now, but without needing an extra review from a dom person for every change. But otherwise, the patch could be left as is.
Flags: needinfo?(enndeakin)
That sounds like we should allow IsChromeOrXBL things to live in chrome-webidl.  How about we do that?  I filed bug 1466255.
Comment on attachment 8982389 [details] [diff] [review]
Part 1 - Implement frame subclass

>+++ b/dom/webidl/XULFrameElement.webidl

Once bug 1466255 is fixed this can go in chrome-webidl.

>+++ b/dom/xul/XULFrameElement.cpp
>+already_AddRefed<nsIDocShell>
>+XULFrameElement::GetDocShell()

Why not just return nsIDocShell*?

>+already_AddRefed<nsIDocument>
>+XULFrameElement::GetContentDocument()

And just nsIDocument* here.

>+++ b/dom/xul/XULFrameElement.h
>+struct JSContext;

Or #include "js/TypeDecls.h" might be better.

>+++ b/dom/xul/nsXULElement.cpp
>+    return NS_NewXULFrameElement(nodeInfo.forget());

This is the only use of NS_NewXULFrameElement, right?  Why does that function even exist, instead of just calling the constructor directly?
Attachment #8982389 - Flags: review?(bzbarsky) → review+
Address comments
This mostly just moves code.
Attachment #8976015 - Attachment is obsolete: true
Attachment #8982389 - Attachment is obsolete: true
Attachment #8987015 - Flags: review?(timdream)
Don't forget to add the constructor here, it needed for customized built-in elements.

https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/dom/bindings/BindingUtils.cpp#3832-3839

if we are going to have more XULElement constructors, perhaps we would need something akin to sConstructorGetterCallback that maps tag names to them.
Comment on attachment 8987015 [details] [diff] [review]
Part 2 - move the frame loader to XULFrameLoader

I don't think I am qualified to review this? Were you meant to find someone else?
Attachment #8987015 - Flags: review?(timdream)
Attachment #8987015 - Flags: review?(bzbarsky)
> Don't forget to add the constructor here, it needed for customized built-in elements.

Good catch.  Neil, can you make sure this happens, please?
Flags: needinfo?(enndeakin)
Comment on attachment 8987015 [details] [diff] [review]
Part 2 - move the frame loader to XULFrameLoader

>+interface MozFrameLoaderOwner {

I'm not a fan of putting this in chrome-webidl, because this mixin is implemented by web-exposed things.  Nothing in our binding infrastructure prevents someone from adding a non-chromeonly thing on this mixin, and there would be no DOM peer review to catch the resulting problem...

Please leave this mixin somewhere in dom/webidl.

>+++ b/dom/html/nsGenericHTMLFrameElement.cpp
>+#include "XULFrameElement.h"

mozilla/dom/XULFrameElement.h

>+++ b/dom/xul/XULFrameElement.cpp
>+  tmp->mFrameLoader = nullptr;
>+  tmp->mOpener = nullptr;

  NS_IMPL_CYCLE_COLLECTION_UNLINK(mFrameLoader, mOpener)

>+XULFrameElement::LoadSrc()

Starting here there's a bunch of 4-space-indented copied code, but this file is 2-space-indented otherwise... please be consistent?

I sort of wish we had a way to do this copy while preserving blame, but part 1 already created the file...

>+XULFrameElement::SwapFrameLoaders(HTMLIFrameElement& aOtherLoaderOwner,
>+    nsCOMPtr<nsIFrameLoaderOwner> flo = do_QueryInterface(ToSupports(this));

Is this QI needed?  "this" just inherits from nsIFrameLoaderOwner, no?

>+XULFrameElement::SwapFrameLoaders(XULFrameElement& aOtherLoaderOwner,
>+    nsCOMPtr<nsIFrameLoaderOwner> flo = do_QueryInterface(ToSupports(this));

Same thing.

>+XULFrameElement::SwapFrameLoaders(nsIFrameLoaderOwner* aOtherLoaderOwner,
>+    nsCOMPtr<nsIFrameLoaderOwner> flo = do_QueryInterface(ToSupports(this));

And here.

>+XULFrameElement::AfterSetAttr(int32_t aNamespaceID, nsAtom* aName,
>+ return nsXULElement::AfterSetAttr(aNamespaceID, aName,

Weird indent here and on next line.

>+++ b/dom/xul/XULFrameElement.h
>+  NS_IMETHOD_(already_AddRefed<nsFrameLoader>) GetFrameLoader() override

We should consider some nostdcall in the idl for nsIFrameLoaderOwner... followup, please.

>+    RefPtr<nsFrameLoader> loader = mFrameLoader;
>+    return loader.forget();

  return do_AddRef(mFrameLoader);

r=me with the nits fixed.
Attachment #8987015 - Flags: review?(bzbarsky) → review+
Pushed by neil@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8319d08895c
move frame, browser and editor to be based on XULFrameElement, a new subclass of XULElement, r=paolo,bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b3654594194
move frame loader property from XULElement to XULFrameElement, removing many checks that only apply to child frames instead of every XUL element. Since it is assumed that most frames/browsers will have frame loaders created for them, and that there aren't many of them, we can use a member field instead of slots, so remove the slot property, r=bz
https://hg.mozilla.org/mozilla-central/rev/d8319d08895c
https://hg.mozilla.org/mozilla-central/rev/9b3654594194
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Flags: needinfo?(enndeakin)
See Also: → 1478139
See Also: → 1441935
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.