1670183 - Combine testHasValue and testGetValue in C++/JS API

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Jan-Erik Rediger [:janerik]]

Right now we expose both a testHasValue and a testGetValue function on metrics for both C++ and JavaScript.
In Rust we only have the latter, which returns an Option<T>, with None signalling that there is just no value.
We can combine the functions in a similar way for C++ and JavaScript.

• In C++ use mozilla::Maybe to return Some(T) or Nothing()
• In JavaScript throw a proper JavaScript exception from testGetValue instead (or use null?)

Comment 1

[Alessio Placitelli [:Dexter]]

Please note that this should probably be validated with developers as well.

The reason why we have testHasValue and testGetValue is that it was strongly advocated for by a group of devs. We could have unified them in Kotlin, too!

Comment 2

[Jan-Erik Rediger [:janerik]]

(In reply to Alessio Placitelli [:Dexter] from comment #1)

Please note that this should probably be validated with developers as well.

The reason why we have testHasValue and testGetValue is that it was strongly advocated for by a group of devs. We could have unified them in Kotlin, too!

I wasn't aware this was by request. Interesting.
The Option<T> way is definitely much more idiomatic in Rust (plus it kinda prevents the small window where the value could change inbetween hasValue and getValue)

Comment 3

[Alessio Placitelli [:Dexter]]

(In reply to Jan-Erik Rediger [:janerik] from comment #2)

(In reply to Alessio Placitelli [:Dexter] from comment #1)

Please note that this should probably be validated with developers as well.

The reason why we have testHasValue and testGetValue is that it was strongly advocated for by a group of devs. We could have unified them in Kotlin, too!

I wasn't aware this was by request. Interesting.
The Option<T> way is definitely much more idiomatic in Rust (plus it kinda prevents the small window where the value could change inbetween hasValue and getValue)

Yup, and for reference, I'm with you on this proposal, and I made the mistake of not pushing back hard enough on the request back then :-)

Comment 4

[Chris H-C :chutten]

From my experience the idiomatic gecko JS way would be to return null or undefined (and document which one). Exceptions would be a bit loud and weird.

Comment 5

[Chris H-C :chutten]

Alrighty, looks like we can do this if we return jsval from xpidl, and use mozilla::Maybe in C++. Design for how to handle "no value" is to return undefined in JS and Nothing() in C++.

Comment 6

[Chris H-C :chutten]

Attachment: Bug 1670183 - Combine testHas and testGet methods for FOG C++ and JS r?janerik!

Comment 7

[Chris H-C :chutten]

Attachment: Bug 1670183 - Clean up style of FOG bindings r?janerik!

Depends on D97032

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ddff3b34f104
Combine testHas and testGet methods for FOG C++ and JS r=janerik
https://hg.mozilla.org/integration/autoland/rev/87c9fbffd69d
Clean up style of FOG bindings r=janerik

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/ddff3b34f104
https://hg.mozilla.org/mozilla-central/rev/87c9fbffd69d