Status

()

RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Blocks: 2 bugs)

Trunk
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 5 obsolete attachments)

(Assignee)

Description

9 years ago
I have some code for this. Will upload the patch once I've cleaned up the code.
(Assignee)

Updated

9 years ago
Depends on: 565633
(Assignee)

Comment 1

9 years ago
Created attachment 446590 [details] [diff] [review]
patch

Uploaded this to tryserver.
I could split this to parts, though most of the patch is just mechanical
simple changes.
It might be better to take already_AddRefed<nsINodeInfo> instead of nsCOMPtr<nsINodeInfo>; that would make it clear what's going on at the callsite, where a forget() call would be needed...

Why do we need to out-of-line the nsINode ctor?
(Assignee)

Comment 3

9 years ago
well, if we care about OOM situation (which we may not anymore)
already_AddRefed<nsINodeInfo> can't really handle that.
(Assignee)

Comment 4

9 years ago
Created attachment 446719 [details] [diff] [review]
already_addrefed
> already_AddRefed<nsINodeInfo> can't really handle that.

How was nsCOMPtr handling it any better?

Do you really need the ctor body with the already_AddRefed?  Can you not just use initializer syntax?  For that matter, is there a need for the separate ctor with no nodeinfo arg?  Seems like you could just use the already_AddRefed ctor and pass null...

You should probably make the ctor take a non-const reference, not an object.  Otherwise code like this:

  nsCOMPtr<nsINodeInfo> ni = aNodeInfo;
  nsCommentNode *it = new nsCommentNode(ni); // forgot forget()

will compile, right?  But with a non-const reference, it shouldn't...  I'd hope the forget() variant would, though.
(Assignee)

Comment 6

