Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2191743002:
Setup the delegate in Chrome, so we may put chrome java functions we need in blimp to the delegate … (Closed)
|
Created:
4 years, 4 months ago by xingliu Modified:
4 years, 4 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSetup the delegate in Chrome for java calls.
Previously we have some separate impelmentation to call embedder
functionality, such as BlimpSettingsCallbacks.
This CL add a new way to call embedder's s java function in
ChromeBlimpClientContextDelegate class, and add a java interface
BlimpClientContextDelegate so in Blimp we can talk to this interface to
ask the embedder to do stuff in java.
The different from previous callback mechanism is that
ChromeBlimpClientContextDelegate is also backed by native class, and it
can associate itself with certain BlimpClientContext based on certain
Profile in c++.
Under current test infrastructure, it's very hard to test this delegate
mechanism, so pure java callbacks will still in use.
BUG=611097
Committed: https://crrev.com/448013905e45fc47bbc4d648ad64ec81efc1392a
Cr-Commit-Position: refs/heads/master@{#409675}
Patch Set 1 #Patch Set 2 : Fix blimp_unittests target. #Patch Set 3 : Resovle merge conflicts. #
Total comments: 18
Patch Set 4 : Change comments and function names. #Patch Set 5 : Move delegate creation into blocking call. #Patch Set 6 : Fix issue when moving around code. #
Total comments: 6
Patch Set 7 : Fix based on codereview. #Patch Set 8 : Rebased. #
Total comments: 5
Patch Set 9 : Java delegate is now set in Java. #
Total comments: 6
Patch Set 10 : Fix nit issues. #Messages
Total messages: 63 (47 generated)
Description was changed from ========== Setup the delegate in Chrome, so we may put chrome java functions we need in blimp to the delegate class. BUG= ========== to ========== Setup the delegate in Chrome, so we may put chrome java functions we need in blimp to the delegate class. Previously we don't have a way to call java functions in chrome from blimp. In setting page CL, there is BlimpSettingCallback java class created for this purpose. This CL move the chrome's java function we need to the ChromeBlimpClientContextDelegate class, and add a java interface BlimpClientContextDelegate so in Blimp we can talk to this interface to ask the embedder to do stuff. The first patch don't contain test code, will create second patch to add tests. BUG= ==========
xingliu@chromium.org changed reviewers: + dtrainor@chromium.org, nyquist@chromium.org
Make ChromeBlimpClientContextDelegate can be access from BlimpClientContext, and we can put Chrome calls needed for blimp in this class.
The CQ bit was checked by xingliu@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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by xingliu@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-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by xingliu@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-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) 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_gyp_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gyp_...)
Description was changed from ========== Setup the delegate in Chrome, so we may put chrome java functions we need in blimp to the delegate class. Previously we don't have a way to call java functions in chrome from blimp. In setting page CL, there is BlimpSettingCallback java class created for this purpose. This CL move the chrome's java function we need to the ChromeBlimpClientContextDelegate class, and add a java interface BlimpClientContextDelegate so in Blimp we can talk to this interface to ask the embedder to do stuff. The first patch don't contain test code, will create second patch to add tests. BUG= ========== to ========== Setup the delegate in Chrome, so we may put chrome java functions we need in blimp to the delegate class. Previously we don't have a way to call java functions in chrome from blimp. In setting page CL, there is BlimpSettingCallback java class created for this purpose. This CL move the chrome's java function we need to the ChromeBlimpClientContextDelegate class, and add a java interface BlimpClientContextDelegate so in Blimp we can talk to this interface to ask the embedder to do stuff. Didn't figure out a good way to test this class though. BUG= ==========
The CQ bit was checked by xingliu@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...
Could you also update the CL description like we discussed? https://codereview.chromium.org/2191743002/diff/40001/blimp/client/core/andro... File blimp/client/core/android/blimp_client_context_impl_android.h (right): https://codereview.chromium.org/2191743002/diff/40001/blimp/client/core/andro... blimp/client/core/android/blimp_client_context_impl_android.h:31: base::android::ScopedJavaLocalRef<jobject> GetClientContextDelegate( Could this be GetDelegateJavaObject? https://codereview.chromium.org/2191743002/diff/40001/blimp/client/core/andro... File blimp/client/core/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java (right): https://codereview.chromium.org/2191743002/diff/40001/blimp/client/core/andro... blimp/client/core/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java:25: * Get embedder delegate, so we can access chrome functionalities. "Get embedder delegate which provides necessary functionality and callbacks." Also, change to 'embedder' instead of Chrome below as well. https://codereview.chromium.org/2191743002/diff/40001/blimp/client/core/andro... blimp/client/core/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java:28: public BlimpClientContextDelegate getClientContextDelegate() { Just getDelegate() to match the C++ BlimpClientContextImpl function. https://codereview.chromium.org/2191743002/diff/40001/blimp/client/core/blimp... File blimp/client/core/blimp_client_context_impl.h (right): https://codereview.chromium.org/2191743002/diff/40001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.h:41: BlimpClientContextDelegate* GetDelegate(); This is not part of the BlimpClientContext implementation, so it should be moved out separately, and a comment added. https://codereview.chromium.org/2191743002/diff/40001/blimp/client/public/and... File blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContextDelegate.java (right): https://codereview.chromium.org/2191743002/diff/40001/blimp/client/public/and... blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContextDelegate.java:8: * Interface for java functionalities in chrome that are called from Blimp. I think we can just use 'embedder' instead of Chrome, since the plan would be for the blimp shell also to maybe use this. You can probably just use the same as in C++: BlimpClientContextDelegate is how the BlimpClientContext gets thefunctionality it needs from its embedder. https://codereview.chromium.org/2191743002/diff/40001/blimp/client/public/and... blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContextDelegate.java:16: public void restartBrowser(); I think this can be removed for now. https://codereview.chromium.org/2191743002/diff/40001/blimp/client/test/test_... File blimp/client/test/test_blimp_client_context_delegate.h (right): https://codereview.chromium.org/2191743002/diff/40001/blimp/client/test/test_... blimp/client/test/test_blimp_client_context_delegate.h:33: // Returns a Java object of the type BlimpClientContextDelegate. Remove the comment and move it up to the "BlimpClientContextDelegate implemenation" section. https://codereview.chromium.org/2191743002/diff/40001/chrome/browser/android/... File chrome/browser/android/blimp/chrome_blimp_client_context_delegate.h (right): https://codereview.chromium.org/2191743002/diff/40001/chrome/browser/android/... chrome/browser/android/blimp/chrome_blimp_client_context_delegate.h:36: base::android::ScopedJavaLocalRef<jobject> GetJavaObject() override; Can this be removed from this class, since it's never instantiated on Android? https://codereview.chromium.org/2191743002/diff/40001/chrome/browser/android/... File chrome/browser/android/blimp/chrome_blimp_client_context_delegate_android.h (right): https://codereview.chromium.org/2191743002/diff/40001/chrome/browser/android/... chrome/browser/android/blimp/chrome_blimp_client_context_delegate_android.h:25: base::android::ScopedJavaLocalRef<jobject> GetJavaObject() override; I guess this should technically have "// BlimpClientContextDelegate implementation." above it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Setup the delegate in Chrome, so we may put chrome java functions we need in blimp to the delegate class. Previously we don't have a way to call java functions in chrome from blimp. In setting page CL, there is BlimpSettingCallback java class created for this purpose. This CL move the chrome's java function we need to the ChromeBlimpClientContextDelegate class, and add a java interface BlimpClientContextDelegate so in Blimp we can talk to this interface to ask the embedder to do stuff. Didn't figure out a good way to test this class though. BUG= ========== to ========== Setup the delegate in Chrome for java calls. Previously we have some separate impelmentation to call embedder functionality, such as BlimpSettingsCallbacks. This CL add a new way to call embedder's s java function in ChromeBlimpClientContextDelegate class, and add a java interface BlimpClientContextDelegate so in Blimp we can talk to this interface to ask the embedder to do stuff in java. The different from previous callback mechanism is that ChromeBlimpClientContextDelegate is also backed by native class, and it can associate itself with certain BlimpClientContext based on certain Profile in c++. Under current test infrastructure, it's very hard to test this delegate mechanism, so pure java callbacks will still in use. BUG= ==========
The CQ bit was checked by xingliu@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_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 xingliu@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...
Fix based on code review. Move creation of delegate object in Java to finishNativeInitialization. Since previous point of creation is in an async call, and can be slow and cause getDelegate to return null. Also add comment to address this issue in function comment. https://codereview.chromium.org/2191743002/diff/40001/blimp/client/core/andro... File blimp/client/core/android/blimp_client_context_impl_android.h (right): https://codereview.chromium.org/2191743002/diff/40001/blimp/client/core/andro... blimp/client/core/android/blimp_client_context_impl_android.h:31: base::android::ScopedJavaLocalRef<jobject> GetClientContextDelegate( On 2016/07/28 19:15:34, nyquist wrote: > Could this be GetDelegateJavaObject? Done. https://codereview.chromium.org/2191743002/diff/40001/blimp/client/core/andro... File blimp/client/core/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java (right): https://codereview.chromium.org/2191743002/diff/40001/blimp/client/core/andro... blimp/client/core/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java:25: * Get embedder delegate, so we can access chrome functionalities. On 2016/07/28 19:15:34, nyquist wrote: > "Get embedder delegate which provides necessary functionality and callbacks." > > Also, change to 'embedder' instead of Chrome below as well. Done. https://codereview.chromium.org/2191743002/diff/40001/blimp/client/core/andro... blimp/client/core/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java:28: public BlimpClientContextDelegate getClientContextDelegate() { On 2016/07/28 19:15:34, nyquist wrote: > Just getDelegate() to match the C++ BlimpClientContextImpl function. Done. https://codereview.chromium.org/2191743002/diff/40001/blimp/client/core/blimp... File blimp/client/core/blimp_client_context_impl.h (right): https://codereview.chromium.org/2191743002/diff/40001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.h:41: BlimpClientContextDelegate* GetDelegate(); On 2016/07/28 19:15:34, nyquist wrote: > This is not part of the BlimpClientContext implementation, so it should be moved > out separately, and a comment added. Done. https://codereview.chromium.org/2191743002/diff/40001/blimp/client/public/and... File blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContextDelegate.java (right): https://codereview.chromium.org/2191743002/diff/40001/blimp/client/public/and... blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContextDelegate.java:8: * Interface for java functionalities in chrome that are called from Blimp. On 2016/07/28 19:15:34, nyquist wrote: > I think we can just use 'embedder' instead of Chrome, since the plan would be > for the blimp shell also to maybe use this. > > You can probably just use the same as in C++: > BlimpClientContextDelegate is how the BlimpClientContext gets thefunctionality > it needs from its embedder. Done. https://codereview.chromium.org/2191743002/diff/40001/blimp/client/public/and... blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContextDelegate.java:16: public void restartBrowser(); On 2016/07/28 19:15:34, nyquist wrote: > I think this can be removed for now. Done. https://codereview.chromium.org/2191743002/diff/40001/blimp/client/test/test_... File blimp/client/test/test_blimp_client_context_delegate.h (right): https://codereview.chromium.org/2191743002/diff/40001/blimp/client/test/test_... blimp/client/test/test_blimp_client_context_delegate.h:33: // Returns a Java object of the type BlimpClientContextDelegate. On 2016/07/28 19:15:34, nyquist wrote: > Remove the comment and move it up to the "BlimpClientContextDelegate > implemenation" section. Done. https://codereview.chromium.org/2191743002/diff/40001/chrome/browser/android/... File chrome/browser/android/blimp/chrome_blimp_client_context_delegate.h (right): https://codereview.chromium.org/2191743002/diff/40001/chrome/browser/android/... chrome/browser/android/blimp/chrome_blimp_client_context_delegate.h:36: base::android::ScopedJavaLocalRef<jobject> GetJavaObject() override; On 2016/07/28 19:15:34, nyquist wrote: > Can this be removed from this class, since it's never instantiated on Android? Done, but didn't change it to abstract class. https://codereview.chromium.org/2191743002/diff/40001/chrome/browser/android/... File chrome/browser/android/blimp/chrome_blimp_client_context_delegate_android.h (right): https://codereview.chromium.org/2191743002/diff/40001/chrome/browser/android/... chrome/browser/android/blimp/chrome_blimp_client_context_delegate_android.h:25: base::android::ScopedJavaLocalRef<jobject> GetJavaObject() override; On 2016/07/28 19:15:35, nyquist wrote: > I guess this should technically have "// BlimpClientContextDelegate > implementation." above it. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This mostly looks good. Could you update the CL description as well? https://codereview.chromium.org/2191743002/diff/100001/blimp/client/public/an... File blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContextDelegate.java (right): https://codereview.chromium.org/2191743002/diff/100001/blimp/client/public/an... blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContextDelegate.java:10: * Nit: Remove empty line. https://codereview.chromium.org/2191743002/diff/100001/blimp/client/public/bl... File blimp/client/public/blimp_client_context_delegate.h (right): https://codereview.chromium.org/2191743002/diff/100001/blimp/client/public/bl... blimp/client/public/blimp_client_context_delegate.h:12: #include "base/android/jni_android.h" Nit: Unindent https://codereview.chromium.org/2191743002/diff/100001/blimp/client/test/test... File blimp/client/test/test_blimp_client_context_delegate.h (right): https://codereview.chromium.org/2191743002/diff/100001/blimp/client/test/test... blimp/client/test/test_blimp_client_context_delegate.h:12: #include "base/android/jni_android.h" Nit: Unindent https://codereview.chromium.org/2191743002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2191743002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:236: private ChromeBlimpClientContextDelegate mBlimpClientContextDelegate; Could you add an empty line and then add a comment as to what this is? https://codereview.chromium.org/2191743002/diff/100001/chrome/browser/android... File chrome/browser/android/blimp/chrome_blimp_client_context_delegate.cc (right): https://codereview.chromium.org/2191743002/diff/100001/chrome/browser/android... chrome/browser/android/blimp/chrome_blimp_client_context_delegate.cc:31: Nit: Remove empty line.
Thanks for the review. The new patch fixes all the nit issues and added a comment in ChromeActivity for the delegate. https://codereview.chromium.org/2191743002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2191743002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:236: private ChromeBlimpClientContextDelegate mBlimpClientContextDelegate; On 2016/08/01 18:24:07, nyquist wrote: > Could you add an empty line and then add a comment as to what this is? Done.
The CQ bit was checked by xingliu@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: This issue passed the CQ dry run.
Also maybe format the cl description to 72 chars and add: BUG=611097
Description was changed from ========== Setup the delegate in Chrome for java calls. Previously we have some separate impelmentation to call embedder functionality, such as BlimpSettingsCallbacks. This CL add a new way to call embedder's s java function in ChromeBlimpClientContextDelegate class, and add a java interface BlimpClientContextDelegate so in Blimp we can talk to this interface to ask the embedder to do stuff in java. The different from previous callback mechanism is that ChromeBlimpClientContextDelegate is also backed by native class, and it can associate itself with certain BlimpClientContext based on certain Profile in c++. Under current test infrastructure, it's very hard to test this delegate mechanism, so pure java callbacks will still in use. BUG= ========== to ========== Setup the delegate in Chrome for java calls. Previously we have some separate impelmentation to call embedder functionality, such as BlimpSettingsCallbacks. This CL add a new way to call embedder's s java function in ChromeBlimpClientContextDelegate class, and add a java interface BlimpClientContextDelegate so in Blimp we can talk to this interface to ask the embedder to do stuff in java. The different from previous callback mechanism is that ChromeBlimpClientContextDelegate is also backed by native class, and it can associate itself with certain BlimpClientContext based on certain Profile in c++. Under current test infrastructure, it's very hard to test this delegate mechanism, so pure java callbacks will still in use. BUG=611097 ==========
On 2016/08/03 02:03:11, nyquist wrote: > Also maybe format the cl description to 72 chars and add: > BUG=611097 CL description updated.
https://codereview.chromium.org/2191743002/diff/140001/blimp/client/core/andr... File blimp/client/core/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java (right): https://codereview.chromium.org/2191743002/diff/140001/blimp/client/core/andr... blimp/client/core/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java:33: return nativeGetDelegateJavaObject(mNativeBlimpClientContextImplAndroid); assert mNativeBlimpClientContextImplAndroid != 0? If it is 0 should we just return null? https://codereview.chromium.org/2191743002/diff/140001/blimp/client/public/bl... File blimp/client/public/blimp_client_context_delegate.h (right): https://codereview.chromium.org/2191743002/diff/140001/blimp/client/public/bl... blimp/client/public/blimp_client_context_delegate.h:25: virtual base::android::ScopedJavaLocalRef<jobject> GetJavaObject() = 0; Would a static GetJavaObject(Delegate* delegate) be cleaner? I think I've seen both ways of doing this in Chrome. I'm fine with either way as it won't really be used except for the JNI bridge access. https://codereview.chromium.org/2191743002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2191743002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:863: mBlimpClientContextDelegate.destroy(); mBlimpClientContextDelegate = null; after? https://codereview.chromium.org/2191743002/diff/140001/chrome/browser/android... File chrome/browser/android/blimp/chrome_blimp_client_context_delegate.h (right): https://codereview.chromium.org/2191743002/diff/140001/chrome/browser/android... chrome/browser/android/blimp/chrome_blimp_client_context_delegate.h:37: base::android::ScopedJavaLocalRef<jobject> GetJavaObject() override; Do we need this access at the Chrome layer now?
The CQ bit was checked by xingliu@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: This issue passed the CQ dry run.
Thanks for the review, this patch make the java delegate set in java. It's much simpler now. https://codereview.chromium.org/2191743002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2191743002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:863: mBlimpClientContextDelegate.destroy(); On 2016/08/03 19:39:54, David Trainor wrote: > mBlimpClientContextDelegate = null; after? Done.
The CQ bit was checked by xingliu@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...
I think this looks great! Could you please update the CL description to reflect this new simple world? :-) https://codereview.chromium.org/2191743002/diff/160001/blimp/client/core/andr... File blimp/client/core/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java (right): https://codereview.chromium.org/2191743002/diff/160001/blimp/client/core/andr... blimp/client/core/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java:24: private BlimpClientContextDelegate mDelegate; Do we want to explain what this member field is? https://codereview.chromium.org/2191743002/diff/160001/blimp/client/public/an... File blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContextDelegate.java (right): https://codereview.chromium.org/2191743002/diff/160001/blimp/client/public/an... blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContextDelegate.java:8: * BlimpClientContextDelegate contains all embedder's Java functions used by Blimp java classes. Nit: "Java" https://codereview.chromium.org/2191743002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java (right): https://codereview.chromium.org/2191743002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java:45: // Set java delegate object. Nit: "Set ourselves as the Java delegate."
Fixed a couple of nit issues. https://codereview.chromium.org/2191743002/diff/160001/blimp/client/core/andr... File blimp/client/core/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java (right): https://codereview.chromium.org/2191743002/diff/160001/blimp/client/core/andr... blimp/client/core/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java:24: private BlimpClientContextDelegate mDelegate; On 2016/08/03 22:16:14, nyquist wrote: > Do we want to explain what this member field is? Done. https://codereview.chromium.org/2191743002/diff/160001/blimp/client/public/an... File blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContextDelegate.java (right): https://codereview.chromium.org/2191743002/diff/160001/blimp/client/public/an... blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContextDelegate.java:8: * BlimpClientContextDelegate contains all embedder's Java functions used by Blimp java classes. On 2016/08/03 22:16:14, nyquist wrote: > Nit: "Java" Done. https://codereview.chromium.org/2191743002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java (right): https://codereview.chromium.org/2191743002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java:45: // Set java delegate object. On 2016/08/03 22:16:14, nyquist wrote: > Nit: "Set ourselves as the Java delegate." Done.
The CQ bit was checked by xingliu@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...
lgtm
On 2016/08/03 23:05:43, nyquist wrote: > lgtm Thanks!
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 xingliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Setup the delegate in Chrome for java calls. Previously we have some separate impelmentation to call embedder functionality, such as BlimpSettingsCallbacks. This CL add a new way to call embedder's s java function in ChromeBlimpClientContextDelegate class, and add a java interface BlimpClientContextDelegate so in Blimp we can talk to this interface to ask the embedder to do stuff in java. The different from previous callback mechanism is that ChromeBlimpClientContextDelegate is also backed by native class, and it can associate itself with certain BlimpClientContext based on certain Profile in c++. Under current test infrastructure, it's very hard to test this delegate mechanism, so pure java callbacks will still in use. BUG=611097 ========== to ========== Setup the delegate in Chrome for java calls. Previously we have some separate impelmentation to call embedder functionality, such as BlimpSettingsCallbacks. This CL add a new way to call embedder's s java function in ChromeBlimpClientContextDelegate class, and add a java interface BlimpClientContextDelegate so in Blimp we can talk to this interface to ask the embedder to do stuff in java. The different from previous callback mechanism is that ChromeBlimpClientContextDelegate is also backed by native class, and it can associate itself with certain BlimpClientContext based on certain Profile in c++. Under current test infrastructure, it's very hard to test this delegate mechanism, so pure java callbacks will still in use. BUG=611097 Committed: https://crrev.com/448013905e45fc47bbc4d648ad64ec81efc1392a Cr-Commit-Position: refs/heads/master@{#409675} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/448013905e45fc47bbc4d648ad64ec81efc1392a Cr-Commit-Position: refs/heads/master@{#409675} |
