1435944 - stylo: Have nsMediaFeature::eIdent use atom rather than string

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Xidorn Quan [:xidorn] UTC+10]

From profile in bug 1420423 comment 56, I see function string_from_chars_pointer shows up in the profile. It takes a total of 8ms (which is 2% of the regression, and ~0.2% of the total time). It is not a lot of time, but it should be an easy fix anyway.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Feeling tired of staring at profiles and reviewing code... Let me do something simpler :)

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1435944 part 1 - Add ToAddRefed() for nsStaticAtom.

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

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1435944 part 2 - Use atom for identifier media features.

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8948940 [details]
Bug 1435944 part 2 - Use atom for identifier media features.

https://reviewboard.mozilla.org/r/218368/#review224126

r=me, thanks!

::: layout/style/nsMediaFeatures.cpp:491
(Diff revision 1)
>      return;
>  
>    // Look up the appropriate theme string
> -  for (size_t i = 0; i < ArrayLength(themeStrings); ++i) {
> -    if (windowsThemeId == themeStrings[i].id) {
> -      aResult.SetStringValue(nsDependentString(themeStrings[i].name),
> +  for (const auto& theme : kThemeStrings) {
> +    if (windowsThemeId == theme.mId) {
> +      aResult.SetAtomIdentValue((*theme.mName)->ToAddRefed());

Would it make sense to add an `nsStaticAtom*` overload to `SetAtomIdentValue` itself? It may look nicer.

::: layout/style/nsMediaList.cpp:450
(Diff revision 1)
>                nsCSSProps::ValueToKeyword(expr.mValue.GetIntValue(),
>                                           feature->mData.mKeywordTable),
>                aString);
>            break;
>          case nsMediaFeature::eIdent:
> -          NS_ASSERTION(expr.mValue.GetUnit() == eCSSUnit_Ident,
> +          aString.Append(nsDependentAtomString(expr.mValue.GetAtomValue()));

Otherwise this would need escaping as well.

::: servo/components/style/gecko/media_queries.rs:396
(Diff revision 1)
>              nsMediaFeature_ValueType::eEnumerated => {
>                  let value = css_value.integer_unchecked() as i16;
>                  Some(MediaExpressionValue::Enumerated(value))
>              }
>              nsMediaFeature_ValueType::eIdent => {
> -                debug_assert!(css_value.mUnit == nsCSSUnit::eCSSUnit_Ident);
> +                debug_assert!(css_value.mUnit == nsCSSUnit::eCSSUnit_AtomIdent);

debug_assert_eq! while you're here?

::: servo/components/style/gecko/media_queries.rs:432
(Diff revision 1)
>                  dest.write_char('/')?;
>                  b.to_css(dest)
>              },
>              MediaExpressionValue::Resolution(ref r) => r.to_css(dest),
>              MediaExpressionValue::Ident(ref ident) => {
> -                CssStringWriter::new(dest).write_str(ident)
> +                serialize_identifier(&ident.to_string(), dest)

I think it's fine for this to just do `ident.to_css` or something without needing to allocate it to do CSS escaping, since we know the strings statically.

Worth pointing it out in a comment or something I guess. Also we really need to fix the CSS identifier serialization to not allocate, but that's another matter :(

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8948940 [details]
Bug 1435944 part 2 - Use atom for identifier media features.

https://reviewboard.mozilla.org/r/218368/#review224126

> Would it make sense to add an `nsStaticAtom*` overload to `SetAtomIdentValue` itself? It may look nicer.

I thought about this, but I suppose this way is more general since other places may also want `already_AddRefed<nsAtom>` while we just have an `nsStaticAtom`.

I'm not particularly happy with this interface, though... I was thinking about some free function to take an `nsStaticAtom*` rather than having a method... Maybe not that much different.

> debug_assert_eq! while you're here?

I briefly recall there was an RFC which is going to deprecate the `_eq`/`_ne` variants of `assert`s (actually it's RFC 2011), so I don't think it makes much sense to convert this here.

> I think it's fine for this to just do `ident.to_css` or something without needing to allocate it to do CSS escaping, since we know the strings statically.
> 
> Worth pointing it out in a comment or something I guess. Also we really need to fix the CSS identifier serialization to not allocate, but that's another matter :(

Do we support `to_css` on `Atom`? I thought we don't but if it does we can do that, I guess... Given no content-exposed media feature actually uses identifier.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8948940 [details]
Bug 1435944 part 2 - Use atom for identifier media features.

https://reviewboard.mozilla.org/r/218368/#review224126

> Otherwise this would need escaping as well.

And I'm not keen on having the old style system to have the same behavior as stylo in this case... but I guess I can have it escaped.

> Do we support `to_css` on `Atom`? I thought we don't but if it does we can do that, I guess... Given no content-exposed media feature actually uses identifier.

Apparently `Atom` doesn't implement `ToCss`, so we need to use string... Anyway, I don't consider the performance here very critical, since this is really internal-only, and I don't think people want to serialize media query a lot either...

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8948940 [details]
Bug 1435944 part 2 - Use atom for identifier media features.

https://reviewboard.mozilla.org/r/218368/#review224126

> And I'm not keen on having the old style system to have the same behavior as stylo in this case... but I guess I can have it escaped.

Sounds good, it doesn't really matter much.

> I briefly recall there was an RFC which is going to deprecate the `_eq`/`_ne` variants of `assert`s (actually it's RFC 2011), so I don't think it makes much sense to convert this here.

Yeah, there is, but it hasn't got so much traction... Actually a contributor did it in https://hg.mozilla.org/integration/autoland/rev/fd7764a996d8, so...

> Apparently `Atom` doesn't implement `ToCss`, so we need to use string... Anyway, I don't consider the performance here very critical, since this is really internal-only, and I don't think people want to serialize media query a lot either...

Ok, sounds good then :)

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8948940 [details]
Bug 1435944 part 2 - Use atom for identifier media features.

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

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8948939 [details]
Bug 1435944 part 1 - Add ToAddRefed() for nsStaticAtom.

https://reviewboard.mozilla.org/r/218366/#review224206

r=me, I guess.

::: xpcom/ds/nsAtom.h:119
(Diff revision 1)
> +  already_AddRefed<nsAtom> ToAddRefed() {
> +    return already_AddRefed<nsAtom>(static_cast<nsAtom*>(this));
> +  }

I'm assuming here that the machinery we want to pass `staticAtom->ToAddRefed()` to already deals with `already_AddRefed` dynamic atoms elsewhere, so we don't really want to add another overload (and more stuff on the Rust side)?

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8948939 [details]
Bug 1435944 part 1 - Add ToAddRefed() for nsStaticAtom.

https://reviewboard.mozilla.org/r/218366/#review224206

> I'm assuming here that the machinery we want to pass `staticAtom->ToAddRefed()` to already deals with `already_AddRefed` dynamic atoms elsewhere, so we don't really want to add another overload (and more stuff on the Rust side)?

So the reason of adding this is that, there are functions taking `already_AddRefed<nsAtom>` for transferring ownership without changing the refcount, and normal `nsAtom*` can just use `do_AddRef(atom)` in case it's a weak reference. However, this is not possible for `nsStaticAtom` because it has `AddRef`/`Release` explicitly deleted, so we need to cast it to `nsAtom*` first to create an `already_AddRef` anyway.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8948940 [details]
Bug 1435944 part 2 - Use atom for identifier media features.

https://reviewboard.mozilla.org/r/218368/#review224126

> Yeah, there is, but it hasn't got so much traction... Actually a contributor did it in https://hg.mozilla.org/integration/autoland/rev/fd7764a996d8, so...

I see what did you mean now when I'm rebasing my patch... ok, then I'll just change based on the new code here.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8948939 [details]
Bug 1435944 part 1 - Add ToAddRefed() for nsStaticAtom.

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

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8948940 [details]
Bug 1435944 part 2 - Use atom for identifier media features.

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43a1e6e51a52
part 1 - Add ToAddRefed() for nsStaticAtom. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/332427963060
part 2 - Use atom for identifier media features. r=emilio

Comment 15

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/43a1e6e51a52
https://hg.mozilla.org/mozilla-central/rev/332427963060