824823 - Convert to WebIDL the node classes used in Dromaeo's dom-traverse test

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

As far as I can tell that means these:

  HTMLAnchorElement
  HTMLSharedListElement
  HTMLLIElement
  TextNode, probably
  HTMLParagraphElement
  HTMLPreElement
  HTMLSpanElement
  HTMLTableElement
  HTMLTableSectionElement
  HTMLTableCellElement
  HTMLTableRowElement

Comment 1

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

I've got half a patch for CharacterData, which I need to disentangle from my queue

Comment 2

[Boris Zbarsky [:bzbarsky]]

I actually have all the non-table stuff above done locally already.

Comment 3

[Boris Zbarsky [:bzbarsky]]

And a comparison of a vanilla build to a build with the non-table bits done:

http://dromaeo.com/?id=187055,187054

Since the table stuff is a bit involved, I've spun off bug 824907 on converting it.

Comment 4

[Peter Van der Beken [:peterv]]

I've been doing some table stuff.

For HTMLSpanElement it'd be nice to have a base class to share with HTMLElement. It would do NS_FORWARD_NSIDOMNODE_TO_NSINODE, NS_FORWARD_NSIDOMELEMENT_TO_GENERIC, NS_FORWARD_NSIDOMHTMLELEMENT_TO_GENERIC and implement AsDOMNode. HTMLElement and HTMLSpanElement would just need to implement WrapNode and Clone (and for now QI and GetClassInfo). As we phase out nsIDOMHTML*Element interfaces we can have more and more elements use that base class.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Rename nsHTMLAnchorElement to mozilla::dom::HTMLAnchorElement.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. WebIDL binding for HTMLAnchorElement.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Rename nsHTMLOListElement.cpp to something sane and move the class declaration into a header.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. WebIDL binding for HTML list elements. that I had to create the separate subclasses because we can't map

different WebIDL interfaces to the same concrete type: it causes the
PrototypeIDMap templates to completely fail to deal.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Rename nsHTMLLIElement to mozilla::dom::HTMLLIElement and mov ethe class declaration into a header.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Move HTMLLIElement to WebIDL bindings.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Implement WebIDL CharacterData API on nsGenericDOMDataNode.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Convert nsTextNode to WebIDL.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Convert Comment to WebIDL.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 10. Rename nsHTMLParagraphElement to mozilla::dom::HTMLParagraphElement and move the class declaration into a header.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 11. Switch HTMLParagraphElement to WebIDL.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 12. Rename nsHTMLPreElement to mozilla::dom::HTMLPreElement and move the class declaration into a header.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: part 13. Switch HTMLPreElement to WebIDL.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: part 14. Rename nsHTMLSpanElement to mozilla::dom::HTMLSpanElement and move the class declaration into a header.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: part 15. Switch HTMLSpanElement to WebIDL.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: part 15. Switch HTMLSpanElement to WebIDL.

Comment 21

[Boris Zbarsky [:bzbarsky]]

https://tbpl.mozilla.org/?tree=Try&rev=5e21d62bc22e shows various test failures that I'll look into tomorrow (off today).

Comment 22

[Boris Zbarsky [:bzbarsky]]

Hmm.  The fail in js/xpconnect/tests/mochitest/test_bug655297.html looks like us collecting the new-binding wrappers when they're weakmap keys.  Andrew, do you know what's up with that?

Comment 23

[Andrew McCreight [:mccr8]]

We don't support any new-bindings objects as weakmap keys right now. It requires doing something like wrapper preservation. See bug 777385.

Right now, we special case them for wrapped natives that are nodes, which is probably why this works now, but doesn't after you convert them. The logic for this lives in the function PreserveWrapper in XPCJSRuntime.

Comment 24

[Andrew McCreight [:mccr8]]

Maybe we could come up with a weaker version of the patch in 777385 that would only apply to nodes, which would at least allow this to not be blocked on the weirdness there...

Comment 25

[Boris Zbarsky [:bzbarsky]]

That would be a really good idea, since we already (on m-c) have some elements on new bindings, so right now some elements can be used as weakmap keys and some can't, which is pretty bad.  :(

Comment 26

[Andrew McCreight [:mccr8]]

Ah, yes, I didn't think about that consequence of elements being converted. That is quite bad.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Andrew, should I just change the test to use a node type that's still on XPConnect bindings, or wait for bug 825053?

Comment 28

[Andrew McCreight [:mccr8]]

Sure, that's an easy workaround, and we've already kind of crossed the rubicon there in 20. I'm not sure how bad 825053 will be. Hopefully I can fix it in the next week...

Comment 29

[Boris Zbarsky [:bzbarsky]]

OK.  So the test failure from content/html/content/test/test_bug389797.html is due to SpecialPowers.wrap() being completely busted for Xrays around <a>.  And the reason for that is this function in testing/specialpowers/content/specialpowersAPI.js:

