Closed Bug 1269975 Opened 8 years ago Closed 8 years ago

Use EnabledState for nsCSSPseudoClasses::GetPseudoType

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(4 files)

In bug 1268749, we make it possible to conditional enable a CSS pseudo class according to its context, like what we can do for CSS properties.

But nsCSSPseudoClasses::GetPseudoType takes two bool params which is error-prone, so we want to reuse nsCSSProps::EnabledState somehow to make things more clear.
Blocks: 1269976
Assignee: nobody → bugzilla
Comment on attachment 8748983 [details]
MozReview Request: Bug 1269975 part 1 - Move nsCSSProps::EnabledState to a top level enum class mozilla::CSSEnabledState. r?heycam

https://reviewboard.mozilla.org/r/50665/#review47983

r=me with the name changes, or happy to hear other suggestions.

::: layout/style/CSSEnabledState.h:32
(Diff revision 1)
> +  eChrome = 0x02,
> +  // Special value to unconditionally enable everything. This implies
> +  // all the bits above, but is strictly more than just their OR-ed
> +  // union. This just skips any test so a feature will be enabled even
> +  // if it would have been disabled with all the bits above set.
> +  eIgnore = 0xff

I think this name sounds a bit misleading, now.  "eIgnore" sounds like the property will be disabled everywhere.  Before the name was "IgnoreEnabledState" so it was a bit clearer that it meant the EnabledState was being ignored.

And I think we should keep the prepositions in the names for the other three.  So how about:

  CSSEnabledState::eForAllContent
  CSSEnabledState::eInUASheets
  CSSEnabledState::eInChrome
  CSSEnabledState::eIgnoreEnabledState
  
The last one is a bit wordy, but I think it's clearer.
Attachment #8748983 - Flags: review?(cam) → review+
Comment on attachment 8748984 [details]
MozReview Request: Bug 1269975 part 2 - Rename nsCSSParser::PropertyEnabledState() to EnabledState(). r?heycam

https://reviewboard.mozilla.org/r/50667/#review47985
Attachment #8748984 - Flags: review?(cam) → review+
Attachment #8748985 - Flags: review?(cam) → review+
Comment on attachment 8748985 [details]
MozReview Request: Bug 1269975 part 3 - Make some static arrays in nsCSSPseudoClasses.cpp static members of the class. r?heycam

https://reviewboard.mozilla.org/r/50669/#review47987
Attachment #8748986 - Flags: review?(cam) → review+
Comment on attachment 8748986 [details]
MozReview Request: Bug 1269975 part 4 - Make nsCSSPseudoClasses::GetPseudoType() take CSSEnabledState rather than two bool params. r?heycam

https://reviewboard.mozilla.org/r/50671/#review47989

::: layout/style/nsCSSPseudoClasses.h:70
(Diff revision 1)
>    static void PseudoTypeToString(Type aType, nsAString& aString);
>  
> -private:
> -  static uint32_t FlagsForPseudoClass(const Type aType);
> +  static bool IsEnabled(Type aType, EnabledState aEnabledState)
> +  {
> +    auto index = static_cast<size_t>(aType);
> +    if (sPseudoClassEnabled[index] ||

Might be worth adding an assertion in here that index is in range.  (Less likely to be a problem with Type being an enum class, but still possible.)
https://hg.mozilla.org/integration/mozilla-inbound/rev/d181f8e7e4c32776e77630093cf6542ad07effc3
Bug 1269975 part 1 - Move nsCSSProps::EnabledState to a top level enum class mozilla::CSSEnabledState. r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e4a2ee61ef3a22433aa16708bc6e89772ba9ed0
Bug 1269975 part 2 - Rename nsCSSParser::PropertyEnabledState() to EnabledState(). r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/5321545b938ccdb01d30a1448cb12d73f940e8f3
Bug 1269975 part 3 - Make some static arrays in nsCSSPseudoClasses.cpp static members of the class. r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/a55ecedea260452b5c099c668531e1d61a762fba
Bug 1269975 part 4 - Make nsCSSPseudoClasses::GetPseudoType() take CSSEnabledState rather than two bool params. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2b5e29468766c10fde0f7653cf7a49aeece567e
Bug 1269975 part 1 - Move nsCSSProps::EnabledState to a top level enum class mozilla::CSSEnabledState. r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/7754dd0b76b008f5164a3f8cf6bba6cb36e794c3
Bug 1269975 part 2 - Rename nsCSSParser::PropertyEnabledState() to EnabledState(). r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0769e80038115a28d497bcdc1166ae216f46747
Bug 1269975 part 3 - Make some static arrays in nsCSSPseudoClasses.cpp static members of the class. r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/e29698cb0cf5ab1c3e16eab9fc0099ab06cba7d5
Bug 1269975 part 4 - Make nsCSSPseudoClasses::GetPseudoType() take CSSEnabledState rather than two bool params. r=heycam
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: