Status

()

Core
DOM: Core & HTML
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: sicking, Assigned: sicking)

Tracking

({dev-doc-complete})

Trunk
mozilla7
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

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.
Attachment #534486 - Flags: review?(peterv)
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 :)
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)

Comment 3

6 years ago
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

6 years ago
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.
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.
Attachment #534486 - Attachment is obsolete: true
Attachment #534486 - Flags: review?(peterv)
Attachment #534974 - Flags: review?(peterv)
Blocks: 659539
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.
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.
Attachment #534974 - Flags: review?(peterv)
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.
Attachment #537508 - Flags: review?(peterv)
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.
Attachment #537509 - Flags: review?(peterv)
Attachment #534974 - Attachment is obsolete: true
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.
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).
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
Last Resolved: 6 years ago
Flags: in-testsuite-
Keywords: dev-doc-needed
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla7

Updated

6 years ago
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 → ---

Updated

6 years ago
Blocks: 664138
Created attachment 539321 [details] [diff] [review]
Fix isEqualNode on doctypes

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
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
http://hg.mozilla.org/mozilla-central/rev/0fe7b4e83cb9
Trevor wrote this up.
Keywords: dev-doc-needed → dev-doc-complete
Duplicate of this bug: 548579
Landed

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

for Gecko 9.

Comment 30

6 years ago
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
Depends on: 693615
Depends on: 694754
You need to log in before you can comment on or make changes to this bug.