9 years ago
(In reply to comment #5)
> How was nsCOMPtr handling it any better?
nsCOMPtr releases nsINodeInfo there is OOM when creating some node.


> 
> Do you really need the ctor body with the already_AddRefed?  Can you not just
> use initializer syntax?
I don't understand this.


  For that matter, is there a need for the separate ctor
> with no nodeinfo arg?  Seems like you could just use the already_AddRefed ctor
> and pass null...
Left over from the nsCOMPtr patch

> You should probably make the ctor take a non-const reference, not an object. 
What you mean here? The nsCOMPtr patch takes reference.
And already_AddRefed patch takes object, but new nsCommentNode(ni) doesn't compile.
(Assignee)

Comment 7

9 years ago
But since |new| is infallible, do I need to care about OOM?
> nsCOMPtr releases nsINodeInfo there is OOM when creating some node.

Ah, ok.  Indeed.  I think we can ignore this given infallible new.

> I don't understand this.

  mNodeInfo(aNodeInfo)

before the ctor body instead of:

  mNodeInfo = aNodeInfo;

in body.

> And already_AddRefed patch takes object, but new nsCommentNode(ni) doesn't
> compile.

Oh?  What's the error message?  Does nsCommentNode(ni.get()) compile?
(Assignee)

Comment 9

9 years ago
(In reply to comment #8)
 
>   mNodeInfo(aNodeInfo)
> 
> before the ctor body instead of:
> 
>   mNodeInfo = aNodeInfo;
> 
> in body.
Uh, right. I read the comment somehow very differently.

> Oh?  What's the error message?  Does nsCommentNode(ni.get()) compile?
I could re-check, but it was something about using reference to a temporary object.
> it was something about using reference to a temporary object.

Perfect.  If the .get() version also doesn't compile, we're good!
(Assignee)

Comment 11

9 years ago
(In reply to comment #9)
> > Oh?  What's the error message?  Does nsCommentNode(ni.get()) compile?
> I could re-check, but it was something about using reference to a temporary
> object.
Oh, wait, that was for something else, I think.
I was using reference to already_AddRefed.

But I'll test few different syntaxes.
(Assignee)

Comment 12

9 years ago
(1) So, when using already_AddRefed<nsINodeInfo>&
NS_NewElement(getter_AddRefs(content), ns, nodeInfo.forget(), PR_FALSE);
doesn't compile. 
error: invalid initialization of non-const reference of type ‘already_AddRefed<nsINodeInfo>&’ from a temporary of type ‘already_AddRefed<nsINodeInfo>’

(2) When using already_AddRefed<nsINodeInfo>, NS_NewElement(getter_AddRefs(content), ns, nodeInfo, PR_FALSE);
does not compile.
error: conversion from ‘nsCOMPtr<nsINodeInfo>’ to non-scalar type ‘already_AddRefed<nsINodeInfo>’ requested 
.get() does compile.

I think (2) is still better, and should be good enough.
Hmm.  It makes it easy to underflow a refcount due to someone accidentally passing in a raw pointer without addreffing.  Maybe that's not a problem....
(Assignee)

Comment 14

9 years ago
Created attachment 446790 [details] [diff] [review]
still few fixes

I uploaded this to tryserver
Attachment #446590 - Attachment is obsolete: true
Attachment #446719 - Attachment is obsolete: true
(Assignee)

Comment 15

9 years ago
Created attachment 447087 [details] [diff] [review]
up-to-date

There were some SVG/SMIL change
(Assignee)

Comment 16

9 years ago
Tryserver found still some problem.
I'll try to fix that and request a review tomorrow.

(On 64bit linux this drops my createElement testcase from 17 to 10ms)
(Assignee)

Comment 17

9 years ago
Created attachment 447305 [details] [diff] [review]
patch

I think it wouldn't make things any easier if I'd split the patch.
But I could do it if needed.

The changes are:
- pass already_AddRefed<nsINodeInfo> to nsINode ctors
- virtual nsXPCClassInfo* GetClassInfo() in each nsINode implementation.
  That greatly reduces QIing/Addref/release in ConstructSlimWrapper
- Change nodeinfo hash to hash using string not atoms. This way we don't
  have to find first atom (using string key) and then nodeinfo using
  atom key. Only string key is enough.
- non-virtual ::createElement in nsDocument.cpp
- non-virtual createTextNode in nsDocument.cpp
- qsDOMObjectHelper to quickstubs. With that one addred/release can be reduced, and
  it gives a fast way to access classinfo.
  The usage of it isn't perhaps the prettiest in quickstub code, but should work.
- custom quickstubs for createElement and createTextNode
Attachment #446790 - Attachment is obsolete: true
Attachment #447087 - Attachment is obsolete: true
Attachment #447305 - Flags: review?(bzbarsky)
(Assignee)

Comment 18

9 years ago
Boris, feel free to move the review to someone else if you don't have time for
it.
Peter should perhaps review the quickstub parts. He might have suggestions
for it.
Comment on attachment 447305 [details] [diff] [review]
patch

jst, do you have time to review this?  If not, just punt back to me...
Attachment #447305 - Flags: review?(bzbarsky) → review?(jst)
(Assignee)

Updated

9 years ago
Blocks: 568244
(Assignee)

Updated

9 years ago
Blocks: 565742

Updated

9 years ago
Attachment #447305 - Flags: review?(peterv)
Attachment #447305 - Flags: review?(jst)
Attachment #447305 - Flags: review+
Comment on attachment 447305 [details] [diff] [review]
patch

 #ifdef MOZILLA_INTERNAL_API
-  nsINode(nsINodeInfo* aNodeInfo)
-    : mNodeInfo(aNodeInfo),
-      mParentPtrBits(0),
-      mFlagsOrSlots(NODE_DOESNT_HAVE_SLOTS),
-      mNextSibling(nsnull),
-      mPreviousSibling(nsnull),
-      mFirstChild(nsnull)
+  nsINode(already_AddRefed<nsINodeInfo> aNodeInfo)
+  : mNodeInfo(aNodeInfo),
+    mParentPtrBits(0),
+    mFlagsOrSlots(NODE_DOESNT_HAVE_SLOTS),
+    mNextSibling(nsnull),
+    mPreviousSibling(nsnull),
+    mFirstChild(nsnull)

I personally prefer the old indentation style here, but I can deal either way, until I see you use the above old style later on in this patch :) So maybe change the indentation back to what it was here? :)

- In content/base/src/nsDocument.cpp:

+#include "nsXMLEventsManager.h"
+

Looks like an unrelated change. nsDocument.h also has a forward declaration of nsXMLEventsManager which looks unrelated.

- In nsNodeInfoManager::GetNodeInfo():

+  nsRefPtr<nsNodeInfo> newNodeInfo = nsNodeInfo::Create();
+  NS_ENSURE_TRUE(newNodeInfo, nsnull);

This method returns an nsresult, so nsnull is wrong there. NS_ERROR_OUT_OF_MEMORY is probably what you want here.

+  he = PL_HashTableAdd(mNodeInfoHash, &newNodeInfo->mInner, newNodeInfo);
+  NS_ENSURE_TRUE(he, NS_ERROR_FAILURE);

This can't fail for any other reason than OOM, so use NS_ERROR_OUT_OF_MEMORY here too.

- In nsGenericHTMLElement::SetInnerHTML():

-  if (doc->IsHTML() && nsHtml5Module::sEnabled) {
+  if (doc->IsHTML() && PR_FALSE && nsHtml5Module::sEnabled) {

Might not want to check this in :)

- In js/src/xpconnect/src/xpcprivate.h:

@@ -3034,45 +3037,49 @@ public:
      * @param Interface the interface of src that we want
      * @param cache the wrapper cache for src (may be null, in which case src
      *              will be QI'ed to get the cache)
      * @param scope the default scope to put on the new JSObject's parent chain
      * @param allowNativeWrapper if true, this method may wrap the resulting
      *        JSObject in an XPCNativeWrapper and return that, as needed.
      * @param isGlobal
      * @param pErr [out] relevant error code, if any.
+     * @param src_is_identity optional performance hint. Set to PR_TRUE only
+     *                        if src is the identity pointer.
      */

That comment change doesn't match the code change.

- In xpc_qsGetWrapperCache():

+/** Convert an XPCOM pointer to jsval. Return JS_TRUE on success. 
+ * aIdentity is a performance optimization. Set it to PR_TRUE,
+ * only if p is the identity pointer.
+ */

This comment change doesn't match the code change.

- In  ConstructSlimWrapper():

     // Transfer ownership to the wrapper's private.
-    identityObj.forget();
+    if (strongIdentity) {
+        strongIdentity.forget();
+    } else {
+      aIdentity->forgetOwnership();
+    }

Stick to 2-space indentation :)

- In layout/forms/nsIsIndexFrame.cpp:

  * The Initial Developer of the Original Code is
  * Netscape Communications Corporation.
  * Portions created by the Initial Developer are Copyright (C) 1998
  * the Initial Developer. All Rights Reserved.
  *
  * Contributor(s):
- *
+ *                               

Just adding whitespace... Undo this?

r=jst with that, but I would like to see peterv look over the (small) changes in xpconnect/src.
(Assignee)

Comment 21

9 years ago
Yeah, Peter, would be great if you had time to look at the xpconnect changes.
Perhaps you even have suggestions how to achieve the same thing with
less ugly way. Note, all the Addref/Release and QIing shows up badly in the
profiles.
(Assignee)

Comment 22

9 years ago
(In reply to comment #20)
> - In nsGenericHTMLElement::SetInnerHTML():
> 
> -  if (doc->IsHTML() && nsHtml5Module::sEnabled) {
> +  if (doc->IsHTML() && PR_FALSE && nsHtml5Module::sEnabled) {
> 
Oops :)
Left-over from some testing.
Comment on attachment 447305 [details] [diff] [review]
patch

I think split patches would have been better (especially with the fixes for bug 565742 in that bug).

There's a bunch of long lines you can easily resolve.

I think you should make nsINode::Clone() take an already_AddRefed<nsINodeInfo> too. The only tricky part is if Clone is not implemented (returns NS_ERROR_NOT_IMPLEMENTED) then you need to make sure to release that pointer.

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

>     nsIAtom*            mName;
>     nsIAtom*            mPrefix;
>     PRInt32             mNamespaceID;
>+    const nsAString*    mTmpName;

mNameString?

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

>+nsresult
>+nsDocument::CreateElement(const nsAString& aTagName,
>+                          nsIContent** aReturn)
>+{
>+  *aReturn = nsnull;
>+  nsresult rv;
>+
>+  // if we are in quirks, allow surrounding '<' '>' for IE compat
>+  PRBool ieCompat = (mCompatMode == eCompatibility_NavQuirks &&
>+                     aTagName.Length() > 2 &&
>+                     aTagName.First() == '<' &&
>+                     aTagName.Last() == '>');
>+  PRBool needsLowercase = IsHTML() && !IsLowercaseASCII(aTagName);
>+  nsAutoString tagName;
>+  PRBool ieCompatOrLowerCase = ieCompat || needsLowercase;
>+  if (ieCompat) {
>+    tagName = Substring(aTagName, 1, aTagName.Length() - 2);
>+  }
>+
>+  rv = nsContentUtils::CheckQName(ieCompatOrLowerCase ? 
>+                                  tagName : aTagName, PR_FALSE);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  return CallQueryInterface(content, aReturn);
>+  if (needsLowercase) {
>+    ToLowerCase(tagName);
>+  }
>+
>+  rv = CreateElem(ieCompatOrLowerCase ? tagName : aTagName, nsnull,
>+                  IsHTML() ? kNameSpaceID_XHTML : GetDefaultNamespaceID(),
>+                  PR_TRUE, aReturn);
>+  return rv;
> }

I don't understand how this can work for a non-lowercase tagname without surrounding '<' '>', tagName is never set? Please add a test for that if there isn't one already.
This looks cleaner to me (if it works):

  // if we are in quirks, allow surrounding '<' '>' for IE compat
  PRBool ieCompat = (mCompatMode == eCompatibility_NavQuirks &&
                     aTagName.Length() > 2 &&
                     aTagName.First() == '<' &&
                     aTagName.Last() == '>');
  const nsAString& tagName = ieCompat ?
                             Substring(aTagName, 1, aTagName.Length() - 2) :
                             aTagName;

  rv = nsContentUtils::CheckQName(tagName, PR_FALSE);
  NS_ENSURE_SUCCESS(rv, rv);

  PRBool needsLowercase = IsHTML() && !IsLowercaseASCII(tagName);
  nsAutoString lcTagName;
  if (needsLowercase) {
    ToLowerCase(tagName, lcTagName);
  }

  return CreateElem(needsLowercase ? lcTagName : tagName, nsnull,
                    IsHTML() ? kNameSpaceID_XHTML : GetDefaultNamespaceID(),
                    PR_TRUE, aReturn);

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

>+#define DOMCI_NODE_DATA(_interface, _class)                             \
>+  DOMCI_DATA(_interface, _class)                                        \
>+  nsXPCClassInfo* _class::GetClassInfo()                                \
>+  {                                                                     \
>+    return static_cast<nsXPCClassInfo*>(                                \
>+      NS_GetDOMClassInfoInstance(eDOMClassInfo_##_interface##_id));     \

Can we make NS_GetDOMClassInfoInstance return nsXPCClassInfo*?

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

> PRIntn
> nsNodeInfoManager::NodeInfoInnerKeyCompare(const void *key1, const void *key2)
> {
>   NS_ASSERTION(key1 && key2, "Null key passed to NodeInfoInnerKeyCompare!");
> 
>   const nsINodeInfo::nsNodeInfoInner *node1 =
>     reinterpret_cast<const nsINodeInfo::nsNodeInfoInner *>(key1);
>   const nsINodeInfo::nsNodeInfoInner *node2 =
>     reinterpret_cast<const nsINodeInfo::nsNodeInfoInner *>(key2);
> 
>-  return (node1->mName == node2->mName &&
>+  if (node1->mName) {
>+    if (node2->mName) {
>+      return (node1->mName == node2->mName &&
>+              node1->mPrefix == node2->mPrefix &&
>+              node1->mNamespaceID == node2->mNamespaceID);
>+    }
>+    return (node1->mName->Equals(*(node2->mTmpName)) &&
>+            node1->mPrefix == node2->mPrefix &&
>+            node1->mNamespaceID == node2->mNamespaceID);
>+  } else if (node2->mName) {
>+    return (node2->mName->Equals(*(node1->mTmpName)) &&
>+            node1->mPrefix == node2->mPrefix &&
>+            node1->mNamespaceID == node2->mNamespaceID);
>+  }
>+  return (node1->mTmpName->Equals(*(node2->mTmpName)) &&
>           node1->mPrefix == node2->mPrefix &&
>           node1->mNamespaceID == node2->mNamespaceID);

I think this would be more readable if you returned early if node1->mPrefix != node2->mPrefix or node1->mNamespaceID != node2->mNamespaceID.

> nsresult
> nsNodeInfoManager::GetNodeInfo(const nsAString& aName, nsIAtom *aPrefix,
>                                PRInt32 aNamespaceID, nsINodeInfo** aNodeInfo)
> {
>-  nsCOMPtr<nsIAtom> name = do_GetAtom(aName);
>-  *aNodeInfo = nsNodeInfoManager::GetNodeInfo(name, aPrefix, aNamespaceID).get();
>-  return *aNodeInfo ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
>+  NS_ASSERTION(!aName.Equals(EmptyString()),

Why not IsEmpty()?

>+  PLHashEntry *he;
>+  he = PL_HashTableAdd(mNodeInfoHash, &newNodeInfo->mInner, newNodeInfo);
>+  NS_ENSURE_TRUE(he, NS_ERROR_FAILURE);
>+
>+  *aNodeInfo = newNodeInfo.forget().get();

I think |newNodeInfo.forget(aNodeInfo);| would work here.

>+
>+  return NS_OK;;

Stray semicolon

>diff --git a/content/html/content/src/nsGenericHTMLElement.h b/content/html/content/src/nsGenericHTMLElement.h

> #define NS_IMPL_NS_NEW_HTML_ELEMENT(_elementName)                            \
> nsGenericHTMLElement*                                                        \
>-NS_NewHTML##_elementName##Element(nsINodeInfo *aNodeInfo, PRBool aFromParser)\
>+NS_NewHTML##_elementName##Element(already_AddRefed<nsINodeInfo> aNodeInfo,  \

Realign the \ here and in other places.

>diff --git a/content/html/content/src/nsHTMLSharedElement.cpp b/content/html/content/src/nsHTMLSharedElement.cpp

>+nsXPCClassInfo* 
>+nsHTMLSharedElement::GetClassInfo()
>+{
>+  nsIClassInfo* info = nsnull;
>+  if (mNodeInfo->Equals(nsGkAtoms::param)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLParamElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::wbr)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLWBRElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::isindex)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLIsIndexElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::base)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLBaseElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::spacer)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLSpacerElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::dir)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLDirectoryElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::menu)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLMenuElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::q)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLQuoteElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::blockquote)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLQuoteElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::head)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLHeadElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::html)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLHtmlElement_id);
>+  }
>+  return static_cast<nsXPCClassInfo*>(info);
>+}

It'd be nice if we could somehow share these with the QI implementations in classes that can represent multiple types of elements. Maybe have a static GetClassInfo that the virtual and QI can call?

>diff --git a/content/html/document/src/nsHTMLContentSink.cpp b/content/html/document/src/nsHTMLContentSink.cpp

> nsresult
>-NS_NewHTMLElement(nsIContent** aResult, nsINodeInfo *aNodeInfo,
>+NS_NewHTMLElement(nsIContent** aResult, already_AddRefed<nsINodeInfo> aNodeInfo,
>                   PRBool aFromParser)
> {
>   *aResult = nsnull;
> 
>   nsIParserService* parserService = nsContentUtils::GetParserService();
>   if (!parserService)
>     return NS_ERROR_OUT_OF_MEMORY;

Maybe put the nodeinfo in a local comptr so it won't leak? This shouldn't happen, but still.
>diff --git a/js/src/xpconnect/src/dom_quickstubs.qsconf b/js/src/xpconnect/src/dom_quickstubs.qsconf

>+    'nsIDOMDocument_CreateElement': {
>+        'thisType': 'nsIDocument',
>+        'code': '    nsIContent* result = nsnull;\n'
>+                '    rv = static_cast<nsDocument*>(self)->CreateElement(arg0, &result);\n'

thisType should just be nsDocument.

>+    'nsIDOMDocument_CreateTextNode': {
>+        'thisType': 'nsIDocument',
>+        'code': '    nsIContent* result = nsnull;\n'
>+                '    rv = static_cast<nsDocument*>(self)->CreateTextNode(arg0, &result);\n'

Same here.

>diff --git a/js/src/xpconnect/src/qsgen.py b/js/src/xpconnect/src/qsgen.py

>@@ -675,19 +675,21 @@ def writeResultConv(f, type, jsvalPtr, j
>             return
>         # else fall through; this type isn't supported yet
>     elif isInterfaceType(type):
>         if isVariantType(type):
>             f.write("    return xpc_qsVariantToJsval(lccx, result, %s);\n"
>                     % jsvalPtr)
>             return
>         else:
>-            f.write("    return xpc_qsXPCOMObjectToJsval(lccx, "
>+            f.write("    qsDOMObjectHelper doh(identity);\n"

I really dislike this identity variable, because it means that the generated quickstubs code has to opt-in. There are already a number of quickstubs that have a nsINode result and afaict they're not getting the benefits of this patch. We should have a constructor for qsDOMObjectHelper that just sets mRef to nsnull for objects for which we don't have the canonical nsISupports. See below for more.

>+                    "    return xpc_qsXPCOMObjectToJsval(lccx, "
>                     "ToSupports(result), xpc_qsGetWrapperCache(result), "
>-                    "&NS_GET_IID(%s), &interfaces[k_%s], %s);\n"
>+                    "&NS_GET_IID(%s), &interfaces[k_%s], %s,"

Space after comma.

>@@ -1196,19 +1198,21 @@ def writeTraceableResultConv(f, type):

>+            f.write("    qsDOMObjectHelper doh(identity);\n"
>+                    "    JSBool ok = xpc_qsXPCOMObjectToJsval(lccx, "
>                     "ToSupports(result), xpc_qsGetWrapperCache(result), "
>-                    "&NS_GET_IID(%s), &interfaces[k_%s], &vp.array[0]);\n"
>+                    "&NS_GET_IID(%s), &interfaces[k_%s], &vp.array[0],"
>+                    " doh.get() ? &doh : nsnull);\n"

Space after comma, not before.

>diff --git a/js/src/xpconnect/src/xpcquickstubs.h b/js/src/xpconnect/src/xpcquickstubs.h

>@@ -71,16 +73,51 @@ struct xpc_qsHashEntry {
>     const xpc_qsFunctionSpec *functions;
>     const xpc_qsTraceableSpec *traceables;
>     // These last two fields index to other entries in the same table.
>     // XPC_QS_NULL_ENTRY indicates there are no more entries in the chain.
>     size_t parentInterface;
>     size_t chain;
> };
> 
>+// This is for nsINode objects for now.

I think we should generalize this a bit (but keep some special code for nsINode). The problem we're running into is that we lazily get a number of things in NativeInterface2JSObject, and sometimes we already have them before calling NativeInterface2JSObject. We should also be caching these things sometimes, for example if ConstructSlimWrapper fails we call XPCWrappedNative::GetNewOrUsed which gets the canonical nsISupports and the classinfo again. So I think that we should actually replace |nsISupports* src| with |qsDOMObjectHelper& helper| (probably renaming qsDOMObjectHelper), and have that store:

- the object to be wrapped
- the canonical nsISupports pointer for the object to be wrapped
- the nsWrapperCache for the object to be wrapped
- the classinfo for the object to be wrapped

It should also have getters that either return the existing value or get it and cache it. Because sometimes we have a weak pointer to some of these and sometimes a strong we might need to have both a weak and a nsCOMPtr. I think we need that for the canonical nsISupports and for the classinfo (probably weak nsXPCClassInfo and nsCOMPtr<nsIClassInfo>).
For now we also probably need to store nsINode (for GetClassInfo), ideally we'd make that more general too but I don't have any good ideas there.

We'd pass in ToSupports(result), ToCanonicalSupports(result) and xpc_qsGetWrapperCache(result) to the constructor of the helper. We'll need a ToCanonicalSupports that takes a already_AddRefed<nsINode> (returning already_AddRefed<nsISupports>) and one that takes a nsINode* (returning nsISupports*), and two constructors. Might also add ToCanonicalSupports for nsINodeList. And one that takes void and returns null.

We should then pass that helper to ConstructSlimWrapper and XPCWrappedNative::GetNewOrUsed.

What do you think? It's a bit complicated, but there doesn't seem to be an easy solution to cover all cases (don't have canonical, have weak canonical, have strong canonical, with or withour wrappercache and with or without fast access to CI).

> JSBool
> xpc_qsXPCOMObjectToJsval(XPCLazyCallContext &lccx,
>-                         nsISupports *p,
>+                         nsISupports* p,

Don't.
Attachment #447305 - Flags: review?(peterv) → review-
(Assignee)

Comment 24

9 years ago
(In reply to comment #23)
> I think you should make nsINode::Clone() take an already_AddRefed<nsINodeInfo>
> too. The only tricky part is if Clone is not implemented (returns
> NS_ERROR_NOT_IMPLEMENTED) then you need to make sure to release that pointer.
That would be a followup bug.


> 
> >     nsIAtom*            mName;
> >     nsIAtom*            mPrefix;
> >     PRInt32             mNamespaceID;
> >+    const nsAString*    mTmpName;
> 
> mNameString?
Ok


> 
> >diff --git a/content/base/src/nsDocument.cpp b/content/base/src/nsDocument.cpp
> 
> >+nsresult
> >+nsDocument::CreateElement(const nsAString& aTagName,
> >+                          nsIContent** aReturn)
> >+{
> >+  *aReturn = nsnull;
> >+  nsresult rv;
> >+
> >+  // if we are in quirks, allow surrounding '<' '>' for IE compat
> >+  PRBool ieCompat = (mCompatMode == eCompatibility_NavQuirks &&
> >+                     aTagName.Length() > 2 &&
> >+                     aTagName.First() == '<' &&
> >+                     aTagName.Last() == '>');
> >+  PRBool needsLowercase = IsHTML() && !IsLowercaseASCII(aTagName);
> >+  nsAutoString tagName;
> >+  PRBool ieCompatOrLowerCase = ieCompat || needsLowercase;
> >+  if (ieCompat) {
> >+    tagName = Substring(aTagName, 1, aTagName.Length() - 2);
> >+  }
> >+
> >+  rv = nsContentUtils::CheckQName(ieCompatOrLowerCase ? 
> >+                                  tagName : aTagName, PR_FALSE);
> >   NS_ENSURE_SUCCESS(rv, rv);
> > 
> >-  return CallQueryInterface(content, aReturn);
> >+  if (needsLowercase) {
> >+    ToLowerCase(tagName);
> >+  }
> >+
> >+  rv = CreateElem(ieCompatOrLowerCase ? tagName : aTagName, nsnull,
> >+                  IsHTML() ? kNameSpaceID_XHTML : GetDefaultNamespaceID(),
> >+                  PR_TRUE, aReturn);
> >+  return rv;
> > }
> 
> I don't understand how this can work for a non-lowercase tagname without
> surrounding '<' '>', tagName is never set? Please add a test for that if there
> isn't one already.
Argh, there is a bug indeed. It used to be ok when I had nsString* tagName

> This looks cleaner to me (if it works):
> 
>   // if we are in quirks, allow surrounding '<' '>' for IE compat
>   PRBool ieCompat = (mCompatMode == eCompatibility_NavQuirks &&
>                      aTagName.Length() > 2 &&
>                      aTagName.First() == '<' &&
>                      aTagName.Last() == '>');
>   const nsAString& tagName = ieCompat ?
>                              Substring(aTagName, 1, aTagName.Length() - 2) :
>                              aTagName;
> 
>   rv = nsContentUtils::CheckQName(tagName, PR_FALSE);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   PRBool needsLowercase = IsHTML() && !IsLowercaseASCII(tagName);
>   nsAutoString lcTagName;
>   if (needsLowercase) {
>     ToLowerCase(tagName, lcTagName);
>   }
> 
>   return CreateElem(needsLowercase ? lcTagName : tagName, nsnull,
>                     IsHTML() ? kNameSpaceID_XHTML : GetDefaultNamespaceID(),
>                     PR_TRUE, aReturn);
OK, will use this.


> >+#define DOMCI_NODE_DATA(_interface, _class)                             \
> >+  DOMCI_DATA(_interface, _class)                                        \
> >+  nsXPCClassInfo* _class::GetClassInfo()                                \
> >+  {                                                                     \
> >+    return static_cast<nsXPCClassInfo*>(                                \
> >+      NS_GetDOMClassInfoInstance(eDOMClassInfo_##_interface##_id));     \
> 
> Can we make NS_GetDOMClassInfoInstance return nsXPCClassInfo*?
Perhaps we can. I don't care.

 
> I think this would be more readable if you returned early if node1->mPrefix !=
> node2->mPrefix or node1->mNamespaceID != node2->mNamespaceID.
ok


> > nsNodeInfoManager::GetNodeInfo(const nsAString& aName, nsIAtom *aPrefix,
> >                                PRInt32 aNamespaceID, nsINodeInfo** aNodeInfo)
> > {
> >-  nsCOMPtr<nsIAtom> name = do_GetAtom(aName);
> >-  *aNodeInfo = nsNodeInfoManager::GetNodeInfo(name, aPrefix, aNamespaceID).get();
> >-  return *aNodeInfo ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
> >+  NS_ASSERTION(!aName.Equals(EmptyString()),
> 
> Why not IsEmpty()?
DEBUG code, I don't care. But will change.

> >+  PLHashEntry *he;
> >+  he = PL_HashTableAdd(mNodeInfoHash, &newNodeInfo->mInner, newNodeInfo);
> >+  NS_ENSURE_TRUE(he, NS_ERROR_FAILURE);
> >+
> >+  *aNodeInfo = newNodeInfo.forget().get();
> 
> I think |newNodeInfo.forget(aNodeInfo);| would work here.
Sure, it does the same thing. I can change.


> It'd be nice if we could somehow share these with the QI implementations in
> classes that can represent multiple types of elements. Maybe have a static
> GetClassInfo that the virtual and QI can call?
Sounds good.

> 
> Maybe put the nodeinfo in a local comptr so it won't leak? This shouldn't
> happen, but still.
It really shouldn't happen, but I could do that.

> I really dislike this identity variable, because it means that the generated
> quickstubs code has to opt-in. There are already a number of quickstubs that
> have a nsINode result and afaict they're not getting the benefits of this
> patch. We should have a constructor for qsDOMObjectHelper that just sets mRef
> to nsnull for objects for which we don't have the canonical nsISupports. See
> below for more.
This is the kind of review I was hoping to get :)


> 
> >+                    "    return xpc_qsXPCOMObjectToJsval(lccx, "
> >                     "ToSupports(result), xpc_qsGetWrapperCache(result), "
> >-                    "&NS_GET_IID(%s), &interfaces[k_%s], %s);\n"
> >+                    "&NS_GET_IID(%s), &interfaces[k_%s], %s,"
> 
> Space after comma.
I don't know which comma, but perhaps I'll figure out.


> 
> >@@ -1196,19 +1198,21 @@ def writeTraceableResultConv(f, type):
> 
> >+            f.write("    qsDOMObjectHelper doh(identity);\n"
> >+                    "    JSBool ok = xpc_qsXPCOMObjectToJsval(lccx, "
> >                     "ToSupports(result), xpc_qsGetWrapperCache(result), "
> >-                    "&NS_GET_IID(%s), &interfaces[k_%s], &vp.array[0]);\n"
> >+                    "&NS_GET_IID(%s), &interfaces[k_%s], &vp.array[0],"
> >+                    " doh.get() ? &doh : nsnull);\n"
> 
> Space after comma, not before.
Same here.


> I think we should generalize this a bit (but keep some special code for
> nsINode). The problem we're running into is that we lazily get a number of
> things in NativeInterface2JSObject, and sometimes we already have them before
> calling NativeInterface2JSObject. We should also be caching these things
> sometimes, for example if ConstructSlimWrapper fails we call
> XPCWrappedNative::GetNewOrUsed which gets the canonical nsISupports and the
> classinfo again. So I think that we should actually replace |nsISupports* src|
> with |qsDOMObjectHelper& helper| (probably renaming qsDOMObjectHelper), and
> have that store:
> 
> - the object to be wrapped
> - the canonical nsISupports pointer for the object to be wrapped
> - the nsWrapperCache for the object to be wrapped
> - the classinfo for the object to be wrapped
> 
> It should also have getters that either return the existing value or get it and
> cache it. Because sometimes we have a weak pointer to some of these and
> sometimes a strong we might need to have both a weak and a nsCOMPtr. I think we
> need that for the canonical nsISupports and for the classinfo (probably weak
> nsXPCClassInfo and nsCOMPtr<nsIClassInfo>).
> For now we also probably need to store nsINode (for GetClassInfo), ideally we'd
> make that more general too but I don't have any good ideas there.
> 
> We'd pass in ToSupports(result), ToCanonicalSupports(result) and
> xpc_qsGetWrapperCache(result) to the constructor of the helper. We'll need a
> ToCanonicalSupports that takes a already_AddRefed<nsINode> (returning
> already_AddRefed<nsISupports>) and one that takes a nsINode* (returning
> nsISupports*), and two constructors. Might also add ToCanonicalSupports for
> nsINodeList. And one that takes void and returns null.
> 
> We should then pass that helper to ConstructSlimWrapper and
> XPCWrappedNative::GetNewOrUsed.
> 
> What do you think? It's a bit complicated, but there doesn't seem to be an easy
> solution to cover all cases (don't have canonical, have weak canonical, have
> strong canonical, with or withour wrappercache and with or without fast access
> to CI).
I think this sounds good. A bit complicated but way better what I did.
All the qs code is just very new to me.
(Assignee)

Comment 25

9 years ago
Created attachment 451024 [details] [diff] [review]
some tests
(Assignee)

Comment 26

9 years ago
Created attachment 451029 [details] [diff] [review]
up-to-date

Backing up the patch here. Misses still some xpconnect changes.
(Assignee)

Comment 27

9 years ago
Created attachment 451269 [details] [diff] [review]
patch

This has a bit less-ugly qs implementation.
Couldn't use qsObjectHelper ctor for everything because of 
C++'s idiotic parameter evaluation order.
Attachment #451269 - Flags: review?(peterv)
Created attachment 458999 [details] [diff] [review]
patch

I think this is the unbitrotted version of attachment 451269 [details] [diff] [review] (sorry for the delays). I'll put an additional patch for qsObjectHelper in bug 565742, so that this can just land.

Please don't forget to file the bug for making Clone take an already_AddRefed<nsINodeInfo>.

>diff --git a/content/base/src/nsDocument.cpp b/content/base/src/nsDocument.cpp
>--- a/content/base/src/nsDocument.cpp
>+++ b/content/base/src/nsDocument.cpp
>@@ -191,16 +191,18 @@ static NS_DEFINE_CID(kDOMEventGroupCID, 
> #include "nsSVGUtils.h"
> #endif // MOZ_SMIL
> 
> // FOR CSP (autogenerated by xpidl)
> #include "nsIContentSecurityPolicy.h"
> #include "nsHTMLStyleSheet.h"
> #include "nsHTMLCSSStyleSheet.h"
> 
>+#include "nsXMLEventsManager.h"

Not needed.

>diff --git a/content/base/src/nsDocument.h b/content/base/src/nsDocument.h
>--- a/content/base/src/nsDocument.h
>+++ b/content/base/src/nsDocument.h
>@@ -125,16 +125,17 @@ class nsDocument;
> class nsIDTD;
> class nsIRadioVisitor;
> class nsIFormControl;
> struct nsRadioGroupStruct;
> class nsOnloadBlocker;
> class nsUnblockOnloadEvent;
> class nsChildContentList;
> class nsXMLEventsManager;
>+class nsXMLEventsManager;

Not needed.

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

> PRIntn
> nsNodeInfoManager::NodeInfoInnerKeyCompare(const void *key1, const void *key2)
> {
>   NS_ASSERTION(key1 && key2, "Null key passed to NodeInfoInnerKeyCompare!");
> 
>   const nsINodeInfo::nsNodeInfoInner *node1 =
>     reinterpret_cast<const nsINodeInfo::nsNodeInfoInner *>(key1);
>   const nsINodeInfo::nsNodeInfoInner *node2 =
>     reinterpret_cast<const nsINodeInfo::nsNodeInfoInner *>(key2);
> 
>-  return (node1->mName == node2->mName &&
>-          node1->mPrefix == node2->mPrefix &&
>-          node1->mNamespaceID == node2->mNamespaceID);
>+  if (node1->mPrefix != node2->mPrefix ||
>+      node1->mNamespaceID != node2->mNamespaceID) {
>+    return 0;
>+  }
>+
>+  if (node1->mName) {
>+    if (node2->mName) {
>+      return (node1->mName == node2->mName);
>+    }
>+    return (node1->mName->Equals(*(node2->mNameString)));
>+  } else if (node2->mName) {
>+    return (node2->mName->Equals(*(node1->mNameString)));
>+  }
>+  return (node1->mNameString->Equals(*(node2->mNameString)));

Much better, now remove the else-after-returns :-).

>diff --git a/content/html/content/src/nsGenericHTMLElement.h b/content/html/content/src/nsGenericHTMLElement.h

>+#define NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_IF_TAG \
>+  if (aIID.Equals(NS_GET_IID(nsIClassInfo)) ||      \
>+      aIID.Equals(NS_GET_IID(nsXPCClassInfo))) {    \
>+    foundInterface = GetClassInfoInternal();        \
>+    if (!foundInterface) {                          \
>+      *aInstancePtr = nsnull;                       \
>+      return NS_ERROR_OUT_OF_MEMORY;                \
>+    }                                               \
>   } else

I'd make this NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_GETTER(_getter) and use NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_GETTER(GetClassInfoInternal) where needed. There isn't really anything related to a tag in the implementation of the macro.

>diff --git a/content/html/content/src/nsHTMLDelElement.cpp b/content/html/content/src/nsHTMLDelElement.cpp

>+nsIClassInfo* 
>+nsHTMLModElement::GetClassInfoInternal()
>+{
>+  if (mNodeInfo->Equals(nsGkAtoms::del)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLDelElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::ins)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLInsElement_id);
>+  }

else-after-return.

>diff --git a/content/html/content/src/nsHTMLOListElement.cpp b/content/html/content/src/nsHTMLOListElement.cpp

>+nsIClassInfo* 
>+nsHTMLSharedListElement::GetClassInfoInternal()
>+{
>+  if (mNodeInfo->Equals(nsGkAtoms::ol)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLOListElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::dl)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLDListElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::ul)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLUListElement_id);
>+  }

else-after-return.

>diff --git a/content/html/content/src/nsHTMLSharedElement.cpp b/content/html/content/src/nsHTMLSharedElement.cpp

>+nsIClassInfo*
>+nsHTMLSharedElement::GetClassInfoInternal()
>+{
>+  if (mNodeInfo->Equals(nsGkAtoms::param)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLParamElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::wbr)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLWBRElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::isindex)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLIsIndexElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::base)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLBaseElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::spacer)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLSpacerElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::dir)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLDirectoryElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::menu)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLMenuElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::q)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLQuoteElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::blockquote)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLQuoteElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::head)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLHeadElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::html)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLHtmlElement_id);
>+  }

else-after-return

>diff --git a/content/html/content/src/nsHTMLSharedObjectElement.cpp b/content/html/content/src/nsHTMLSharedObjectElement.cpp

>+nsIClassInfo*
>+nsHTMLSharedObjectElement::GetClassInfoInternal()
>+{
>+  if (mNodeInfo->Equals(nsGkAtoms::applet)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLAppletElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::embed)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLEmbedElement_id);
>+  }

else-after-return

>diff --git a/content/svg/content/src/nsSVGElement.h b/content/svg/content/src/nsSVGElement.h

> #define NS_IMPL_NS_NEW_SVG_ELEMENT(_elementName)                             \
> nsresult                                                                     \
> NS_NewSVG##_elementName##Element(nsIContent **aResult,                       \
>-                                 nsINodeInfo *aNodeInfo)                     \
>+                                 already_AddRefed<nsINodeInfo> aNodeInfo)           \

> #define NS_IMPL_NS_NEW_SVG_ELEMENT_CHECK_PARSER(_elementName)                \
> nsresult                                                                     \
> NS_NewSVG##_elementName##Element(nsIContent **aResult,                       \
>-                                 nsINodeInfo *aNodeInfo,                     \
>+                                 already_AddRefed<nsINodeInfo> aNodeInfo,           \

Realign the backslashes.

>diff --git a/js/src/xpconnect/src/Makefile.in b/js/src/xpconnect/src/Makefile.in

>diff --git a/layout/forms/nsIsIndexFrame.cpp b/layout/forms/nsIsIndexFrame.cpp
>--- a/layout/forms/nsIsIndexFrame.cpp
>+++ b/layout/forms/nsIsIndexFrame.cpp
>@@ -15,17 +15,17 @@
>  * The Original Code is mozilla.org code.
>  *
>  * The Initial Developer of the Original Code is
>  * Netscape Communications Corporation.
>  * Portions created by the Initial Developer are Copyright (C) 1998
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>- *
>+ *                               

Not needed.
Attachment #451269 - Attachment is obsolete: true
Attachment #458999 - Flags: review+
Attachment #451269 - Flags: review?(peterv)
(Assignee)

Comment 29

8 years ago
Created attachment 459396 [details] [diff] [review]
patch
Attachment #459396 - Flags: approval2.0?
(Assignee)

Updated

8 years ago
Blocks: 580984

Updated

8 years ago
Attachment #459396 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 30

8 years ago
http://hg.mozilla.org/mozilla-central/rev/58101a16aff7
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Depends on: 581984

Comment 31

8 years ago
How "faster" is the new implementation?
(Assignee)

Comment 32

8 years ago
Comparing to what?

~45% faster than 3.6.

Comment 33

8 years ago
to 3.6. Thank you!
Depends on: 595708
You need to log in before you can comment on or make changes to this bug.