|PrepareAcceptLanguages| can be rewrite with modern nsCString API to avoid false alarm in cppcheck linter, as mentioned in bug 1372065 comment #1. Here is the TODO list: - avoid unecessary string copy - use nsCCharSeparatedTokenizer as the string tokenizer - use nsTString::Trim to replace the usage of net_FindCharInSet and net_FindCharNotInSet

It might also be appropriate to use rust for the method rewrite.

I'm willing to take this bug on. Here's a sample patch. Two major questions I have (notwithstanding problems in the code): 1. I haven't written a gtest, nor have I run the full test suite. When I try to run `./mach gtest netwerk`, I get this error: ``` 1:46.09 In file included from /Users/jhemphill/oss/firefox/obj-x86_64-apple-darwin16.7.0/dom/file/Unified_cpp_dom_file0.cpp:110: 1:46.09 /Users/jhemphill/oss/firefox/dom/file/MutableBlobStorage.cpp:10:10: fatal error: 'mozilla/dom/ipc/TemporaryIPCBlobChild.h' file not found 1:46.09 #include "mozilla/dom/ipc/TemporaryIPCBlobChild.h" 1:46.09 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1:46.25 1 error generated. ``` I'm rebased onto the tip of the source tree (71a2dacbbb8ba2f3522432ff7e8fc0151ca591f9). Does this mean trunk is broken? How do I find a commit on which gtest will work? 2. I'm willing to rewrite this logic in Rust, but I'm not sure how you want to handle migrating code to Rust. Should I start a crate directly in the netwerk directory? Or should I create an external crate, then set that crate as a dependency? It seems like overkill, since PrepareAcceptLanguages is just a private helper function. But maybe it will be easier to move more code in once we have the infrastructure.

> 2. I'm willing to rewrite this logic in Rust, but I'm not sure how you want > to handle migrating code to Rust. Should I start a crate directly in the > netwerk directory? Or should I create an external crate, then set that crate > as a dependency? It seems like overkill, since PrepareAcceptLanguages is > just a private helper function. But maybe it will be easier to move more > code in once we have the infrastructure. I imagine it would look something like a new crate in netwerk/base called rust_helper (or something similar) Then add a header and .rs implementation similar to rust-url-capi The rust implementation should avoid performing any temporary allocations. The signatures would look something similar to: #[no_mangle] pub extern "C" fn PrepareAcceptLanguages(iAcceptLanguages: &nsACString, oAcceptLanguages: &mut nsACString) -> nsresult #[no_mangle] pub extern "C" fn CanonicalizeLanguageTag(languageTag: &mut nsACString)

I'd like to see more necko code move to rust. @valentin can you be the mentor? since you have more experience on rust. For testing this modification, I'll suggest using xpcshell test instead of gtest. By changing the "intl.accept_languages" preference you can hit the AcceptLanguage generation. Verify the request header in next HTTP request like https://searchfox.org/mozilla-central/source/services/common/tests/unit/test_hawkrequest.js#47

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #4) > I'd like to see more necko code move to rust. @valentin can you be the > mentor? since you have more experience on rust. Sure.

Here's what I have so far. The individual units compile, and the Rust crate passes its own tests, but everything fails at the linker step. Which is fair, since I don't know how to link the C++ files to the Rust files. I've tried to decipher the moz.build scripts, but I'm still not sure what is being built, or how we distinguish between .cpp files and .o files. Also not sure what the compilation units are. When I type `./mach build netwerk`, what does it build? Is it a giant netwerk.o library? Is moz.build a replacement for Make, or does it generate a Makefile, and if so, how? I see a bunch of Rust stuff in toolkit/library/shared/rust. Do we just add Rust crates there, since whatever arcane stuff we need to do is already done there?

