1516076 - Having classes called SVGAnimatedTransformList and nsSVGAnimatedTransformList is confusing

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

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.

Comment 1

[Robert Longson [:longsonr]]

Attachment: Part 1 - correct include

SVGGeometryFrame actually needs the other include

Comment 2

[Robert Longson [:longsonr]]

Attachment: Part 2 - Rename SVGAnimatedTransformList to DOMSVGAnimatedTransformList

Comment 3

[Robert Longson [:longsonr]]

Attachment: Part 2 - Rename SVGAnimatedTransformList to DOMSVGAnimatedTransformList

The other list classes don't need to be exported and neither does this one.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 9033066 [details] [diff] [review]
Part 1 - correct include

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

r=me

Comment 5

[Daniel Holbert [:dholbert]]

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.)

Comment 6

[Robert Longson [:longsonr]]

(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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e44348f6ba9
Part 1 correct one of the include file for SVGGeometryFrame.cpp r=dholbert

Comment 8

[Daniel Holbert [: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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3ddcde3d72c
Part 2 Rename SVGAnimatedTransformList to DOMSVGAnimatedTransformList r=dholbert

Comment 10

[Robert Longson [:longsonr]]

Attachment: Part 3 - rename nsSVGAnimatedTransformList to SVGAnimatedTransformList

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.

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/1e44348f6ba9
https://hg.mozilla.org/mozilla-central/rev/a3ddcde3d72c

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 9033111 [details] [diff] [review]
Part 3 - rename nsSVGAnimatedTransformList to SVGAnimatedTransformList

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

r=me

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae6bc0d3736e
Part 3 rename nsSVGAnimatedTransformList to SVGAnimatedTransformList and move it to the mozilla namespace r=dholbert

Comment 14

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/ae6bc0d3736e

Comment 15

[Robert Longson [:longsonr]]

Attachment: Part 4 - make the include order of some files more sensible

Comment 16

[Daniel Holbert [:dholbert]]

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21ce5e3998e3
Part 4 - make include order more sensible for some files r=dholbert

Comment 18

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/21ce5e3998e3