Last Comment Bug 659053 - Remove nsIDOM3Node
: Remove nsIDOM3Node
Status: VERIFIED FIXED
: 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) PTO Until July 5th
:
Mentors:
: 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) PTO Until July 5th
Modified: 2012-01-18 09:49 PST (History)
10 users (show)
Ms2ger: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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

Description Jonas Sicking (:sicking) PTO Until July 5th 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) PTO Until July 5th 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) PTO Until July 5th 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) PTO Until July 5th 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) PTO Until July 5th 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] - away till Aug 1st 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) PTO Until July 5th 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] - away till Aug 1st 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) PTO Until July 5th 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) PTO Until July 5th 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) PTO Until July 5th 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) PTO Until July 5th 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) PTO Until July 5th 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] - away till Aug 1st 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

>+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) PTO Until July 5th 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) PTO Until July 5th 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 (⌚ UTC+1/+2) 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) PTO Until July 5th 2011-06-14 03:12:09 PDT
Oops, that's wrong. r=me on changing that to PRBool
Comment 22 :Ms2ger (⌚ UTC+1/+2) 2011-06-14 07:32:49 PDT
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) PTO Until July 5th 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) PTO Until July 5th 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) PTO Until July 5th 2011-06-14 19:39:48 PDT
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 (⌚ UTC+1/+2) 2011-06-15 07:59:16 PDT
http://hg.mozilla.org/mozilla-central/rev/0fe7b4e83cb9
Comment 27 Eric Shepherd [:sheppy] 2011-06-17 07:21:30 PDT
Trevor wrote this up.
Comment 28 Peter Van der Beken [:peterv] - away till Aug 1st 2011-06-18 02:39:45 PDT
*** Bug 548579 has been marked as a duplicate of this bug. ***
Comment 29 :Ms2ger (⌚ UTC+1/+2) 2011-08-31 08:31:27 PDT
Landed

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

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.