659053 - Remove nsIDOM3Node

Status: VERIFIED FIXED

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

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

We no longer need this interface since we've unfrozen nsIDOMNode.

I also removed the GetFeature function since it's pretty obviously useless (and removed from DOM Core), and I didn't want to even temporarily move the code over to nsIDOMNode.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

I accidentally rewrote this patch having forgotten that ms2ger already had. However turns out this was a good thing as he's telling me that his patch doesn't quite work.

So here is mine :)

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 534494 [details] [diff] [review]
Patch to fix

Wrong bug!

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Uh, you're removing my FeatureFactory. Not that anyone uses it. :)
It was added years ago to prototype active instance data documents in XForms.
So, feel free to remove it.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Oh, it wasn't for active instance documents, but to allow
extensions to add new stuff to hasFeature.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, it seemed enough like unused code that I didn't bother digging around to see who added it. Hope that was ok. Seems unlikely that people have actually hooked into it to an extent that there are sites out there that depends on it.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix v1.1

This contains two fixes.

1. I had the nullcheck the wrong way in nsDOMAttribute::IsEqualNode
2. Some wrong mq foo meant that the change to remove the nsIDOM3Node tearoff from
   one QI implementation was missing.

Comment 7

[Peter Van der Beken [:peterv]]

Why are you undoing the nsINode-based versions of these methods and their custom quickstubs? I think those optimizations are more important than being able to use the forwarding macro. If you really want to use a macro, write a custom one.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I actually removed them more for cleanup reasons than macro reasons. It's pretty easy to use the macros by simply naming the quickstubbed methods something slightly different to avoid C++ hiding issues.

But do we really care about having extra optimized versions of isSameNode, lookupPrefix, isDefaultNamespace etc? It looked to me like the reason they got special treatment in the quickstubs was to avoid creating the nsIDOM3Node tearoff which obviously isn't a problem any more.

If you feel otherwise, let me know which ones you think we should have specialized methods for and I'll add back quickstubs for them.

Comment 9

[Peter Van der Beken [:peterv]]

