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

RTCIceGathererIceErrorEvent hostCandidate #474

Closed
robin-raymond opened this Issue Apr 19, 2016 · 6 comments

Comments

Projects
None yet
2 participants
@robin-raymond
Contributor

robin-raymond commented Apr 19, 2016

We have defined hostCandidate as a DOMString:

hostCandidate of type DOMString
The hostCandidate attribute is the local IP address and port used to communicate with the STUN or TURN server.

partial dictionary RTCIceGathererIceErrorEventInit : EventInit {
             DOMString      hostCandidate;
};

But shouldn't it be the actual structure:

partial dictionary RTCIceGathererIceErrorEventInit : EventInit {
             RTCIceGatherCandidate hostCandidate;
};

Seems it would be easier to correlate if we made an exact description.

Also, what would happen if there are no host candidates (privacy filter)?

enum RTCIceGatherPolicy {
    "all",
    "nohost",
    "relay"
};

nohost
The RTCIceGatherer gathers all ICE candidate types except for host candidates.

@robin-raymond robin-raymond added the 1.1 label Apr 19, 2016

@robin-raymond

This comment has been minimized.

Show comment
Hide comment
@robin-raymond

robin-raymond Apr 19, 2016

Contributor

Also last WebRTC has differences in definition...

WebRTC:

dictionary RTCPeerConnectionIceErrorEventInit : EventInit {
             DOMString      hostCandidate;
             DOMString      url;
             unsigned short errorCode;
             USVString      statusText;
};

[ Constructor (DOMString type, RTCPeerConnectionIceErrorEventInit eventInitDict)]
interface RTCPeerConnectionIceErrorEvent : Event {
    readonly        attribute DOMString      hostCandidate;
    readonly        attribute DOMString      url;
    readonly        attribute unsigned short errorCode;
    readonly        attribute USVString      errorText;
};

ORTC:

dictionary RTCIceGathererIceErrorEventInit : EventInit {
             DOMString      hostCandidate;
             DOMString      url;
             unsigned short errorCode;
             USVString      errorText;
};

[ Constructor (DOMString type, RTCIceGathererIceErrorEventInit eventInitDict)]
interface RTCIceGathererIceErrorEvent : Event {
    readonly        attribute DOMString      hostCandidate;
    readonly        attribute DOMString      url;
    readonly        attribute unsigned short errorCode;
    readonly        attribute USVString      errorText;
};

I think ti's WebRTC that has the mistake with statusText...

Contributor

robin-raymond commented Apr 19, 2016

Also last WebRTC has differences in definition...

WebRTC:

dictionary RTCPeerConnectionIceErrorEventInit : EventInit {
             DOMString      hostCandidate;
             DOMString      url;
             unsigned short errorCode;
             USVString      statusText;
};

[ Constructor (DOMString type, RTCPeerConnectionIceErrorEventInit eventInitDict)]
interface RTCPeerConnectionIceErrorEvent : Event {
    readonly        attribute DOMString      hostCandidate;
    readonly        attribute DOMString      url;
    readonly        attribute unsigned short errorCode;
    readonly        attribute USVString      errorText;
};

ORTC:

dictionary RTCIceGathererIceErrorEventInit : EventInit {
             DOMString      hostCandidate;
             DOMString      url;
             unsigned short errorCode;
             USVString      errorText;
};

[ Constructor (DOMString type, RTCIceGathererIceErrorEventInit eventInitDict)]
interface RTCIceGathererIceErrorEvent : Event {
    readonly        attribute DOMString      hostCandidate;
    readonly        attribute DOMString      url;
    readonly        attribute unsigned short errorCode;
    readonly        attribute USVString      errorText;
};

I think ti's WebRTC that has the mistake with statusText...

@aboba

This comment has been minimized.

Show comment
Hide comment
@aboba

aboba Apr 20, 2016

Contributor

Yes, WebRTC has the error with statusText.

If RTCIceGatherPolicy === "nohost" then no host candidates would be gathered but the host candidate would still exist and be filled in within the dictionary.

Contributor

aboba commented Apr 20, 2016

Yes, WebRTC has the error with statusText.

If RTCIceGatherPolicy === "nohost" then no host candidates would be gathered but the host candidate would still exist and be filled in within the dictionary.

@aboba

This comment has been minimized.

Show comment
Hide comment
@aboba

aboba Apr 20, 2016

Contributor

All we want here is the IP address and port of the host candidate, not everything that is in the RTCIceGatherCandidate object:

dictionary RTCIceCandidate {
DOMString foundation;
unsigned long priority;
DOMString ip;
RTCIceProtocol protocol;
unsigned short port;
RTCIceCandidateType type;
RTCIceTcpCandidateType tcpType;
DOMString relatedAddress = "";
unsigned short relatedPort;
};

Contributor

aboba commented Apr 20, 2016

All we want here is the IP address and port of the host candidate, not everything that is in the RTCIceGatherCandidate object:

dictionary RTCIceCandidate {
DOMString foundation;
unsigned long priority;
DOMString ip;
RTCIceProtocol protocol;
unsigned short port;
RTCIceCandidateType type;
RTCIceTcpCandidateType tcpType;
DOMString relatedAddress = "";
unsigned short relatedPort;
};

@robin-raymond

This comment has been minimized.

Show comment
Hide comment
@robin-raymond

robin-raymond Apr 20, 2016

Contributor

@aboba Assuming we need the host information at all (which is a different justification), then I would say a pointer to the candidate object (not duplicate of the information contained within) is needed to more easily correlate fully, not just the DOMString; unless the assumption is that this will only happen with UDP-based host candidates, never TCP, and thus there's enough to disambiguate with just the host ip+port string (probably true I'm guessing). But then again, I'm not sure what the original justification was about...

Contributor

robin-raymond commented Apr 20, 2016

@aboba Assuming we need the host information at all (which is a different justification), then I would say a pointer to the candidate object (not duplicate of the information contained within) is needed to more easily correlate fully, not just the DOMString; unless the assumption is that this will only happen with UDP-based host candidates, never TCP, and thus there's enough to disambiguate with just the host ip+port string (probably true I'm guessing). But then again, I'm not sure what the original justification was about...

@robin-raymond

This comment has been minimized.

Show comment
Hide comment
@robin-raymond

robin-raymond Apr 21, 2016

Contributor
dictionary RTCIceGathererIceErrorEventInit : EventInit {
             RTCIceCandidate? hostCandidate;
             DOMString      url;
             unsigned short errorCode;
             USVString      statusText;
};

If unreachable, the hostCandidate would be null since all hosts are unreachable to the destination. Likewise, the hostCandidate would be null if the browser is in a privacy mode disallowing emitting of host candidates.

Contributor

robin-raymond commented Apr 21, 2016

dictionary RTCIceGathererIceErrorEventInit : EventInit {
             RTCIceCandidate? hostCandidate;
             DOMString      url;
             unsigned short errorCode;
             USVString      statusText;
};

If unreachable, the hostCandidate would be null since all hosts are unreachable to the destination. Likewise, the hostCandidate would be null if the browser is in a privacy mode disallowing emitting of host candidates.

aboba added a commit that referenced this issue Apr 21, 2016

@aboba aboba added the PR exists label Apr 21, 2016

@robin-raymond

This comment has been minimized.

Show comment
Hide comment
@robin-raymond

robin-raymond Apr 22, 2016

Contributor

Great.

Contributor

robin-raymond commented Apr 22, 2016

Great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment