Closed Bug 729142 Opened 12 years ago Closed 12 years ago

Convert layout/style to MOZ_STATIC_ASSERT

Categories

(Core :: CSS Parsing and Computation, defect)

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: 12 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.

Attachment

General

Created:
Updated:
Size: