Closed
Bug 1437638
Opened 7 years ago
Closed 6 years ago
Implement iframe as a child class of nsXULElement
Categories
(Core :: XUL, task, P5)
Core
XUL
Tracking
()
RESOLVED
FIXED
mozilla63
People
(Reporter: Gijs, Assigned: enndeakin)
References
Details
Attachments
(3 files, 6 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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).
Updated•7 years ago
|
![]() |
||
Comment 1•7 years ago
|
||
> 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).
Reporter | ||
Comment 2•7 years ago
|
||
(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)
![]() |
||
Comment 3•7 years ago
|
||
> 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)
Assignee | ||
Comment 4•7 years ago
|
||
I tried an implementation of both a) subclassing nsXULElement and b) changing WrapNode for popup in bug 1446961.
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Priority: -- → P5
Comment 5•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
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
Assignee | ||
Comment 6•7 years ago
|
||
OK, here is a patch to implement iframe (and browser/editor) that way.
Assignee: nobody → enndeakin
Assignee | ||
Comment 7•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8971993 -
Attachment is obsolete: true
Comment 9•7 years ago
|
||
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.
Comment 10•7 years ago
|
||
(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">).
Assignee | ||
Comment 11•6 years ago
|
||
Now with all tests fixed.
Attachment #8971996 -
Attachment is obsolete: true
Assignee | ||
Comment 12•6 years ago
|
||
This part moves the frame loader to the subclass, allowing various checks that currently occur on every element to be removed.
Assignee | ||
Comment 13•6 years ago
|
||
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.
Comment 14•6 years ago
|
||
(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.
Assignee | ||
Comment 15•6 years ago
|
||
I generally think the first approach is probably the way to go anyway.
Assignee | ||
Comment 16•6 years ago
|
||
Adds some additonal tests.
Attachment #8974088 -
Attachment is obsolete: true
Attachment #8982389 -
Flags: review?(paolo.mozmail)
Assignee | ||
Comment 17•6 years ago
|
||
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)
![]() |
||
Comment 18•6 years ago
|
||
> 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 19•6 years ago
|
||
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+
Assignee | ||
Comment 20•6 years ago
|
||
(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)
![]() |
||
Comment 21•6 years ago
|
||
That sounds like we should allow IsChromeOrXBL things to live in chrome-webidl. How about we do that? I filed bug 1466255.
![]() |
||
Comment 22•6 years ago
|
||
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+
Assignee | ||
Comment 24•6 years ago
|
||
This mostly just moves code.
Attachment #8976015 -
Attachment is obsolete: true
Attachment #8982389 -
Attachment is obsolete: true
Attachment #8987015 -
Flags: review?(timdream)
Comment 25•6 years ago
|
||
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 26•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Attachment #8987015 -
Flags: review?(bzbarsky)
![]() |
||
Comment 27•6 years ago
|
||
> 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 28•6 years ago
|
||
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+
Comment 29•6 years ago
|
||
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
Comment 30•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d8319d08895c https://hg.mozilla.org/mozilla-central/rev/9b3654594194
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(enndeakin)
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
status-firefox61:
--- → wontfix
status-firefox62:
--- → wontfix
Updated•5 years ago
|
Type: enhancement → task
You need to log in
before you can comment on or make changes to this bug.
Description
•