Closed Bug 1435477 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: SVG, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: longsonr, Assigned: longsonr)

Details

Attachments

(1 file, 2 obsolete files)

post bug 792059 we generate constants in the binding so we should use them and not define our own values.
Attached patch convert preserveAspectRatio (obsolete) — Splinter Review
Start with PreserveAspectRatio's bindings. 

If this is OK, I'll continue with the others, probably one patch per binding.
Assignee: nobody → longsonr
Attachment #8948083 - Flags: review?(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.)
Attachment #8948083 - Flags: review?(dholbert) → review-
Attached patch patch (obsolete) — Splinter Review
This does everything except filters which I'll do as a separate step.
Attachment #8950083 - Flags: review?(dholbert)
Attachment #8948083 - Attachment is obsolete: true
Attached patch fix typoSplinter Review
Attachment #8950083 - Attachment is obsolete: true
Attachment #8950083 - Flags: review?(dholbert)
Attachment #8950086 - Flags: review?(dholbert)
Summary: use WebIDL constants from the binding → Make the SVG dom code use the WebIDL constants from the bindings
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.
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?
Flags: needinfo?(jwatt)
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.)
Attachment #8950086 - Flags: review?(dholbert) → review?(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.
Flags: needinfo?(jwatt)
Attachment #8950086 - Flags: review?(jwatt) → review+
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/917a211c3efa
Make the SVG dom code use the WebIDL constants from the bindings r=jwatt
https://hg.mozilla.org/mozilla-central/rev/917a211c3efa
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.