Closed Bug 1516551 Opened 11 months ago Closed 11 months ago

Having classes called SVGTransform and nsSVGTransform is confusing

Categories

(Core :: SVG, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: longsonr, Assigned: longsonr)

Details

Attachments

(3 files)

SVGTransform should be DOMSVGTransform which then allows nsSVGTransform to become SVGTransform. I.e. same process as bug 1516076
Assignee: nobody → longsonr
Attachment #9033440 - Flags: review?(dholbert)
Hmm, I'm realizing part 1 here is basically a reversal of bug 855597 part 3 ("Rename DOMSVGTransform to SVGTransform"), and forthcoming part 2 will probably be a reversal of bug 855597 part 2 ("Rename SVGTransform to nsSVGTransform")

Perhaps jwatt should weigh in on these renames?

It seems like dzbarsky&jwatt's reasoning from bug 855597 comment 4 was that it's more convenient to have the DOM-exposed class be named "SVGTransform" because that's what the binding expects [which is presumably why we need the bindings.conf change here].  That doesn't seem like a huge issue to me, given that we have bindings.conf;  but still, an equally-valid approach would be to do something like the following:
 * mozilla::dom::SVGTransform stays the same (no renaming)
 * nsSVGTransform --> mozilla::SVGTransformInternal (or whatever name makes sense; "ns" sort of indicates "gecko-internal" in the current pairing of these transform classes)

I don't have a strong opinion, but I do want to be sure we don't go too far down this renaming path if jwatt does have strong opinions (& e.g. wants to stick with the plan from 6 years ago in bug 855597).
Flags: needinfo?(jwatt)
(One way or another, I agree in spirit with the goals of (a) removing "ns"-prefixed naming and (b) avoiding having two classes with the same name in mozilla:: and mozilla::dom)
Priority: -- → P3
What we have now is...

DOMSVGAnimatedLengthList
SVGAnimatedLengthList
DOMSVGAnimatedNumberList
SVGAnimatedNumberList
DOMSVGLengthList
SVGLengthList
DOMSVGLength
SVGLength
DOMSVGNumberList
SVGNumberList
DOMSVGNumber
SVGNumber
DOMSVGPoint
SVGPoint
DOMSVGPointList
SVGPointList
DOMSVGStringList
SVGStringList
DOMSVGTransformList
SVGTransformList

separate files, all in the mozilla namespace


DOMSVGAnimatedTransformList
SVGAnimatedTransformList

separate files all with DOMSVGAnimatedTransformList in the mozilla::dom namespace and the other in the mozilla namespace


DOMSVGPreserveAspectRatio
SVGPreserveAspectRatio

both classes in a single file DOMSVGPreserveAspectRatio in the mozilla::dom namespace, the other in the mozilla namespace


SVGTransform
nsSVGTransform
SVGAngle
nsSVGAngle

separate files SVG... in the mozilla::dom namespace, nsSVG... in the mozilla namespace


Changing the namespace is fairly straightforward and it would seem to make sense to put the DOM classes in the mozilla::dom namespace and the non-DOM classes in the mozilla namespace.

Only dom/bindings and dom/svg require the DOM files so putting them both together is a bit of a waste as layout doesn't need to know about the DOM classes.


It's a lot of work to change all of the first list about, I don't think I want to tackle that. I'd fix up the namespace inconsistency and the SVGTransform/nsSVGTransform SVGAngle/nsSVGAngle naming to be consistent with everything else and maybe move DOMSVGPreserveAspectRatio/SVGPreserveAspectRatio to different files to match everything else as that's the least work/churn for the most consistent result.
Comment on attachment 9033440 [details] [diff] [review]
Part 1 - Rename SVGTransform as DOMSVGTransform

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

