Closed
Bug 1312338
Opened 8 years ago
Closed 8 years ago
Stylo: use nsACString pointer rather than (*const u8, u32) to pass string parameters
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
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 | ||
Updated•8 years ago
|
Assignee: nobody → xidorn+moz
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•8 years ago
|
||
mozreview-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/#review86972 We could try to use `Option<&nsACString>` in the bindings itself, but fine as-is.
Attachment #8803826 -
Flags: review?(manishearth) → review+
Comment 4•8 years ago
|
||
mozreview-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 5•8 years ago
|
||
mozreview-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.
Comment 6•8 years ago
|
||
> 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.
Assignee | ||
Comment 7•8 years ago
|
||
mozreview-review-reply |
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
Comment 9•8 years ago
|
||
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.
Assignee | ||
Comment 10•8 years ago
|
||
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f9418278a1bb https://hg.mozilla.org/mozilla-central/rev/053b72f53c35
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•