1329076 - stylo: Add :-moz-browser-frame pseudo-class for fullscreen

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

This is an internal pseudo-class specifically used for fullscreen stuff in Gecko for a UA sheet rule.

It is necessary to make fullscreen not crash with stylo.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1329076 part 1 - Change a bit about the code struct of pseudo-class matching in SelectorMatches.

Review commit: https://reviewboard.mozilla.org/r/102834/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102834/

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1329076 part 2 - Move some pseudo-classes which depend on only the element to an independent function.

I'm a bit concerned about the additional, probably un-inlineable, function call
in this path. I'm not sure how hot this path would be.

Review commit: https://reviewboard.mozilla.org/r/102836/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102836/

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Add function to convert NonTSPseudoClass to CSSPseudoClassType.

Review commit: https://reviewboard.mozilla.org/r/102838/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102838/

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Add NonTSPseudoClass::is_internal function.

Review commit: https://reviewboard.mozilla.org/r/102840/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102840/

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1329076 part 3 - Add :-moz-browser-frame pseudo-class.

Review commit: https://reviewboard.mozilla.org/r/102842/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102842/

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Adding this pseudo-class makes me think we should probably use a list file like Gecko's nsCSSPseudoClassList.h, rather than having every function update separately for each new one.

I'm going to do that change after this lands.

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8824315 [details]
Bug 1329076 part 1 - Change a bit about the code struct of pseudo-class matching in SelectorMatches.

https://reviewboard.mozilla.org/r/102834/#review103284

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8824316 [details]
Bug 1329076 part 2 - Move some pseudo-classes which depend on only the element to an independent function.

https://reviewboard.mozilla.org/r/102836/#review103286

::: layout/style/nsCSSPseudoClasses.h:90
(Diff revision 1)
>        return true;
>      }
>      return false;
>    }
>  
> +  // Checks whether the given pseudo class matches the element.

Can you document here when we return a Some and when we return a Nothing?

::: layout/style/nsCSSPseudoClasses.cpp:139
(Diff revision 1)
> +    case CSSPseudoClassType::mozNativeAnonymous: {
> +      return Some(aElement->IsInNativeAnonymousSubtree());
> +    }

Nit: can remove the braces if you want.

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8824317 [details]
Add function to convert NonTSPseudoClass to CSSPseudoClassType.

https://reviewboard.mozilla.org/r/102838/#review103288

::: servo/components/style/gecko/selector_parser.rs:180
(Diff revision 1)
> +            Hover => hover,
> +            Enabled => enabled,
> +            Disabled => disabled,
> +            Checked => checked,
> +            Indeterminate => indeterminate,
> +            ReadWrite | ReadOnly => { return None; }

Why None for these two and not mozReadWrite and mozReadOnly?  (Or do the moz ones differ from the unprefixed ones a bit?)

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8824318 [details]
Add NonTSPseudoClass::is_internal function.

https://reviewboard.mozilla.org/r/102840/#review103290

::: servo/components/style/gecko/selector_parser.rs:145
(Diff revision 1)
>          })
>      }
>  }
>  
>  impl NonTSPseudoClass {
> +    pub fn is_internal(&self) -> bool {

Can you add a comment describing what it means to be internal?

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8824319 [details]
Bug 1329076 part 3 - Add :-moz-browser-frame pseudo-class.

https://reviewboard.mozilla.org/r/102842/#review103292

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8824317 [details]
Add function to convert NonTSPseudoClass to CSSPseudoClassType.

https://reviewboard.mozilla.org/r/102838/#review103288

> Why None for these two and not mozReadWrite and mozReadOnly?  (Or do the moz ones differ from the unprefixed ones a bit?)

Given bug 312971, I suppose there is some difference.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/36096df673ca
part 1 - Change a bit about the code struct of pseudo-class matching in SelectorMatches. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/68b4cc7e30ce
part 2 - Move some pseudo-classes which depend on only the element to an independent function. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/79053ff9ea68
part 3 - Add :-moz-browser-frame pseudo-class. r=heycam

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7df260e79769
followup - Fix bustage.

Comment 15

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/36096df673ca
https://hg.mozilla.org/mozilla-central/rev/68b4cc7e30ce
https://hg.mozilla.org/mozilla-central/rev/79053ff9ea68
https://hg.mozilla.org/mozilla-central/rev/7df260e79769