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)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: zwol, Assigned: zwol)
References
Details
Attachments
(1 file, 1 obsolete file)
38.19 KB,
patch
|
zwol
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #599428 -
Flags: review?(dbaron)
Comment 2•12 years ago
|
||
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
Assignee | ||
Comment 3•12 years ago
|
||
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+
Assignee | ||
Comment 5•12 years ago
|
||
With dbaron's requested changes. https://hg.mozilla.org/integration/mozilla-inbound/rev/36c73ec83bb3
Attachment #599428 -
Attachment is obsolete: true
Attachment #600036 -
Flags: review+
Comment 6•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/36c73ec83bb3
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Assignee | ||
Comment 8•11 years ago
|
||
(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)
Comment 9•11 years ago
|
||
(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.
Description
•