Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Methods that can return empty lists (and nullable attributes) #433

Closed
aboba opened this issue Mar 16, 2016 · 2 comments
Closed

Methods that can return empty lists (and nullable attributes) #433

aboba opened this issue Mar 16, 2016 · 2 comments

Comments

@aboba
Copy link
Contributor

aboba commented Mar 16, 2016

Throughout the specification, there appear to be issues with methods that can return null or attributes that can be null. For example:

  1. If getLocalCandidates() is called before gather() had been called, there are no local candidates. So should getLocalCandidates() return an empty list?
  2. What does getRemoteCandidates() return if there aren't any remote candidates yet? An empty list?
  3. In getRemoteCertificates(), what if the remote certificates aren't available yet? Can getRemoteCandidates return an empty list?
  4. Why is RTCDtlsTransportState nullable in RTCDtlsTransportStateChangedEventInit?
    dictionary RTCDtlsTransportStateChangedEventInit : EventInit {
    RTCDtlsTransportState? state;
    };
  5. In the RtpSender, what if an rtcpTransport isn't provided? Shouldn't the rtcpTransport attribute be nullable?

partial interface RTCRtpSender : RTCStatsProvider {
readonly attribute RTCDtlsTransport? rtcpTransport;
};

  1. In the RtpReceiver, what if an rtcpTransport isn't provided? Shouldn't the rtcpTransport attribute be nullable?

partial interface RTCRtpReceiver : RTCStatsProvider {
readonly attribute RTCDtlsTransport? rtcpTransport;
};

  1. The RTCIceTransportController
    What if there aren't any ICE transports added yet? Does getTransports() return an empty list?
@aboba aboba added the 1.1 label Mar 16, 2016
aboba added a commit that referenced this issue Mar 16, 2016
Fix for Issue #433
@aboba aboba mentioned this issue Mar 16, 2016
@aboba aboba changed the title Methods that can return null (and attributes that can be null) Methods that can return empty lists (and nullable attributes) Mar 16, 2016
@robin-raymond
Copy link
Contributor

Throughout the specification, there appear to be issues with methods that can return null or attributes that can be null. For example:

  1. If getLocalCandidates() is called before gather() had been called, there are no local candidates. So can getLocalCandidates() return null?

sequence<RTCIceCandidate>? getLocalCandidates ();

I would suggest just returning an empty sequence rather than allowing null, but that's just me...

  1. gather() takes an optional RTCIceGatherOptions argument. Should that argument also be nullable?
    void gather (optional RTCIceGatherOptions? options);

No. Optional but not nullable.

  1. In the RTCIceTransport constructor, the RTCIceGatherer argument is optional, but not nullable. Should it also be nullable?

[ Constructor (optional RTCIceGatherer? gatherer)]

No, optional but not nullable.

Note that the iceGatherer attribute is nullable:
readonly attribute RTCIceGatherer? iceGatherer;

Correct, result is nullable. Allowing ? in the constructor would mean that a null value is legal to be specified and should have meaning. I think you don't want to allow ? in the constructor because you really want undefined to mean not specified and null as a valid value when the option is specified.

  1. Can getRemoteCandidates() return null if there aren't any remote candidates yet?
    sequence? getRemoteCandidates ();

Again, my personal preference is an empty sequence since there's no additional meaning value to allowing null.

  1. In RTCIceTransport.start(), should the ICE role be nullable as well as optional?
    void start (RTCIceGatherer gatherer, RTCIceParameters remoteParameters, optional RTCIceRole? role);

No. If specified, it can't be null.

  1. In getRemoteCertificates(), what if the remote certificates aren't available yet? Can getRemoteCandidates return null?
    sequence<ArrayBuffer>? getRemoteCertificates ();

Hmmm... null might have a different meaning if it means that no certificates are exchanged yet versus no certificates were present but since I don't see how it's possible to have a legal empty sequence then I would say that null is not required here (again personal preference).

  1. Should RTCDtlsTransportState be nullable in RTCDtlsTransportStateChangedEventInit?
dictionary RTCDtlsTransportStateChangedEventInit : EventInit {
             RTCDtlsTransportState? state;
};

I don't see why null should be legal here.

If so, why isn't it nullable in the interface?

[ Constructor (DOMString type, RTCDtlsTransportStateChangedEventInit eventInitDict)]
interface RTCDtlsTransportStateChangedEvent : Event {
    readonly        attribute RTCDtlsTransportState? state;
};

Think that was wrong.

  1. In the RtpSender, what if an rtcpTransport isn't provided? Should rtcpTransport be nullable in the constructor as well as optional? Should the rtcpTransport attribute be nullable?
[ Constructor (MediaStreamTrack track, RTCDtlsTransport transport, optional RTCDtlsTransport? rtcpTransport)]
partial interface RTCRtpSender : RTCStatsProvider {
    readonly        attribute RTCDtlsTransport? rtcpTransport;
    void                      setTransport (RTCDtlsTransport transport, optional RTCDtlsTransport? rtcpTransport);
};

No. I don't think it should be nullable.

  1. Similar questions for the RTCRtpReceiver:
[ Constructor (DOMString kind, RTCDtlsTransport transport, optional RTCDtlsTransport? rtcpTransport)]
partial interface RTCRtpReceiver : RTCStatsProvider {
    readonly        attribute RTCDtlsTransport? rtcpTransport;
    void                               setTransport (RTCDtlsTransport transport, optional RTCDtlsTransport? rtcpTransport);
    static RTCRtpCapabilities          getCapabilities (optional DOMString? kind);
};

Same as previous.

  1. The RTCIceTransportController
    What if there aren't any ICE transports added yet? can getTransports() return null? Should the optional constructor argument also be nullable?
[Constructor()]
    void                      addTransport (RTCIceTransport transport, optional unsigned long? index);
    sequence<RTCIceTransport>? getTransports ();
};

No, I don't think it should be nullable index. I prefer empty lists (personal preference).

@ibc
Copy link
Contributor

ibc commented Mar 17, 2016

If getLocalCandidates() is called before gather() had been called, there are no local candidates. So can getLocalCandidates() return null?
sequence? getLocalCandidates ();
I would suggest just returning an empty sequence rather than allowing null, but that's just me...

Agreed. Typically any API method that returns a sequence must always return a sequence, even if empty.

gather() takes an optional RTCIceGatherOptions argument. Should that argument also be nullable? void gather (optional RTCIceGatherOptions? options);
No. Optional but not nullable.

Agreed.

In the RTCIceTransport constructor, the RTCIceGatherer argument is optional, but not nullable. Should it also be nullable?
[ Constructor (optional RTCIceGatherer? gatherer)]
No, optional but not nullable.

Agreed.

Note that the iceGatherer attribute is nullable:
readonly attribute RTCIceGatherer? iceGatherer;
Correct, result is nullable. Allowing ? in the constructor would mean that a null value is legal to be specified and should have meaning. I think you don't want to allow ? in the constructor because you really want undefined to mean not specified and null as a valid value when the option is specified.

Agreed. An API method returning null means that such a property is not (yet) defined. That's not the same as allowing setting it to nulll in the constructor or a setter.

Can getRemoteCandidates() return null if there aren't any remote candidates yet? sequence? getRemoteCandidates ();
Again, my personal preference is an empty sequence since there's no additional meaning value to allowing null.

Agreed. Always return a sequence (avoid if (candidates && candidates.length) ...).

In RTCIceTransport.start(), should the ICE role be nullable as well as optional? void start (RTCIceGatherer gatherer, RTCIceParameters remoteParameters, optional RTCIceRole? role);
No. If specified, it can't be null.

Agreed.

In getRemoteCertificates(), what if the remote certificates aren't available yet? Can getRemoteCandidates return null? sequence? getRemoteCertificates ();
Hmmm... null might have a different meaning if it means that no certificates are exchanged yet versus no certificates were present but since I don't see how it's possible to have a legal empty sequence then I would say that null is not required here (again personal preference).

Return an empty sequence.

Should RTCDtlsTransportState be nullable in RTCDtlsTransportStateChangedEventInit?

dictionary RTCDtlsTransportStateChangedEventInit : EventInit {
             RTCDtlsTransportState? state;
};

I don't see why null should be legal here.

Agreed.

In the RtpSender, what if an rtcpTransport isn't provided? Should rtcpTransport be nullable in the constructor as well as optional? Should the rtcpTransport attribute be nullable?

[ Constructor (MediaStreamTrack track, RTCDtlsTransport transport, optional RTCDtlsTransport? rtcpTransport)]
partial interface RTCRtpSender : RTCStatsProvider {
    readonly        attribute RTCDtlsTransport? rtcpTransport;
    void                      setTransport (RTCDtlsTransport transport, optional RTCDtlsTransport? rtcpTransport);
};

No. I don't think it should be nullable.

Agreed. transport is mandatory and rtcpTransport is optional (not the same as nullable).

The RTCIceTransportController What if there aren't any ICE transports added yet? can getTransports() return null? Should the optional constructor argument also be nullable?
No, I don't think it should be nullable index. I prefer empty lists (personal preference).

Agreed. Always empty list.

@aboba aboba closed this as completed Mar 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants