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

RESOLVED FIXED in mozilla20

Status

()

defect
RESOLVED FIXED
6 years ago
a month ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks 1 bug)

unspecified
mozilla20
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(15 attachments, 4 obsolete attachments)

32.22 KB, patch
peterv
: review+
Details | Diff | Splinter Review
22.62 KB, patch
peterv
: review+
Details | Diff | Splinter Review
19.29 KB, patch
peterv
: review+
Details | Diff | Splinter Review
10.48 KB, patch
peterv
: review+
Details | Diff | Splinter Review
12.49 KB, patch
peterv
: review+
Details | Diff | Splinter Review
10.06 KB, patch
peterv
: review+
Details | Diff | Splinter Review
10.86 KB, patch
peterv
: review+
Details | Diff | Splinter Review
5.37 KB, patch
peterv
: review+
Details | Diff | Splinter Review
12.02 KB, patch
peterv
: review+
Details | Diff | Splinter Review
5.17 KB, patch
peterv
: review+
Details | Diff | Splinter Review
6.91 KB, patch
peterv
: review+
Details | Diff | Splinter Review
4.17 KB, patch
peterv
: review+
Details | Diff | Splinter Review
10.27 KB, patch
peterv
: review+
Details | Diff | Splinter Review
12.50 KB, patch
peterv
: review+
Details | Diff | Splinter Review
5.55 KB, patch
peterv
: review+
Details | Diff | Splinter Review
As far as I can tell that means these:

  HTMLAnchorElement
  HTMLSharedListElement
  HTMLLIElement
  TextNode, probably
  HTMLParagraphElement
  HTMLPreElement
  HTMLSpanElement
  HTMLTableElement
  HTMLTableSectionElement
  HTMLTableCellElement
  HTMLTableRowElement
Depends on: 824857
No longer depends on: 824857
I've got half a patch for CharacterData, which I need to disentangle from my queue
I actually have all the non-table stuff above done locally already.
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.
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.
different WebIDL interfaces to the same concrete type: it causes the
PrototypeIDMap templates to completely fail to deal.
Attachment #695960 - Flags: review?(peterv)
Attachment #695967 - Flags: review?(peterv)
Attachment #695980 - Flags: review?(peterv)
Attachment #695980 - Attachment is obsolete: true
Attachment #695980 - Flags: review?(peterv)
Attachment #695956 - Flags: review?(peterv) → review+
https://tbpl.mozilla.org/?tree=Try&rev=5e21d62bc22e shows various test failures that I'll look into tomorrow (off today).
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?
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.
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...
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.  :(
Ah, yes, I didn't think about that consequence of elements being converted. That is quite bad.
Depends on: 825053
Andrew, should I just change the test to use a node type that's still on XPConnect bindings, or wait for bug 825053?
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...
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...
Flags: needinfo?(peterv)
Attachment #695963 - Attachment is obsolete: true
Attachment #695963 - Flags: review?(peterv)
Attachment #695967 - Attachment is obsolete: true
Attachment #695967 - Flags: review?(peterv)
Attachment #695962 - Attachment is obsolete: true
Attachment #695962 - Flags: review?(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.
Attachment #695958 - Flags: review?(peterv) → review+
Flags: needinfo?(peterv)
> 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?
And still waiting on feedback for comment 29...
Flags: needinfo?(peterv)
Attachment #695959 - Flags: review?(peterv) → review+
Bobby, see comment 29?
Flags: needinfo?(bobbyholley+bmo)
(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.
Flags: needinfo?(peterv)
Attachment #695960 - Flags: review?(peterv) → review+
Attachment #695961 - Flags: review?(peterv) → review+
(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.
Attachment #696226 - Flags: review?(peterv) → review+
Cu.isXrayWrapper is fine.
Flags: needinfo?(bobbyholley+bmo)
Whiteboard: [need review]
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 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 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?
Attachment #696224 - Flags: review?(peterv) → review+
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?
Attachment #696225 - Flags: review?(peterv) → review+
Attachment #695973 - Flags: review?(peterv) → review+
Attachment #695975 - Flags: review?(peterv) → review+
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
Attachment #695976 - Flags: review?(peterv) → review+
Attachment #695977 - Flags: review?(peterv) → review+
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
Attachment #695978 - Flags: review?(peterv) → review+
Attachment #695979 - Flags: review?(peterv) → review+
Attachment #695984 - Flags: review?(peterv) → review+
(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?
Blocks: 826414
> 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.
Addressed the other review comments.
Whiteboard: [need review] → [need dependencies fixed]
Depends on: 826423
(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.
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.
Blocks: 827214

Updated

6 years ago
Depends on: 827468
No longer blocks: 827214
Depends on: 827214
Depends on: 826703
Duplicate of this bug: 668821
Depends on: 879838
No longer depends on: 879838
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.