(In reply to comment #8)
> It looked to me like the reason they
> got special treatment in the quickstubs was to avoid creating the
> nsIDOM3Node tearoff which obviously isn't a problem any more.

The tearoff was one performance problem, but unwrapping to nsIDOMNode is still quite a bit slower than unwrapping to nsINode (because we can just cast from the canonical nsISupports). I don't think we should undo performance optimizations like these, unless you can prove that we're not regressing.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I really don't think that these are worth optimizing simply because I don't think anyone is actually using them (much less in a performance critical path). And things like isSameNode and getUserData are functions we should remove anyway, right?

I could possibly see isEqualNode being useful but I still doubt it's actually used.

Long term we're rewriting the DOM bindings anyway right, in which case having specialized code is just more code to weed through and clean up.

I'll run some perf numbers on these functions to see how much of a perf impact this patch has.

Comment 11

[Alex Vincent [:WeirdAl]]

(In reply to comment #10)
> I could possibly see isEqualNode being useful but I still doubt it's
> actually used.

Oh, I think I'd be using it as an editor.

I also use getUserData/setUserData, but really only to store properties.  I can certainly do without them.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hmm.. wait, most of these can get optimal performance by just setting the thisType in the qsconf file. The only exceptions are isSameNode and isEqualNode. I have a plan for those which should make everyone happy.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Interdiff for main patch

This adds back quickstubs to most of the functions. Only IsEqualNode still does a QI for the first argument.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix v1.5

This is the new main patch which only slows down the quickstubs for IsEqualNode's first argument.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Speed up IsEqualNode

This speeds up IsEqualNode by both making the first argument nsINode and thus not needing QI. It also makes the implementation non-recursive which should be even faster.

It also fixes a bug in our current implementation which doesn't check the nodeName for doctypes and PIs.

Comment 16

[Peter Van der Beken [:peterv]]

Comment on attachment 537508 [details] [diff] [review]
Patch to fix v1.5

Please do s/_retval/aResult/ or s/_retval/aReturn/.

>diff --git a/content/base/public/nsINode.h b/content/base/public/nsINode.h

>@@ -964,23 +965,26 @@ public:

>+  nsresult GetDOMBaseURI(nsAString &aURI);

Why not const?

>diff --git a/content/base/src/nsDOMDocumentType.h b/content/base/src/nsDOMDocumentType.h

>+class nsDOMDocumentTypeForward : public nsGenericDOMDataNode,

Ugh :-(.

>diff --git a/content/base/src/nsDocumentFragment.cpp b/content/base/src/nsDocumentFragment.cpp

>+class nsDocumentFragmentForward : public nsGenericElement,

More ugh :-(.

>diff --git a/content/base/src/nsGenericElement.cpp b/content/base/src/nsGenericElement.cpp

>+nsresult
> nsINode::LookupPrefix(const nsAString& aNamespaceURI, nsAString& aPrefix)

>+          prefix = localName == nsGkAtoms::xmlns ? nsnull : localName;
>+          content = nsnull;
>+          break;

I'd return early here:

          if (localName != nsGkAtoms::xmlns) {
            localName->ToString(aPrefix);
          }
          else {
            SetDOMStringToNull(aPrefix);
          }
          return NS_OK;

>diff --git a/content/xslt/src/xpath/nsXPathNSResolver.cpp b/content/xslt/src/xpath/nsXPathNSResolver.cpp
>--- a/content/xslt/src/xpath/nsXPathNSResolver.cpp
>+++ b/content/xslt/src/xpath/nsXPathNSResolver.cpp

> nsXPathNSResolver::nsXPathNSResolver(nsIDOMNode* aNode)
> {
>-    mNode = do_QueryInterface(aNode);
>+    mNode = aNode;

Change this to an initialization list.

>diff --git a/js/src/xpconnect/src/dom_quickstubs.qsconf b/js/src/xpconnect/src/dom_quickstubs.qsconf
>--- a/js/src/xpconnect/src/dom_quickstubs.qsconf
>+++ b/js/src/xpconnect/src/dom_quickstubs.qsconf
>@@ -152,21 +152,30 @@ members = [
>     'nsIDOMNode.replaceChild',
>     'nsIDOMNode.localName',
>     'nsIDOMNode.lastChild',
>     'nsIDOMNode.ownerDocument',
>     'nsIDOMNode.parentNode',
>     'nsIDOMNode.removeChild',
>     'nsIDOMNode.hasAttributes',
>     'nsIDOMNode.attributes',
>+    'nsIDOMNode.baseURI',
>+    'nsIDOMNode.compareDocumentPosition',
>+    'nsIDOMNode.textContent',
>+    'nsIDOMNode.isSameNode',
>+    'nsIDOMNode.lookupPrefix',
>+    'nsIDOMNode.isDefaultNamespace',
>+    'nsIDOMNode.lookupNamespaceURI',
>+    'nsIDOMNode.isEqualNode',
>+    'nsIDOMNode.setUserData',
>+    'nsIDOMNode.getUserData',

Let's just make it nsIDOMNode.* please.

>@@ -633,16 +642,23 @@ customMethodCalls = {

>+    'nsIDOMNode_GetNodeName': {
>+        'thisType': 'nsINode',
>+        'code': 'nsString result; \n'

Remove the trailing whitespace.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The nsDocumentFragmentForward goes away in bug 659539. But yeah, the doctype-forward is ugly, but seems better than the alternatives, especially as we'll surely be modifying nsIDOMNode a lot more going forward.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Fix regression

The lastest patch introduced a regression for CompareDocumentPosition. This restores the nullcheck that we currently have on trunk. (Found this from a test which specifically checks for this nullcheck).

Comment 19

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

Comment on attachment 537509 [details] [diff] [review]
Speed up IsEqualNode

>--- a/js/src/xpconnect/src/dom_quickstubs.qsconf
>+++ b/js/src/xpconnect/src/dom_quickstubs.qsconf
>     'nsIDOMNode_IsEqualNode': {
>         'thisType': 'nsINode',
>+        'arg0Type': 'nsINode',
>+        'code': '    PRUint16 result = self->IsEqualTo(arg0);',

Why PRUint16?

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oops, that's wrong. r=me on changing that to PRBool

Comment 21

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

http://hg.mozilla.org/mozilla-central/rev/9c49f69d8bab
http://hg.mozilla.org/mozilla-central/rev/f4ff8d137c1f
http://hg.mozilla.org/mozilla-central/rev/1b11c64ffcdf

You play this game well...

Comment 22

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

But not well enough, I'm afraid.

http://hg.mozilla.org/mozilla-central/rev/8ca16e3ff5e0
http://hg.mozilla.org/mozilla-central/rev/e00f1b194440

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Fix isEqualNode on doctypes

Since you backed me out, you'll get to review the fix.

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 539321 [details] [diff] [review]
Fix isEqualNode on doctypes

Bent reviewed it on the train.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in

http://hg.mozilla.org/mozilla-central/rev/589c2dd84ce6
http://hg.mozilla.org/mozilla-central/rev/0438ee84824c

Thanks for the reviews!!

Comment 26

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

http://hg.mozilla.org/mozilla-central/rev/0fe7b4e83cb9

Comment 27

[Eric Shepherd [:sheppy]]

Trevor wrote this up.

Comment 29

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

Landed

http://hg.mozilla.org/mozilla-central/rev/e043bf9fa4c1

for Gecko 9.

Comment 30

[Ioana (away)]

From the changes visible in the link from the above comment, nsIDOM3Node has been replaced by nsIDOMNode:
    1.12 -    'nsIDOM3Node_IsSameNode': {
    1.13 +    'nsIDOMNode_IsSameNode': {