Closed Bug 1449068 Opened 2 years ago Closed 2 years ago

Port nsCSSCounterStyleRule to be backed by Servo data

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

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.
Blocks: 1449087
Assignee: nobody → xidorn+moz
No longer blocks: 1449087
Summary: Port nsCSSFontFaceRule and nsCSSCounterStyleRule to be backed by Servo data → Port nsCSSCounterStyleRule to be backed by Servo data
Blocks: 1346988
Blocks: 1451178
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 :/
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...
Attachment #8964820 - Attachment is obsolete: true
Blocks: 1451289
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.
Attachment #8964818 - Flags: review?(emilio) → review+
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).
Attachment #8964819 - Flags: review?(emilio) → review+
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?
Attachment #8964847 - Flags: review?(emilio) → review+
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?
Attachment #8964848 - Flags: review?(emilio) → review+
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.
Attached file Servo PR
Pushed by xquan@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/3511b66ee335
https://hg.mozilla.org/mozilla-central/rev/fe4da1136b50
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.