1516206 - Move DOM classes that aren't in the mozilla:dom namespace into the mozilla::dom namespace

Status: RESOLVED FIXED

(Core :: SVG, defect, P3)

Comment 1

[Robert Longson [:longsonr]]

Attachment: Part 1 - Lengths and Numbers

Comment 2

[Robert Longson [:longsonr]]

future parts will be PathSeg, Points and StringLists

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 9033147 [details] [diff] [review]
Part 1 - Lengths and Numbers

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

r=me with the fix noted below.

Right now the patch gives me this build error:
====
dom/svg/SVGFEConvolveMatrixElement.cpp:80:29: error: return type of out-of-line definition of 'mozilla::dom::SVGFEConvolveMatrixElement::KernelMatrix' differs from that in the declaration

 0:04.41 SVGFEConvolveMatrixElement::KernelMatrix() {
 0:04.41                             ^
 0:04.41 ../../dist/include/mozilla/dom/SVGFEConvolveMatrixElement.h:61:46: note: previous declaration is here
 0:04.41   already_AddRefed<DOMSVGAnimatedNumberList> KernelMatrix();
 0:04.41   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
====

The issue is that SVGFEConvolveMatrixElement.h has a forward-decl for DOMSVGAnimatedNumberList in the wrong namespace, in "namespace mozilla { ...}".  That forward-decl needs to move down a few lines into "namespace dom {...}" to remain correct after this change.  Here's a link, for reference:
https://searchfox.org/mozilla-central/rev/8a135a9c5a96b59269f544fcaee76d8fd5a7026a/dom/svg/SVGFEConvolveMatrixElement.h#22-26

Comment 4

[Daniel Holbert [:dholbert]]

...and while you're at it: it seems SVGFEColorMatrixElement.h needs that same forward-declare, for "class DOMSVGAnimatedNumberList" inside of its existing "namespace mozilla { namespace dom { ... }}" block.

Otherwise, I get a build failure much like the one in comment 3 (but for SVGFEColorMatrixElement.cpp) if I disable unified compilation and fix SVGFEColorMatrixElement.cpp to include its own .h file first.

(While reviewing this bug, I noticed a few files that don't include their own .h file first while I was reviewing this patch here, so I decided to fix all of them in bug 1516576.  And while doing that, I noticed this issue with SVGFEColorMatrixElement.cpp and this DOMSVGAnimatedNumberList class.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: (bonus forward-decl changes that should be merged into part 1)

For convenience (and since I've already got the changes locally to test bug 1516576), here are the missing forward decls that I think make sense to merge into "part 1" here.

Assuming you agree, feel free to just merge this into part 1 and mark this attachment as obsolete.

Comment 6

[Robert Longson [:longsonr]]

Are you comfortable with the whole move DOM classes to the dom namespace change Jonathan?

Comment 7

[Daniel Holbert [:dholbert]]

(At robert's suggestion, I landed bug 1516576 with a needed forward-decl in SVGFEColorMatrixElement.h (declaring DOMSVGAnimatedNumberList in namespace mozilla {...} block).

That forward decl will need to be moved a few lines down as part of this bug's patch to stay correct.

Comment 8

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson [:longsonr] from comment #6)
> Are you comfortable with the whole move DOM classes to the dom namespace
> change Jonathan?

I don't have a strong opinion. It may be a little strange to have classes in namespace mozilla::dom prefixed with "DOM" but that's okay I guess.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67b47998282b
Part 1 Move DOM lengths and numbers into the mozilla::dom namespace r=dholbert

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/67b47998282b

Comment 11

[Robert Longson [:longsonr]]

Any further work will be in a new bug.