Closed Bug 1216043 Opened 4 years ago Closed 4 years ago

rename nsStyleSheet::sheetType and make it an enum class

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(1 file)

I got sick of looking at that initial lowercase letter in nsStyleSheet::sheetType's name, so I'm renaming it, and while I'm at it:

* making it an enum class
* moving it to a separate header
* making all uses of uint8_t/uint16_t storage for sheet types just use the now-accessible-without-including-nsStyleSet.h enum

Note that I also renamed nsIDocument::SheetTypeCount to AdditionalSheetTypeCount just so that it's a bit more obvious that it's unrelated to SheetType.  (nsIDocument::additionalSheetType could itself do with renaming and possibly enum-class-ification.)
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8675515 - Flags: review?(dbaron)
Comment on attachment 8675515 [details] [diff] [review]
Rename nsStyleSheet::sheetType and make it an enum class.

>-  for (uint32_t i = 0; i < SheetTypeCount; ++i)
>-    mAdditionalSheets[i].Clear();
>+  for (auto& sheet : mAdditionalSheets) {
>+    sheet.Clear();
>+  }

Probably sheets rather than sheet, since it's an array (no?).


In ConvertAdditionalSheetType, please change the NS_ASSERTION to a
MOZ_ASSERT.

Perhaps nsCSSRuleProcessor.h and nsCSSRules.h and nsRuleData.h and
nsRuleWalker.h should explicitly include mozilla/SheetType.h, on the
principle of include-what-you-use?

>+DirtyBit(SheetType aType)

Maybe this should MOZ_ASSERT that SheetType::Count <= 10, given that
mDirty has 10 bits?  (And a comment, or even a macro, could tie the two
together?)

In gCascadeLevels in nsStyleSet.cpp, please fix the indentation of the
continued lines as well.

>-    // be sure to keep the number of bits in |mDirty| below and in
>-    // NS_RULE_NODE_LEVEL_MASK updated when changing the number of sheet
>-    // types

Please preserve this comment from nsStyleSet.h into SheetType.h (but
you can drop the mDirty half).

I wonder if there are files that were previously including nsStyleSet.h
that could now include only mozilla/SheetType.h.  Maybe worth a followup?

Also maybe worth calling out the change from 0 to Unknown(0xff) in
FontFaceSet.cpp in the commit message, since it seems the most likely to
possibly break something.
Attachment #8675515 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #3)
> Perhaps nsCSSRuleProcessor.h and nsCSSRules.h and nsRuleData.h and
> nsRuleWalker.h should explicitly include mozilla/SheetType.h, on the
> principle of include-what-you-use?

Probably, although in the age of unified builds I tend to care a bit less about include purity.

> >+DirtyBit(SheetType aType)
> 
> Maybe this should MOZ_ASSERT that SheetType::Count <= 10, given that
> mDirty has 10 bits?  (And a comment, or even a macro, could tie the two
> together?)

Since mDirty is defined to be SheetType::Count bits long, I don't think we need to.

> >-    // be sure to keep the number of bits in |mDirty| below and in
> >-    // NS_RULE_NODE_LEVEL_MASK updated when changing the number of sheet
> >-    // types
> 
> Please preserve this comment from nsStyleSet.h into SheetType.h (but
> you can drop the mDirty half).

I dropped this since we now assert that NS_RULE_NODE_LEVEL_MASK is big enough, but I guess it's still a useful pointer for someone adding a sheet level.

> Also maybe worth calling out the change from 0 to Unknown(0xff) in
> FontFaceSet.cpp in the commit message, since it seems the most likely to
> possibly break something.

OK.
https://hg.mozilla.org/mozilla-central/rev/240c0cff264b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.