1370501 - stylo: Implement ServoStyleRule::GetSpecificity

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Bug 1370501 - stylo: Fix ServoStyleRule::GetSelectorTextFromIndex.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8876042 [details]
Bug 1370501 - stylo: Fix ServoStyleRule::GetSelectorTextFromIndex.

https://reviewboard.mozilla.org/r/147496/#review151788

::: servo/components/selectors/parser.rs:191
(Diff revision 1)
>      }
> +
> +    pub fn specificity(&self, from_index: usize) -> u32 {
> +        let mut result = 0;
> +        let iter = self.0.iter().skip(from_index);
> +        for selector_and_hashes in iter {

So, this is wrong, afaict. You're summing the specificity of the complex selectors in the selector list, and that's definitely not what this code does on Gecko.

The `aSelectorIndex` parameter means the selector in this selector list at index `aSelectorIndex`.

The confusion is that the equivalent to `Selector` in Servo is `nsCSSSelectorList*`. Oh well.

So this needs to effectively return `self.0[index].specificity()`, but not crash if the index is invalid.

I misreviewed the `GetSelectorTextFromIndex` because you named the function `FromIndex`, but what it is doing is `AtIndex`. So it needs to do `self.0[index].to_css()`, not all the following selectors. Please fix that too.

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Bug 1370501 - stylo: Implement ServoStyleRule::GetSpecificity.

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

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8876042 [details]
Bug 1370501 - stylo: Fix ServoStyleRule::GetSelectorTextFromIndex.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147496/diff/1-2/

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8876042 [details]
Bug 1370501 - stylo: Fix ServoStyleRule::GetSelectorTextFromIndex.

https://reviewboard.mozilla.org/r/147496/#review151858

::: servo/components/selectors/parser.rs:171
(Diff revision 2)
>      /// Creates a SelectorList from a Vec of selectors. Used in tests.
>      pub fn from_vec(v: Vec<Selector<Impl>>) -> Self {
>          SelectorList(v.into_iter().map(SelectorAndHashes::new).collect())
>      }
>  
> -    pub fn to_css_from_index<W>(&self, from_index: usize, dest: &mut W)
> +    pub fn to_css_at_index<W>(&self, index: usize, dest: &mut W)

I'd consider to just inline this in the glue.rs file, it's unlikely we want to reuse this.

::: servo/components/selectors/parser.rs:176
(Diff revision 2)
> -        first.selector.to_css(dest)?;
> -        for selector_and_hashes in iter {
> -            dest.write_str(", ")?;
> -            selector_and_hashes.selector.to_css(dest)?;
>          }
> -        Ok(())
> +        self.0[index].to_css()

`to_css(dest)`, right?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8876151 [details]
Bug 1370501 - stylo: Implement ServoStyleRule::GetSpecificity.

https://reviewboard.mozilla.org/r/147562/#review151860

::: servo/components/selectors/parser.rs:176
(Diff revision 1)
>      pub fn to_css_at_index<W>(&self, index: usize, dest: &mut W)
>          -> fmt::Result where W: fmt::Write {
>          if index >= self.0.len() {
>              return Ok(());
>          }
> -        self.0[index].to_css()
> +        self.0[index].selector.to_css(dest)

heh, you fixed it here, please move this part to the previous commit.

::: servo/components/selectors/parser.rs:179
(Diff revision 1)
>              return Ok(());
>          }
> -        self.0[index].to_css()
> +        self.0[index].selector.to_css(dest)
> +    }
> +
> +    pub fn specificity(&self, index: usize) -> u32 {

similarly, the error handling here is unconventional, so let's inline this in glue.rs

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8876042 [details]
Bug 1370501 - stylo: Fix ServoStyleRule::GetSelectorTextFromIndex.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147496/diff/2-3/

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8876151 [details]
Bug 1370501 - stylo: Implement ServoStyleRule::GetSpecificity.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147562/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/31d8aedf2c5a
stylo: Fix ServoStyleRule::GetSelectorTextFromIndex. r=emilio
https://hg.mozilla.org/integration/autoland/rev/d38ff648548c
stylo: Implement ServoStyleRule::GetSpecificity. r=emilio

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/31d8aedf2c5a
https://hg.mozilla.org/mozilla-central/rev/d38ff648548c