(In reply to jthemphill from comment #6) > Created attachment 8918515 [details] > How do I link C++ to a new Rust Crate? > > Here's what I have so far. The individual units compile, and the Rust crate > passes its own tests, but everything fails at the linker step. Which is > fair, since I don't know how to link the C++ files to the Rust files. Take a look at this: https://developer.mozilla.org/en-US/Firefox/Building_Firefox_with_Rust_code#Adding_Rust_code > I've tried to decipher the moz.build scripts, but I'm still not sure what is > being built, or how we distinguish between .cpp files and .o files. Also not > sure what the compilation units are. When I type `./mach build netwerk`, > what does it build? Is it a giant netwerk.o library? Is moz.build a > replacement for Make, or does it generate a Makefile, and if so, how? It's a very complicated build system that I don't fully understand, but you shouldn't need to deal with that right now. > I see a bunch of Rust stuff in toolkit/library/shared/rust. Do we just add > Rust crates there, since whatever arcane stuff we need to do is already done > there? Pretty much, yes. The rust code should live in each their appropriate folder (in this case netwerk/ ) but it should referenced as stated in the link above, so it's linked into the same library.

Ah, okay. I was trying to run a partial build with `./mach build netwerk`, but apparently I have to run a full build, every time. Anyway, the code is actually ready for review now.

Comment on attachment 8918735 [details] [diff] [review] ready-for-review.hgpatch Review of attachment 8918735 [details] [diff] [review]: ----------------------------------------------------------------- This looks quite good. Just a few changes necessary from my point of view. For the next version we should ask nfroyd@mozilla.com for review as well - he's a Rust module peer. ::: netwerk/base/rust-helper/src/lib.rs @@ +1,3 @@ > +static HTTP_LWS: &'static [u8] = &[' ' as u8, '\t' as u8]; > + > +pub mod protocol; Place all the code into this file. There's no need to over-complicate this. ::: netwerk/base/rust-helper/src/protocol/http/mod.rs @@ +1,1 @@ > +pub mod nshttphandler; This file should not exist. ::: netwerk/base/rust-helper/src/protocol/http/nshttphandler.rs @@ +1,1 @@ > +use HTTP_LWS; This code should be in lib.rs @@ +26,5 @@ > +} > + > +#[no_mangle] > +#[allow(non_snake_case)] > +/// Allocates a C string into that contains a ISO 639 language list This comment is wrong. It doesn't technically an C string, it creates an nsACString. @@ +128,5 @@ > + } > + let sub_tag_len = sub_tag.len(); > + > + if sub_tag_len == 1 { > + // x x? ::: netwerk/base/rust-helper/src/protocol/http/rust-nsHttpHandler.h @@ +1,1 @@ > +#ifndef RUST_NS_HTTP_HANDLER move this to netwerk/base/rust-helper/src/helper.h or something similar in that same folder. Also call the define something more generic RUST_NS_NET_HELPER ? ::: netwerk/base/rust-helper/src/protocol/mod.rs @@ +1,1 @@ > +pub mod http; This file should not exist. ::: netwerk/protocol/http/nsHttpHandler.cpp @@ +58,4 @@ > #include "nsIXULRuntime.h" > #include "nsCharSeparatedTokenizer.h" > #include "nsRFPService.h" > +#include "rust-helper/src/protocol/http/rust-nsHttpHandler.h" This would then be rust-helper/src/helper.h @@ +1907,4 @@ > if (!i_AcceptLanguages) > return NS_OK; > > + const nsCString ns_accept_languages(i_AcceptLanguages); instead of nsCString use nsAutoCString acceptLanguages(i_AcceptLanguages)

Hm, okay. I structured the crate with the intention that more code would be added, and that we'd want to keep the submodules, header files, etc. organized accordingly. But I'll assume YAGNI and keep it simple for this bug :)

According to xpcom/rust/nsstring/src/lib.rs: //! Use `ns[C]String` (`ns[C]String` in C++) for string struct members, and as //! an intermediate between rust string data structures (such as `String` or //! `Vec<u16>`) and `&{mut,} nsA[C]String` (using `ns[C]String::from(value)`). //! These conversions will attempt to re-use the passed-in buffer, appending a //! null. //! //! ... //! //! There is currently no Rust equivalent to nsAuto[C]String. Implementing a //! type that contains a pointer to an inline buffer is difficult in Rust due //! to its move semantics, which require that it be safe to move a value by //! copying its bits. If such a type is genuinely needed at some point, //! https://bugzilla.mozilla.org/show_bug.cgi?id=1403506#c6 has a sketch of how //! to emulate it via macros. So how does nsAutoCString work here?

Nevermind, I get it. Rust can only interact with the nsAString proxy object, so it never has a chance to screw up the internals of an nsAutoString. But we can't build an nsAutoString inside Rust without some ridiculous unsafeness.

Comment on attachment 8920442 [details] [diff] [review] Need second opinion from nfroyd Review of attachment 8920442 [details] [diff] [review]: ----------------------------------------------------------------- This is very cool! Incrementally moving some pretty gross C code to Rust is fantastic! Just two small things to note. ::: netwerk/base/rust-helper/src/lib.rs @@ +69,5 @@ > + let o_token = o_accept_languages.to_mut(); > + canonicalize_language_tag(&mut o_token[token_pos..]); > + } > + > + Nit: we can remove a blank line here. @@ +125,5 @@ > + } > + > + let sub_tag_len = sub_tag.len(); > + > + if sub_tag_len == 1 { You may want to write this as: match sub_tag.len() { // Singleton tag... 1 => break, // ISO 3166-1... 2 => ..., // ISO 15924... 4 => ..., _ => continue, } if possible, which is slightly more concise.

Thanks for the quick review! This should be ready for try-server and checkin :)

Comment on attachment 8920625 [details] [diff] [review] ship-it.hgpatch checkin? is usually used for when a patch is ready to land (and better yet, the checkin-needed bug keyword). Moving that over to a needinfo request so this bug doesn't show up on the "patches that are ready to land" bug queries :)

Running this on try produced much unhappiness: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8e132d34ec983370397da247426eec40fbb39bc Reading through the patch again, I see that toolkit/library/gtest/rust/Cargo.lock didn't get updated, which is probably the source of the problems. Jeff, can you update that bit, and we can try again?

Updated toolkit/library/gtest/rust/Cargo.lock as well. Grepped for "rust_url_capi" and couldn't find any other lockfiles.

I was just about to check this in, but I realized I hadn't seen a formal r+ from Valentin, just a feedback+. I wanted to make sure the changes suggested in comment 9 had been made, or if there are other things that need to be fixed.

Comment on attachment 8920744 [details] [diff] [review] updated-gtest-dependency.hgpatch Review of attachment 8920744 [details] [diff] [review]: ----------------------------------------------------------------- Only one minor nit that wasn't fixed. Not that important, we could even fix later. r+ ::: netwerk/protocol/http/nsHttpHandler.cpp @@ +1929,4 @@ > if (!i_AcceptLanguages) > return NS_OK; > > + const nsAutoCString ns_accept_languages(i_AcceptLanguages); nit: by the c++ style naming convention this variable would be called acceptLanguages;

Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c4c253a9a462 Port nsHttpHandler::PrepareAcceptLanguages over to Rust; r=valentin,froydnj