[WebSocket][Android] make protocols argument work fixes #5810 #6223

zxcpoiu · 2016-03-01T08:38:57Z

Hey there and thanks for submitting a pull request! Please have a look at the following checklist so that others have enough information to review your pull request:

motivation

WebSocket spec supports Sec-WebSocket-Protocol as a standard way for negotiate a sub protocol between client and server.

ios WebSocket implementation supports it.
android WebSocket implementation ignores this header, leave a comment syas: "OkHttp will overrides it", so it did not implement.
after some test, OkHttp doesn't override the header we add.

Test plan (required)

run and react-native app on android
at the main page, invoke: var ws = new WebSocket('ws://example.ws-service.fakedomain.com', 'my-sub-protocol');
see the header if it send the correct header, ex, use ngrep: sudo ngrep -t -Wbyline -deth0 host example.ws-service.fakedomain.com and port 80

you should see the WebSocket initial GET handshake includes header:
Sec-WebSocket-Protocol: my-sub-protocol
and the value should equals to the value you provided in js.

Closes #5810

facebook-github-bot · 2016-03-01T08:39:20Z

By analyzing the blame information on this pull request, we identified @aprock and @satya164 to be potential reviewers.

zxcpoiu · 2016-03-05T08:49:40Z

NOTE: this PR does NOT change any api NOR add any new functionality.

It is just only un-comment the original implementation which considered okHttp will swallow headers we added thus commented out the protocols argument.

facebook-github-bot · 2016-03-10T15:52:19Z

@zxcpoiu updated the pull request.

satya164 · 2016-03-10T22:21:42Z

ReactAndroid/src/main/java/com/facebook/react/modules/websocket/WebSocketModule.java

+      StringBuilder protocolsValue = new StringBuilder("");
+      for (int i = 0; i < protocols.size(); i++) {
+        String v = protocols.getString(i).trim();
+        if (!TextUtils.isEmpty(v) && !v.contains(",")) {


Why not use String.isEmpty?

thanks, it can be avoided to import additional TextUtils.

Just a side note:

String.isEmpty()
only checks length of string and introduced since API level 9 (Android 2.3)
so it maybe caused some crashes on earlier versions of Android.

TextUtils.isEmpty()
checks both length of string and null.
is available since API level 1

@zxcpoiu It's okay, because we only support Android 4.1 and above.

satya164 · 2016-03-10T22:27:58Z

Related #6016

christopherdro · 2016-03-11T04:13:36Z

@zxcpoiu I'm wondering if my PR thats linked here will work since it allows you to specify custom headers.

zxcpoiu · 2016-03-11T06:33:16Z

Hi @christopherdro

thanks for the great PR, it really helps!
And, yes, of course your PR can solve any additional headers issues.

But I think the reason that original WebSocket implementation left an argument protocols is to compatible with the web spec which requires:
var ws = new WebSocket(url: string, protocols: ?Array<string>)

So I guess it is related PR but not overlaps.

Original implementation protocols argument
WebSocketModule.java#L66-L67

Libraries/WebSocket/WebSocket.js#L36-L39

facebook-github-bot · 2016-03-11T09:54:12Z

@zxcpoiu updated the pull request.

zxcpoiu · 2016-03-11T10:00:59Z

@satya164

I've refined the PR by:

remove import TextUtils
use String.isEmpty() and StringBuilder.length() rather than TextUtils.isEmpty()

diff about this change

diff --git a/ReactAndroid/src/main/java/com/facebook/react/modules/websocket/WebSocketModule.java b/ReactAndroid/src/main/java/com/facebook/react/modules/websocket/WebSocketModule.java
index ea2329a..af4f092 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/modules/websocket/WebSocketModule.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/modules/websocket/WebSocketModule.java
@@ -38,8 +38,6 @@ import java.util.HashMap;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;

-import android.text.TextUtils;
-
 import okio.Buffer;
 import okio.BufferedSource;

@@ -91,12 +89,12 @@ public class WebSocketModule extends ReactContextBaseJavaModule {
       StringBuilder protocolsValue = new StringBuilder("");
       for (int i = 0; i < protocols.size(); i++) {
         String v = protocols.getString(i).trim();
-        if (!TextUtils.isEmpty(v) && !v.contains(",")) {
+        if (!v.isEmpty() && !v.contains(",")) {
           protocolsValue.append(v);
           protocolsValue.append(",");
         }
       }
-      if (!TextUtils.isEmpty(protocolsValue)) {
+      if (protocolsValue.length() > 0) {
         protocolsValue.replace(protocolsValue.length() - 1, protocolsValue.length(), "");
         builder.addHeader("Sec-WebSocket-Protocol", protocolsValue.toString());
       }

christopherdro · 2016-03-11T14:58:18Z

@zxcpoiu Just a heads up that my related PR just got shipped.

zxcpoiu · 2016-03-11T15:20:46Z

@christopherdro
hurrah!!
great thanks!!

facebook-github-bot · 2016-03-16T05:47:17Z

@zxcpoiu updated the pull request.

zxcpoiu · 2016-03-16T05:51:44Z

fixed conflicts follow up #6016

zxcpoiu · 2016-04-05T11:41:59Z

Hi @satya164
Is there anything I need to revise?

satya164 · 2016-04-06T15:15:29Z

@christopherdro Do you think this is good to go?

christopherdro · 2016-04-06T15:20:46Z

satya This looks good to me. Just needs to be rebased to master.

zxcpoiu · 2016-04-06T15:46:32Z

thanks @satya164 @christopherdro
rebased!

…acebook#5810

facebook-github-bot · 2016-04-11T08:31:27Z

@zxcpoiu updated the pull request.

christopherdro · 2016-04-11T15:45:45Z

@facebook-github-bot shipit

facebook-github-bot · 2016-04-11T15:55:28Z

Thanks for importing. If you are an FB employee go to Phabricator to review.

Summary:Hey there and thanks for submitting a pull request! Please have a look at the following checklist so that others have enough information to review your pull request: **motivation** WebSocket spec supports [Sec-WebSocket-Protocol](https://tools.ietf.org/html/rfc6455#section-11.3.4) as a standard way for negotiate a sub protocol between client and server. * ios WebSocket implementation supports it. * android WebSocket implementation ignores this header, leave a comment syas: "OkHttp will overrides it", so it did not implement. * after some test, OkHttp doesn't override the header we add. **Test plan (required)** 1. run and react-native app on android 2. at the main page, invoke: `var ws = new WebSocket('ws://example.ws-service.fakedomain.com', 'my-sub-protocol');` 3. see the header if it send the correct header, ex, use ngrep: `sudo ngrep -t -Wbyline -deth0 host example.ws-service.fakedomain.com and port 80` you should see the WebSocket initial GET handshake includes header: `Sec-WebSocke Closes facebook#6223 Differential Revision: D3162822 fb-gh-sync-id: a00f1c0f3e1c24ad6aa234329cbb2abad7664264 fbshipit-source-id: a00f1c0f3e1c24ad6aa234329cbb2abad7664264

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Mar 1, 2016

zxcpoiu changed the title ~~[WebSocket][Android] Support WebSocket sub-protocols on Android fixes…~~ [WebSocket][Android] Support WebSocket sub-protocols fixes #5810 Mar 1, 2016

zxcpoiu force-pushed the master branch from f69c834 to 0b54084 Compare March 4, 2016 05:11

zxcpoiu force-pushed the master branch from 0b54084 to 37cee34 Compare March 10, 2016 15:51

satya164 reviewed Mar 10, 2016
View reviewed changes

zxcpoiu force-pushed the master branch from 37cee34 to b8d866b Compare March 11, 2016 09:53

zxcpoiu force-pushed the master branch from b8d866b to 9717e60 Compare March 11, 2016 09:54

zxcpoiu force-pushed the master branch 2 times, most recently from fb80688 to d687c4a Compare March 16, 2016 05:47

zxcpoiu force-pushed the master branch from d687c4a to 1c551e9 Compare March 23, 2016 05:42

zxcpoiu force-pushed the master branch from 1c551e9 to d5bd550 Compare April 5, 2016 09:15

zxcpoiu force-pushed the master branch from d5bd550 to 6842da5 Compare April 6, 2016 15:45

zxcpoiu force-pushed the master branch 2 times, most recently from 286e633 to 6999fe2 Compare April 6, 2016 15:57

ianlin mentioned this pull request Apr 7, 2016

Expose WebSocket protocol getter attribute on iOS #6137

Closed

[WebSocket][Android] Support WebSocket sub-protocols on Android fixes f…

f88ca4c

…acebook#5810

zxcpoiu force-pushed the master branch from 6999fe2 to f88ca4c Compare April 11, 2016 08:29

zxcpoiu changed the title ~~[WebSocket][Android] Support WebSocket sub-protocols fixes #5810~~ [WebSocket][Android] make protocols argument work fixes #5810 fix #6137 Apr 11, 2016

facebook-github-bot added GH Review: accepted and removed GH Review: review-needed labels Apr 11, 2016

ghost closed this in 914f33c Apr 11, 2016

zxcpoiu changed the title ~~[WebSocket][Android] make protocols argument work fixes #5810 fix #6137~~ [WebSocket][Android] make protocols argument work fixes #5810 Apr 11, 2016

This pull request was closed.

[WebSocket][Android] make protocols argument work fixes #5810 #6223

[WebSocket][Android] make protocols argument work fixes #5810 #6223

Uh oh!

Conversation

zxcpoiu commented Mar 1, 2016

Uh oh!

facebook-github-bot commented Mar 1, 2016

Uh oh!

zxcpoiu commented Mar 5, 2016

Uh oh!

facebook-github-bot commented Mar 10, 2016

Uh oh!

satya164 Mar 10, 2016

Choose a reason for hiding this comment

Uh oh!

zxcpoiu Mar 11, 2016

Choose a reason for hiding this comment

Uh oh!

satya164 Mar 11, 2016

Choose a reason for hiding this comment

Uh oh!

satya164 commented Mar 10, 2016

Uh oh!

christopherdro commented Mar 11, 2016

Uh oh!

zxcpoiu commented Mar 11, 2016

Uh oh!

facebook-github-bot commented Mar 11, 2016

Uh oh!

zxcpoiu commented Mar 11, 2016

Uh oh!

christopherdro commented Mar 11, 2016

Uh oh!

zxcpoiu commented Mar 11, 2016

Uh oh!

facebook-github-bot commented Mar 16, 2016

Uh oh!

zxcpoiu commented Mar 16, 2016

Uh oh!

zxcpoiu commented Apr 5, 2016

Uh oh!

satya164 commented Apr 6, 2016

Uh oh!

christopherdro commented Apr 6, 2016

Uh oh!

zxcpoiu commented Apr 6, 2016

Uh oh!

facebook-github-bot commented Apr 11, 2016

Uh oh!

christopherdro commented Apr 11, 2016

Uh oh!

facebook-github-bot commented Apr 11, 2016

Uh oh!

Uh oh!