Closed Bug 659053 Opened 8 years ago Closed 8 years ago

Remove nsIDOM3Node

Categories

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

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla7

People

(Reporter: sicking, Assigned: sicking)

References

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 4 obsolete files)

Attached patch Patch to fix (obsolete) — Splinter Review
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.
Attachment #534486 - Flags: review?(peterv)
Attached patch Patch to fix (obsolete) — Splinter Review
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 :)
Assignee: nobody → jonas
Attachment #534486 - Attachment is obsolete: true
Attachment #534486 - Flags: review?(peterv)
Attachment #534494 - Flags: review?(Ms2ger)
Comment on attachment 534494 [details] [diff] [review]
Patch to fix

Wrong bug!
Attachment #534494 - Attachment is obsolete: true
Attachment #534494 - Flags: review?(Ms2ger)
Attachment #534486 - Attachment is obsolete: false
Attachment #534486 - Flags: review?(peterv)
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.
Oh, it wasn't for active instance documents, but to allow
extensions to add new stuff to hasFeature.
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.
Attached patch Patch to fix v1.1 (obsolete) — Splinter Review
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.
Attachment #534486 - Attachment is obsolete: true
Attachment #534486 - Flags: review?(peterv)
Attachment #534974 - Flags: review?(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.
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.
(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.
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.
(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.
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.
Attached patch Interdiff for main patch (obsolete) — Splinter Review
This adds back quickstubs to most of the functions. Only IsEqualNode still does a QI for the first argument.
This is the new main patch which only slows down the quickstubs for IsEqualNode's first argument.
Attachment #537508 - Flags: review?(peterv)
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.
Attachment #537509 - Flags: review?(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.
Attachment #537508 - Flags: review?(peterv) → review+
Attachment #537509 - Flags: review?(peterv) → review+
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.
Attached patch Fix regressionSplinter Review
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).
Attachment #537507 - Attachment is obsolete: true
Attachment #538465 - Flags: review?(peterv)
Attachment #538465 - Flags: review?(peterv) → review+
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?
Oops, that's wrong. r=me on changing that to PRBool
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...
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite-
Keywords: dev-doc-needed
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Depends on: 664123
But not well enough, I'm afraid.

http://hg.mozilla.org/mozilla-central/rev/8ca16e3ff5e0
http://hg.mozilla.org/mozilla-central/rev/e00f1b194440
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 664138
Since you backed me out, you'll get to review the fix.
Attachment #539321 - Flags: review?(Ms2ger)
Comment on attachment 539321 [details] [diff] [review]
Fix isEqualNode on doctypes

Bent reviewed it on the train.
Attachment #539321 - Flags: review?(Ms2ger) → review+
Checked in

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

Thanks for the reviews!!
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Trevor wrote this up.
Duplicate of this bug: 548579
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': {
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.