Closed Bug 1321495 Opened 7 years ago Closed 7 years ago

Preemptively fix unified bustage in layout/style

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(3 files)

This bug is one in a series of "fix latent missing-#include/declaration/namespace compile errors which are currently being hidden by unified builds" bugs.

This one's for layout/style. I have a partial patch locally; I'll post it at some point tomorrow.
Flags: needinfo?(dholbert)
Note: I provided explanations for the changes in "part 1" (the one that touches header files) in the extended commit message.  I think it's important for that one, since header files are where we need extra scrutiny around #includes/namespaces (since they have leaky side-effects) -- so changes to header files need extra scrutiny, to be sure we're not adding anything unnecessary.

Parts 2 and 3 (changes to .cpp file) don't need quite as much scrutiny (and there are more files tweaked there), so I didn't bother writing out explanations there -- though I'm happy to explain any changes that you've got questions about there.

One thing I'll note: in e.g. nsCSSScanner.cpp, the current code has a mix of things with explicit namespaces, e.g.:
> nsCSSValue::nsCSSValue(mozilla::css::URLValue* aValue)
https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSValue.cpp#99

...and things without explicit namespaces, e.g.:
> nsCSSValue::SetRGBAColorValue(const RGBAColorData& aValue)
(where RGBAColorData technically lives inside the mozilla::css namespace)

The latter (and a few other similar lines) are what caused me to add "using namespace mozilla::css" here.

We could remove the namespaces for cases like the former (and I noticed you've been good enough to do that in some of your similar patches).  I'm not bothering to do that here, since that's a slightly bigger undertaking.  For now, I'm just aiming to formalize the "using" and #includes that we're already getting (& legitimately depending on) from other files during unification.
Flags: needinfo?(dholbert)
Comment on attachment 8816203 [details]
Bug 1321495 part 1: Add needed includes/namepsaces to headers within layout/style, to preemptively fix unified-build bustage.

https://reviewboard.mozilla.org/r/96972/#review97444
Attachment #8816203 - Flags: review?(tlin) → review+
Comment on attachment 8816204 [details]
Bug 1321495 part 2: Add needed "using namespace" declarations to several .cpp files in layout/style, to preemptively fix unified-build bustage.

https://reviewboard.mozilla.org/r/96974/#review97446
Attachment #8816204 - Flags: review?(tlin) → review+
Comment on attachment 8816205 [details]
Bug 1321495 part 3: Add needed #includes to various .cpp files in layout/style, to preemptively fix unified-build bustage.

https://reviewboard.mozilla.org/r/96976/#review97448
Attachment #8816205 - Flags: review?(tlin) → review+
Thanks your for the explanation in comment 4.

For part 1, those #includes all look reasonable to me. For part 2 & 3, I do not scrutinize closely to tell  whether those #includes are able to be replaced by forward declaration or not. Since #includes in the cpp files is not as contagious as in the header, I think that's fine.

> For now, I'm just aiming to formalize the "using" and #includes that we're
> already getting (& legitimately depending on) from other files during
> unification.

Yeh! This could reduce the effort when the order of the file unification are changed due to renames. Thank you!
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #8)
> For part 1, those #includes all look reasonable to me.

Thanks!

> For part 2 & 3, I do
> not scrutinize closely to tell  whether those #includes are able to be
> replaced by forward declaration or not. Since #includes in the cpp files is
> not as contagious as in the header, I think that's fine.

(Yeah -- I agree forward-decl-vs.-include isn't a big concern for .cpp files.)
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/99c9d215e0b8
part 1: Add needed includes/namepsaces to headers within layout/style, to preemptively fix unified-build bustage. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/1f74e31e7231
part 2: Add needed "using namespace" declarations to several .cpp files in layout/style, to preemptively fix unified-build bustage. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/821b0005c39b
part 3: Add needed #includes to various .cpp files in layout/style, to preemptively fix unified-build bustage. r=TYLin
Blocks: 1326574
You need to log in before you can comment on or make changes to this bug.