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)
Core
CSS Parsing and Computation
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.
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(dholbert)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
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 5•7 years ago
|
||
mozreview-review |
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 6•7 years ago
|
||
mozreview-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 7•7 years ago
|
||
mozreview-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+
Comment 8•7 years ago
|
||
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!
Assignee | ||
Comment 9•7 years ago
|
||
(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•7 years ago
|
||
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
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/99c9d215e0b8 https://hg.mozilla.org/mozilla-central/rev/1f74e31e7231 https://hg.mozilla.org/mozilla-central/rev/821b0005c39b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•