deCOMtaminate nsINodeInfo

RESOLVED FIXED in mozilla33

Status

()

RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

(Blocks: 1 bug)

unspecified
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment on attachment 608044 [details] [diff] [review]
Patch

bent gave me an rs for this.
Attachment #608044 - Flags: review?(jst) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b00bf7f3869c - something started leaking in mochitest-1, sometimes including a domwindow from a media test and sometimes not, beginning with your push.
Among the various possibilities is that the leaks actually came from bug 633602 or one of the things in that push that I also backed out, so I pushed you to try in https://tbpl.mozilla.org/?tree=Try&rev=7e3313380616
Hmm, looks like it was me :-(
Depends on: 750570
Posted patch Patch (obsolete) — Splinter Review
I tried to be consistent here and use nsINode::Info in anything already already using nsINode.h, otherwise forward declare mozilla::dom::NodeInfo.
Assignee: nobody → khuey
Attachment #608044 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #795550 - Flags: review?(bugs)
Comment on attachment 795550 [details] [diff] [review]
Patch


> #define NS_IMPL_ELEMENT_CLONE_WITH_INIT(_elementName)                       \
> nsresult                                                                    \
>-_elementName::Clone(nsINodeInfo *aNodeInfo, nsINode **aResult) const        \
>+_elementName::Clone(nsINode::Info *aNodeInfo, nsINode **aResult) const  \
> {                                                                           \
>-  *aResult = nullptr;                                                        \
>-  nsCOMPtr<nsINodeInfo> ni = aNodeInfo;                                     \
>+  *aResult = nullptr;                                                       \
>+  nsRefPtr<nsINode::Info> ni = aNodeInfo;                               \
>   _elementName *it = new _elementName(ni.forget());                         \
>   if (!it) {                                                                \
>     return NS_ERROR_OUT_OF_MEMORY;                                          \
>   }                                                                         \
Align \


> class nsINode : public mozilla::dom::EventTarget
> {
> public:
>   NS_DECLARE_STATIC_IID_ACCESSOR(NS_INODE_IID)
> 
>+  typedef mozilla::dom::NodeInfo Info;
So this makes the code hard to understand.
One needs to know that there is NodeInfo and that nsINode::Info is the same thing.
There is no need for nsINode::Info, or if we want that, 'class Info' could be declared inside nsINode.
But please, no both Info and NodeInfo
Attachment #795550 - Flags: review?(bugs) → review-
Target Milestone: mozilla14 → ---
Posted patch PatchSplinter Review
Attachment #795550 - Attachment is obsolete: true
Attachment #8442269 - Flags: review?(bugs)
Comment on attachment 8442269 [details] [diff] [review]
Patch

Changes to NameSpaceManager look rather unrelated, but fine.

Feels odd to move NodeInfo to dom/base before nsINode, but ok, no need to do the
move later.

I'm not going to complain about wrong style for *. (it should go with the type)

You could have used dom::NodeInfo in many places, no?

> nsNodeInfoManager::GetNodeInfoInnerHashValue(const void *key)
> {
>-  NS_ASSERTION(key, "Null key passed to nsNodeInfo::GetHashValue!");
>+  MOZ_ASSERT(key, "Null key passed to NodeInfo::GetHashValue!");
> 
>-  const nsINodeInfo::nsNodeInfoInner *node =
>-    reinterpret_cast<const nsINodeInfo::nsNodeInfoInner *>(key);
>+  auto *node = reinterpret_cast<const NodeInfo::NodeInfoInner*>(key);
> 
>   return node->mName ? node->mName->hash() : HashString(*(node->mNameString));
> }
> 
> 
> int
> nsNodeInfoManager::NodeInfoInnerKeyCompare(const void *key1, const void *key2)
> {
>-  NS_ASSERTION(key1 && key2, "Null key passed to NodeInfoInnerKeyCompare!");
>+  MOZ_ASSERT(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);
>+  auto *node1 = reinterpret_cast<const NodeInfo::NodeInfoInner*>(key1);
>+  auto *node2 = reinterpret_cast<const NodeInfo::NodeInfoInner*>(key2);

There is really no reason for auto usage here.


>-  already_AddRefed<nsINodeInfo> GetNodeInfo(nsIAtom *aName, nsIAtom *aPrefix,
>+  already_AddRefed<mozilla::dom::NodeInfo> GetNodeInfo(nsIAtom *aName, nsIAtom *aPrefix,
>                                             int32_t aNamespaceID,
>                                             uint16_t aNodeType,
>                                             nsIAtom* aExtraName = nullptr);
Please fix indentation






>     nsPrototypeArray         mChildren;
> 
>-    nsCOMPtr<nsINodeInfo>    mNodeInfo;           // [OWNER]
>+    nsRefPtr<mozilla::dom::NodeInfo>    mNodeInfo;           // [OWNER]
No need for so many empty spaces between > and m


>+NodeInfo::GetName(nsAString& aName) const
>+{
>+  mInner.mName->ToString(aName);
>+}
>+
>+void
>+NodeInfo::GetPrefix(nsAString& aPrefix) const
>+{
>+  if (mInner.mPrefix) {
>+    mInner.mPrefix->ToString(aPrefix);
>+  } else {
>+    SetDOMStringToNull(aPrefix);
>+  }
>+}
Why you un-inline these?
Could you just move to NodeInfoInlines.h ?

>- * nsINodeInfo is an interface to node info, such as name, prefix, namespace
>- * ID and possibly other data that is shared between nodes (elements
>- * and attributes) that have the same name, prefix and namespace ID within
>- * the same document.
>+ * Class that represents a prefix/namespace/localName triple; a single
>+ * nodeinfo is shared by all elements in a document that have that
>+ * prefix, namespace, and localName.
That isn't quite accurate description, since there is also qname etc.
So perhaps s/ triple/\/etc. /

>  *
>  * nsNodeInfoManager's are internal objects that manage a list of
>- * nsINodeInfo's, every document object should hold a strong reference to
>- * a nsNodeInfoManager and every nsINodeInfo also holds a strong reference
>- * to their owning manager. When a nsINodeInfo is no longer used it will
>+ * NodeInfo's, every document object should hold a strong reference to
Shouldn't it be nsNodeInfoManagers are....
manage a list of NodeInfos.




>+ * a nsNodeInfoManager and every NodeInfo also holds a strong reference
>+ * to their owning manager. When a NodeInfo is no longer used it will
>  * automatically remove itself from its owner manager, and when all
>- * nsINodeInfo's have been removed from a nsNodeInfoManager and all external
>+ * NodeInfo's have been removed from a nsNodeInfoManager and all external
Shouldn't this be NodeInfos

>-  nsNodeInfoManager *NodeInfoManager() const
>+  nsNodeInfoManager* NodeInfoManager() const
Ooh, correct style here

> 
>   // nsNodeInfoManager needs to pass mInner to the hash table.
>-  friend class nsNodeInfoManager;
>+  friend class ::nsNodeInfoManager;
Hmm, :: is needed here, I guess

>  
>+++ b/dom/xslt/xslt/txMozillaTextOutput.cpp
>@@ -14,16 +14,17 @@
> #include "nsCharsetSource.h"
> #include "nsIPrincipal.h"
> #include "txURIUtils.h"
> #include "nsContentCreatorFunctions.h"
> #include "nsContentUtils.h"
> #include "nsGkAtoms.h"
> #include "mozilla/dom/EncodingUtils.h"
> #include "nsTextNode.h"
>+#include "nsNamespaceManager.h"
Is this really needed?
Attachment #8442269 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #9)
> Comment on attachment 8442269 [details] [diff] [review]
> Patch
> 
> Changes to NameSpaceManager look rather unrelated, but fine.
> 
> Feels odd to move NodeInfo to dom/base before nsINode, but ok, no need to do
> the
> move later.
> 
> I'm not going to complain about wrong style for *. (it should go with the
> type)

Yeah, I didn't want to try to change the world.

> You could have used dom::NodeInfo in many places, no?

Yes.  It would have made this a lot less automatic to write though.
 
> > nsNodeInfoManager::GetNodeInfoInnerHashValue(const void *key)
> > {
> >-  NS_ASSERTION(key, "Null key passed to nsNodeInfo::GetHashValue!");
> >+  MOZ_ASSERT(key, "Null key passed to NodeInfo::GetHashValue!");
> > 
> >-  const nsINodeInfo::nsNodeInfoInner *node =
> >-    reinterpret_cast<const nsINodeInfo::nsNodeInfoInner *>(key);
> >+  auto *node = reinterpret_cast<const NodeInfo::NodeInfoInner*>(key);
> > 
> >   return node->mName ? node->mName->hash() : HashString(*(node->mNameString));
> > }
> > 
> > 
> > int
> > nsNodeInfoManager::NodeInfoInnerKeyCompare(const void *key1, const void *key2)
> > {
> >-  NS_ASSERTION(key1 && key2, "Null key passed to NodeInfoInnerKeyCompare!");
> >+  MOZ_ASSERT(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);
> >+  auto *node1 = reinterpret_cast<const NodeInfo::NodeInfoInner*>(key1);
> >+  auto *node2 = reinterpret_cast<const NodeInfo::NodeInfoInner*>(key2);
> 
> There is really no reason for auto usage here.

Sure there is, we avoid writing the (rather lengthy) type twice.

> >-  already_AddRefed<nsINodeInfo> GetNodeInfo(nsIAtom *aName, nsIAtom *aPrefix,
> >+  already_AddRefed<mozilla::dom::NodeInfo> GetNodeInfo(nsIAtom *aName, nsIAtom *aPrefix,
> >                                             int32_t aNamespaceID,
> >                                             uint16_t aNodeType,
> >                                             nsIAtom* aExtraName = nullptr);
> Please fix indentation

Done.

> >     nsPrototypeArray         mChildren;
> > 
> >-    nsCOMPtr<nsINodeInfo>    mNodeInfo;           // [OWNER]
> >+    nsRefPtr<mozilla::dom::NodeInfo>    mNodeInfo;           // [OWNER]
> No need for so many empty spaces between > and m

Fixed.

> >+NodeInfo::GetName(nsAString& aName) const
> >+{
> >+  mInner.mName->ToString(aName);
> >+}
> >+
> >+void
> >+NodeInfo::GetPrefix(nsAString& aPrefix) const
> >+{
> >+  if (mInner.mPrefix) {
> >+    mInner.mPrefix->ToString(aPrefix);
> >+  } else {
> >+    SetDOMStringToNull(aPrefix);
> >+  }
> >+}
> Why you un-inline these?
> Could you just move to NodeInfoInlines.h ?

They're used all over the place so it would require including it everywhere.

> >- * nsINodeInfo is an interface to node info, such as name, prefix, namespace
> >- * ID and possibly other data that is shared between nodes (elements
> >- * and attributes) that have the same name, prefix and namespace ID within
> >- * the same document.
> >+ * Class that represents a prefix/namespace/localName triple; a single
> >+ * nodeinfo is shared by all elements in a document that have that
> >+ * prefix, namespace, and localName.
> That isn't quite accurate description, since there is also qname etc.
> So perhaps s/ triple/\/etc. /
> 
> >  *
> >  * nsNodeInfoManager's are internal objects that manage a list of
> >- * nsINodeInfo's, every document object should hold a strong reference to
> >- * a nsNodeInfoManager and every nsINodeInfo also holds a strong reference
> >- * to their owning manager. When a nsINodeInfo is no longer used it will
> >+ * NodeInfo's, every document object should hold a strong reference to
> Shouldn't it be nsNodeInfoManagers are....
> manage a list of NodeInfos.
> 
> 
> 
> 
> >+ * a nsNodeInfoManager and every NodeInfo also holds a strong reference
> >+ * to their owning manager. When a NodeInfo is no longer used it will
> >  * automatically remove itself from its owner manager, and when all
> >- * nsINodeInfo's have been removed from a nsNodeInfoManager and all external
> >+ * NodeInfo's have been removed from a nsNodeInfoManager and all external
> Shouldn't this be NodeInfos

Yeah, fixed.

> >-  nsNodeInfoManager *NodeInfoManager() const
> >+  nsNodeInfoManager* NodeInfoManager() const
> Ooh, correct style here
> 
> > 
> >   // nsNodeInfoManager needs to pass mInner to the hash table.
> >-  friend class nsNodeInfoManager;
> >+  friend class ::nsNodeInfoManager;
> Hmm, :: is needed here, I guess
> 
> >  
> >+++ b/dom/xslt/xslt/txMozillaTextOutput.cpp
> >@@ -14,16 +14,17 @@
> > #include "nsCharsetSource.h"
> > #include "nsIPrincipal.h"
> > #include "txURIUtils.h"
> > #include "nsContentCreatorFunctions.h"
> > #include "nsContentUtils.h"
> > #include "nsGkAtoms.h"
> > #include "mozilla/dom/EncodingUtils.h"
> > #include "nsTextNode.h"
> >+#include "nsNamespaceManager.h"
> Is this really needed?

Yes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac426472ceec
https://hg.mozilla.org/mozilla-central/rev/ac426472ceec
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.