92 function isXrayWrapper(x) {
93   try {
94     return /XrayWrapper/.exec(x.toString());
95   } catch(e) {
96     // The toString() implementation could theoretically throw. But it never
97     // throws for Xray, so we can just assume non-xray in that case.
98     return false;
99   }
100 }

But for new bindings toString() on an Xray does not prepend the "object XrayWrapper" bits if the object has a toString in the IDL (which <a> does, via the stringifier).  So this incorrectly returns false and then the resulting object doesn't expose any useful properties whatsoever.

Peter, thoughts?  We could add the "object XrayWrapper" crud to this case, or we could add a Components.utils.isXrayWrapper or something...

Comment 30

[Boris Zbarsky [:bzbarsky]]

Attachment: CharacterData patch updated to fix test failures

Comment 31

[Boris Zbarsky [:bzbarsky]]

Attachment: Textnode patch updated to fix test failures

Comment 32

[Boris Zbarsky [:bzbarsky]]

Attachment: <li> patch updated to fix test failures

Comment 33

[Peter Van der Beken [:peterv]]

Comment on attachment 695958 [details] [diff] [review]
part 2.  WebIDL binding for HTMLAnchorElement.

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

Please also remove the quickstubs.

::: content/html/content/src/HTMLAnchorElement.h
@@ +98,5 @@
>    virtual void OnDNSPrefetchRequested();
>    virtual bool HasDeferredDNSPrefetchRequest();
>  
> +  // WebIDL API
> +  NS_IMPL_WEBIDL_URI_ATTR(Href, href)

I decided not to use macros (I don't think they're that useful), but ok. I guess we'll need to decide at some point on a common approach.

Comment 34

[Boris Zbarsky [:bzbarsky]]

> I decided not to use macros (I don't think they're that useful)

I think I was just feeling lazy about all the copy/pasting... but on the other hand, I don't think I used these macros in any other patch in this bug.  Want me to just drop them here too?

Comment 35

[Boris Zbarsky [:bzbarsky]]

And still waiting on feedback for comment 29...

Comment 36

[Boris Zbarsky [:bzbarsky]]

Bobby, see comment 29?

Comment 37

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky (:bz) from comment #29)
> Peter, thoughts?  We could add the "object XrayWrapper" crud to this case,
> or we could add a Components.utils.isXrayWrapper or something...

I think I'd rather do Components.utils.isXrayWrapper, but I'm interested to hear what bholley thinks.

Comment 38

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky (:bz) from comment #34)
> Want me to just drop them here too?

If you don't feel strongly about it, yes.

Comment 39

[Bobby Holley (:bholley)]

Cu.isXrayWrapper is fine.

Comment 40

[Peter Van der Beken [:peterv]]

Comment on attachment 695960 [details] [diff] [review]
part 4.  WebIDL binding for HTML list elements.   that I had to create the separate subclasses because we can't map

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

