1483288 - Provide more number conversions for mozprofile::preferences::Pref

Status: RESOLVED FIXED

(Testing :: Mozbase Rust, enhancement)

Description

[Andreas Tolfsen ❲:ato❳]

geckodriver currently coerces the port number (u16) into u64 before
mozprofile::preferences converts it into PrefValue::U64.  We should
provide more implicit conversions in mozprofile so that the explicit
conversion in geckodriver is unnecessary.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: Provide more number conversions for mozprofile::preferences::PrefValue.

Allow i8, u8, i16, u64, i32, and u32 to be implicitly converted into
PrefValue::Int.  u64 is not supported because it would overflow,
so this still needs to be handled manually.

geckodriver stores the port number as u8 and this will allow it to
implicitly convert it to PrefValue::Int without using the unsafe
"as i64" coercion.

Comment 2

[Andreas Tolfsen ❲:ato❳]

Attachment: Avoid "as i64" coercion of marionette.port pref value.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 9000000 [details] [diff] [review]
Provide more number conversions for mozprofile::preferences::PrefValue.

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

::: testing/mozbase/rust/mozprofile/src/preferences.rs
@@ +66,1 @@
>  impl From<i64> for PrefValue {

Please put a short note below here why u64 doesn't work.

@@ +120,5 @@
> +        assert_eq!(PrefValue::from(42u16), PrefValue::Int(42i64));
> +        assert_eq!(PrefValue::from(42i32), PrefValue::Int(42i64));
> +        assert_eq!(PrefValue::from(42u32), PrefValue::Int(42i64));
> +        assert_eq!(PrefValue::from(42i64), PrefValue::Int(42i64));
> +    }

Could we assert for an error in case of u64? Or will it refuse to compile? It might be the latter, right?

Comment 4

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #3)

> ::: testing/mozbase/rust/mozprofile/src/preferences.rs
> @@ +66,1 @@
> >  impl From<i64> for PrefValue {
> 
> Please put a short note below here why u64 doesn't work.

Converting from u64 _could_ work, but you’d need to make the return
type Result<PrefValue>, which I don’t think is a particularly good
idea.

> @@ +120,5 @@
> > +        assert_eq!(PrefValue::from(42u16), PrefValue::Int(42i64));
> > +        assert_eq!(PrefValue::from(42i32), PrefValue::Int(42i64));
> > +        assert_eq!(PrefValue::from(42u32), PrefValue::Int(42i64));
> > +        assert_eq!(PrefValue::from(42i64), PrefValue::Int(42i64));
> > +    }
> 
> Could we assert for an error in case of u64? Or will it refuse to compile?
> It might be the latter, right?

Correct, it will not compile because we don’t implement Into<Pref> for u64.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c727114d364
Provide more number conversions for mozprofile::preferences::PrefValue. r=whimboo
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8fa2901262d
Avoid "as i64" coercion of marionette.port pref value. r=whimboo

Comment 6

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/0c727114d364
https://hg.mozilla.org/mozilla-central/rev/c8fa2901262d