Last Comment Bug 821606 - Turn on WebIDL bindings for Element and HTMLElement
: Turn on WebIDL bindings for Element and HTMLElement
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla20
Assigned To: Peter Van der Beken [:peterv] - away till Aug 1st
:
Mentors:
Depends on: 815149 820577 824301 825053 825138 825473 825638 826892 827232 827546 827692 828606 828728 828767 830445 830631 832320 832334 835838 862090
Blocks: ParisBindings 823394 826738
  Show dependency treegraph
 
Reported: 2012-12-13 22:48 PST by Boris Zbarsky [:bz]
Modified: 2013-04-15 17:17 PDT (History)
9 users (show)
Ms2ger: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (20.48 KB, patch)
2012-12-19 07:31 PST, Peter Van der Beken [:peterv] - away till Aug 1st
no flags Details | Diff | Splinter Review
v1.1 (20.44 KB, patch)
2012-12-19 14:28 PST, Peter Van der Beken [:peterv] - away till Aug 1st
bzbarsky: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2012-12-13 22:48:10 PST
Just to get this on file so we can track it...

We'll need to actually SetIsDOMBinding() on Element and override on its various immediate subclasses, because there are elements that have Element.prototype as the proto; see bug 817420.

This depends on getting reparenting and SOWs in, of course.
Comment 1 Peter Van der Beken [:peterv] - away till Aug 1st 2012-12-19 07:31:16 PST
Created attachment 693874 [details] [diff] [review]
v1
Comment 2 Peter Van der Beken [:peterv] - away till Aug 1st 2012-12-19 07:34:04 PST
(In reply to Boris Zbarsky (:bz) from comment #0)
> We'll need to actually SetIsDOMBinding() on Element and override on its
> various immediate subclasses

I don't think that's true, we just need to SetIsDOMBinding() on nsXMLElement.
Comment 3 Peter Van der Beken [:peterv] - away till Aug 1st 2012-12-19 07:36:25 PST
And I'm actually tempted to make these all non-prefable.
Comment 4 Boris Zbarsky [:bz] 2012-12-19 12:04:21 PST
> I don't think that's true, we just need to SetIsDOMBinding() on nsXMLElement.

Ah, didn't realize we had a concrete class for "random Element".  Great!

> And I'm actually tempted to make these all non-prefable.

I agree.
Comment 5 Peter Van der Beken [:peterv] - away till Aug 1st 2012-12-19 14:28:53 PST
Created attachment 694076 [details] [diff] [review]
v1.1
Comment 6 Boris Zbarsky [:bz] 2012-12-19 17:42:08 PST
Comment on attachment 694076 [details] [diff] [review]
v1.1

Those various WrapNode methods should be MOZ_OVERRIDE, right?

>+++ b/dom/bindings/Bindings.conf
> 'Element': {
>+    'concrete': True,

Why is that needed?  That should be the default.

>+'HTMLDivElement': {
>+    'concrete': True,

And here.  Furthermore, I don't think you need hasXPConnectImpls for HTMLDivElement.  There aren't any of those, since nothing subclasses it, right?

> 'HTMLElement': {
>+    'concrete': True,

Again, should not be needed.

> 'HTMLUnknownElement': {
>+    'concrete': True,

Or here.  And again, HTMLUnknownElement does not need hasXPConnectImpls.

r=me with the above nits picked.  I assume you don't need the NEW_BINDING thing for HTMLDivElement and HTMLUnknownElement is because no one ever uses them as argument types?
Comment 7 Masatoshi Kimura [:emk] 2012-12-19 18:16:57 PST
(In reply to Boris Zbarsky (:bz) from comment #6)
> >+'HTMLDivElement': {
> >+    'concrete': True,
> 
> And here.  Furthermore, I don't think you need hasXPConnectImpls for
> HTMLDivElement.  There aren't any of those, since nothing subclasses it,
> right?

Let's use MOZ_FINAL to ensure that.
Comment 8 Boris Zbarsky [:bz] 2012-12-19 19:39:13 PST
> Let's use MOZ_FINAL to ensure that.

Excellent idea.
Comment 9 Peter Van der Beken [:peterv] - away till Aug 1st 2012-12-21 10:35:00 PST
(In reply to Boris Zbarsky (:bz) from comment #6)
> I assume you don't need the NEW_BINDING
> thing for HTMLDivElement and HTMLUnknownElement is because no one ever uses
> them as argument types?

No. We need a NEW_BINDING for every interface that's in the bitmaps (see DOMCI_CASTABLE_INTERFACES) that can also be implemented by a new binding. Because we special-case unwrapping for interfaces in the bitmaps and there's no fallback if we don't have a bitmap, which will happen with a new binding object, I added a fallback that uses the prototype ID. NEW_BINDING enables that fallback. And it turned out that I forgot to hook up EventTarget to it, so I added that.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6d996266cfe
Comment 11 Jean-Yves Perrier [:teoli] 2013-01-11 01:08:01 PST
As this lead to several compatibility problems with Web sites I would like to be sure to document it into:
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_20

But I can't really grasp what changed (.style removed? On which DOM object) due to the WebIDL migration of these elements. Could you help me?
Comment 12 :Ms2ger 2013-01-11 01:21:00 PST
(In reply to Jean-Yves Perrier [:teoli] from comment #11)
> As this lead to several compatibility problems with Web sites I would like
> to be sure to document it into:
> https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_20
> 
> But I can't really grasp what changed (.style removed? On which DOM object)
> due to the WebIDL migration of these elements. Could you help me?

No attributes should have been removed. The main compat-affecting changes are the following:

* Where we used to have all members of the entire inheritance chain (e.g. HTMLDivElement -> HTMLElement -> Element -> Node -> EventTarget) on the interface prototype object of the leaf class (e.g. HTMLDivElement.prototype === document.createElement("div").__proto), we now put the members of HTMLElement just on HTMLElement.prototype (=== HTMLDivElement.prototype.__proto__). It turns out that Optimizely cleared the members of HTMLElement.prototype. This used to be no problem, because those members would be on HTMLDivElement.prototype, but caused a number of sites to break. (E.g., setting fooElement.innerHTML would no longer have an effect, because the setter that did the work was deleted.)

* Passing |null| to a function that takes a |DOMString| (as opposed to a |DOMString?|, which can take |null|), will stringify to "null" instead of the empty string, as XPConnect did. (In fact, this happened in bug 814195 and bug 818219 already.) In particular, element.setAttribute("foo", null) is apparently somewhat popular.

* (WebIDL object).QueryInterface(any interface) now always succeeds, regardless of whether the object implements the passed interface. I believe we'll fix this (bug 827546).

I may be forgetting something, but those are the important ones, at least.
Comment 13 Boris Zbarsky [:bz] 2013-01-11 08:51:06 PST
A few more that may be worth mentioning:

* instanceof now follows the spec for some elements in that "foo instanceof HTMLFooElement" will test false if the RHS and LHS come from different windows.  This is not the case for all elements that we have converted, but for some of them.

* QueryInterface is no longer visible to web content on the elements that have been converted.

* The exceptions thrown in various cases are now TypeErrors instead of a different kind of exception.

* For chrome code only, members that you had to use QueryInterface to get to are now always visible.  This doesn't affect site compat, of course, except insofar as sites can never get to these anymore.

Note You need to log in before you can comment on or make changes to this bug.