Having classes called SVGAngle and nsSVGAngle is confusing

RESOLVED FIXED in Firefox 66

Status

()

P3
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: longsonr, Assigned: longsonr)

Tracking

unspecified
mozilla66
Points:
---

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(4 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

3 months ago
Assignee: nobody → longsonr
Attachment #9033598 - Flags: review?(dholbert)
(Assignee)

Updated

3 months ago
Attachment #9033598 - Attachment is patch: true
Comment on attachment 9033598 [details] [diff] [review]
Part 1 - simplify creation of SVGAngle objects

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

::: dom/svg/SVGAngle.cpp
@@ +19,5 @@
> +SVGAngle::SVGAngle(SVGElement* aSVGElement)
> +    : mSVGElement(aSVGElement), mType(SVGAngle::CreatedValue) {
> +  nsSVGAngle* angle = new nsSVGAngle();
> +  angle->Init();
> +  mVal = angle;

Given that we don't check for a failure status from Init() you could just assign directly to mVal here.

::: dom/svg/SVGSVGElement.cpp
@@ +266,1 @@
>    return svgangle.forget();

You could use:

  return do_AddRef(new SVGAngle(this));
Attachment #9033598 - Flags: review?(dholbert) → review+
Priority: -- → P3
(Assignee)

Updated

3 months ago
Keywords: leave-open

Comment 3

3 months ago
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f534d2edbd06
Part 1 simplify creation of SVGAngle objects r=jwatt
Comment on attachment 9033682 [details] [diff] [review]
Part 2 - Rename SVGAngle to DOMSVGAngle

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

Can you run `./mach clang-format` before requesting review for future patches? (I'm assuming you're all set up to be able to run that.) Not a big deal, but it's nice to see the patch as it will land.

::: dom/svg/SVGSVGElement.cpp
@@ +14,5 @@
>  #include "mozilla/EventDispatcher.h"
>  #include "mozilla/SMILAnimationController.h"
>  
> +#include "DOMSVGAngle.h"
> +#include "DOMSVGAngle.h"

No need to repeat this. ;)
Attachment #9033682 - Flags: review?(jwatt) → review+

Comment 7

3 months ago
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/43b1e9034e34
Part 2 rename SVGAngle to DOMSVGAngle r=jwatt
Attachment #9033716 - Flags: review?(jwatt) → review+
(Assignee)

Updated

3 months ago
Keywords: leave-open

Comment 10

3 months ago
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e43ba4cbf93
Part 3 rename nsSVGAngle to SVGAngle r=jwatt

Comment 11

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5e43ba4cbf93
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
(Assignee)

Updated

3 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

3 months ago
Status: REOPENED → NEW
(Assignee)

Comment 12

3 months ago
I mistakenly thought it was already in the mozilla namespace like nsSVGTransform, but apparently not.
Attachment #9033872 - Flags: review?(jwatt)
Attachment #9033872 - Flags: review?(jwatt) → review+

Comment 13

3 months ago
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9196c1cefbb
Part 4 move SVGAngle to the mozilla namespace r=jwatt

Comment 14

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a9196c1cefbb
Status: NEW → RESOLVED
Last Resolved: 3 months ago3 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.