::: dom/webidl/HTMLOListElement.webidl
@@ +23,5 @@
> +/*
> +};
> +
> +// http://www.whatwg.org/specs/web-apps/current-work/#other-elements,-attributes-and-apis
> +partial interface HTMLUListElement {

This should be OList.

Comment 41

[Peter Van der Beken [:peterv]]

Comment on attachment 696226 [details] [diff] [review]
<li> patch updated to fix test failures

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

::: dom/webidl/HTMLLIElement.webidl
@@ +19,5 @@
> +/*
> +};
> +
> +// http://www.whatwg.org/specs/web-apps/current-work/#other-elements,-attributes-and-apis
> +partial interface HTMLUListElement {

LIElement

Comment 42

[Peter Van der Beken [:peterv]]

Comment on attachment 696224 [details] [diff] [review]
CharacterData patch updated to fix test failures

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

Do we want to rename nsGenericDOMDataNode to mozilla::dom::CharacterData at some point?

Comment 43

[Peter Van der Beken [:peterv]]

Comment on attachment 696224 [details] [diff] [review]
CharacterData patch updated to fix test failures

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

::: dom/bindings/Bindings.conf
@@ +138,5 @@
>      }
>  },
>  
> +'CharacterData': {
> +    'nativeType': 'nsGenericDOMDataNode',

Hmm, but doesn't this need

     'hasXPConnectImpls': True,
     'hasInstanceInterface': 'nsIDOMCharacterData'

for now?

Comment 44

[Peter Van der Beken [:peterv]]

Comment on attachment 695976 [details] [diff] [review]
part 11.  Switch HTMLParagraphElement to WebIDL.

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

::: dom/webidl/HTMLParagraphElement.webidl
@@ +15,5 @@
> +interface HTMLParagraphElement : HTMLElement {
> +/*};
> +
> +// http://www.whatwg.org/specs/web-apps/current-work/#other-elements,-attributes-and-apis
> +partial interface HTMLHeadingElement {

HTMLParagraphElement

Comment 45

[Peter Van der Beken [:peterv]]

Comment on attachment 695978 [details] [diff] [review]
part 13.  Switch HTMLPreElement to WebIDL.

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

::: dom/webidl/HTMLPreElement.webidl
@@ +15,5 @@
> +interface HTMLPreElement : HTMLElement {
> +/*};
> +
> +// http://www.whatwg.org/specs/web-apps/current-work/#other-elements,-attributes-and-apis
> +partial interface HTMLHeadingElement {

HTMLPreElement

Comment 46

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

(In reply to Peter Van der Beken [:peterv] from comment #43)
> Comment on attachment 696224 [details] [diff] [review]
> CharacterData patch updated to fix test failures
> 
> Review of attachment 696224 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bindings/Bindings.conf
> @@ +138,5 @@
> >      }
> >  },
> >  
> > +'CharacterData': {
> > +    'nativeType': 'nsGenericDOMDataNode',
> 
> Hmm, but doesn't this need
> 
>      'hasXPConnectImpls': True,
>      'hasInstanceInterface': 'nsIDOMCharacterData'
> 
> for now?

and concrete:false?

Comment 47

[Boris Zbarsky [:bzbarsky]]

> Hmm, but doesn't this need

Yes, you're right.  It does.  Fixed.

> and concrete:false?

Keep reading the patches.  ;)  You can't actually flag an interface with no interfaces inheriting from it as "concrete:False" and have it compile, so it happens in a later patch.

> Do we want to rename nsGenericDOMDataNode to mozilla::dom::CharacterData at some point?

Sure; I filed bug 826414.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Addressed the other review comments.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Changed the WeakMap test to use <form> and pushed:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/85b257295469
  https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3ac746bfca
  https://hg.mozilla.org/integration/mozilla-inbound/rev/3a73a1fa423b
  https://hg.mozilla.org/integration/mozilla-inbound/rev/f79cb09fd2cb
  https://hg.mozilla.org/integration/mozilla-inbound/rev/12c938802edd
  https://hg.mozilla.org/integration/mozilla-inbound/rev/9bee1e3e4df2
  https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a25d0475a1
  https://hg.mozilla.org/integration/mozilla-inbound/rev/71b8063ba668
  https://hg.mozilla.org/integration/mozilla-inbound/rev/ee0e5b1c1640
  https://hg.mozilla.org/integration/mozilla-inbound/rev/2861abcd2d79
  https://hg.mozilla.org/integration/mozilla-inbound/rev/595031ea3497
  https://hg.mozilla.org/integration/mozilla-inbound/rev/5ae68b7b5126
  https://hg.mozilla.org/integration/mozilla-inbound/rev/dacf95e8cfcd
  https://hg.mozilla.org/integration/mozilla-inbound/rev/07ec266b93a8
  https://hg.mozilla.org/integration/mozilla-inbound/rev/cdb7095b58b2
  https://hg.mozilla.org/integration/mozilla-inbound/rev/665ead9267b5

Comment 50

[Boris Zbarsky [:bzbarsky]]

(Improvement) Mozilla-Inbound - Dromaeo (DOM) - MacOSX 10.6 (rev4) - 7.5%
(Improvement) Mozilla-Inbound - Dromaeo (DOM) - MacOSX 10.8 - 7.26%
(Improvement) Mozilla-Inbound - Dromaeo (DOM) - MacOSX 10.7 - 6.96%

Yay.

Comment 51

[Boris Zbarsky [:bzbarsky]]

And wins on Linux and Windows too (non-PGO only, though I dunno that there were PGO builds here yet).  10+% wins on dromaeo-dom, 2-3% wins on dromaeo-css.

Comment 52

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/85b257295469
https://hg.mozilla.org/mozilla-central/rev/ce3ac746bfca
https://hg.mozilla.org/mozilla-central/rev/3a73a1fa423b
https://hg.mozilla.org/mozilla-central/rev/f79cb09fd2cb
https://hg.mozilla.org/mozilla-central/rev/12c938802edd
https://hg.mozilla.org/mozilla-central/rev/9bee1e3e4df2
https://hg.mozilla.org/mozilla-central/rev/b1a25d0475a1
https://hg.mozilla.org/mozilla-central/rev/71b8063ba668
https://hg.mozilla.org/mozilla-central/rev/ee0e5b1c1640
https://hg.mozilla.org/mozilla-central/rev/2861abcd2d79
https://hg.mozilla.org/mozilla-central/rev/595031ea3497
https://hg.mozilla.org/mozilla-central/rev/5ae68b7b5126
https://hg.mozilla.org/mozilla-central/rev/dacf95e8cfcd
https://hg.mozilla.org/mozilla-central/rev/07ec266b93a8
https://hg.mozilla.org/mozilla-central/rev/cdb7095b58b2
https://hg.mozilla.org/mozilla-central/rev/665ead9267b5