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

RESOLVED FIXED in Firefox 60

Status

()

defect
P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks 1 bug)

Trunk
mozilla60
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox58 wontfix, firefox59 wontfix, firefox60 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Feeling tired of staring at profiles and reviewing code... Let me do something simpler :)
Assignee: nobody → xidorn+moz
Comment hidden (mozreview-request)

Comment 4

a year ago
mozreview-review
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 :(
Attachment #8948940 - Flags: review?(emilio) → review+
(Assignee)

Comment 5

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 6

a year ago
mozreview-review-reply
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

a year ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 9

a year ago
mozreview-review
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)?
Attachment #8948939 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 10

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 11

a year ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

a year ago
Pushed by xquan@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/43a1e6e51a52
https://hg.mozilla.org/mozilla-central/rev/332427963060
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.