1312338 - Stylo: use nsACString pointer rather than (*const u8, u32) to pass string parameters

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

It is inconvenient to use two parameters for one string every time. We should just pass one nsACString to Rust.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1312338 part 1 - Add as_str_unchecked() to nsACString.

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

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1312338 part 2 - Use nsACString to pass string parameters in binding functions.

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

Comment 3

[Manish Goregaokar [:manishearth]]

Comment on attachment 8803826 [details]
Bug 1312338 part 2 - Use nsACString to pass string parameters in binding functions.

https://reviewboard.mozilla.org/r/87984/#review86972

We could try to use `Option<&nsACString>` in the bindings itself, but fine as-is.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8803825 [details]
Bug 1312338 part 1 - Add as_str_unchecked() to nsACString.

https://reviewboard.mozilla.org/r/87982/#review87020

::: servo/components/style/gecko_bindings/nsstring_vendor/src/lib.rs:575
(Diff revision 1)
>              Gecko_AppendUTF16toCString(self as *mut _, other as *const _);
>          }
>      }
> +
> +    pub unsafe fn as_str_unchecked(&self) -> &str {
> +        str::from_utf8_unchecked(self.deref())

I don't _think_ the .deref() should be necessary. I believe deref coersion would handle it for you.

I'm not opposed to helper methods being added to the string objects, and perhaps this is a worthwhile one, but it does seem like

`from_utf8_unchecked(my_astr.as_ref().unwrap())`
or
`from_utf8_unchecked(&*my_astr)`

is about the same length as

`my_astr.as_ref().unwrap().as_str_unchecked()`
or
`(*my_astr).as_str_unchecked()`

which makes me uncertain what gains you are going for by adding this method.

I'd like an explanation as to why we don't just use from_utf8_unchecked instead of adding a new method, but r=me.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8803826 [details]
Bug 1312338 part 2 - Use nsACString to pass string parameters in binding functions.

https://reviewboard.mozilla.org/r/87984/#review87018

::: servo/ports/geckolib/glue.rs:371
(Diff revision 1)
> -                                      base_length: u32,
> -                                      base: *mut ThreadSafeURIHolder,
>                                        referrer: *mut ThreadSafeURIHolder,
>                                        principal: *mut ThreadSafePrincipalHolder)
>                                        -> RawServoDeclarationBlockStrong {
>      // All this string wrangling is temporary until the Gecko string bindings land (bug 1294742).

You should probably remove this comment, as the string wrangling seems no longer temporary.

Comment 6

[Manish Goregaokar [:manishearth]]

> I don't _think_ the .deref() should be necessary. I believe deref coersion would handle it for you.

Just a note, for deref coercions to kick in you may need to do `from_utf8_unchecked(&self)` instead.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8803825 [details]
Bug 1312338 part 1 - Add as_str_unchecked() to nsACString.

https://reviewboard.mozilla.org/r/87982/#review87020

> I don't _think_ the .deref() should be necessary. I believe deref coersion would handle it for you.
> 
> I'm not opposed to helper methods being added to the string objects, and perhaps this is a worthwhile one, but it does seem like
> 
> `from_utf8_unchecked(my_astr.as_ref().unwrap())`
> or
> `from_utf8_unchecked(&*my_astr)`
> 
> is about the same length as
> 
> `my_astr.as_ref().unwrap().as_str_unchecked()`
> or
> `(*my_astr).as_str_unchecked()`
> 
> which makes me uncertain what gains you are going for by adding this method.
> 
> I'd like an explanation as to why we don't just use from_utf8_unchecked instead of adding a new method, but r=me.

The new form is still shorter. Also the original one should actually be `str::from_utf8_unchecked` isn't it? glue.rs uses `std::str::from_utf8_unchecked` which I'm not a fan of, as it is not completely clear what type does it produce.

Actually I was thinking about naming it `as_str()` without `unsafe` directly, because it doesn't seem to me we usually use encodings other than UTF-8 and ASCII inside Gecko, but apparently you don't like that idea.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9418278a1bb
part 1 - Add as_str_unchecked() to nsACString. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/053b72f53c35
part 2 - Use nsACString to pass string parameters in binding functions. r=manishearth

Comment 9

[Nika Layzell [:nika] (ni? for response)]

CStrings can totally contain non-unicode strings. Gecko treats them just like arbitrary byte streams. I definitely don't think it would have been a good idea to make nsACString deref to &str.

Are you actually sure that all of the callsites from C++ actually are providing you valid UTF-8? Remember that even JS strings are not guaranteed to be valid UTF-8 as they can contain unmatched codepoint pairs.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

JS strings are always passed to DOM code as nsAString (thus UTF-16) in Gecko, IIUC. And we always do a manual conversion from UTF16 to UTF8 in callsites of those functions.

(We probably want a different string type which guarantees to be a UTF-8 string in Gecko side so that we can safely deref it to &str?)

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f9418278a1bb
https://hg.mozilla.org/mozilla-central/rev/053b72f53c35