Closed Bug 1309165 Opened 8 years ago Closed 8 years ago

Use nsstring bindings in Stylo

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: manishearth, Assigned: manishearth)

References

Details

Attachments

(3 files)

Bug 1309165 - Vendor m-c's nsstring in-tree;
58 bytes, text/x-review-board-request
nika
: review+
Details
Bug 1309165 - Implement text-overflow using nsstring bindings;
58 bytes, text/x-review-board-request
nika
: review+
emilio
: review+
Details
Pull request
41 bytes, text/x-github-pull-request
Details | Review 
We should be able to interact with nsString values on the Servo side.
Given that Servo doesn't support multi-valued text-overflow yet, I conditionally compiled the longhand. Perhaps I should just add support? Not sure how hard it is.
Comment on attachment 8799689 [details]
Bug 1309165 - Vendor m-c's nsstring in-tree;

https://reviewboard.mozilla.org/r/84828/#review83436

I hope that you aren't planning to land this into m-c, I really don't want 2 copies of the nsstring bindings there.

Before stylo lands you'll need to find a way to work off of the in-tree crate.

::: servo/components/style/gecko_bindings/nsstring_vendor/src/lib.rs:166
(Diff revision 1)
> +/// This type is zero-sized and uninstantiable. It is used in the definition of
> +/// nsA[C]String to create an opaque zero-sized type which does not raise an
> +/// `improper_ctypes` warning when passed behind a `*const nsA[C]String`.
> +///
> +/// This type is not exported and does not make up part of the external API.
> +enum Impossible {}

This version of the crate is out of date. The most recent version in mozilla-central no longer uses empty enums behind references.

::: servo/components/style/gecko_bindings/sugar/ns_string.rs:5
(Diff revision 1)
> +// We explicitly do not use bindgen to replace structs::nsString with nsstring::nsString
> +// because the latter has a destructor, and passing things with a destructor over FFI needs to be
> +// done carefully. We're passing nsString over FFI within other structs, so it might be easy to slip
> +// up here. Admittedly, so far these structs are always behind pointers, so it might be possible to
> +// do this replacing after all.
> +//
> +// Anyway, if you wish to use nsstring_vendor::nsString directly in FFI (rendering this module
> +// obsolete), please think Very Hard about it.
> +//
> +// In 1.13 this might become safer to do

This is wrong. `nsString<'static>` not only has a destructor, but is not `#[repr(C)]`.

You should instead use `nsstring::nsStringRepr`. You can use that one directly in the FFI from bindgen. It doesn't have a destructor and is `#[repr(C)]`.

That type can be used like nsString through the standard way in the nsstring bindings crate: `&nsACString`. All string types in this crate have a `Deref` impl for that type, and all of the string methods are implemented on it.

