1321495 - Preemptively fix unified bustage in layout/style

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Bug 1321495 part 1: Add needed includes/namepsaces to headers within layout/style, to preemptively fix unified-build bustage.

In CSSLexer.h and ServoCSSRuleList.h, this patch is providing the type for the
header's main class to inherit from.
In nsCSSPseudoClasses.h, this patch is providing an enum type (CSSEnabledState)
that's used in this header file.

Review commit: https://reviewboard.mozilla.org/r/96972/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96972/

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1321495 part 2: Add needed "using namespace" declarations to several .cpp files in layout/style, to preemptively fix unified-build bustage.

Review commit: https://reviewboard.mozilla.org/r/96974/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96974/

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: Bug 1321495 part 3: Add needed #includes to various .cpp files in layout/style, to preemptively fix unified-build bustage.

Review commit: https://reviewboard.mozilla.org/r/96976/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96976/

Comment 4

[Daniel Holbert [: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.

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

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

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

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

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

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

Comment 8

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

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!

Comment 9

[Daniel Holbert [:dholbert]]

(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.)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/99c9d215e0b8
https://hg.mozilla.org/mozilla-central/rev/1f74e31e7231
https://hg.mozilla.org/mozilla-central/rev/821b0005c39b