r=me assuming jwatt is OK with this change generally (which I'll bet he is, but I feel like I should check, given comment 2)
Attachment #9033440 - Flags: review?(dholbert)
Attachment #9033440 - Flags: review+
Attachment #9033440 - Flags: feedback?(jwatt)
Comment on attachment 9033440 [details] [diff] [review]
Part 1 - Rename SVGTransform as DOMSVGTransform

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

::: dom/svg/DOMSVGTransform.cpp
@@ +4,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "mozilla/dom/DOMSVGTransform.h"

FWIW in source files (as opposed to header files) it is useful to directly include headers that live alongside the source files (rather than include their exported copies) since that produces better results (code indexing, jump-to-declaration-file, etc.) for people using tools like Eclipse and VScode. We already unnecessarily include exported headers in some source files (including below this line), but it would be nice to avoid making things worse if you don't mind making that tweak before landing.
Attachment #9033440 - Flags: feedback?(jwatt) → feedback+
Flags: needinfo?(jwatt)
(In reply to Jonathan Watt [:jwatt] from comment #6)
> 
> FWIW in source files (as opposed to header files) it is useful to directly
> include headers that live alongside the source files (rather than include
> their exported copies) since that produces better results (code indexing,
> jump-to-declaration-file, etc.) for people using tools like Eclipse and
> VScode. We already unnecessarily include exported headers in some source
> files (including below this line), but it would be nice to avoid making
> things worse if you don't mind making that tweak before landing.

dholbert seems to be of the opposite opinion. Can you fight it out between you. I'm happy to do either but I want things to be consistent.
Flags: needinfo?(dholbert)
Yeah, I "fixed" some of the include paths in https://phabricator.services.mozilla.com/D15231#384160 . I was going for consistency with e.g. dom/base, where for example (to pick a few local included files):
 - 35 .cpp files include "mozilla/dom/Element.h" with its full export path, vs. 1 lacks it.
 - 7 .cpp files include "mozilla/dom/Text.h" with the path, vs. 0 lack it.
 - 6 .cpp files include "mozilla/dom/Attr.h" with the path, vs. 0 lack it.
 - 6 .cpp files include "mozilla/dom/DOMRect.h" with the path, vs. 0 lack it

I gathered the above numbers using these commands, which I ran in dom/base:
 grep \/DOMRect.h *.cpp
 grep \"DOMRect.h *.cpp

So: at least in dom/base, it seems like we've essentially standardized on using the full include path.  Having said that, if there's a functional reason to do it the other way, that's fine with me since it doesn't seem to be explicitly mentioned in oru c++ coding style guide (or in Google's).

I'll let jwatt have the final say here. :)
Flags: needinfo?(dholbert) → needinfo?(jwatt)
To make IDEs and editors work better with the Moz source there are several outstanding hurdles that I know about. This is just one of them, and to have an impactful change will require a discussion on m.d.platform to making this official and then a bit of code rewriting.

As I said, the request to "not make things worse" is just something that "would be nice". :) This is just a nit and doesn't in any way block this landing. You can change the code or not, Robert, as you like.
Flags: needinfo?(jwatt)
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc7d4e24509b
Part 1 rename SVGTransform to DOMSVGTransform r=dholbert
https://hg.mozilla.org/mozilla-central/rev/bc7d4e24509b
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Status: REOPENED → NEW
Attachment #9033593 - Flags: review?(dholbert)
Comment on attachment 9033593 [details] [diff] [review]
Part 3 - random comment typo fix

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

r=me
Attachment #9033593 - Flags: review?(dholbert) → review+
Comment on attachment 9033592 [details] [diff] [review]
Part 2 - Rename nsSVGTransform to SVGTransform

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

r=me
Attachment #9033592 - Flags: review?(dholbert) → review+
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf6fac6a26dd
Part 2 rename nsSVGTransform to SVGTransform r=dholbert
Keywords: leave-open
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b136d859d516
Part 3 fix comment to refer to the correct class name r=dholbert
https://hg.mozilla.org/mozilla-central/rev/cf6fac6a26dd
https://hg.mozilla.org/mozilla-central/rev/b136d859d516
Status: NEW → RESOLVED
Closed: 11 months ago11 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.