1435477 - Make the SVG dom code use the WebIDL constants from the bindings

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Description

[Robert Longson [:longsonr]]

post bug 792059 we generate constants in the binding so we should use them and not define our own values.

Comment 1

[Robert Longson [:longsonr]]

Attachment: convert preserveAspectRatio

Start with PreserveAspectRatio's bindings. 

If this is OK, I'll continue with the others, probably one patch per binding.

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8948083 [details] [diff] [review]
convert preserveAspectRatio

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

I'm generally on board!  It's a bit sad to be losing the strictness of typed enums.  But it seems like a net win to avoid having these values defined in multiple places with the exact same name (aside from the namespacing).  So I'm on board, generally.

For my own reference: the webidl-generated constants live here:
https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/SVGPreserveAspectRatioBinding.h#30

r- for a coding-style issue that'll require some tweaks, though:

::: dom/svg/SVGPreserveAspectRatio.h
@@ +16,5 @@
>  #include "nsSVGElement.h"
>  
>  namespace mozilla {
> +
> +using namespace dom::SVGPreserveAspectRatioBinding;

Coding style guide forbids this -- << No "using" statements are allowed in header files, except inside class definitions or functions>>:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Namespaces

You can move this inside of the "class SVGPreserveAspectRatio" definition, though, which will let you use it without the namespace in most of its currant usage sites, I think/hope.  And then you'll only need to add the dom::SVGPreserveAspectRatioBinding:: prefix to a few of the constants' usages, potentially... (maybe just the  SVG_ALIGN_MIN_VALID/ SVG_ALIGN_MAX_VALID definitions? If it's just those, that's not so bad.)

Comment 3

[Robert Longson [:longsonr]]

Attachment: patch

This does everything except filters which I'll do as a separate step.

Comment 4

[Robert Longson [:longsonr]]

Attachment: fix typo

Comment 5

[Jonathan Watt [:jwatt]]

Comment on attachment 8950086 [details] [diff] [review]
fix typo

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

::: dom/svg/DOMSVGPathSeg.h
@@ +22,4 @@
>  #define CHECK_ARG_COUNT_IN_SYNC(segType)                                      \
>    MOZ_ASSERT(ArrayLength(mArgs) ==                                            \
>                 SVGPathSegUtils::ArgCountForType(uint32_t(segType)) ||         \
> +             uint32_t(segType) == dom::SVGPathSegBinding::PATHSEG_CLOSEPATH,  \

All your changes (other than to VectorImage.cpp are in the DOM code, so it seems like we shouldn't need to use the dom:: namespace qualifier here. Other than that this all looks great.

Comment 6

[Robert Longson [:longsonr]]

dom/svg/DOMSVGPathSeg.h is in the mozilla namespace but I don't see anything in that file that puts it in the dom namespace. Maybe it should be, but do you really want this bug to be the place to fix that?

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8950086 [details] [diff] [review]
fix typo

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

(Sounds like jwatt already basically reviewed this in comment 5, so I'll transfer the review request to him.)

Comment 8

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson [:longsonr] from comment #6)
> dom/svg/DOMSVGPathSeg.h is in the mozilla namespace but I don't see anything
> in that file that puts it in the dom namespace. Maybe it should be, but do
> you really want this bug to be the place to fix that?

Hmm, I guess not. I was forgetting our content/dom stuff is a bit mixed up with regards to namespacing.

Comment 9

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f1f74dbc0acc75c63f8858431a36e2d069abb0d

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/917a211c3efa
Make the SVG dom code use the WebIDL constants from the bindings r=jwatt

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/917a211c3efa