Last Comment Bug 659053 - Remove nsIDOM3Node
: Remove nsIDOM3Node
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Jonas Sicking (:sicking)
: 548579 (view as bug list)
Depends on: 664123 693615 694754
Blocks: 659539 664138
  Show dependency treegraph
Reported: 2011-05-23 10:59 PDT by Jonas Sicking (:sicking)
Modified: 2012-01-18 09:49 PST (History)
10 users (show)
Ms2ger: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch to fix (134.39 KB, patch)
2011-05-23 10:59 PDT, Jonas Sicking (:sicking)
no flags Details | Diff | Review
Patch to fix (21.24 KB, patch)
2011-05-23 11:17 PDT, Jonas Sicking (:sicking)
no flags Details | Diff | Review
Patch to fix v1.1 (136.05 KB, patch)
2011-05-24 19:34 PDT, Jonas Sicking (:sicking)
no flags Details | Diff | Review
Interdiff for main patch (10.74 KB, patch)
2011-06-06 01:25 PDT, Jonas Sicking (:sicking)
no flags Details | Diff | Review
Patch to fix v1.5 (137.39 KB, patch)
2011-06-06 01:27 PDT, Jonas Sicking (:sicking)
peterv: review+
Details | Diff | Review
Speed up IsEqualNode (15.19 KB, patch)
2011-06-06 01:30 PDT, Jonas Sicking (:sicking)
peterv: review+
Details | Diff | Review
Fix regression (1.89 KB, patch)
2011-06-10 02:11 PDT, Jonas Sicking (:sicking)
peterv: review+
Details | Diff | Review
Fix isEqualNode on doctypes (2.58 KB, patch)
2011-06-14 14:21 PDT, Jonas Sicking (:sicking)
jonas: review+
Details | Diff | Review

Description Jonas Sicking (:sicking) 2011-05-23 10:59:22 PDT
Created attachment 534486 [details] [diff] [review]
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) 2011-05-23 11:17:15 PDT
Created attachment 534494 [details] [diff] [review]
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) 2011-05-23 11:18:03 PDT
Comment on attachment 534494 [details] [diff] [review]
Patch to fix

Wrong bug!
Comment 3 Olli Pettay [:smaug] 2011-05-24 02:30:04 PDT
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] 2011-05-24 02:32:04 PDT
Oh, it wasn't for active instance documents, but to allow
extensions to add new stuff to hasFeature.
Comment 5 Jonas Sicking (:sicking) 2011-05-24 02:45:57 PDT
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) 2011-05-24 19:34:43 PDT
Created attachment 534974 [details] [diff] [review]
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] 2011-05-30 04:38:57 PDT
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) 2011-05-30 08:16:00 PDT
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] 2011-06-05 02:38:46 PDT
(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) 2011-06-05 03:09:36 PDT
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] 2011-06-05 11:53:45 PDT
(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) 2011-06-05 13:21:34 PDT
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) 2011-06-06 01:25:45 PDT
Created attachment 537507 [details] [diff] [review]
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) 2011-06-06 01:27:50 PDT
Created attachment 537508 [details] [diff] [review]
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) 2011-06-06 01:30:04 PDT
Created attachment 537509 [details] [diff] [review]
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] 2011-06-08 15:42:10 PDT
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

> 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) {
          else {
          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) 2011-06-08 22:47:53 PDT
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) 2011-06-10 02:11:29 PDT
Created attachment 538465 [details] [diff] [review]
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 2011-06-14 02:57:38 PDT
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) 2011-06-14 03:12:09 PDT
Oops, that's wrong. r=me on changing that to PRBool
Comment 23 Jonas Sicking (:sicking) 2011-06-14 14:21:54 PDT
Created attachment 539321 [details] [diff] [review]
Fix isEqualNode on doctypes

Since you backed me out, you'll get to review the fix.
Comment 24 Jonas Sicking (:sicking) 2011-06-14 18:30:20 PDT
Comment on attachment 539321 [details] [diff] [review]
Fix isEqualNode on doctypes

Bent reviewed it on the train.
Comment 25 Jonas Sicking (:sicking) 2011-06-14 19:39:48 PDT
Checked in

Thanks for the reviews!!
Comment 27 Eric Shepherd [:sheppy] 2011-06-17 07:21:30 PDT
Trevor wrote this up.
Comment 28 Peter Van der Beken [:peterv] 2011-06-18 02:39:45 PDT
*** Bug 548579 has been marked as a duplicate of this bug. ***
Comment 29 :Ms2ger 2011-08-31 08:31:27 PDT

for Gecko 9.
Comment 30 Ioana (away) 2011-09-12 07:24:20 PDT
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': {

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