Closed Bug 1516076 Opened 11 months ago Closed 11 months ago

Having classes called SVGAnimatedTransformList and nsSVGAnimatedTransformList is confusing

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: longsonr, Assigned: longsonr)

Details

Attachments

(4 files, 1 obsolete file)

There are 3 pairs of animated list classes with wrappers:

DOMSVGAnimatedLengthList
SVGAnimatedLengthList

DOMSVGAnimatedNumberList
SVGAnimatedNumberList

SVGAnimatedTransformList
nsSVGAnimatedTransformList

Let's fix the odd pair to match the other two

In addition DOMSVGAnimatedLengthList and DOMSVGAnimatedNumberList are in the mozilla namespace but SVGAnimatedTransformList is in the mozilla::dom namespace.
SVGGeometryFrame actually needs the other include
Assignee: nobody → longsonr
Attachment #9033066 - Flags: review?(dholbert)
Keywords: leave-open
The other list classes don't need to be exported and neither does this one.
Attachment #9033067 - Attachment is obsolete: true
Attachment #9033067 - Flags: review?(dholbert)
Attachment #9033070 - Flags: review?(dholbert)
Comment on attachment 9033066 [details] [diff] [review]
Part 1 - correct include

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

r=me
Attachment #9033066 - Flags: review?(dholbert) → review+
Comment on attachment 9033070 [details] [diff] [review]
Part 2 - Rename SVGAnimatedTransformList to DOMSVGAnimatedTransformList

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

Nearly r+, but I want to understand the Bindings.conf change slightly better before signing off (see question (2) there below)

::: dom/bindings/Bindings.conf
@@ +783,5 @@
>  
> +'SVGAnimatedTransformList': {
> +    'nativeType': 'mozilla::dom::DOMSVGAnimatedTransformList',
> +    'headerFile': 'DOMSVGAnimatedTransformList.h'
> +},

I'm not super-familiar with this file, but its documentation guides me well enough.

Two questions/thoughts:
 (1) should this insert a few lines lower down? ("Transform" sorts after "Preserve", alphabetically)
 (2) Why didn't we need this before but we need it now? (How were things working at all beforehand, assuming this is a necessary change?)

::: dom/svg/nsSVGAnimatedTransformList.cpp
@@ +7,4 @@
>  #include "nsSVGAnimatedTransformList.h"
>  
>  #include "mozilla/dom/MutationEventBinding.h"
> +#include "DOMSVGAnimatedTransformList.h"

Ideally it'd be great to have a followup to fix the #include sorting here & elsewhere.

(Right now this patch is leaving the include list with this no-directory include sandwiched in the middle of a sorted "mozilla/" include section. That makes some sense for a mechanical change, but still nice to clean up after it.)
(In reply to Daniel Holbert [:dholbert] from comment #5)
> Comment on attachment 9033070 [details] [diff] [review]
> Part 2 - Rename SVGAnimatedTransformList to DOMSVGAnimatedTransformList
> 
> Review of attachment 9033070 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nearly r+, but I want to understand the Bindings.conf change slightly better
> before signing off (see question (2) there below)
> 
> ::: dom/bindings/Bindings.conf
> @@ +783,5 @@
> >  
> > +'SVGAnimatedTransformList': {
> > +    'nativeType': 'mozilla::dom::DOMSVGAnimatedTransformList',
> > +    'headerFile': 'DOMSVGAnimatedTransformList.h'
> > +},
> 
> I'm not super-familiar with this file, but its documentation guides me well
> enough.
> 
> Two questions/thoughts:
>  (1) should this insert a few lines lower down? ("Transform" sorts after
> "Preserve", alphabetically)

The three files form a trio, I wanted to keep them together.

DOMSVGAnimatedLengthList
DOMSVGAnimatedNumberList

and this one. I can split them so it's alphabetic but I think this makes sense the way I've done it.


>  (2) Why didn't we need this before but we need it now? (How were things
> working at all beforehand, assuming this is a necessary change?)

The webidl class name no longer matches the class name we're using, as we use two classes, a tear off and an underlying model class. This is how the other two members of the trio work, they already have such entries in Bindings.conf

> 
> Ideally it'd be great to have a followup to fix the #include sorting here &
> elsewhere.

I can do a subsequent fix for that.
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e44348f6ba9
Part 1 correct one of the include file for SVGGeometryFrame.cpp r=dholbert
Comment on attachment 9033070 [details] [diff] [review]
Part 2 - Rename SVGAnimatedTransformList to DOMSVGAnimatedTransformList

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

Sounds good. r=me
Attachment #9033070 - Flags: review?(dholbert) → review+
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3ddcde3d72c
Part 2 Rename SVGAnimatedTransformList to DOMSVGAnimatedTransformList r=dholbert
Most of this patch is mechanical

sed -i '.bak' 's/nsSVGAnimatedTransformList/SVGAnimatedTransformList/g' *.cpp
sed -i '.bak' 's/nsSVGAnimatedTransformList/SVGAnimatedTransformList/g' *.h

A future Part 4 will fix any include order issues from both this and part 2.
Attachment #9033111 - Flags: review?(dholbert)
Attachment #9033111 - Attachment is patch: true
Comment on attachment 9033111 [details] [diff] [review]
Part 3 - rename nsSVGAnimatedTransformList to SVGAnimatedTransformList

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

r=me
Attachment #9033111 - Flags: review?(dholbert) → review+
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae6bc0d3736e
Part 3 rename nsSVGAnimatedTransformList to SVGAnimatedTransformList and move it to the mozilla namespace r=dholbert
Keywords: leave-open
Comment on attachment 9033418 [details] [diff] [review]
Part 4 - make the include order of some files more sensible

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

Seems fine.  Thanks for doing this cleanup!

r=me
Attachment #9033418 - Flags: review?(dholbert) → review+
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21ce5e3998e3
Part 4 - make include order more sensible for some files r=dholbert
https://hg.mozilla.org/mozilla-central/rev/21ce5e3998e3
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.