Closed Bug 886420 Opened 7 years ago Closed 6 years ago

Move SVGNumber to WebIDL

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch patch (obsolete) — Splinter Review
Attachment #777176 - Flags: review?(Ms2ger)
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
Attachment #777176 - Flags: review?(Ms2ger) → review-
Attached patch Patch v2Splinter Review
Assignee: nobody → Ms2ger
Attachment #777176 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8424488 - Flags: review?(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
Attachment #8424488 - Flags: review?(bzbarsky) → review+
Ms2ger, are you planning on landing this?  Thanks!
Flags: needinfo?(Ms2ger)
https://hg.mozilla.org/mozilla-central/rev/287aca97315e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Flags: needinfo?(Ms2ger)
You need to log in before you can comment on or make changes to this bug.