::: servo/components/style/gecko_bindings/sugar/ns_string.rs:30
(Diff revision 1)
> +    }
> +}
> +
> +impl From<nsString<'static>> for FFIString {
> +    fn from(from: nsString<'static>) -> FFIString {
> +        unsafe{ transmute(from) }

There's no way this compiles on stable. nsString<'static> will have a different size than FFIString because of the drop flags.
Attachment #8799689 - Flags: review?(michael) → review-
Comment on attachment 8799690 [details]
Bug 1309165 - Implement text-overflow using nsstring bindings;

https://reviewboard.mozilla.org/r/84830/#review83438

I havent looked too closely at this, as I feel like it will improve quite a bit once we use nsStringRepr instead of FFIString.

::: servo/components/style/properties/gecko.mako.rs:1704
(Diff revision 1)
> +                // XXXManishearth perhaps nsstring should have a clone impl?
> +                let nsstring = unsafe { other.mString.to_string() };
> +                let bytes: Vec<u16> = (**nsstring).into();
> +                let cloned: nsString = bytes.into_boxed_slice().into();
> +                side.mString = cloned.into();
> +                mem::forget(nsstring);

Does `side.mString.assign(&other.mString)` work once you actually use `nsStringRepr` instead of `FFIString`?
Attachment #8799690 - Flags: review?(michael) → review-
Addressed.

> I hope that you aren't planning to land this into m-c, I really don't want 2 copies of the nsstring bindings there.

Yeah, this is temporary. As long as the CI is still Servo-side we can't refer to things outside m-c. Once stylo's gating CI is the autolander-dance that builds both, we can refer to anything within m-c.

This bug cannot land on the servo side till https://github.com/servo/servo/pull/13700 does, since it introduces an external dependency in the structs_foo.rs files.
Comment on attachment 8799689 [details]
Bug 1309165 - Vendor m-c's nsstring in-tree;

https://reviewboard.mozilla.org/r/84828/#review83472

lgtm

::: servo/components/style/binding_tools/regen.py:434
(Diff revision 2)
> -
> +    if "blacklist_types" in current_target:
> +        for ty in current_target["blacklist_types"]:
> +            flags.append("--blacklist-type")
> +            flags.append(ty)

nit: There should be blank lines around this if statement.
Attachment #8799689 - Flags: review?(michael) → review+
Comment on attachment 8799690 [details]
Bug 1309165 - Implement text-overflow using nsstring bindings;

https://reviewboard.mozilla.org/r/84830/#review83474

I only reviewed the nsstring crate usage, and it looks good.
Attachment #8799690 - Flags: review?(michael) → review+
Comment on attachment 8799690 [details]
Bug 1309165 - Implement text-overflow using nsstring bindings;

https://reviewboard.mozilla.org/r/84830/#review83488

r=me with that fixed.

::: servo/components/style/properties/gecko.mako.rs:1662
(Diff revision 3)
>  
> +
> +    fn clear_overflow_sides(&mut self) {
> +        use gecko_bindings::structs::nsStyleTextOverflowSide;
> +        use nsstring::nsString;
> +        fn clear(side: &mut nsStyleTextOverflowSide) {

`clear_if_string` would be a more representative name. Same for the method name.

::: servo/components/style/properties/gecko.mako.rs:1693
(Diff revision 3)
> +        self.clear_overflow_sides();
> +        if v.second.is_none() {
> +            self.gecko.mTextOverflow.mLogicalDirections = true;
> +        }
> +
> +        let SpecifiedValue {first, second} = v;

nit: Tidy will yell at you, please space aroun braces properly.

::: servo/components/style/properties/gecko.mako.rs:1694
(Diff revision 3)
> +        if v.second.is_none() {
> +            self.gecko.mTextOverflow.mLogicalDirections = true;
> +        }
> +
> +        let SpecifiedValue {first, second} = v;
> +        let second = second.unwrap_or(first.clone());

Quite a bit unfortunate the unconditional clone, maybe you could pass a reference to `set`, and avoid that.

If you can't avoid it in any way, which seems unlikely, please use `unwrap_or_else` so we don't do it even when we don't use it.

::: servo/components/style/properties/longhand/text.mako.rs:16
(Diff revision 3)
>                           additional_methods=[Method("has_underline", "bool"),
>                                               Method("has_overline", "bool"),
>                                               Method("has_line_through", "bool")]) %>
>  
> +% if product == "servo":
> -${helpers.single_keyword("text-overflow", "clip ellipsis", animatable=False)}
> +  ${helpers.single_keyword("text-overflow", "clip ellipsis", animatable=False)}

nit: This indentation should be four spaces.

That being said, text-oveflow: "foo" would be easy enough to support in Servo (see `Fragment::transform_into_ellipsis`), and we'd get rid of yet another completely different path.

It's totally fine to leave it like this for now, but please open a Servo issue and reference it from here.

Hmm... though the second side might be harder...
Attachment #8799690 - Flags: review?(ecoal95) → review+
Comment on attachment 8799690 [details]
Bug 1309165 - Implement text-overflow using nsstring bindings;

https://reviewboard.mozilla.org/r/84830/#review83488

> nit: This indentation should be four spaces.
> 
> That being said, text-oveflow: "foo" would be easy enough to support in Servo (see `Fragment::transform_into_ellipsis`), and we'd get rid of yet another completely different path.
> 
> It's totally fine to leave it like this for now, but please open a Servo issue and reference it from here.
> 
> Hmm... though the second side might be harder...

Yeah, I'm planning on looking at the layout code later and seeing how hard this would be. Might be possible as an E-easy, at least for one side.

In general I do plan on at least investigating the servo-side of stylo things that get implemented, just not in the same PR.
Attachment #8799689 - Flags: review?(ecoal95)
Attachment #8799689 - Flags: review?(cam)
Attachment #8799690 - Flags: review?(cam)
I realized that there's nothing to land on the Gecko side here (and hence I don't need a peer to sign off on it) -- the Cargo.lock changes are to the stylo version only, and that will happen automatically in the next resync.

I'll just copy it over to Servo then.
Attached file Pull request — 

    
    

    
  
Merged on the servo side
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: