Move SVGStringList to WebIDL; remove nsIDOMSVGStringList

RESOLVED FIXED in mozilla23

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

(Blocks: 1 bug)

Trunk
mozilla23
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Updated

5 years ago
Blocks: 580070
(Assignee)

Comment 1

5 years ago
Created attachment 737243 [details] [diff] [review]
Patch v1
Attachment #737243 - Flags: review?(cam)
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.
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
Comment on attachment 737243 [details] [diff] [review]
Patch v1

Smaug, can you look at the CC stuff?
Attachment #737243 - Flags: review?(bugs)

Updated

5 years ago
Attachment #737243 - Flags: review?(bugs) → review+
Comment on attachment 737243 [details] [diff] [review]
Patch v1

r=me for the rest, with comment 2 addressed.
Attachment #737243 - Flags: review?(cam) → review+
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/mozilla-central/rev/0d2e8dcbec44
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23

Updated

5 years ago
Depends on: 865880
You need to log in before you can comment on or make changes to this bug.