Closed Bug 1312338 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(2 files)

It is inconvenient to use two parameters for one string every time. We should just pass one nsACString to Rust.
Assignee: nobody → xidorn+moz
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.
Attachment #8803826 - Flags: review?(manishearth) → review+
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.
Attachment #8803825 - Flags: review?(michael) → review+
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.
> 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 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.
Pushed by xquan@mozilla.com:
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
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.
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?)
Blocks: 1294299
https://hg.mozilla.org/mozilla-central/rev/f9418278a1bb
https://hg.mozilla.org/mozilla-central/rev/053b72f53c35
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.