Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18856012:
Create new remoting_client_jni target (Closed)
|
Created:
7 years, 5 months ago by solb Modified:
7 years, 5 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, weitaosu+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCreate new remoting_client_jni target
This will house material analogous to the Pepper implementation remoting_client_plugin, except designed to interface instead with JNI, the Java Native Interface.
For now, this folder contains the jni_interface family of functions (which will receive calls FROM Java) and the ChromotingJNIInstance class, which implements ClientUserInterface (while being responsible for calls TO Java).
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211073
Patch Set 1 #
Total comments: 17
Patch Set 2 : Eliminate prior stub classes and introduce ChromotingJNIInstance and jni_interface #Patch Set 3 : Restrict build target to Android OS #Patch Set 4 : Make ChromotingJNIInstance a singleton #Patch Set 5 : Add missing virtual keyword to destructor declaration #
Total comments: 8
Patch Set 6 : Gary's recommendations #
Total comments: 40
Messages
Total messages: 12 (0 generated)
If this passes the trybots and CQ without blowing anything up, we'll be ready to start putting some more serious code in this tree.
https://codereview.chromium.org/18856012/diff/1/build/all_android.gyp File build/all_android.gyp (right): https://codereview.chromium.org/18856012/diff/1/build/all_android.gyp#newcode128 build/all_android.gyp:128: '../remoting/remoting.gyp:remoting_client_jni', I don't think we want this exception - we don't expect this to fail since your CLs will have to run through the try-bots successfully before they land https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/OWNERS File remoting/client/jni/OWNERS (right): https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/OWNERS#ne... remoting/client/jni/OWNERS:1: solb@chromium.org Better add myself and Gary here as well. https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... File remoting/client/jni/audio_player_impl.cc (right): https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... remoting/client/jni/audio_player_impl.cc:14: LOG(WARNING) << "AudioPlayerImpl::GetSamplesPerFrame() [unimplemented]"; nit: NOTIMPLEMENTED() https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... remoting/client/jni/audio_player_impl.cc:15: return 0; // TODO(solb) This is a stub. No need for this comment, nor the one below. https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... remoting/client/jni/audio_player_impl.cc:20: LOG(WARNING) << "AudioPlayerImpl::ResetAudioPlayer(...) [unimplemented]"; nit: As above. https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... File remoting/client/jni/audio_player_impl.h (right): https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... remoting/client/jni/audio_player_impl.h:12: // Basic AudioPlayer implementation for client implementations that use JNI. It's not a basic implementation, it's dummy implementation, since it does nothing. Why do we need this? Can't we just configure the client not to request audio and pass a NULL AudioPlayer? https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... remoting/client/jni/audio_player_impl.h:18: // nit: Get rid of the blank line comments. https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... remoting/client/jni/audio_player_impl.h:19: // AudioPlayer implementation: nit: : -> . https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... remoting/client/jni/audio_player_impl.h:22: nit: No need for a blank line between the overridden methods here. https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/token_fet... File remoting/client/jni/token_fetcher.cc (right): https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/token_fet... remoting/client/jni/token_fetcher.cc:15: LOG(INFO) << "TokenFetcher::FetchThirdPartyToken(...)"; NOTIMPLEMENTED() https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/token_fet... File remoting/client/jni/token_fetcher.h (right): https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/token_fet... remoting/client/jni/token_fetcher.h:14: // Basic TokenFetcher implementation for client implementations that use JNI. Basic -> Dummy https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/token_fet... remoting/client/jni/token_fetcher.h:14: // Basic TokenFetcher implementation for client implementations that use JNI. As for AudioPlayer, do we actually need this? Can't we configure the client not to support token auth and not pass a TokenFetcher? https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/token_fet... remoting/client/jni/token_fetcher.h:22: // See AudioPlayer re comment format/
On 2013/07/09 17:34:47, Wez wrote: > https://codereview.chromium.org/18856012/diff/1/build/all_android.gyp > File build/all_android.gyp (right): > > https://codereview.chromium.org/18856012/diff/1/build/all_android.gyp#newcode128 > build/all_android.gyp:128: '../remoting/remoting.gyp:remoting_client_jni', > I don't think we want this exception - we don't expect this to fail since your > CLs will have to run through the try-bots successfully before they land > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/OWNERS > File remoting/client/jni/OWNERS (right): > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/OWNERS#ne... > remoting/client/jni/OWNERS:1: mailto:solb@chromium.org > Better add myself and Gary here as well. > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... > File remoting/client/jni/audio_player_impl.cc (right): > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... > remoting/client/jni/audio_player_impl.cc:14: LOG(WARNING) << > "AudioPlayerImpl::GetSamplesPerFrame() [unimplemented]"; > nit: NOTIMPLEMENTED() > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... > remoting/client/jni/audio_player_impl.cc:15: return 0; // TODO(solb) This is a > stub. > No need for this comment, nor the one below. > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... > remoting/client/jni/audio_player_impl.cc:20: LOG(WARNING) << > "AudioPlayerImpl::ResetAudioPlayer(...) [unimplemented]"; > nit: As above. > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... > File remoting/client/jni/audio_player_impl.h (right): > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... > remoting/client/jni/audio_player_impl.h:12: // Basic AudioPlayer implementation > for client implementations that use JNI. > It's not a basic implementation, it's dummy implementation, since it does > nothing. Why do we need this? Can't we just configure the client not to request > audio and pass a NULL AudioPlayer? > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... > remoting/client/jni/audio_player_impl.h:18: // > nit: Get rid of the blank line comments. > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... > remoting/client/jni/audio_player_impl.h:19: // AudioPlayer implementation: > nit: : -> . > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... > remoting/client/jni/audio_player_impl.h:22: > nit: No need for a blank line between the overridden methods here. > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/token_fet... > File remoting/client/jni/token_fetcher.cc (right): > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/token_fet... > remoting/client/jni/token_fetcher.cc:15: LOG(INFO) << > "TokenFetcher::FetchThirdPartyToken(...)"; > NOTIMPLEMENTED() > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/token_fet... > File remoting/client/jni/token_fetcher.h (right): > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/token_fet... > remoting/client/jni/token_fetcher.h:14: // Basic TokenFetcher implementation for > client implementations that use JNI. > Basic -> Dummy > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/token_fet... > remoting/client/jni/token_fetcher.h:14: // Basic TokenFetcher implementation for > client implementations that use JNI. > As for AudioPlayer, do we actually need this? Can't we configure the client not > to support token auth and not pass a TokenFetcher? > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/token_fet... > remoting/client/jni/token_fetcher.h:22: // > See AudioPlayer re comment format/ See https://codereview.chromium.org/18452006, which eliminates the need for AudioPlayerImpl.
With those stub classes gone, here's a very basic version of ChromotingJNIInstance and its accompanying JNI entry points instead. https://codereview.chromium.org/18856012/diff/1/build/all_android.gyp File build/all_android.gyp (right): https://codereview.chromium.org/18856012/diff/1/build/all_android.gyp#newcode128 build/all_android.gyp:128: '../remoting/remoting.gyp:remoting_client_jni', On 2013/07/09 17:34:47, Wez wrote: > I don't think we want this exception - we don't expect this to fail since your > CLs will have to run through the try-bots successfully before they land Done. https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/OWNERS File remoting/client/jni/OWNERS (right): https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/OWNERS#ne... remoting/client/jni/OWNERS:1: solb@chromium.org On 2013/07/09 17:34:47, Wez wrote: > Better add myself and Gary here as well. Done. https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... File remoting/client/jni/audio_player_impl.h (right): https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... remoting/client/jni/audio_player_impl.h:12: // Basic AudioPlayer implementation for client implementations that use JNI. On 2013/07/09 17:34:47, Wez wrote: > It's not a basic implementation, it's dummy implementation, since it does > nothing. Why do we need this? Can't we just configure the client not to request > audio and pass a NULL AudioPlayer? I got rid of this whole class, which compiles now and will actually run once that other patch lands. https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/token_fet... File remoting/client/jni/token_fetcher.h (right): https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/token_fet... remoting/client/jni/token_fetcher.h:14: // Basic TokenFetcher implementation for client implementations that use JNI. On 2013/07/09 17:34:47, Wez wrote: > As for AudioPlayer, do we actually need this? Can't we configure the client not > to support token auth and not pass a TokenFetcher? Done.
On 2013/07/10 00:01:19, solb wrote: > With those stub classes gone, here's a very basic version of > ChromotingJNIInstance and its accompanying JNI entry points instead. > > https://codereview.chromium.org/18856012/diff/1/build/all_android.gyp > File build/all_android.gyp (right): > > https://codereview.chromium.org/18856012/diff/1/build/all_android.gyp#newcode128 > build/all_android.gyp:128: '../remoting/remoting.gyp:remoting_client_jni', > On 2013/07/09 17:34:47, Wez wrote: > > I don't think we want this exception - we don't expect this to fail since your > > CLs will have to run through the try-bots successfully before they land > > Done. > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/OWNERS > File remoting/client/jni/OWNERS (right): > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/OWNERS#ne... > remoting/client/jni/OWNERS:1: mailto:solb@chromium.org > On 2013/07/09 17:34:47, Wez wrote: > > Better add myself and Gary here as well. > > Done. > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... > File remoting/client/jni/audio_player_impl.h (right): > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/audio_pla... > remoting/client/jni/audio_player_impl.h:12: // Basic AudioPlayer implementation > for client implementations that use JNI. > On 2013/07/09 17:34:47, Wez wrote: > > It's not a basic implementation, it's dummy implementation, since it does > > nothing. Why do we need this? Can't we just configure the client not to > request > > audio and pass a NULL AudioPlayer? > > I got rid of this whole class, which compiles now and will actually run once > that other patch lands. > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/token_fet... > File remoting/client/jni/token_fetcher.h (right): > > https://codereview.chromium.org/18856012/diff/1/remoting/client/jni/token_fet... > remoting/client/jni/token_fetcher.h:14: // Basic TokenFetcher implementation for > client implementations that use JNI. > On 2013/07/09 17:34:47, Wez wrote: > > As for AudioPlayer, do we actually need this? Can't we configure the client > not > > to support token auth and not pass a TokenFetcher? > > Done. I made some more changes, and I think this is again ready for review.
tip: If you're adding a message to the CL and it doesn't relate to previous messages, use Publish+Mail comments to send it, so you don't end up re-quoting previous messages.
lgtm lgtm after nits https://codereview.chromium.org/18856012/diff/33017/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_instance.cc (right): https://codereview.chromium.org/18856012/diff/33017/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:133: env->DeleteGlobalRef(class_); // TODO(solb) detach all threads from JVM Is this TODO really associated with the code on the same line? It seems like this comment should be on a line all by itself (it'll be easier to see as well). https://codereview.chromium.org/18856012/diff/33017/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_instance.h (right): https://codereview.chromium.org/18856012/diff/33017/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:28: : public ClientUserInterface { nit: doesn't this fit on one line? https://codereview.chromium.org/18856012/diff/33017/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:60: // reusable between sessions: nit: Reusable https://codereview.chromium.org/18856012/diff/33017/remoting/client/jni/jni_i... File remoting/client/jni/jni_interface.cc (right): https://codereview.chromium.org/18856012/diff/33017/remoting/client/jni/jni_i... remoting/client/jni/jni_interface.cc:46: auth_token, align arguments with paren
Wez? https://codereview.chromium.org/18856012/diff/33017/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_instance.cc (right): https://codereview.chromium.org/18856012/diff/33017/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:133: env->DeleteGlobalRef(class_); // TODO(solb) detach all threads from JVM On 2013/07/10 21:56:18, garykac wrote: > Is this TODO really associated with the code on the same line? > > It seems like this comment should be on a line all by itself (it'll be easier to > see as well). Done. https://codereview.chromium.org/18856012/diff/33017/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_instance.h (right): https://codereview.chromium.org/18856012/diff/33017/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:28: : public ClientUserInterface { On 2013/07/10 21:56:18, garykac wrote: > nit: doesn't this fit on one line? Done. https://codereview.chromium.org/18856012/diff/33017/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:60: // reusable between sessions: On 2013/07/10 21:56:18, garykac wrote: > nit: Reusable Done. https://codereview.chromium.org/18856012/diff/33017/remoting/client/jni/jni_i... File remoting/client/jni/jni_interface.cc (right): https://codereview.chromium.org/18856012/diff/33017/remoting/client/jni/jni_i... remoting/client/jni/jni_interface.cc:46: auth_token, On 2013/07/10 21:56:18, garykac wrote: > align arguments with paren Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/solb@chromium.org/18856012/50001
https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... File remoting/client/jni/chromoting_jni_instance.cc (right): https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... remoting/client/jni/chromoting_jni_instance.cc:22: // For now, this just gives us access to the strings supplied from Java. Remove this comment; it should be clear from the NOTIMPLEMENTED() below what's going on. https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... remoting/client/jni/chromoting_jni_instance.cc:42: // TODO(solb) Initiate connection from here. NOTIMPLEMENTED() here. https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... remoting/client/jni/chromoting_jni_instance.cc:70: protocol::ErrorCode error) {} nit: This is a multi-line definition, so braces go on separate lines. https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... remoting/client/jni/chromoting_jni_instance.cc:72: void ChromotingJNIInstance::OnConnectionReady(bool ready) {} Why are this and the next three methods not NOTIMPLEMENTED()? https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... remoting/client/jni/chromoting_jni_instance.cc:77: const protocol::PairingResponse& response) {} nit: See above re braces https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... remoting/client/jni/chromoting_jni_instance.cc:91: // want to print an error on every run. This doesn't make sense - if it's going to be implemented but isn't yet then use NOTIMPLEMENTED. If it's not likely to be implemented any time soon then just return NULL and include a comment explaining that we don't support ThirdParty auth on Android, without logging anything (even at INFO level). https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... remoting/client/jni/chromoting_jni_instance.cc:110: net::android::RegisterJni(env); nit: Comment before this blobk to explain why we need it. https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... remoting/client/jni/chromoting_jni_instance.cc:112: LOG(INFO) << "starting main message loop"; nit: Capital at beginning of sentence, please. https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... remoting/client/jni/chromoting_jni_instance.cc:114: ui_loop_->Start(); Add a comment to explain this, since it differs from main-stream Chrome. https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... remoting/client/jni/chromoting_jni_instance.cc:134: // TODO(solb) detach all threads from JVM Why not just add the detaching here in this CL? https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... File remoting/client/jni/chromoting_jni_instance.h (right): https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... remoting/client/jni/chromoting_jni_instance.h:27: class ChromotingJNIInstance : public ClientUserInterface { Having ChromotingJNIInstance be a singleton, and implement ClientUserInterface means we're intrinsically tied to supporting only one client connection per process. Is that desirable? https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... remoting/client/jni/chromoting_jni_instance.h:29: static ChromotingJNIInstance* GetInstance(); nit: Add a comment to explain this, e.g. why it only makes sense for there to be one of these. https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... remoting/client/jni/chromoting_jni_instance.h:31: // Call from UI thread. See Google C++ style guide for comment style requirements, here and below. https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... remoting/client/jni/chromoting_jni_instance.h:42: // ClientUserInterface implementation: nit: : -> . https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... remoting/client/jni/chromoting_jni_instance.h:59: // Reusable between sessions: As above, : -> . and see style guide re comment style https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... remoting/client/jni/chromoting_jni_instance.h:62: scoped_ptr<base::MessageLoopForUI> ui_loop_; Add a comment to explain what |ui_loop_| is for, i.e. that it hooks us into the Java message loop. https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... remoting/client/jni/chromoting_jni_instance.h:63: scoped_refptr<AutoThreadTaskRunner> ui_runner_; nit: ui_task_runner_ https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... remoting/client/jni/chromoting_jni_instance.h:64: scoped_refptr<AutoThreadTaskRunner> net_runner_; nit: network_task_runner_ https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... remoting/client/jni/chromoting_jni_instance.h:65: scoped_refptr<AutoThreadTaskRunner> disp_runner_; nit: display_task_runner_ https://chromiumcodereview.appspot.com/18856012/diff/50001/remoting/client/jn... remoting/client/jni/chromoting_jni_instance.h:82: const char* pin_cstr_; Replace these and the jstring references with std::strings, and copy the contents across at Connect() time.
This is almost through CQ at this point, and there are other CLs waiting in line behind it, so I'm going to let it land. However, I'm addressing these concerns at https://codereview.chromium.org/18612018/. https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_instance.cc (right): https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:22: // For now, this just gives us access to the strings supplied from Java. On 2013/07/11 00:56:23, Wez wrote: > Remove this comment; it should be clear from the NOTIMPLEMENTED() below what's > going on. Done. https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:42: // TODO(solb) Initiate connection from here. On 2013/07/11 00:56:23, Wez wrote: > NOTIMPLEMENTED() here. Done. https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:70: protocol::ErrorCode error) {} On 2013/07/11 00:56:23, Wez wrote: > nit: This is a multi-line definition, so braces go on separate lines. Done. https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:72: void ChromotingJNIInstance::OnConnectionReady(bool ready) {} On 2013/07/11 00:56:23, Wez wrote: > Why are this and the next three methods not NOTIMPLEMENTED()? Although I am required (by ClientUserInterface's contract) to implement these methods, there is nothing to be done in response to their signals. There is neither an implementation nor an intent to ever create one. https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:77: const protocol::PairingResponse& response) {} On 2013/07/11 00:56:23, Wez wrote: > nit: See above re braces Done. https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:91: // want to print an error on every run. On 2013/07/11 00:56:23, Wez wrote: > This doesn't make sense - if it's going to be implemented but isn't yet then use > NOTIMPLEMENTED. If it's not likely to be implemented any time soon then just > return NULL and include a comment explaining that we don't support ThirdParty > auth on Android, without logging anything (even at INFO level). Done (for the latter case). Since scoped_ptr has no implicit constructor, what you see is the only NULL I can write here. https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:110: net::android::RegisterJni(env); On 2013/07/11 00:56:23, Wez wrote: > nit: Comment before this blobk to explain why we need it. Done. https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:112: LOG(INFO) << "starting main message loop"; On 2013/07/11 00:56:23, Wez wrote: > nit: Capital at beginning of sentence, please. Done. https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:114: ui_loop_->Start(); On 2013/07/11 00:56:23, Wez wrote: > Add a comment to explain this, since it differs from main-stream Chrome. Done. https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:134: // TODO(solb) detach all threads from JVM On 2013/07/11 00:56:23, Wez wrote: > Why not just add the detaching here in this CL? I just don't have an implementation ready at this point. I've tried it briefly, but it blew up the entire app with an unclean shutdown. I'll write up a permanent fix as soon as I have a chance, but for now I'm prioritizing getting things usable. https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_instance.h (right): https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:27: class ChromotingJNIInstance : public ClientUserInterface { On 2013/07/11 00:56:23, Wez wrote: > Having ChromotingJNIInstance be a singleton, and implement ClientUserInterface > means we're intrinsically tied to supporting only one client connection per > process. Is that desirable? Yes, we can only handle a single *concurrent* connection per process. However, ChromotingJNIInstance tries to be reusable in that calling its DisconnectFromHost() method restores it to a state where it can establish a *subsequent* connection, reusing many of the components from the first. Supporting multiple connections concurrently would require a whole boatload of changes across the application in any case: To start, we'd need some mechanism of communicating which client instance we were dealing with over JNI (reengineering the way we do JNI calls). Is this a goal? https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:29: static ChromotingJNIInstance* GetInstance(); On 2013/07/11 00:56:23, Wez wrote: > nit: Add a comment to explain this, e.g. why it only makes sense for there to be > one of these. Done. https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:31: // Call from UI thread. On 2013/07/11 00:56:23, Wez wrote: > See Google C++ style guide for comment style requirements, here and below. Done. https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:42: // ClientUserInterface implementation: On 2013/07/11 00:56:23, Wez wrote: > nit: : -> . Done. https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:59: // Reusable between sessions: On 2013/07/11 00:56:23, Wez wrote: > As above, : -> . and see style guide re comment style Done. https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:62: scoped_ptr<base::MessageLoopForUI> ui_loop_; On 2013/07/11 00:56:23, Wez wrote: > Add a comment to explain what |ui_loop_| is for, i.e. that it hooks us into the > Java message loop. Done. https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:63: scoped_refptr<AutoThreadTaskRunner> ui_runner_; On 2013/07/11 00:56:23, Wez wrote: > nit: ui_task_runner_ Done. https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:64: scoped_refptr<AutoThreadTaskRunner> net_runner_; On 2013/07/11 00:56:23, Wez wrote: > nit: network_task_runner_ Done. https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:65: scoped_refptr<AutoThreadTaskRunner> disp_runner_; On 2013/07/11 00:56:23, Wez wrote: > nit: display_task_runner_ Done. https://codereview.chromium.org/18856012/diff/50001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:82: const char* pin_cstr_; On 2013/07/11 00:56:23, Wez wrote: > Replace these and the jstring references with std::strings, and copy the > contents across at Connect() time. Done.
Message was sent while issue was closed.
Change committed as 211073 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
