Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2464243002:
Add Datachannel support to Android AppRTCMobile (Closed)
DescriptionAdd Datachannel support to Android AppRTCMobile
BUG=webrtc:6647
Committed: https://crrev.com/610c454cf9287a7f4ee9b2fbc56552f93dece09c
Cr-Commit-Position: refs/heads/master@{#15145}
Patch Set 1 #Patch Set 2 : modify authors #
Total comments: 36
Patch Set 3 : fix preferences.xml,PeerConnectionClient,CallActivity comments #Patch Set 4 : xml ordering #
Total comments: 9
Patch Set 5 : data channel patch set 4 #
Total comments: 4
Patch Set 6 : Fix patch set 5 #Patch Set 7 : fix settings #Patch Set 8 : Fix ClassCast when gettin shared int pref #Patch Set 9 : Fix ClassCast when gettin shared int pref [rebase] #Patch Set 10 : handle empty integer setting #
Total comments: 1
Patch Set 11 : prettify traces #
Total comments: 2
Patch Set 12 : restore videoCapturerStopped = true #
Messages
Total messages: 39 (8 generated)
Description was changed from ========== Add Datachannel support to Android AppRTCMobile. See issue: https://bugs.chromium.org/p/webrtc/issues/detail?id=6647 BUG=6647 ========== to ========== Add Datachannel support to Android AppRTCMobile. See issue: https://bugs.chromium.org/p/webrtc/issues/detail?id=6647 BUG=6647 ==========
hekra01@gmail.com changed reviewers: + glaznev@webrtc.org, magjed@webrtc.org, perkj@webrtc.org, tkchin@webrtc.org
Description was changed from ========== Add Datachannel support to Android AppRTCMobile. See issue: https://bugs.chromium.org/p/webrtc/issues/detail?id=6647 BUG=6647 ========== to ========== Add Datachannel support to Android AppRTCMobile BUG=webrtc:6647 ==========
hekra - Do you want us to review this? (you need to press 'Publish + Mail Draft Comments' in order to send out a notification)
Please review this. thanks
magjed@webrtc.org changed reviewers: + sakal@webrtc.org - glaznev@webrtc.org, perkj@webrtc.org, tkchin@webrtc.org
On 2016/11/09 09:41:41, magjed_webrtc wrote: > mailto:magjed@webrtc.org changed reviewers: > + mailto:sakal@webrtc.org > - mailto:glaznev@webrtc.org, mailto:perkj@webrtc.org, mailto:tkchin@webrtc.org I'm limiting the number of reviewers. You only need approval from me and sakal@.
Right now this code will unused, right? Do you have an use case for it in mind? https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... File webrtc/examples/androidapp/res/values/strings.xml (right): https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:155: <string name="pref_enable_datachannel_dlg">Enable datachannel.</string> Not used, please remove. https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:156: <string name="pref_enable_datachannel_default" translatable="true">true</string> This shouldn't be translatable. You can also add translatable="false" to other defaults you have added. We are incorrectly not specifying it on most of the defaults in this file. translatable="false" https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:160: <string name="pref_ordered_dlg">Order messages.</string> Not used, please remove. https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:161: <string name="pref_ordered_default" translatable="true">true</string> ditto https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:163: <string name="pref_data_protocol_key">Subprotocol</string> Please keep order in this file and in preferences.xml in sync. https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:166: <string name="pref_data_protocol_default" translatable="false">""</string> This is a very strange default value. https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:170: <string name="pref_negotiated_dlg">Negotiated.</string> Not used, please remove. https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:171: <string name="pref_negotiated_default" translatable="true">false</string> translatable="false" https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:176: <string name="pref_max_retransmit_time_ms_default">-1</string> translatable="false" https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:181: <string name="pref_max_retransmits_default">-1</string> translatable="false" https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:186: <string name="pref_data_id_default">-1</string> translatable="false" https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... File webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java (right): https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:17: import org.appspot.apprtc.util.LooperExecutor; Why did you add this? https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... File webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java (right): https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:608: videoCapturerStopped = true; This is probably correct but please do not add unrelated changes. https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:1144: Log.d(TAG, "NEW Data channel " + dc.label()); nit: Don't make new all caps https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:1150: public void onBufferedAmountChange(long var1){ nit: var1 is not a very descriptive name here. Maybe it should also be logged? How often is this callback triggered, does it spam the logs? https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:1175: Log.d(TAG, "Got msg: " + strData + " over " + dc); Maybe we should show the message in a toast? https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:1178: executor.execute(command); Why does this have to be posted?
On 2016/11/09 11:45:03, sakal wrote: > Right now this code will unused, right? Do you have an use case for it in mind? > > https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... > File webrtc/examples/androidapp/res/values/strings.xml (right): > > https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... > webrtc/examples/androidapp/res/values/strings.xml:155: <string > name="pref_enable_datachannel_dlg">Enable datachannel.</string> > Not used, please remove. > > https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... > webrtc/examples/androidapp/res/values/strings.xml:156: <string > name="pref_enable_datachannel_default" translatable="true">true</string> > This shouldn't be translatable. You can also add translatable="false" to other > defaults you have added. We are incorrectly not specifying it on most of the > defaults in this file. > > translatable="false" > > https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... > webrtc/examples/androidapp/res/values/strings.xml:160: <string > name="pref_ordered_dlg">Order messages.</string> > Not used, please remove. > > https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... > webrtc/examples/androidapp/res/values/strings.xml:161: <string > name="pref_ordered_default" translatable="true">true</string> > ditto > > https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... > webrtc/examples/androidapp/res/values/strings.xml:163: <string > name="pref_data_protocol_key">Subprotocol</string> > Please keep order in this file and in preferences.xml in sync. > > https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... > webrtc/examples/androidapp/res/values/strings.xml:166: <string > name="pref_data_protocol_default" translatable="false">""</string> > This is a very strange default value. > > https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... > webrtc/examples/androidapp/res/values/strings.xml:170: <string > name="pref_negotiated_dlg">Negotiated.</string> > Not used, please remove. > > https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... > webrtc/examples/androidapp/res/values/strings.xml:171: <string > name="pref_negotiated_default" translatable="true">false</string> > translatable="false" > > https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... > webrtc/examples/androidapp/res/values/strings.xml:176: <string > name="pref_max_retransmit_time_ms_default">-1</string> > translatable="false" > > https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... > webrtc/examples/androidapp/res/values/strings.xml:181: <string > name="pref_max_retransmits_default">-1</string> > translatable="false" > > https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... > webrtc/examples/androidapp/res/values/strings.xml:186: <string > name="pref_data_id_default">-1</string> > translatable="false" > > https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... > File webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java > (right): > > https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... > webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:17: import > org.appspot.apprtc.util.LooperExecutor; > Why did you add this? > > https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... > File webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java > (right): > > https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... > webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:608: > videoCapturerStopped = true; > This is probably correct but please do not add unrelated changes. > > https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... > webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:1144: > Log.d(TAG, "NEW Data channel " + dc.label()); > nit: Don't make new all caps > > https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... > webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:1150: > public void onBufferedAmountChange(long var1){ > nit: var1 is not a very descriptive name here. Maybe it should also be logged? > > How often is this callback triggered, does it spam the logs? > > https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... > webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:1175: > Log.d(TAG, "Got msg: " + strData + " over " + dc); > Maybe we should show the message in a toast? > > https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... > webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:1178: > executor.execute(command); > Why does this have to be posted? Comments acknowleged Datachannel could be used with https://github.com/webrtc/apprtc/pull/389 is integrated. The use case would be to have chat/file transfer in addition to A/V conferencing
First batch of Datachannel review comments taken into account https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... File webrtc/examples/androidapp/res/values/strings.xml (right): https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:155: <string name="pref_enable_datachannel_dlg">Enable datachannel.</string> On 2016/11/09 11:45:02, sakal wrote: > Not used, please remove. my bad. forgot to upload ConnectActivity which uses it. Now fixed https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:156: <string name="pref_enable_datachannel_default" translatable="true">true</string> On 2016/11/09 11:45:03, sakal wrote: > This shouldn't be translatable. You can also add translatable="false" to other > defaults you have added. We are incorrectly not specifying it on most of the > defaults in this file. > > translatable="false" Done. https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:160: <string name="pref_ordered_dlg">Order messages.</string> On 2016/11/09 11:45:02, sakal wrote: > Not used, please remove. ditto https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:161: <string name="pref_ordered_default" translatable="true">true</string> On 2016/11/09 11:45:03, sakal wrote: > ditto Done. https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:163: <string name="pref_data_protocol_key">Subprotocol</string> On 2016/11/09 11:45:03, sakal wrote: > Please keep order in this file and in preferences.xml in sync. Done. https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:166: <string name="pref_data_protocol_default" translatable="false">""</string> On 2016/11/09 11:45:02, sakal wrote: > This is a very strange default value. removed https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:170: <string name="pref_negotiated_dlg">Negotiated.</string> On 2016/11/09 11:45:03, sakal wrote: > Not used, please remove. same as before https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:171: <string name="pref_negotiated_default" translatable="true">false</string> On 2016/11/09 11:45:03, sakal wrote: > translatable="false" Done. https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:176: <string name="pref_max_retransmit_time_ms_default">-1</string> On 2016/11/09 11:45:03, sakal wrote: > translatable="false" Done. https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:181: <string name="pref_max_retransmits_default">-1</string> On 2016/11/09 11:45:02, sakal wrote: > translatable="false" Done. https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:186: <string name="pref_data_id_default">-1</string> On 2016/11/09 11:45:02, sakal wrote: > translatable="false" Done. https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... File webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java (right): https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:17: import org.appspot.apprtc.util.LooperExecutor; On 2016/11/09 11:45:03, sakal wrote: > Why did you add this? DatChannelParams for what is set in settings Looper removed https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... File webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java (right): https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:608: videoCapturerStopped = true; On 2016/11/09 11:45:03, sakal wrote: > This is probably correct but please do not add unrelated changes. Done. https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:1144: Log.d(TAG, "NEW Data channel " + dc.label()); On 2016/11/09 11:45:03, sakal wrote: > nit: Don't make new all caps Done. https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:1150: public void onBufferedAmountChange(long var1){ On 2016/11/09 11:45:03, sakal wrote: > nit: var1 is not a very descriptive name here. Maybe it should also be logged? > > How often is this callback triggered, does it spam the logs? Naming fixed No spam.Callback doeas not seem called when tested https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:1175: Log.d(TAG, "Got msg: " + strData + " over " + dc); On 2016/11/09 11:45:03, sakal wrote: > Maybe we should show the message in a toast? DataChannel use cases would be to enable chat/file transfer in addition to A/V. This callback can be a starting point to do this later. Currently , only toast would not be very useful for the end user, this is why I only log at the moment. Let me know if yo want toast all the same https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:1178: executor.execute(command); On 2016/11/09 11:45:03, sakal wrote: > Why does this have to be posted? Done because of comments in the PeerConnectionClient constructor: // Executor thread is started once in private ctor and is used for all // peer connection API calls to ensure new peer connection factory is // created on the same thread as previously destroyed factory.
Starting to look good. https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... File webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java (right): https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:1178: executor.execute(command); On 2016/11/10 02:32:20, hekra01 wrote: > On 2016/11/09 11:45:03, sakal wrote: > > Why does this have to be posted? > Done because of comments in the PeerConnectionClient constructor: > // Executor thread is started once in private ctor and is used for all > // peer connection API calls to ensure new peer connection factory is > // created on the same thread as previously destroyed factory. But you are not making any peerconnection API calls here? https://codereview.webrtc.org/2464243002/diff/60001/webrtc/examples/androidap... File webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java (right): https://codereview.webrtc.org/2464243002/diff/60001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java:478: //Get datachannel options nit: space after // https://codereview.webrtc.org/2464243002/diff/60001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java:478: //Get datachannel options Please move reading the preferences out of the if-statement to mirror with the other settings. Also, you should read the defaults from resources like rest of the settings. You can use existing helper methods to do that (sharedPrefGetString / sharedPrefGetBoolean). https://codereview.webrtc.org/2464243002/diff/60001/webrtc/examples/androidap... File webrtc/examples/androidapp/src/org/appspot/apprtc/SettingsActivity.java (right): https://codereview.webrtc.org/2464243002/diff/60001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/SettingsActivity.java:93: keyprefEnableDataChannel = getString(R.string.pref_enable_datachannel_key); nit: move before keyPrefRoomServerUrl to match order in the settings https://codereview.webrtc.org/2464243002/diff/60001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/SettingsActivity.java:145: nit: remove space and move all settings before keyPrefRoomServerUrl
review comment taken into account https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... File webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java (right): https://codereview.webrtc.org/2464243002/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:1178: executor.execute(command); On 2016/11/10 08:31:42, sakal wrote: > On 2016/11/10 02:32:20, hekra01 wrote: > > On 2016/11/09 11:45:03, sakal wrote: > > > Why does this have to be posted? > > Done because of comments in the PeerConnectionClient constructor: > > // Executor thread is started once in private ctor and is used for all > > // peer connection API calls to ensure new peer connection factory is > > // created on the same thread as previously destroyed factory. > > But you are not making any peerconnection API calls here? Correct. Removed the post https://codereview.webrtc.org/2464243002/diff/60001/webrtc/examples/androidap... File webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java (right): https://codereview.webrtc.org/2464243002/diff/60001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java:478: //Get datachannel options On 2016/11/10 08:31:42, sakal wrote: > nit: space after // Done. https://codereview.webrtc.org/2464243002/diff/60001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java:478: //Get datachannel options On 2016/11/10 08:31:42, sakal wrote: > nit: space after // Done. https://codereview.webrtc.org/2464243002/diff/60001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java:478: //Get datachannel options On 2016/11/10 08:31:42, sakal wrote: > Please move reading the preferences out of the if-statement to mirror with the > other settings. Also, you should read the defaults from resources like rest of > the settings. You can use existing helper methods to do that > (sharedPrefGetString / sharedPrefGetBoolean). Done. I re-added the pref_data_protocol_default in the strings.xml and preferences.xml. Empty string "" seems a valid default value for data protocol as stated in org.webrtc.DataChannel.Init and https://developer.mozilla.org/fr/docs/Web/API/RTCDataChannel https://codereview.webrtc.org/2464243002/diff/60001/webrtc/examples/androidap... File webrtc/examples/androidapp/src/org/appspot/apprtc/SettingsActivity.java (right): https://codereview.webrtc.org/2464243002/diff/60001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/SettingsActivity.java:93: keyprefEnableDataChannel = getString(R.string.pref_enable_datachannel_key); On 2016/11/10 08:31:42, sakal wrote: > nit: move before keyPrefRoomServerUrl to match order in the settings Done. https://codereview.webrtc.org/2464243002/diff/60001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/SettingsActivity.java:145: On 2016/11/10 08:31:42, sakal wrote: > nit: remove space and move all settings before keyPrefRoomServerUrl Done.
https://codereview.webrtc.org/2464243002/diff/80001/webrtc/examples/androidap... File webrtc/examples/androidapp/res/values/strings.xml (right): https://codereview.webrtc.org/2464243002/diff/80001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:166: <string name="pref_data_protocol_default" translatable="false">""</string> What I meant is that I think this should be: <string name="pref_data_protocol_default" translatable="false"></string> https://codereview.webrtc.org/2464243002/diff/80001/webrtc/examples/androidap... File webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java (right): https://codereview.webrtc.org/2464243002/diff/80001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java:457: int maxRetr = Integer.parseInt( I would like a new method sharedPrefGetInteger instead.
fixes for previous patch set https://codereview.webrtc.org/2464243002/diff/80001/webrtc/examples/androidap... File webrtc/examples/androidapp/res/values/strings.xml (right): https://codereview.webrtc.org/2464243002/diff/80001/webrtc/examples/androidap... webrtc/examples/androidapp/res/values/strings.xml:166: <string name="pref_data_protocol_default" translatable="false">""</string> On 2016/11/10 14:23:06, sakal wrote: > What I meant is that I think this should be: > <string name="pref_data_protocol_default" translatable="false"></string> Done. https://codereview.webrtc.org/2464243002/diff/80001/webrtc/examples/androidap... File webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java (right): https://codereview.webrtc.org/2464243002/diff/80001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java:457: int maxRetr = Integer.parseInt( On 2016/11/10 14:23:07, sakal wrote: > I would like a new method sharedPrefGetInteger instead. Done.
I tried this patch locally. Some of the checkboxes in the settings do not have summary below them. Summaries don't update correctly when the values are changed. Please see other settings for directions on how to implement this. App also crashes when starting a loopback call. Please fix.
On 2016/11/11 11:28:33, sakal wrote: > I tried this patch locally. Some of the checkboxes in the settings do not have > summary below them. Summaries don't update correctly when the values are > changed. Please see other settings for directions on how to implement this. > > App also crashes when starting a loopback call. Please fix. Settings fixed. Regarding the crash, when doing loopback, I observe the below trace (datachannel prevented in loopback), but no crash: E/libjingle: (peerconnection.cc:2055): InternalCreateDataChannel: Data is not supported in this call.
PTAL
I am still observing a crash on freshly installed copy when starting a loopback call. I believe you have to read a string from the shared preferences and parse it as an integer. The stack trace is: 11-14 14:07:20.398 17854 17854 E AndroidRuntime: java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Integer 11-14 14:07:20.398 17854 17854 E AndroidRuntime: at android.app.SharedPreferencesImpl.getInt(SharedPreferencesImpl.java:242) 11-14 14:07:20.398 17854 17854 E AndroidRuntime: at org.appspot.apprtc.ConnectActivity.sharedPrefGetInteger(ConnectActivity.java:307) 11-14 14:07:20.398 17854 17854 E AndroidRuntime: at org.appspot.apprtc.ConnectActivity.connectToRoom(ConnectActivity.java:469) 11-14 14:07:20.398 17854 17854 E AndroidRuntime: at org.appspot.apprtc.ConnectActivity.onOptionsItemSelected(ConnectActivity.java:209) 11-14 14:07:20.398 17854 17854 E AndroidRuntime: at android.app.Activity.onMenuItemSelected(Activity.java:3204) Also, this CL no longer applies to HEAD. Please rebase. (Please do the rebase in a separate patchset.)
I am still observing a crash on freshly installed copy when starting a loopback call. I believe you have to read a string from the shared preferences and parse it as an integer. The stack trace is: 11-14 14:07:20.398 17854 17854 E AndroidRuntime: java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Integer 11-14 14:07:20.398 17854 17854 E AndroidRuntime: at android.app.SharedPreferencesImpl.getInt(SharedPreferencesImpl.java:242) 11-14 14:07:20.398 17854 17854 E AndroidRuntime: at org.appspot.apprtc.ConnectActivity.sharedPrefGetInteger(ConnectActivity.java:307) 11-14 14:07:20.398 17854 17854 E AndroidRuntime: at org.appspot.apprtc.ConnectActivity.connectToRoom(ConnectActivity.java:469) 11-14 14:07:20.398 17854 17854 E AndroidRuntime: at org.appspot.apprtc.ConnectActivity.onOptionsItemSelected(ConnectActivity.java:209) 11-14 14:07:20.398 17854 17854 E AndroidRuntime: at android.app.Activity.onMenuItemSelected(Activity.java:3204) Also, this CL no longer applies to HEAD. Please rebase. (Please do the rebase in a separate patchset.)
On 2016/11/14 13:09:24, sakal wrote: > I am still observing a crash on freshly installed copy when starting a loopback > call. I believe you have to read a string from the shared preferences and parse > it as an integer. The stack trace is: > 11-14 14:07:20.398 17854 17854 E AndroidRuntime: java.lang.ClassCastException: > java.lang.String cannot be cast to java.lang.Integer > 11-14 14:07:20.398 17854 17854 E AndroidRuntime: at > android.app.SharedPreferencesImpl.getInt(SharedPreferencesImpl.java:242) > 11-14 14:07:20.398 17854 17854 E AndroidRuntime: at > org.appspot.apprtc.ConnectActivity.sharedPrefGetInteger(ConnectActivity.java:307) > 11-14 14:07:20.398 17854 17854 E AndroidRuntime: at > org.appspot.apprtc.ConnectActivity.connectToRoom(ConnectActivity.java:469) > 11-14 14:07:20.398 17854 17854 E AndroidRuntime: at > org.appspot.apprtc.ConnectActivity.onOptionsItemSelected(ConnectActivity.java:209) > 11-14 14:07:20.398 17854 17854 E AndroidRuntime: at > android.app.Activity.onMenuItemSelected(Activity.java:3204) > > Also, this CL no longer applies to HEAD. Please rebase. (Please do the rebase in > a separate patchset.) Done. Fix in Patch Set 8, rebase in Patch Set 9
PTAL. Uploaded fix & rebase in Patch sets 8 & 9
Setting empty value to one of the integers in settings, results in a crash when starting a call. Setting empty string should be prevented or empty value should lead to default value being used. Please fix.
On 2016/11/15 16:09:36, sakal wrote: > Setting empty value to one of the integers in settings, results in a crash when > starting a call. Setting empty string should be prevented or empty value should > lead to default value being used. Please fix. Done in patch set 10
The code is looking good to me but it would be nice if there was a way to test this functionality before landing it. https://codereview.webrtc.org/2464243002/diff/180001/webrtc/examples/androida... File webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java (right): https://codereview.webrtc.org/2464243002/diff/180001/webrtc/examples/androida... webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java:312: Log.e(TAG, "Wrong setting for :" + attributeName + ":" + value); nit: space after :, not before
On 2016/11/16 11:58:20, sakal wrote: > The code is looking good to me but it would be nice if there was a way to test > this functionality before landing it. > > https://codereview.webrtc.org/2464243002/diff/180001/webrtc/examples/androida... > File webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java > (right): > > https://codereview.webrtc.org/2464243002/diff/180001/webrtc/examples/androida... > webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java:312: > Log.e(TAG, "Wrong setting for :" + attributeName + ":" + value); > nit: space after :, not before Fixed traces in ps11. Regarding testing, -I do it manually using this PR https://github.com/webrtc/apprtc/pull/389. I send strings from one peer (web or android) and check they appear in the traces of the other peer. -If you have a way to automate functional testing of AppRTCMobile, let me know.
Okay, lgtm. magjed, PTAL
https://codereview.webrtc.org/2464243002/diff/200001/webrtc/examples/androida... File webrtc/examples/androidapp/res/xml/preferences.xml (right): https://codereview.webrtc.org/2464243002/diff/200001/webrtc/examples/androida... webrtc/examples/androidapp/res/xml/preferences.xml:173: android:key="@string/pref_data_protocol_key" Do we really need to expose all these data settings at this point? What use case do we have that needs anything else than the default settings? https://codereview.webrtc.org/2464243002/diff/200001/webrtc/examples/androida... File webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java (left): https://codereview.webrtc.org/2464243002/diff/200001/webrtc/examples/androida... webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:543: videoCapturerStopped = true; Don't remove this.
On 2016/11/17 10:51:05, magjed_webrtc wrote: > https://codereview.webrtc.org/2464243002/diff/200001/webrtc/examples/androida... > File webrtc/examples/androidapp/res/xml/preferences.xml (right): > > https://codereview.webrtc.org/2464243002/diff/200001/webrtc/examples/androida... > webrtc/examples/androidapp/res/xml/preferences.xml:173: > android:key="@string/pref_data_protocol_key" > Do we really need to expose all these data settings at this point? What use case > do we have that needs anything else than the default settings? > > https://codereview.webrtc.org/2464243002/diff/200001/webrtc/examples/androida... > File webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java > (left): > > https://codereview.webrtc.org/2464243002/diff/200001/webrtc/examples/androida... > webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:543: > videoCapturerStopped = true; > Don't remove this. Restored restore videoCapturerStopped = true in patch set 12 Regarding the data settings, they are all exposed as public to the application in org.webrtc.Datachannel.Init. This is why I dont select which ones to hide. Having them in settings could be useful for testing purpose or real use case (ordered = true for chat, ordered = false for file transfers, etc...)
On 2016/11/17 13:25:25, hekra01 wrote: > On 2016/11/17 10:51:05, magjed_webrtc wrote: > > > https://codereview.webrtc.org/2464243002/diff/200001/webrtc/examples/androida... > > File webrtc/examples/androidapp/res/xml/preferences.xml (right): > > > > > https://codereview.webrtc.org/2464243002/diff/200001/webrtc/examples/androida... > > webrtc/examples/androidapp/res/xml/preferences.xml:173: > > android:key="@string/pref_data_protocol_key" > > Do we really need to expose all these data settings at this point? What use > case > > do we have that needs anything else than the default settings? > > > > > https://codereview.webrtc.org/2464243002/diff/200001/webrtc/examples/androida... > > File > webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java > > (left): > > > > > https://codereview.webrtc.org/2464243002/diff/200001/webrtc/examples/androida... > > > webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:543: > > videoCapturerStopped = true; > > Don't remove this. > > Restored restore videoCapturerStopped = true in patch set 12 > Regarding the data settings, they are all exposed as public to the application > in org.webrtc.Datachannel.Init. > This is why I dont select which ones to hide. > Having them in settings could be useful for testing purpose or real use case > (ordered = true for chat, ordered = false for file transfers, etc...) I understand that they might be needed in the future, but I think it was an overkill to expose them all at this point. The settings and code for AppRTCMobile is already bloated, so I think it would have been better to lazily add just what you need. But since you have gone through all the trouble to do add them, I will accept it this time, but don't add more unnecessary settings in the future.
lgtm
Thanks I am not a commiter. Could you please submit for me?
Thanks I am not a commiter. Could you please submit for me?
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sakal@webrtc.org Link to the patchset: https://codereview.webrtc.org/2464243002/#ps220001 (title: "restore videoCapturerStopped = true")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Add Datachannel support to Android AppRTCMobile BUG=webrtc:6647 ========== to ========== Add Datachannel support to Android AppRTCMobile BUG=webrtc:6647 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add Datachannel support to Android AppRTCMobile BUG=webrtc:6647 ========== to ========== Add Datachannel support to Android AppRTCMobile BUG=webrtc:6647 Committed: https://crrev.com/610c454cf9287a7f4ee9b2fbc56552f93dece09c Cr-Commit-Position: refs/heads/master@{#15145} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/610c454cf9287a7f4ee9b2fbc56552f93dece09c Cr-Commit-Position: refs/heads/master@{#15145} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
