Closed
Bug 1516551
Opened 5 years ago
Closed 5 years ago
Having classes called SVGTransform and nsSVGTransform is confusing
Categories
(Core :: SVG, enhancement, P3)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla66
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: longsonr, Assigned: longsonr)
Details
Attachments
(3 files)
30.06 KB,
patch
|
dholbert
:
review+
jwatt
:
feedback+
|
Details | Diff | Splinter Review |
20.19 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
697 bytes,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
SVGTransform should be DOMSVGTransform which then allows nsSVGTransform to become SVGTransform. I.e. same process as bug 1516076
Assignee | ||
Comment 1•5 years ago
|
||
Assignee: nobody → longsonr
Attachment #9033440 -
Flags: review?(dholbert)
Comment 2•5 years ago
|
||
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)
Comment 3•5 years ago
|
||
(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)
Updated•5 years ago
|
Priority: -- → P3
Assignee | ||
Comment 4•5 years ago
|
||
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 5•5 years ago
|
||
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 6•5 years ago
|
||
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+
Updated•5 years ago
|
Flags: needinfo?(jwatt)
Assignee | ||
Comment 7•5 years ago
|
||
(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)
Comment 8•5 years ago
•
|
||
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)
Comment 9•5 years ago
|
||
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)
Comment 10•5 years ago
|
||
Pushed by longsonr@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bc7d4e24509b Part 1 rename SVGTransform to DOMSVGTransform r=dholbert
Comment 11•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bc7d4e24509b
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Status: REOPENED → NEW
Assignee | ||
Comment 12•5 years ago
|
||
Attachment #9033592 -
Flags: review?(dholbert)
Assignee | ||
Comment 13•5 years ago
|
||
Attachment #9033593 -
Flags: review?(dholbert)
Comment 14•5 years ago
|
||
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 15•5 years ago
|
||
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+
Comment 16•5 years ago
|
||
Pushed by longsonr@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/cf6fac6a26dd Part 2 rename nsSVGTransform to SVGTransform r=dholbert
Assignee | ||
Updated•5 years ago
|
Keywords: leave-open
Comment 17•5 years ago
|
||
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
Comment 18•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cf6fac6a26dd https://hg.mozilla.org/mozilla-central/rev/b136d859d516
Status: NEW → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•