Closed Bug 729142 Opened 8 years ago Closed 8 years ago

Convert layout/style to MOZ_STATIC_ASSERT

Categories

(Core :: CSS Parsing and Computation, defect, trivial)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: zwol, Assigned: zwol)

References

Details

Attachments

(1 file, 1 obsolete file)

Ms2ger requested in bug 96971 that I use MOZ_STATIC_ASSERT instead of PR_STATIC_ASSERT for a new static assertion in nsRuleNode.cpp; instead I propose to do a bulk conversion of layout/style to MOZ_STATIC_ASSERT in a bug that isn't about something else.

Patch may not appear immediately.
Attached patch patch (obsolete) — Splinter Review
Attachment #599428 - Flags: review?(dbaron)
Try run for 0fa125e1bd02 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0fa125e1bd02
Results (out of 15 total builds):
    success: 14
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/zackw@panix.com-0fa125e1bd02
For the record, the 1 failure is due to some sort of infrastructure glitch (timeout retrieving malloc.log from stage.m.o -- there doesn't seem to be an existing bug for this); I retriggered it and it succeeded.
Comment on attachment 599428 [details] [diff] [review]
patch

In Declaration::GetValue, I'd rather keep the asserts there -- it
was intentional to have the asserts at each place making the assumption.

In nsCSSSelector::nsCSSSelector, could you leave the assert in the
function?

In CSSParserImpl::ParseBackgroundItem, could you keep the assertions?

In nsCSSProps.h:
>+MOZ_STATIC_ASSERT((CSS_PROPERTY_PARSE_PROPERTY_MASK &
>+                   CSS_PROPERTY_VALUE_PARSER_FUNCTION) == 0,
>+                  "ParseSingleValueProperty needs these to not collide");

Make the text "didn't leave enough room for the parse property constants"

(The "See" above was  for the CSS_PROPRTY_VALUE_PARSER_FUNCTION constant;
the assertion is just to make sure that enough room was left for the
thing that used multiple bits.)


r=dbaron with those fixed
Attachment #599428 - Flags: review?(dbaron) → review+
Attached patch patch v2Splinter Review
With dbaron's requested changes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/36c73ec83bb3
Attachment #599428 - Attachment is obsolete: true
Attachment #600036 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/36c73ec83bb3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Why did you need the change of nsStyleConsts.h?
Flags: needinfo?(zackw)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 8/13-8/18 JST) from comment #7)
> Why did you need the change of nsStyleConsts.h?

It's been long enough that I don't remember for certain, but I believe it went along with the change to nsCSSParser::ParseTextDecoration(): https://bugzilla.mozilla.org/attachment.cgi?id=600036&action=diff#a/layout/style/nsCSSParser.cpp_sec3  You can see that eDecorationBlink was the only one of that set of local enumerators that didn't have a corresponding NS_STYLE_TEXT_DECORATION_* constant, so I probably figured it ought to be added.
Flags: needinfo?(zackw)
(In reply to Zack Weinberg (:zwol) from comment #8)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 8/13-8/18
> JST) from comment #7)
> > Why did you need the change of nsStyleConsts.h?
> 
> It's been long enough that I don't remember for certain, but I believe it
> went along with the change to nsCSSParser::ParseTextDecoration():
> https://bugzilla.mozilla.org/attachment.cgi?id=600036&action=diff#a/layout/
> style/nsCSSParser.cpp_sec3  You can see that eDecorationBlink was the only
> one of that set of local enumerators that didn't have a corresponding
> NS_STYLE_TEXT_DECORATION_* constant, so I probably figured it ought to be
> added.

Okay, thanks for your reply. But I think you should do it in separated patch with better commit message.
You need to log in before you can comment on or make changes to this bug.