Use nsstring bindings in rust-url-capi

RESOLVED FIXED in Firefox 53

Status

()

Core
Networking
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: manishearth, Assigned: mystor)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment)

(Reporter)

Description

a year ago
We should use Michael's nsstring bindings in rust-url-capi instead of the c_fn_get_buffer stuff.
(Assignee)

Updated

a year ago
Depends on: 1319083
(Assignee)

Comment 1

a year ago
Created attachment 8812790 [details] [diff] [review]
Use the nsstring bindings in rust-url-capi

This patch also drops the pretense that rust-url-capi will be used from
outside of c++, or that it will be used outside of mozilla-central,
removing the ifdef __cplusplus code, and including the C++ header
"nsString.h".

MozReview-Commit-ID: BULhHf3DObe
Attachment #8812790 - Flags: review?(valentin.gosu)
(Assignee)

Updated

a year ago
Assignee: nobody → michael
(Assignee)

Updated

a year ago
Attachment #8812790 - Flags: review?(manishearth)
(Reporter)

Comment 2

a year ago
Comment on attachment 8812790 [details] [diff] [review]
Use the nsstring bindings in rust-url-capi

Review of attachment 8812790 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Needs peer review to land.

Have you tested it with network logging to see if it's still largely working? It shouldn't complain about mismatches on regular sites but will complain with malformed IDNA like https://github.com/servo/rust-url/issues/207
Attachment #8812790 - Flags: review?(manishearth) → review+
Comment on attachment 8812790 [details] [diff] [review]
Use the nsstring bindings in rust-url-capi

Review of attachment 8812790 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Please do a try run before pushing.

::: netwerk/base/rust-url-capi/src/lib.rs
@@ +70,4 @@
>    if urlptr.is_null() {
>      return NSError::InvalidArg.error_code();
>    }
> +  let url = &*(urlptr as *const Url);

Is there an advantage to using this syntax instead of let url: &Url = mem::transmute(urlptr); ?
Attachment #8812790 - Flags: review?(valentin.gosu) → review+
(Reporter)

Comment 4

a year ago
> Is there an advantage to using this syntax instead of let url: &Url = mem::transmute(urlptr); ?

Generally good style to avoid transmuting when you can do it a different way. Transmutes are a swiss army knife, and just as pointy -- if the type of the argument changed here you may end up transmuting the wrong thing, whereas you'd be protected from that to some degree with this.
(Assignee)

Updated

a year ago
Blocks: 1319163
Whiteboard: [necko-active]

Comment 5

a year ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd1c7797c866
Use the nsstring bindings in rust-url-capi, r=valentin

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bd1c7797c866
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.