Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2753963002:
Refactoring and rewriting the chromoting jni instance to be chromoting session. (Closed)
DescriptionRefactoring and rewriting the chromoting jni instance to be chromoting session.
This will allow Android and iOS to share the majority of code related to session
and instance management. The setup flow for each client will be very similar,
this change will allow the iOS app to reuse the session management (xmpp,
client, etc) that was originally written for the JNI layer of Android.
Adding a session delegate allows for reuse and moves most of the code into
a common place and leaves the java/JNI only bits in the JNI layer.
BUG=699788
Review-Url: https://codereview.chromium.org/2753963002
Cr-Commit-Position: refs/heads/master@{#462154}
Committed: https://chromium.googlesource.com/chromium/src/+/a6be869a25aa4de4c6b4629a1b710e5d34f8fd34
Patch Set 1 #Patch Set 2 : A build file change snuck in here. Removing. #Patch Set 3 : Removing change. #Patch Set 4 : Merged with master. #Patch Set 5 : Fixing jni_client, missing the android audio member. #Patch Set 6 : Pointer not obj. #
Total comments: 38
Patch Set 7 : Update based on feedback. #Patch Set 8 : Merge with master. #
Total comments: 4
Patch Set 9 : Jni Runtime Delegate needed a weakptr factory. #Patch Set 10 : Never included JniRuntimeDelegate, fixed. #Patch Set 11 : WeakPtr Factory now makes type ChromotingClientRuntime::Delegate. #
Total comments: 2
Patch Set 12 : JNI client is the session delegate. #
Total comments: 1
Patch Set 13 : Release audio_player on the network thread. #
Total comments: 4
Patch Set 14 : Moving dep on dom_keycode_converter to client. #Patch Set 15 : Fixing override missing per chromium style. #Patch Set 16 : Reworking where stats logs are printed to the log file. #Patch Set 17 : Fixed feedback. #
Total comments: 32
Patch Set 18 : Updating based on feedback. #Messages
Total messages: 161 (134 generated)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Refactoring and rewriting the chromoting jni instance to be chromoting session. This will allow Android and iOS to share the majority of code related to session and instance management. The setup flow for each client will be very similar, this change will allow the iOS app to reuse the session management (xmpp, client, etc) that was originally written for the JNI layer of Android. Adding a session delegate allows for reuse and moves most of the code into a common place and leaves the java/JNI only bits in the JNI layer. BUG=699788 ========== to ========== Refactoring and rewriting the chromoting jni instance to be chromoting session. This will allow Android and iOS to share the majority of code related to session and instance management. The setup flow for each client will be very similar, this change will allow the iOS app to reuse the session management (xmpp, client, etc) that was originally written for the JNI layer of Android. Adding a session delegate allows for reuse and moves most of the code into a common place and leaves the java/JNI only bits in the JNI layer. BUG=699788 ==========
nicholss@chromium.org changed reviewers: + lambroslambrou@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
nicholss@chromium.org changed reviewers: + yuweih@chromium.org
PTAL, I will address comments next week. Wanted to give a chance for review. Thanks!
https://codereview.chromium.org/2753963002/diff/140001/remoting/BUILD.gn File remoting/BUILD.gn (right): https://codereview.chromium.org/2753963002/diff/140001/remoting/BUILD.gn#newc... remoting/BUILD.gn:172: libs += [ "android" ] Why is this needed? https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... File remoting/client/chromoting_session.cc (right): https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_session.cc:51: base::WeakPtr<protocol::AudioStub> audio_player, Could we keep those stuff either all WeakPtr or all unique_ptr? https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_session.cc:299: VLOG(1) << "Route: " << message.c_str(); You probably don't need c_str() any more. You can also inline the message using << if you like. https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_session.cc:327: // JniFrameConsumer get size from the frames and it doesn't use DPI, so this Fix this comment. https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_session.cc:443: // __android_log_print( Couldn't VLOG(1) work for iOS in this case? We can't do LOG(INFO) but we have similar workaround: https://cs.chromium.org/chromium/src/remoting/base/logging.h?q=HOST_LOG+file:... https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... File remoting/client/chromoting_session.h (right): https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_session.h:44: class Delegate { Please comment that all functions will be called on the UI thread. https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_session.h:74: ChromotingSession(ChromotingSession::Delegate* delegate, I guess you need a WeakPtr of the Delegate? You have `new ChromotingSession(GetWeakPtr(), ...)` https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_session.h:174: ChromotingSession::Delegate* delegate_; Why using raw pointer here? The delegate may have trouble killing ChromotingSession before it dies... https://codereview.chromium.org/2753963002/diff/140001/remoting/client/jni/jn... File remoting/client/jni/jni_client.cc (right): https://codereview.chromium.org/2753963002/diff/140001/remoting/client/jni/jn... remoting/client/jni/jni_client.cc:62: display_handler_->CreateVideoRenderer(), audio_player_->GetWeakPtr(), info, Line longer than 80 characters. https://codereview.chromium.org/2753963002/diff/140001/remoting/client/jni/jn... remoting/client/jni/jni_client.cc:196: session_->ProvideSecret(ConvertJavaStringToUTF8(env, pin).c_str(), These c_str() calls are suspicious... Will it build if you drop them? https://codereview.chromium.org/2753963002/diff/140001/remoting/client/jni/jn.... File remoting/client/jni/jni_pairing_secret_fetcher. (right): https://codereview.chromium.org/2753963002/diff/140001/remoting/client/jni/jn... remoting/client/jni/jni_pairing_secret_fetcher.:1: // Copyright 2016 The Chromium Authors. All rights reserved. What happened to this file...? https://codereview.chromium.org/2753963002/diff/140001/remoting/client/native... File remoting/client/native_device_keymap.h (right): https://codereview.chromium.org/2753963002/diff/140001/remoting/client/native... remoting/client/native_device_keymap.h:11: #include <memory> Remove this include?
https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... File remoting/client/chromoting_client_runtime.h (right): https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_client_runtime.h:90: // TODO(nicholss): Auththreads will be leaked because they depend on the main s/Auththreads/AutoThreads ? https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_client_runtime.h:93: // Longer term we should migrate most of these to background tasks execpt the except https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_client_runtime.h:94: // network thread to TaskSchedular, removing the need for threads. TaskScheduler https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... File remoting/client/chromoting_session.cc (right): https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_session.cc:128: base::Unretained(delegate_), token_url, Please add a comment to say why base::Unretained is safe? (Or remove the Unretained if you make delegate_ a WeakPtr) https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_session.cc:440: // TODO(nicholss): When this was JNI only logging to android was possible. These stats are logged for debugging. Note that the WebApp displays the stats on the screen when you activate stats logging. This is the next-best thing for Android so it would be good to keep this logging. You can do what you did elsewhere - just log it at INFO level (or whatever level will be visible without jumping through hoops). https://codereview.chromium.org/2753963002/diff/140001/remoting/client/native... File remoting/client/native_device_keymap.h (right): https://codereview.chromium.org/2753963002/diff/140001/remoting/client/native... remoting/client/native_device_keymap.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2017 We usually update the year when we move or rename a file. https://codereview.chromium.org/2753963002/diff/140001/remoting/client/native... File remoting/client/native_device_keymap_android.cc (right): https://codereview.chromium.org/2753963002/diff/140001/remoting/client/native... remoting/client/native_device_keymap_android.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2017
Thanks for the feedback, I have updated some code and added some questions, PTAL, thanks! https://codereview.chromium.org/2753963002/diff/140001/remoting/BUILD.gn File remoting/BUILD.gn (right): https://codereview.chromium.org/2753963002/diff/140001/remoting/BUILD.gn#newc... remoting/BUILD.gn:172: libs += [ "android" ] On 2017/03/21 20:40:29, Yuwei wrote: > Why is this needed? The unittests would not link without the android lib. https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... File remoting/client/chromoting_client_runtime.h (right): https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_client_runtime.h:90: // TODO(nicholss): Auththreads will be leaked because they depend on the main On 2017/03/24 00:07:14, Lambros wrote: > s/Auththreads/AutoThreads ? Done. https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_client_runtime.h:93: // Longer term we should migrate most of these to background tasks execpt the On 2017/03/24 00:07:14, Lambros wrote: > except Done. https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_client_runtime.h:94: // network thread to TaskSchedular, removing the need for threads. On 2017/03/24 00:07:14, Lambros wrote: > TaskScheduler Done. This was copied from a comment given to me via another code review, I did not proof it enough I guess. :) https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... File remoting/client/chromoting_session.cc (right): https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_session.cc:51: base::WeakPtr<protocol::AudioStub> audio_player, On 2017/03/21 20:40:29, Yuwei wrote: > Could we keep those stuff either all WeakPtr or all unique_ptr? Well this is an interesting one... I think this will have to get reworked later, iOS will need to use the audio player in another place from session so it can't be a unique pointer. I vote for "deal with this later" because it will need to be worked out when I work on adding audio for iOS finally... https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_session.cc:128: base::Unretained(delegate_), token_url, On 2017/03/24 00:07:15, Lambros wrote: > Please add a comment to say why base::Unretained is safe? > (Or remove the Unretained if you make delegate_ a WeakPtr) Done. https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_session.cc:299: VLOG(1) << "Route: " << message.c_str(); On 2017/03/21 20:40:29, Yuwei wrote: > You probably don't need c_str() any more. You can also inline the message using > << if you like. Lambros says the android log is for debugging in app. Any ideas on how to keep the original way for android and just VLOG for everything else? Maybe a #ifdef... Any other thoughts? https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... File remoting/client/chromoting_session.h (right): https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_session.h:44: class Delegate { On 2017/03/21 20:40:29, Yuwei wrote: > Please comment that all functions will be called on the UI thread. Done. https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_session.h:74: ChromotingSession(ChromotingSession::Delegate* delegate, On 2017/03/21 20:40:29, Yuwei wrote: > I guess you need a WeakPtr of the Delegate? You have `new > ChromotingSession(GetWeakPtr(), ...)` base::WeakPtr acts funny some times with polymorphism from my past experience but this seems to work. Fixed. https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_session.h:174: ChromotingSession::Delegate* delegate_; On 2017/03/21 20:40:29, Yuwei wrote: > Why using raw pointer here? The delegate may have trouble killing > ChromotingSession before it dies... Done. https://codereview.chromium.org/2753963002/diff/140001/remoting/client/jni/jn... File remoting/client/jni/jni_client.cc (right): https://codereview.chromium.org/2753963002/diff/140001/remoting/client/jni/jn... remoting/client/jni/jni_client.cc:62: display_handler_->CreateVideoRenderer(), audio_player_->GetWeakPtr(), info, On 2017/03/21 20:40:30, Yuwei wrote: > Line longer than 80 characters. Done. https://codereview.chromium.org/2753963002/diff/140001/remoting/client/jni/jn... remoting/client/jni/jni_client.cc:196: session_->ProvideSecret(ConvertJavaStringToUTF8(env, pin).c_str(), On 2017/03/21 20:40:30, Yuwei wrote: > These c_str() calls are suspicious... Will it build if you drop them? I dropped them, it built locally but it was not an android build so I will have to see if it explodes on the trybots. https://codereview.chromium.org/2753963002/diff/140001/remoting/client/jni/jn.... File remoting/client/jni/jni_pairing_secret_fetcher. (right): https://codereview.chromium.org/2753963002/diff/140001/remoting/client/jni/jn... remoting/client/jni/jni_pairing_secret_fetcher.:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/21 20:40:30, Yuwei wrote: > What happened to this file...? Looks like the file dropped the .h and was added without wanting to. Removed. https://codereview.chromium.org/2753963002/diff/140001/remoting/client/native... File remoting/client/native_device_keymap.h (right): https://codereview.chromium.org/2753963002/diff/140001/remoting/client/native... remoting/client/native_device_keymap.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2017/03/24 00:07:15, Lambros wrote: > 2017 > We usually update the year when we move or rename a file. I have only changed that when it is a new file, not a move. Looks like the style guide does not define what should happen on move: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2753963002/diff/140001/remoting/client/native... remoting/client/native_device_keymap.h:11: #include <memory> On 2017/03/21 20:40:30, Yuwei wrote: > Remove this include? Done. https://codereview.chromium.org/2753963002/diff/140001/remoting/client/native... File remoting/client/native_device_keymap_android.cc (right): https://codereview.chromium.org/2753963002/diff/140001/remoting/client/native... remoting/client/native_device_keymap_android.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2017/03/24 00:07:15, Lambros wrote: > 2017 Done.
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Patchset #11 (id:200001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... File remoting/client/chromoting_session.cc (right): https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_session.cc:299: VLOG(1) << "Route: " << message.c_str(); On 2017/03/29 20:32:35, nicholss wrote: > On 2017/03/21 20:40:29, Yuwei wrote: > > You probably don't need c_str() any more. You can also inline the message > using > > << if you like. > > Lambros says the android log is for debugging in app. Any ideas on how to keep > the original way for android and just VLOG for everything else? Maybe a > #ifdef... > > Any other thoughts? LOG(INFO) on Android is already defined as __android_log_print(ANDROID_LOG_INFO, ...) here: https://cs.chromium.org/chromium/src/base/android/linker/linker_jni.h?l=33&rc... Also since this log is for performance tracking, I think it makes sense to use LOG(INFO) universally. Maybe we can do something similar to HOST_LOG: #define MOBILE_CLIENT_LOG LOG(INFO) If that's not possible I think I'm okay with #ifdef, say defining a MOBILE_CLIENT_LOG as either __android_log_print(INFO...) or VLOG(1) based on the platform. https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... File remoting/client/chromoting_session.h (right): https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_session.h:44: class Delegate { On 2017/03/29 20:32:36, nicholss wrote: > On 2017/03/21 20:40:29, Yuwei wrote: > > Please comment that all functions will be called on the UI thread. > > Done. Sorry. What I meant is all functions of the Delegate will be called on UI thread. Not the ChromotingSession...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
https://codereview.chromium.org/2753963002/diff/220001/remoting/client/jni/jn... File remoting/client/jni/jni_client.cc (right): https://codereview.chromium.org/2753963002/diff/220001/remoting/client/jni/jn... remoting/client/jni/jni_client.cc:61: GetWeakPtr(), display_handler_->CreateCursorShapeStub(), GetWeakPtr() should be the JniRuntimeDelegate
https://codereview.chromium.org/2753963002/diff/220001/remoting/client/jni/jn... File remoting/client/jni/jni_client.cc (right): https://codereview.chromium.org/2753963002/diff/220001/remoting/client/jni/jn... remoting/client/jni/jni_client.cc:61: GetWeakPtr(), display_handler_->CreateCursorShapeStub(), On 2017/03/29 22:55:27, Yuwei wrote: > GetWeakPtr() should be the JniRuntimeDelegate Oh man yes. Thanks nice catch! I had to give the JniRuntimeDelegate a weakptr factory but I am pretty sure it will fail to build and I will have to change the factory to produce ChromotingSession::Delegate type objects...
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2753963002/diff/220001/remoting/client/jni/jn... File remoting/client/jni/jni_client.cc (right): https://codereview.chromium.org/2753963002/diff/220001/remoting/client/jni/jn... remoting/client/jni/jni_client.cc:61: GetWeakPtr(), display_handler_->CreateCursorShapeStub(), On 2017/03/29 23:34:53, nicholss wrote: > On 2017/03/29 22:55:27, Yuwei wrote: > > GetWeakPtr() should be the JniRuntimeDelegate > > Oh man yes. Thanks nice catch! I had to give the JniRuntimeDelegate a weakptr > factory but I am pretty sure it will fail to build and I will have to change the > factory to produce ChromotingSession::Delegate type objects... IIRC WeakPtr should be clever enough to convert JniRuntimeDelegate template into the ChromotingSession::Delegate one, lets see whether it builds :) Otherwise having a factory of ChromotingSession::Delegate is fine...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
On 2017/03/29 23:56:29, Yuwei wrote: > https://codereview.chromium.org/2753963002/diff/220001/remoting/client/jni/jn... > File remoting/client/jni/jni_client.cc (right): > > https://codereview.chromium.org/2753963002/diff/220001/remoting/client/jni/jn... > remoting/client/jni/jni_client.cc:61: GetWeakPtr(), > display_handler_->CreateCursorShapeStub(), > On 2017/03/29 23:34:53, nicholss wrote: > > On 2017/03/29 22:55:27, Yuwei wrote: > > > GetWeakPtr() should be the JniRuntimeDelegate > > > > Oh man yes. Thanks nice catch! I had to give the JniRuntimeDelegate a weakptr > > factory but I am pretty sure it will fail to build and I will have to change > the > > factory to produce ChromotingSession::Delegate type objects... > > IIRC WeakPtr should be clever enough to convert JniRuntimeDelegate template into > the ChromotingSession::Delegate one, lets see whether it builds :) > > Otherwise having a factory of ChromotingSession::Delegate is fine... I have never been able to get WeakPtr to be clever at all. Looks like it failed with this: ../../base/memory/weak_ptr.h:223:40: error: cannot initialize a member subobject of type 'remoting::ChromotingSession::Delegate *' with an lvalue of type 'remoting::JniRuntimeDelegate *' : WeakPtrBase(std::move(other)), ptr_(other.ptr_) {} Which means I need to make the factory for weak pointers of type remoting::ChromotingSession::Delegate. Going to try that now.
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2753963002/diff/220001/remoting/client/jni/jn... File remoting/client/jni/jni_client.cc (right): https://codereview.chromium.org/2753963002/diff/220001/remoting/client/jni/jn... remoting/client/jni/jni_client.cc:61: GetWeakPtr(), display_handler_->CreateCursorShapeStub(), On 2017/03/29 23:34:53, nicholss wrote: > On 2017/03/29 22:55:27, Yuwei wrote: > > GetWeakPtr() should be the JniRuntimeDelegate > > Oh man yes. Thanks nice catch! I had to give the JniRuntimeDelegate a weakptr > factory but I am pretty sure it will fail to build and I will have to change the > factory to produce ChromotingSession::Delegate type objects... Sorry for misleading... Looks like it's taking a ChromotingSession::Delegate rather than a ChromotingClientRuntime::Delegate. You need to mark JniClient a subclass of ChromotingSession::Delegate and just pass GetWeakPtr() instead. You can remove the factory in JniRuntimeDelegate...
App should build after you make all of these three changes. I tried to apply your patch and build the app. It crashes when the session disconnects due to WeakPtr thread inconsistency... I'll help you figure it out https://codereview.chromium.org/2753963002/diff/300001/remoting/client/jni/jn... File remoting/client/jni/jni_client.cc (right): https://codereview.chromium.org/2753963002/diff/300001/remoting/client/jni/jn... remoting/client/jni/jni_client.cc:57: base::Bind(&JniPairingSecretFetcher::FetchSecret, secret_fetcher_); secret_fetcher_.GetWeakPtr() https://codereview.chromium.org/2753963002/diff/300001/remoting/client/jni/jn... remoting/client/jni/jni_client.cc:205: base::Bind(&JniPairingSecretFetcher::ProvideSecret, secret_fetcher_, secret_fetcher_->GetWeakPtr()
On 2017/03/30 19:02:57, Yuwei wrote: > https://codereview.chromium.org/2753963002/diff/220001/remoting/client/jni/jn... > File remoting/client/jni/jni_client.cc (right): > > https://codereview.chromium.org/2753963002/diff/220001/remoting/client/jni/jn... > remoting/client/jni/jni_client.cc:61: GetWeakPtr(), > display_handler_->CreateCursorShapeStub(), > On 2017/03/29 23:34:53, nicholss wrote: > > On 2017/03/29 22:55:27, Yuwei wrote: > > > GetWeakPtr() should be the JniRuntimeDelegate > > > > Oh man yes. Thanks nice catch! I had to give the JniRuntimeDelegate a weakptr > > factory but I am pretty sure it will fail to build and I will have to change > the > > factory to produce ChromotingSession::Delegate type objects... > > Sorry for misleading... Looks like it's taking a ChromotingSession::Delegate > rather than a ChromotingClientRuntime::Delegate. > > You need to mark JniClient a subclass of ChromotingSession::Delegate and just > pass GetWeakPtr() instead. You can remove the factory in JniRuntimeDelegate... Yeah good catch, I was out thinking about other things for a week and lost my place in this code change.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2753963002/diff/320001/remoting/client/jni/jn... File remoting/client/jni/jni_client.cc (right): https://codereview.chromium.org/2753963002/diff/320001/remoting/client/jni/jn... remoting/client/jni/jni_client.cc:80: audio_player_.reset(); You will need to delete audio_player_ on the network thread, similar to session_ and secret_fetcher_. I'm not sure why it's bound to the network thread though...
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
On 2017/03/30 21:18:56, Yuwei wrote: > https://codereview.chromium.org/2753963002/diff/320001/remoting/client/jni/jn... > File remoting/client/jni/jni_client.cc (right): > > https://codereview.chromium.org/2753963002/diff/320001/remoting/client/jni/jn... > remoting/client/jni/jni_client.cc:80: audio_player_.reset(); > You will need to delete audio_player_ on the network thread, similar to session_ > and secret_fetcher_. I'm not sure why it's bound to the network thread though... done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The changes I have had to make in the last few patches are related to the build failing based on chromium style rules failing the build. The build is passing locally in all the ways I know how to build it...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #22 (id:430001) has been deleted
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #12 (id:280001) has been deleted
Patchset #16 (id:380001) has been deleted
Patchset #15 (id:360001) has been deleted
Patchset #8 (id:180001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #16 (id:450001) has been deleted
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Updated the logging to use #if defines, I think other comments have been addressed. The build is working locally so lets see if it passes the try bots :) PTAL, Thanks! https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... File remoting/client/chromoting_session.cc (right): https://codereview.chromium.org/2753963002/diff/140001/remoting/client/chromo... remoting/client/chromoting_session.cc:440: // TODO(nicholss): When this was JNI only logging to android was possible. On 2017/03/24 00:07:14, Lambros wrote: > These stats are logged for debugging. Note that the WebApp displays the stats on > the screen when you activate stats logging. This is the next-best thing for > Android so it would be good to keep this logging. > You can do what you did elsewhere - just log it at INFO level (or whatever level > will be visible without jumping through hoops). I moved this log message to the logger. If this is a useful log then the logger should make it so all clients can print the message without needing to copy this code block.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Patchset #16 (id:470001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Patchset #16 (id:490001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Will it be easier to provide a function in remoting/base/logging like this:
LogClientInfo(component, message) {
#ifdef ANDROID
__android_log_info(component, message.c_str());
#else
VLOG(1) << component + ": " + message;
#endif
}
https://codereview.chromium.org/2753963002/diff/340001/remoting/client/chromo...
File remoting/client/chromoting_client_runtime.h (right):
https://codereview.chromium.org/2753963002/diff/340001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.h:90: // TODO(nicholss): AuthThreads
will be leaked because they depend on the main
s/AuthThreads/AutoThreads/
https://codereview.chromium.org/2753963002/diff/340001/remoting/client/chromo...
File remoting/client/chromoting_session.h (right):
https://codereview.chromium.org/2753963002/diff/340001/remoting/client/chromo...
remoting/client/chromoting_session.h:40: // called on the UI thread.
Change this back to network?
On 2017/04/03 19:29:22, Yuwei wrote:
> Will it be easier to provide a function in remoting/base/logging like this:
>
> LogClientInfo(component, message) {
> #ifdef ANDROID
> __android_log_info(component, message.c_str());
> #else
> VLOG(1) << component + ": " + message;
> #endif
> }
I did not do this because the android log injected values into the message in
the string format, but the remaining usage is just messages. I am going to leave
this as is and if there are more andorid/other logging that has to happen we can
look at refactoring at that time.
Patchset #17 (id:530001) has been deleted
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/03 19:45:16, nicholss wrote:
> On 2017/04/03 19:29:22, Yuwei wrote:
> > Will it be easier to provide a function in remoting/base/logging like this:
> >
> > LogClientInfo(component, message) {
> > #ifdef ANDROID
> > __android_log_info(component, message.c_str());
> > #else
> > VLOG(1) << component + ": " + message;
> > #endif
> > }
>
> I did not do this because the android log injected values into the message in
> the string format, but the remaining usage is just messages. I am going to
leave
> this as is and if there are more andorid/other logging that has to happen we
can
> look at refactoring at that time.
What about something like:
LogClientInfo(component, format, ...) {
va_list args;
va_start(args, format);
#ifdef ANDROID
__android_log_vprint(ANDROID_LOG_INFO, component, format, args);
#else
VLOG(1) << component << ": " << base::StringPrintV(format, args);
#endif
va_end(args);
}
I don't feel it's good to leave it there for now since there are already
multiple duplicated code for logging, especially the PrintLogStatistics function
may be painful to keep in sync.
On 2017/04/03 20:49:59, Yuwei wrote:
> On 2017/04/03 19:45:16, nicholss wrote:
> > On 2017/04/03 19:29:22, Yuwei wrote:
> > > Will it be easier to provide a function in remoting/base/logging like
this:
> > >
> > > LogClientInfo(component, message) {
> > > #ifdef ANDROID
> > > __android_log_info(component, message.c_str());
> > > #else
> > > VLOG(1) << component + ": " + message;
> > > #endif
> > > }
> >
> > I did not do this because the android log injected values into the message
in
> > the string format, but the remaining usage is just messages. I am going to
> leave
> > this as is and if there are more andorid/other logging that has to happen we
> can
> > look at refactoring at that time.
>
>
>
> What about something like:
>
> LogClientInfo(component, format, ...) {
> va_list args;
> va_start(args, format);
>
> #ifdef ANDROID
> __android_log_vprint(ANDROID_LOG_INFO, component, format, args);
> #else
> VLOG(1) << component << ": " << base::StringPrintV(format, args);
> #endif
>
> va_end(args);
> }
>
> I don't feel it's good to leave it there for now since there are already
> multiple duplicated code for logging, especially the PrintLogStatistics
function
> may be painful to keep in sync.
it is not duplicated code, just a standard ifdef. I don't really want to get
into the business of trying to handle all the edge cases for how someone might
log something, that logic is already in LOG and __android_log_vprint.
https://codereview.chromium.org/2753963002/diff/340001/remoting/client/chromo... File remoting/client/chromoting_client_runtime.h (right): https://codereview.chromium.org/2753963002/diff/340001/remoting/client/chromo... remoting/client/chromoting_client_runtime.h:90: // TODO(nicholss): AuthThreads will be leaked because they depend on the main On 2017/04/03 19:29:22, Yuwei wrote: > s/AuthThreads/AutoThreads/ Done. https://codereview.chromium.org/2753963002/diff/340001/remoting/client/chromo... File remoting/client/chromoting_session.h (right): https://codereview.chromium.org/2753963002/diff/340001/remoting/client/chromo... remoting/client/chromoting_session.h:40: // called on the UI thread. On 2017/04/03 19:29:22, Yuwei wrote: > Change this back to network? Done.
LGTM once comments are addressed. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/client... File remoting/client/client_telemetry_logger.cc (right): https://codereview.chromium.org/2753963002/diff/550001/remoting/client/client... remoting/client/client_telemetry_logger.cc:100: VLOG(1) << base::StringPrintf( What about putting the #ifdefs inside the function body? https://codereview.chromium.org/2753963002/diff/550001/remoting/client/ios/br... File remoting/client/ios/bridge/client_instance.cc (right): https://codereview.chromium.org/2753963002/diff/550001/remoting/client/ios/br... remoting/client/ios/bridge/client_instance.cc:17: //#include "remoting/client/client_status_logger.h" Remove this include?
Lambros: Please take another look. Thanks. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/client... File remoting/client/client_telemetry_logger.cc (right): https://codereview.chromium.org/2753963002/diff/550001/remoting/client/client... remoting/client/client_telemetry_logger.cc:100: VLOG(1) << base::StringPrintf( On 2017/04/03 21:40:46, Yuwei wrote: > What about putting the #ifdefs inside the function body? I don't think that makes this more readable. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/ios/br... File remoting/client/ios/bridge/client_instance.cc (right): https://codereview.chromium.org/2753963002/diff/550001/remoting/client/ios/br... remoting/client/ios/bridge/client_instance.cc:17: //#include "remoting/client/client_status_logger.h" On 2017/04/03 21:40:46, Yuwei wrote: > Remove this include? I will when I work on this next. This code does not get built or do anything in chromium yet and might even get deleted entirely. I am not really interested in fixing it up. I am removing client_status_logger because it was left un-migrated when the telemetry logger was added :(
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2753963002/diff/550001/remoting/client/chromo... File remoting/client/chromoting_session.cc (right): https://codereview.chromium.org/2753963002/diff/550001/remoting/client/chromo... remoting/client/chromoting_session.cc:7: #if defined(OS_ANDROID) Conditional #include should be after the others. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/chromo... remoting/client/chromoting_session.cc:127: __android_log_print(ANDROID_LOG_INFO, "ThirdPartyAuth", Personally, I'd be happy with logging this on Android with VLOG(1) - I don't think we need to jump through hoops. I'm not convinced we even need to log it at all. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/chromo... remoting/client/chromoting_session.cc:145: "Fetching Third Party Token from user."); Same here. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/chromo... remoting/client/chromoting_session.cc:154: #if defined(OS_ANDROID) I don't think we need #ifdef - I'm happy to use LOG(WARNING) on Android here. It would log under "chromium" instead of "ThirdPartyAuth" but I don't care :) https://codereview.chromium.org/2753963002/diff/550001/remoting/client/chromo... File remoting/client/chromoting_session.h (right): https://codereview.chromium.org/2753963002/diff/550001/remoting/client/chromo... remoting/client/chromoting_session.h:73: // The session does not take ownership of |delegate|. The comment about ownership is not needed for a WeakPtr. Maybe document which threads the WeakPtr objects will be used on? https://codereview.chromium.org/2753963002/diff/550001/remoting/client/chromo... remoting/client/chromoting_session.h:149: base::WeakPtr<ChromotingSession> GetWeakPtr(); It's a bit sad that we expose the WeakPtr to |this|, and have to ask the caller nicely to please bind it to the right thread. Out of scope of this CL I guess, and I don't have any better ideas anyway :( https://codereview.chromium.org/2753963002/diff/550001/remoting/client/chromo... remoting/client/chromoting_session.h:174: base::WeakPtr<ChromotingSession::Delegate> delegate_; Used on the Network thread? https://codereview.chromium.org/2753963002/diff/550001/remoting/client/chromo... remoting/client/chromoting_session.h:188: base::WeakPtr<protocol::AudioStub> audio_player_; Used on the Network thread? https://codereview.chromium.org/2753963002/diff/550001/remoting/client/client... File remoting/client/client_telemetry_logger.cc (right): https://codereview.chromium.org/2753963002/diff/550001/remoting/client/client... remoting/client/client_telemetry_logger.cc:7: #if defined(OS_ANDROID) Move below the other #includes. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/client... remoting/client/client_telemetry_logger.cc:101: "Bandwidth:%.0f FrameRate:%.1f;" Duplicating this code makes me sad. Could you write this to a temporary string object, then use #ifdef to log the string differently on Android? On Android, I think we do want to see this particular log, so we should keep the #ifdef in this case. Ideally, we would display the numbers on the screen in real-time (and provide an option for the user to toggle this off/on). Writing to the system log is the next-best thing. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/client... File remoting/client/client_telemetry_logger.h (right): https://codereview.chromium.org/2753963002/diff/550001/remoting/client/client... remoting/client/client_telemetry_logger.h:62: // Print Log Statistics. Remove comment, or document where the stats get "printed" to? https://codereview.chromium.org/2753963002/diff/550001/remoting/client/ios/br... File remoting/client/ios/bridge/client_instance.h (right): https://codereview.chromium.org/2753963002/diff/550001/remoting/client/ios/br... remoting/client/ios/bridge/client_instance.h:39: // class ClientStatusLogger; Remove this instead of commenting out. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/ios/br... remoting/client/ios/bridge/client_instance.h:43: // ClientInstance. ClientUserInterface that indirectly makes and receives OBJ_C Grammar of final sentence? https://codereview.chromium.org/2753963002/diff/550001/remoting/client/ios/br... remoting/client/ios/bridge/client_instance.h:164: // std::unique_ptr<ClientStatusLogger> client_status_logger_; This is commented out, so remove both lines?
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the review! https://codereview.chromium.org/2753963002/diff/550001/remoting/client/chromo... File remoting/client/chromoting_session.cc (right): https://codereview.chromium.org/2753963002/diff/550001/remoting/client/chromo... remoting/client/chromoting_session.cc:7: #if defined(OS_ANDROID) On 2017/04/05 01:31:19, Lambros wrote: > Conditional #include should be after the others. Removed. android logging no longer happening in this file. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/chromo... remoting/client/chromoting_session.cc:127: __android_log_print(ANDROID_LOG_INFO, "ThirdPartyAuth", On 2017/04/05 01:31:18, Lambros wrote: > Personally, I'd be happy with logging this on Android with VLOG(1) - I don't > think we need to jump through hoops. I'm not convinced we even need to log it at > all. Removed the trace style log messages. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/chromo... remoting/client/chromoting_session.cc:145: "Fetching Third Party Token from user."); On 2017/04/05 01:31:18, Lambros wrote: > Same here. Done. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/chromo... remoting/client/chromoting_session.cc:154: #if defined(OS_ANDROID) On 2017/04/05 01:31:18, Lambros wrote: > I don't think we need #ifdef - I'm happy to use LOG(WARNING) on Android here. It > would log under "chromium" instead of "ThirdPartyAuth" but I don't care :) Done. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/chromo... File remoting/client/chromoting_session.h (right): https://codereview.chromium.org/2753963002/diff/550001/remoting/client/chromo... remoting/client/chromoting_session.h:73: // The session does not take ownership of |delegate|. On 2017/04/05 01:31:19, Lambros wrote: > The comment about ownership is not needed for a WeakPtr. > Maybe document which threads the WeakPtr objects will be used on? Acknowledged. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/chromo... remoting/client/chromoting_session.h:149: base::WeakPtr<ChromotingSession> GetWeakPtr(); On 2017/04/05 01:31:19, Lambros wrote: > It's a bit sad that we expose the WeakPtr to |this|, and have to ask the caller > nicely to please bind it to the right thread. Out of scope of this CL I guess, > and I don't have any better ideas anyway :( Acknowledged. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/chromo... remoting/client/chromoting_session.h:174: base::WeakPtr<ChromotingSession::Delegate> delegate_; On 2017/04/05 01:31:19, Lambros wrote: > Used on the Network thread? Added a comment, UI thread. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/chromo... remoting/client/chromoting_session.h:188: base::WeakPtr<protocol::AudioStub> audio_player_; On 2017/04/05 01:31:19, Lambros wrote: > Used on the Network thread? Added a comment, UI thread. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/client... File remoting/client/client_telemetry_logger.cc (right): https://codereview.chromium.org/2753963002/diff/550001/remoting/client/client... remoting/client/client_telemetry_logger.cc:7: #if defined(OS_ANDROID) On 2017/04/05 01:31:19, Lambros wrote: > Move below the other #includes. Acknowledged. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/client... remoting/client/client_telemetry_logger.cc:101: "Bandwidth:%.0f FrameRate:%.1f;" On 2017/04/05 01:31:19, Lambros wrote: > Duplicating this code makes me sad. Could you write this to a temporary string > object, then use #ifdef to log the string differently on Android? > > On Android, I think we do want to see this particular log, so we should keep the > #ifdef in this case. Ideally, we would display the numbers on the screen in > real-time (and provide an option for the user to toggle this off/on). Writing to > the system log is the next-best thing. That type of feature is pretty easy now with the addition of drawables. Which is why I implemented it. I will look into adding this when I get to the rendering part of the app. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/client... File remoting/client/client_telemetry_logger.h (right): https://codereview.chromium.org/2753963002/diff/550001/remoting/client/client... remoting/client/client_telemetry_logger.h:62: // Print Log Statistics. On 2017/04/05 01:31:19, Lambros wrote: > Remove comment, or document where the stats get "printed" to? Done. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/ios/br... File remoting/client/ios/bridge/client_instance.h (right): https://codereview.chromium.org/2753963002/diff/550001/remoting/client/ios/br... remoting/client/ios/bridge/client_instance.h:39: // class ClientStatusLogger; On 2017/04/05 01:31:19, Lambros wrote: > Remove this instead of commenting out. Acknowledged, but not going to change this file. The file might be removed next cl after integration with the current cl is finished. ios/bridge/client_instance should be replaced with session... Keeping the commented out bits here to remember what the class looked like to help with integration next. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/ios/br... remoting/client/ios/bridge/client_instance.h:43: // ClientInstance. ClientUserInterface that indirectly makes and receives OBJ_C On 2017/04/05 01:31:19, Lambros wrote: > Grammar of final sentence? Acknowledged. See above. https://codereview.chromium.org/2753963002/diff/550001/remoting/client/ios/br... remoting/client/ios/bridge/client_instance.h:164: // std::unique_ptr<ClientStatusLogger> client_status_logger_; On 2017/04/05 01:31:19, Lambros wrote: > This is commented out, so remove both lines? Acknowledged. See above.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nicholss@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yuweih@chromium.org, lambroslambrou@chromium.org Link to the patchset: https://codereview.chromium.org/2753963002/#ps570001 (title: "Updating based on feedback.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 570001, "attempt_start_ts": 1491416889110170,
"parent_rev": "fa80dd944e1937f0ab144bee0ac385d537bbd74a", "commit_rev":
"a6be869a25aa4de4c6b4629a1b710e5d34f8fd34"}
Message was sent while issue was closed.
Description was changed from ========== Refactoring and rewriting the chromoting jni instance to be chromoting session. This will allow Android and iOS to share the majority of code related to session and instance management. The setup flow for each client will be very similar, this change will allow the iOS app to reuse the session management (xmpp, client, etc) that was originally written for the JNI layer of Android. Adding a session delegate allows for reuse and moves most of the code into a common place and leaves the java/JNI only bits in the JNI layer. BUG=699788 ========== to ========== Refactoring and rewriting the chromoting jni instance to be chromoting session. This will allow Android and iOS to share the majority of code related to session and instance management. The setup flow for each client will be very similar, this change will allow the iOS app to reuse the session management (xmpp, client, etc) that was originally written for the JNI layer of Android. Adding a session delegate allows for reuse and moves most of the code into a common place and leaves the java/JNI only bits in the JNI layer. BUG=699788 Review-Url: https://codereview.chromium.org/2753963002 Cr-Commit-Position: refs/heads/master@{#462154} Committed: https://chromium.googlesource.com/chromium/src/+/a6be869a25aa4de4c6b4629a1b71... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:570001) as https://chromium.googlesource.com/chromium/src/+/a6be869a25aa4de4c6b4629a1b71... |
