Use EnabledState for nsCSSPseudoClasses::GetPseudoType

RESOLVED FIXED in Firefox 49

Status

()

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

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1269976
(Assignee)

Updated

2 years ago
Assignee: nobody → bugzilla
(Assignee)

Comment 1

2 years ago
Created attachment 8748983 [details]
MozReview Request: Bug 1269975 part 1 - Move nsCSSProps::EnabledState to a top level enum class mozilla::CSSEnabledState. r?heycam

Review commit: https://reviewboard.mozilla.org/r/50665/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50665/
Attachment #8748983 - Flags: review?(cam)
Attachment #8748984 - Flags: review?(cam)
Attachment #8748985 - Flags: review?(cam)
Attachment #8748986 - Flags: review?(cam)
(Assignee)

Comment 2

2 years ago
Created attachment 8748984 [details]
MozReview Request: Bug 1269975 part 2 - Rename nsCSSParser::PropertyEnabledState() to EnabledState(). r?heycam

Review commit: https://reviewboard.mozilla.org/r/50667/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50667/
(Assignee)

Comment 3

2 years ago
Created attachment 8748985 [details]
MozReview Request: Bug 1269975 part 3 - Make some static arrays in nsCSSPseudoClasses.cpp static members of the class. r?heycam

Review commit: https://reviewboard.mozilla.org/r/50669/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50669/
(Assignee)

Comment 4

2 years ago
Created attachment 8748986 [details]
MozReview Request: Bug 1269975 part 4 - Make nsCSSPseudoClasses::GetPseudoType() take CSSEnabledState rather than two bool params. r?heycam

Review commit: https://reviewboard.mozilla.org/r/50671/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50671/
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.)
(Assignee)

Comment 9

2 years ago
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
(Assignee)

Comment 11

2 years ago
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

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b2b5e2946876
https://hg.mozilla.org/mozilla-central/rev/7754dd0b76b0
https://hg.mozilla.org/mozilla-central/rev/d0769e800381
https://hg.mozilla.org/mozilla-central/rev/e29698cb0cf5
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.