Closed Bug 1516206 Opened 11 months ago Closed 8 months ago

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

Categories

(Core :: SVG, defect, P3)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: longsonr, Assigned: longsonr)

References

Details

Attachments

(2 files)

No description provided.
Assignee: nobody → longsonr
Attachment #9033147 - Flags: review?(dholbert)
Keywords: leave-open
future parts will be PathSeg, Points and StringLists
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
Attachment #9033147 - Flags: review?(dholbert) → review+
Blocks: 1516576
...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.)
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.
Flags: needinfo?(longsonr)
Are you comfortable with the whole move DOM classes to the dom namespace change Jonathan?
Flags: needinfo?(longsonr) → needinfo?(jwatt)
(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.
(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.
Flags: needinfo?(jwatt)
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67b47998282b
Part 1 Move DOM lengths and numbers into the mozilla::dom namespace r=dholbert
Priority: -- → P3

Any further work will be in a new bug.

Status: NEW → RESOLVED
Closed: 8 months ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.