1269975 - Use EnabledState for nsCSSPseudoClasses::GetPseudoType

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: 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/

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: 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/

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Attachment: 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/

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: 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 5

[Cameron McCormack (:heycam)]

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.

Comment 6

[Cameron McCormack (:heycam)]

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

Comment 7

[Cameron McCormack (:heycam)]

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

Comment 8

[Cameron McCormack (:heycam)]

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

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9235ed9eac26

https://treeherder.mozilla.org/logviewer.html#?job_id=27554597&repo=mozilla-inbound#L2690

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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