Comment on attachment 737243 [details] [diff] [review] Patch v1 Review of attachment 737243 [details] [diff] [review]: ----------------------------------------------------------------- The rest of this looks fine. Can you teach me about the CC macros used here, or point me to some documentation so I know what those changes do exactly? ::: content/svg/content/src/DOMSVGStringList.cpp @@ +25,3 @@ > > +NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(DOMSVGStringList, AddRef) > +NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(DOMSVGStringList, Release) I don't really know anything about correct use of CC macros. I assume these changes are because we're no longer implementing nsISupports? @@ +117,5 @@ > +void > +DOMSVGStringList::InsertItemBefore(const nsAString& aNewItem, uint32_t aIndex, > + nsAString& aRetval, ErrorResult& aRv) > +{ > + if (aNewItem.IsEmpty()) { // takes care of DOMStringIsNull too Remove the comment, since we shouldn't get null in here now. @@ +146,2 @@ > { > + if (aNewItem.IsEmpty()) { // takes care of DOMStringIsNull too As above. @@ +196,1 @@ > return tests->mStringListAttributes[mAttrEnum]; You can remove the "dom::" with the "using namespace" earlier on. ::: content/svg/content/src/DOMSVGStringList.h @@ +47,4 @@ > { > public: > + NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(DOMSVGStringList) > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS(DOMSVGStringList) I assume these change since we need to define our own native AddRef/Release rather than nsISupports ones. I don't understand the "_SCRIPT_HOLDER" bit though.

(In reply to Cameron McCormack (:heycam) from comment #2) > Comment on attachment 737243 [details] [diff] [review] > Patch v1 > > Review of attachment 737243 [details] [diff] [review]: > ----------------------------------------------------------------- > > The rest of this looks fine. Can you teach me about the CC macros used > here, or point me to some documentation so I know what those changes do > exactly? > > ::: content/svg/content/src/DOMSVGStringList.cpp > @@ +25,3 @@ > > > > +NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(DOMSVGStringList, AddRef) > > +NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(DOMSVGStringList, Release) > > I don't really know anything about correct use of CC macros. I assume these > changes are because we're no longer implementing nsISupports? Exactly. > ::: content/svg/content/src/DOMSVGStringList.h > @@ +47,4 @@ > > { > > public: > > + NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(DOMSVGStringList) > > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS(DOMSVGStringList) > > I assume these change since we need to define our own native AddRef/Release > rather than nsISupports ones. I don't understand the "_SCRIPT_HOLDER" bit > though. The "_SCRIPT_HOLDER" is necessary because we're wrappercaching now, which means we need to keep out JS reflection alive.

Comment on attachment 737243 [details] [diff] [review] Patch v1 Smaug, can you look at the CC stuff?

Comment on attachment 737243 [details] [diff] [review] Patch v1 r=me for the rest, with comment 2 addressed.