Closed Bug 1435944 Opened 6 years ago Closed 6 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1435944 part 1 - Add ToAddRefed() for nsStaticAtom.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1435944 part 2 - Use atom for identifier media features.
59 bytes, text/x-review-board-request
emilio
: review+
Details 
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.
Feeling tired of staring at profiles and reviewing code... Let me do something simpler :)
Assignee: nobody → xidorn+moz
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+
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 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 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 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+
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 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.
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
Closed: 6 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: