don't require manual syncing of font data (kCSSRawFontDescs et al)

RESOLVED FIXED in mozilla12

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Comments with nsCSSProps.cpp:kCSSRawFontDescs and elsewhere warn the reader about keeping various bits in sync.  No one list is complete and each list is slightly different.

Make the madness stop!  Patch incoming.
(Assignee)

Comment 1

5 years ago
Created attachment 583152 [details] [diff] [review]
patch

Simple patch to create nsCSSFontDescList.h and make various places use it.
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
Attachment #583152 - Flags: review?(bzbarsky)
Could you make the parameters to the macro not be uppercase, but instead have a trailing _ character?  That matches the style of other things in nearby code, in particular, nsCSSPropList.h.  Also, since the fields actually match 2 of the fields in the CSS_PROP macro, could you use the same names, so that you have:

#define CSS_FONT_DESC(name_, method_)

instead of:

#define FONT_DESC(CSS_PROP, NAME)

Otherwise, looks great.
Comment on attachment 583152 [details] [diff] [review]
patch

In addition to what dbaron said, those trailing commas are likely to be a problem, iirc.

Add nsnull at the end of kCSSRawFontDescs, for sure; I think nsCSSFontFaceStyleDecl::Fields should have one too.

r=me with that.
Attachment #583152 - Flags: review?(bzbarsky) → review+
Why does either need a null?  They're only accessed by index, I hope.
Last I checked some compilers warned on the trailing comma.  But maybe not anymore?
(Assignee)

Comment 6

5 years ago
We already have trailing commas elsewhere, e.g. nsCSSProps.cpp:kCSSRawProperties.
OK, then don't worry about it.
(Assignee)

Comment 8

5 years ago
Created attachment 584780 [details] [diff] [review]
patch v2

Addressed bz and dbaron's comments.  Carrying over r+.
Attachment #583152 - Attachment is obsolete: true
Attachment #584780 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/e4fe4d48518f
Keywords: checkin-needed
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/e4fe4d48518f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.