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.

Sign up for GitHub

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

Jump to bottom

rtcicecandidate: add relayProtocol #2763

Merged
merged 4 commits into from
Sep 22, 2022
Merged

rtcicecandidate: add relayProtocol #2763

merged 4 commits into from
Sep 22, 2022

Conversation

fippo
Copy link
Contributor

@fippo fippo commented Aug 16, 2022 •
edited by pr-preview bot
Loading

which is already defined in webrtc-stats
Diff

@fippo
Copy link
Contributor Author

fippo commented Aug 16, 2022

This is mostly a consistency issue between the candidate object and the candidate stats, see here for details.

I did consider (and actually implemented) .url as well but that is already defined to be part of the icecandidate event.
Also this is polyfillable (at least in some browsers, Firefox still uses the same priority for tcp and tls?)

fippo added a commit to webrtcHacks/adapter that referenced this pull request Aug 16, 2022
@fippo
chrome/safari: shim RTCIceCandidate.relayProtocol
e50a0af 
based on the hardcoded mapping of local type preference.
Polyfill for
  w3c/webrtc-pc#2763
alvestrand
alvestrand reviewed Aug 18, 2022
View reviewed changes
@alvestrand
Copy link
Contributor

Editors meeting: Seems uncontroversial.
Might want to call attention to it somehow before merging (posting to ML, or adding to a meeting agenda).

@fippo
rtcicecandidate: add relayProtocol
7e61f68 
which is already defined in webrtc-stats
  
  
          
  


  
    
  
  

    
  fippo 

      added a commit
        to fippo/webrtc-stats
      that referenced
      this pull request

    
      Aug 19, 2022
    
    

      


  

    

      


  

      
        @fippo
  





      

        
          editorial: clarify relayProtocol stats
        

          
            
          

      


      

          

    
    
    

  

      


      

      


      
      

        
          474693e
        
      

    

  

    

      
following
  w3c/webrtc-pc#2763

    




    

  




  


  
    
  
  

    
  fippo 

      added a commit
        to fippo/webrtc-stats
      that referenced
      this pull request

    
      Aug 19, 2022
    
    

      


  

    

      


  

      
        @fippo
  





      

        
          editorial: clarify relayProtocol stats
        

          
            
          

      


      

          

    
    
    

  

      


      

      


      
      

        
          9e96fc2
        
      

    

  

    

      
following
  w3c/webrtc-pc#2763

    




    

  




  

  

    
  


  

      

    @fippo
fippo



    mentioned this pull request
    
      Aug 19, 2022
    





  


  

      
   Merged
jan-ivar
jan-ivar requested changes Sep 1, 2022
View reviewed changes
webrtc.html Outdated
@@ -5976,6 +5976,7 @@ <h4>
readonly attribute DOMString? relatedAddress;
readonly attribute unsigned short? relatedPort;
readonly attribute DOMString? usernameFragment;
readonly attribute DOMString? relayProtocol;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a readonly attribute with predefined values it should probably be an enum.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you define the possible values? https://www.rfc-editor.org/rfc/rfc5928#section-3 is the closest one I could find (Table 1) but that is hardly normative

Copy link
Member

@jan-ivar jan-ivar Sep 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now the PR defines the possible values in prose: "Valid values are "udp", "tcp", or "tls". " which seems equivalent to:

enum RTCIceTransportPolicy {
  "udp",
  "tcp",
  "tls",
};

interface RTCIceCandidate {
  ...
  readonly attribute RTCIceTransportPolicy? relayProtocol;
};

If those are the only valid values then we generally prefer expressing things in WebIDL when possible.

If those are not the only values then we should rephrase.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If those are the only valid values then we generally prefer expressing things in WebIDL when possible.

Or at least that's been my impression. These are only ever used as output, not input so I guess it doesn't matter a whole lot (do we get any free idlharness wpt tests)? I see the stats spec is just using DOMString, and I couldn't find any design principles on this. The WebIDL spec showed no examples of a readonly attribute using enum... YMMV

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but for the other enums we have normative references from which we can draw the values (e.g. For stats this is actually a TODO hidden here:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RTCIceTcpCandidateType is sort of similar here.
Having WebIDL enum is a small UA developer improvement as it can automate some binding generated code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RTCIceServerTransportProtocol won against RTCIceTurnRelayProtocol and variants thereof.

@fippo fippo requested a review from jan-ivar September 20, 2022 15:48
jan-ivar
jan-ivar approved these changes Sep 22, 2022
View reviewed changes
@aboba
Copy link
Contributor

aboba commented Sep 22, 2022 •
edited
Loading

Reference should be to RFC 8656 Section 3.1 (which also defines DTLS transport); RFC 5766 is obsolete.

jan-ivar
jan-ivar reviewed Sep 22, 2022
View reviewed changes
@jan-ivar jan-ivar added the Needs Test Needs a WPT test label Sep 22, 2022
@aboba aboba merged commit 3d94785 into w3c:main Sep 22, 2022
@fippo fippo deleted the candidate branch September 22, 2022 15:15
@fippo fippo mentioned this pull request Sep 23, 2022
Merged
dontcallmedom added a commit that referenced this pull request Sep 27, 2022
@dontcallmedom
#2753 #2759 #…
cc9770d 
#2767
vr000m pushed a commit to w3c/webrtc-stats that referenced this pull request Oct 4, 2022
@fippo
#657)
ad722e9 
* editorial: clarify relayProtocol stats

following
  w3c/webrtc-pc#2763

* use RTCIceServerTransportProtocol
@jan-ivar jan-ivar mentioned this pull request Nov 18, 2022
Open
@dontcallmedom dontcallmedom mentioned this pull request Nov 15, 2023
Merged
23 tasks
This was referenced Jan 30, 2024
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alvestrand alvestrand alvestrand left review comments

@jan-ivar jan-ivar jan-ivar approved these changes

@youennf youennf youennf left review comments

Assignees
No one assigned
Labels
Needs Test Needs a WPT test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fippo @alvestrand @aboba @jan-ivar @youennf

Follow Lee on X/Twitter - Father, Husband, Serial builder creating AI, crypto, games & web tools. We are friends :) AI Will Come To Life!

Check out: eBank.nz (Art Generator) | Netwrck.com (AI Tools) | Text-Generator.io (AI API) | BitBank.nz (Crypto AI) | ReadingTime (Kids Reading) | RewordGame | BigMultiplayerChess | WebFiddle | How.nz | Helix AI Assistant