1216043 - rename nsStyleSheet::sheetType and make it an enum class

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

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.)

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Rename nsStyleSheet::sheetType and make it an enum class.

Comment 2

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18d1681c3776

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 4

[Cameron McCormack (:heycam)]

(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.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/240c0cff264b

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/240c0cff264b

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/240c0cff264b