886420 - Move SVGNumber to WebIDL

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 777176 [details] [diff] [review]
patch

Review of attachment 777176 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/DOMSVGNumber.cpp
@@ +52,5 @@
>                             uint8_t aAttrEnum,
>                             uint32_t aListIndex,
>                             bool aIsAnimValItem)
>    : mList(aList)
> +  , mParent(aList)

Can the number be removed from the list without wrapping it first?

@@ +80,4 @@
>  }
>  
> +/* static */ already_AddRefed<DOMSVGNumber>
> +DOMSVGNumber::Constructor(const dom::GlobalObject& aGlobal, ErrorResult& aRv)

using namespace mozilla::dom; instead

@@ +88,5 @@
> +    return nullptr;
> +  }
> +
> +  nsRefPtr<DOMSVGNumber> number = new DOMSVGNumber();
> +  number->mParent = window;

Bah.

::: content/svg/content/src/DOMSVGNumber.h
@@ -34,5 @@
>   * This class creates the DOM objects that wrap internal SVGNumber objects that
>   * are in an SVGNumberList. It is also used to create the objects returned by
>   * SVGSVGElement.createSVGNumber().
>   *
> - * For the DOM wrapper classes for non-list SVGNumber, see nsSVGNumber2.h.

Why remove this?

::: content/svg/content/src/DOMSVGNumberList.cpp
@@ +141,5 @@
>    }
>  }
>  
> +DOMSVGNumber*
> +DOMSVGNumberList::Initialize(dom::NonNull<DOMSVGNumber>& aItem,

DOMSVGNumber& aItem

@@ +158,5 @@
>    // clone of newItem, it would actually insert newItem. To prevent that from
>    // happening we have to do the clone here, if necessary.
>  
> +  if (aItem.get()->HasOwner()) {
> +    aItem = aItem.get()->Clone();

Keep the nsCOMPtr

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8424488 [details] [diff] [review]
Patch v2

>+++ b/content/svg/content/src/DOMSVGNumber.cpp
>+DOMSVGNumber::SetValue(float aValue, ErrorResult& aRv)
>+  if (!NS_finite(aValue)) {

I don't think you need that.  Both the attribute and the ctor are "float", not "unrestricted float", so can't end up with a non-finite float here.

>+++ b/content/svg/content/src/DOMSVGNumberList.cpp
>+DOMSVGNumberList::Initialize(DOMSVGNumber& aItem,
>+  nsRefPtr<DOMSVGNumber> domItem = aItem.HasOwner() ? aItem.Clone() : &aItem;

At least some compilers used to have issues with expressions like a?b:c where b and c are different types.  Might not be an issue anymore, but keep an eye out for it when landing.  Same in a few other places.

r=me

Comment 5

[(no longer active)]

Ms2ger, are you planning on landing this?  Thanks!

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/287aca97315e