Preemptively fix unified bustage in layout/svg

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
When reviewing the patches in bug 1304598, I was inspired to preemptively fix unified bustage in a few places, so that changes like bug 1304598 will be easier in the future.

We can reveal latent-unified-bustage by simply tweaking moz.build files to replace UNIFIED_SOURCES with SOURCES. I did that in layout/svg and fixed the resulting bustage; patch coming up.
(Assignee)

Comment 1

2 years ago
TYLin, I'll tag you to review this -- hope that's OK with you!
Assignee: nobody → dholbert
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 years ago
* All of the new #includes here are for types of the same name that are directly used in the file that I'm tweaking. We either call functions on a variable of that type (e.g. SVGSVGElement.h in one case, for a call to svgElement->GetViewportSize()), or we return that type by value (e.g. for DrawResult), or we call static methods on that type (e.g. for nsSVGUtils))
* The "using namespace mozilla::image" sprinkled throughout the patch is for DrawResult usages (which live in mozilla::image)
* I'm adding explicit an namespace to IntPoint in nsSVGClipPathFrame.h (which is currently accidentally-relying on "using namespace" declarations that get picked up from other .cpp files right now).

Let me know if anything else isn't clear here. Thanks!
(In reply to Daniel Holbert [:dholbert] from comment #0)
> We can reveal latent-unified-bustage by simply tweaking moz.build files to
> replace UNIFIED_SOURCES with SOURCES. 

This is a good idea!

Comment 5

2 years ago
mozreview-review
Comment on attachment 8815429 [details]
Bug 1321056: Add includes/namespaces to fix latent unified bustage in layout/svg.

https://reviewboard.mozilla.org/r/96326/#review96658

The explanation in comment 3 is clear to me. r=TYLin

::: layout/svg/nsCSSClipPathInstance.cpp:16
(Diff revision 1)
> +#include "nsSVGElement.h"
> +#include "nsSVGUtils.h"
> +#include "nsSVGViewBox.h"

Nit: Put the #includes after "nsRuleNode.h" so that they are in alphabetic order.
Attachment #8815429 - Flags: review?(tlin) → review+
(Assignee)

Comment 6

2 years ago
mozreview-review-reply
Comment on attachment 8815429 [details]
Bug 1321056: Add includes/namespaces to fix latent unified bustage in layout/svg.

https://reviewboard.mozilla.org/r/96326/#review96658

> Nit: Put the #includes after "nsRuleNode.h" so that they are in alphabetic order.

I got the alphabet backwards in my head. :D

Fixed -- thanks!
Comment hidden (mozreview-request)

Comment 8

2 years ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9953c71eb9ba
Add includes/namespaces to fix latent unified bustage in layout/svg. r=TYLin

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9953c71eb9ba
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Updated

2 years ago
Blocks: 1345202
You need to log in before you can comment on or make changes to this bug.