1449068 - Port nsCSSCounterStyleRule to be backed by Servo data

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

These two rules currently store data in C++ side rather than Rust side, this is a problem because we need to maintain an extra descriptor-to-string table in both sides. (Note: descriptor lists cannot be merged anyway, but the map with string can.)

Also currently we cannot strip the serialization code for specified value because of these rules. Once we change them, we should be able to remove majority of serialization code in nsCSSValue, which is quite a bit.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Remove initial value of @counter-style rule descriptors.

This is misleading and makes its accessors harder to use.

For counter styles, a descriptor being unspecified doesn't mean it would
take the initial value. If the system is `extends`, they need to take
corresponding value from the counter style it's extending. Since details
of the extended counter style is unknown for the style itself, it is not
possible to report value for unspecified descriptor in that case.

The "system" descriptor itself, though, should always have a resolved
value (otherwise we would know nothing about the counter style). When
it's not specified, it defaults to `symbolic`. To make things convenient
a separate method `get_system` is added for that.

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

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Add source_location to CounterStyleRule.

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

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1449068 WIP - Use Servo data to back @counter-style rule.

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

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8964820 [details]
Bug 1449068 WIP - Use Servo data to back @counter-style rule.

https://reviewboard.mozilla.org/r/233548/#review239162

::: servo/ports/geckolib/glue.rs:4786
(Diff revision 1)
>  #[no_mangle]
> -pub extern "C" fn Servo_StyleSet_GetCounterStyleRule(
> +pub unsafe extern "C" fn Servo_StyleSet_GetCounterStyleRule(
>      raw_data: RawServoStyleSetBorrowed,
>      name: *mut nsAtom,
> -) -> *mut nsCSSCounterStyleRule {
> +) -> RawServoCounterStyleRuleBorrowedOrNull {
>      let data = PerDocumentStyleData::from_ffi(raw_data).borrow();
> -
> -    unsafe {
> -        Atom::with(name, |name| {
> +    Atom::with(name, |name| {
> -            data.stylist
> +        data.stylist
> -                .iter_extra_data_origins()
> +            .iter_extra_data_origins()
> -                .filter_map(|(d, _)| d.counter_styles.get(name))
> +            .filter_map(|(d, _)| d.counter_styles.get(name))
> -                .next()
> +            .next()
> +            .map(|rule| rule.as_borrowed())
> -        })
> +    })
> -    }.map(|rule| {
> -        let global_style_data = &*GLOBAL_STYLE_DATA;
> -        let guard = global_style_data.shared_lock.read();
> -        rule.read_with(&guard).get()
> -    }).unwrap_or(ptr::null_mut())
>  }

So this is the function that currently doesn't compile, and it's not yet clear to me what's wrong with the lifetime here.

Because of this, this patch is not tested, and I would write more detailed commit message (and probably split some part off) later.

It is just pushed so that I can get some help with this function :/

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

So I think the problem here is that, borrow() breaks the lifetime from raw_data, so data and anything it derefs to can only live in the function scope, and the final returned value has the same lifetime as data, not raw_data, but from the function signature, the compiler expects the returned value has the same lifetime as raw_data.

It is not clear to me what is the right way to solve this problem. It is probably a violation of lifetime from the beginning given that we use AtomicRefCell there.

It seems we actually never return a Borrowed from any Servo_* function. We either return strong reference, or raw pointer. Probably just use raw pointer this time is enough, I guess...

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1449068 part 1 - Wrap content of ServoStyleSetInlines.h in mozilla namespace.

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

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1449068 part 2 - Use Servo data to back @counter-style rule.

This patch basically does:
* Add descriptor setters and generation count to CounterStyleRule in
  Servo. (This code is mostly based on the old code inside
  nsCSSCounterStyleRule for handling mutation.)
* Use RawServoCounterStyleRule in CounterStyleManager.
* Add ServoCounterStyleRule and remove nsCSSCounterStyleRule.

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

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8964848 [details]
Bug 1449068 part 2 - Use Servo data to back @counter-style rule.

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

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8964818 [details]
Remove initial value of @counter-style rule descriptors.

https://reviewboard.mozilla.org/r/233544/#review239164

::: servo/components/style/counter_style/mod.rs:175
(Diff revision 1)
>                  &self.name
>              }
>  
> +            /// Get the system of this counter style rule, default to
> +            /// `symbolic` if not specified.
> +            pub fn get_system(&self) -> &System {

maybe "resolved_system"? It's somewhat weird that `system` returns an `Option<..>` and `get_system` a value.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8964819 [details]
Add source_location to CounterStyleRule.

https://reviewboard.mozilla.org/r/233546/#review239166

::: servo/components/style/counter_style/mod.rs:163
(Diff revision 1)
> +            /// Line and column of the @counter-style rule source code.
> +            pub source_location: SourceLocation,
>          }
>  
>          impl CounterStyleRuleData {
> -            fn empty(name: CustomIdent) -> Self {
> +            fn empty(name: CustomIdent, location: SourceLocation) -> Self {

nit: If you call the argument `source_location` you can use the shorthand syntax (and you can do it for the `name` if you want, while at it).

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8964847 [details]
Bug 1449068 part 1 - Wrap content of ServoStyleSetInlines.h in mozilla namespace.

https://reviewboard.mozilla.org/r/233594/#review239168

Huh, how did this ever work, I guess we always include this file somewhere where we also have a `using namespace mozilla` or something?

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8964848 [details]
Bug 1449068 part 2 - Use Servo data to back @counter-style rule.

https://reviewboard.mozilla.org/r/233596/#review239170

r=me, reluctantly, because what we were doing is basically the same... Though if you don't go the path of requiring a strong reference instead, please do file a bug.

::: layout/style/CounterStyleManager.cpp:2041
(Diff revision 2)
>  
>    // Names are compared case-sensitively here. Predefined names should
>    // have been lowercased by the parser.
>    ServoStyleSet* styleSet = mPresContext->StyleSet();
> -  nsCSSCounterStyleRule* rule = styleSet->CounterStyleRuleForName(aName);
> +  const RawServoCounterStyleRule*
> +    rule = styleSet->CounterStyleRuleForName(aName);

nit: I think we usually don't split assignments in this way and put the variable name and `=` in the upper line. But it's not a big deal.

::: layout/style/ServoCounterStyleRule.cpp:17
(Diff revision 2)
> +#include "nsStyleUtil.h"
> +
> +namespace mozilla {
> +
> +ServoCounterStyleRule::~ServoCounterStyleRule()
> +{

Maybe `= default` in the header?

::: layout/style/ServoCounterStyleRule.cpp:30
(Diff revision 2)
> +}
> +
> +bool
> +ServoCounterStyleRule::IsCCLeaf() const
> +{
> +  return Rule::IsCCLeaf();

Just don't override it?

::: servo/ports/geckolib/glue.rs:4788
(Diff revision 2)
>  }
>  
> +// XXX Ideally this should return a RawServoCounterStyleRuleBorrowedOrNull,
> +// but we cannot, because the value from AtomicRefCell::borrow() can only
> +// live in this function, and thus anything derived from it cannot get the
> +// same lifetime as raw_data in parameter.

Hmm, so this is slightly annoying, because it's pretty much leaking the pointer outside of the borrow... But that's pretty much what we did anyway, on the other hand.

We don't have any way to represent this in C++. I guess the alternative is returning a strong reference, right? Would that be better, given this code is probably not super-hot?

I'd really prefer not doing this, but if you do, can you file a bug and reference it from here so we can track it?

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8964848 [details]
Bug 1449068 part 2 - Use Servo data to back @counter-style rule.

https://reviewboard.mozilla.org/r/233596/#review239170

> Maybe `= default` in the header?

That would require extra dependency to ServoBindingTypes.h... which is probably not too bad, though.

> Just don't override it?

You can't. This method has an annotation on `css::Rule` to force overriding it in subclass.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Servo PR

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3511b66ee335
part 1 - Wrap content of ServoStyleSetInlines.h in mozilla namespace. r=emilio
https://hg.mozilla.org/integration/autoland/rev/fe4da1136b50
part 2 - Use Servo data to back @counter-style rule. r=emilio

Comment 16

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/3511b66ee335
https://hg.mozilla.org/mozilla-central/rev/fe4da1136b50