Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1399613002:
Public glue layer plumbing for View#startActivityForResult (Closed)
|
Created:
5 years, 2 months ago by hush (inactive) Modified:
5 years, 2 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, android-webview-reviews_chromium.org, Torne Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPublic glue layer plumbing for View#startActivityForResult
BUG=521027
Committed: https://crrev.com/a58f351e3e6cc0aceac9f46802c7693c6a562557
Cr-Commit-Position: refs/heads/master@{#355390}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : using reflection for View#startActivityForResult #Patch Set 4 : proguard #Patch Set 5 : #
Total comments: 19
Patch Set 6 : comments #Patch Set 7 : comment s #Patch Set 8 : #
Total comments: 11
Patch Set 9 : address comments #Patch Set 10 : rebase #Patch Set 11 : fix test compile #
Total comments: 6
Patch Set 12 : #
Messages
Total messages: 41 (16 generated)
hush@chromium.org changed reviewers: + sgurun@chromium.org
PTAL
PTAL
https://codereview.chromium.org/1399613002/diff/80001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/FullScreenView.java (right): https://codereview.chromium.org/1399613002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/FullScreenView.java:235: // Intentional no-op We need to support this on FullScreenView, because a dom element can go to fullscreen mode by calling JS API requestFullscreen(). In order to send intents from a FullScreenView(which is a FrameLayout), I think we can pass AwContents into the constructor of FullScreenView and use AwContents to send the intent. But this will probably be done in a separate CL.
torne@chromium.org changed reviewers: + torne@chromium.org
https://codereview.chromium.org/1399613002/diff/80001/android_webview/apk/jav... File android_webview/apk/java/proguard.flags (right): https://codereview.chromium.org/1399613002/diff/80001/android_webview/apk/jav... android_webview/apk/java/proguard.flags:77: public void onActivityResult(int,int,android.content.Intent); Can you add this in a separate block, even though it's on the same class, just to make the comments make more sense? https://codereview.chromium.org/1399613002/diff/80001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): https://codereview.chromium.org/1399613002/diff/80001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:2266: throw new RuntimeException("Invalid reflection", e); Maybe you should API-level guard this function for >M, since even if the reflection succeeds it's not actually going to work on M devices due to the missing onActivityResult plumbing? Do we need a way to check this gracefully instead of throwing runtimeexception?
https://codereview.chromium.org/1399613002/diff/80001/android_webview/apk/jav... File android_webview/apk/java/proguard.flags (right): https://codereview.chromium.org/1399613002/diff/80001/android_webview/apk/jav... android_webview/apk/java/proguard.flags:76: #TODO(hush): remove after N release please add a catchall bug for cleanup after N release related to your feature, and add this clean up as the first item. bugs seems to have a better chance of being addressed in future. https://codereview.chromium.org/1399613002/diff/80001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): https://codereview.chromium.org/1399613002/diff/80001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:2260: // TODO(hush): Use mWebViewPrivate.super_startActivityForResult after N release. add this as a second clean up item in the bug I mentioned. https://codereview.chromium.org/1399613002/diff/80001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:2266: throw new RuntimeException("Invalid reflection", e); On 2015/10/14 13:42:58, Torne wrote: > Maybe you should API-level guard this function for >M, since even if the > reflection succeeds it's not actually going to work on M devices due to the > missing onActivityResult plumbing? Do we need a way to check this gracefully > instead of throwing runtimeexception? +1 also note that, in theory, which api level your frameworks/base changes will go is not known so >M may not work. but that is the best can be done. https://codereview.chromium.org/1399613002/diff/80001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1399613002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2183: context.startActivity(intent); how is this even supposed to work? startActivityForResult and startActivity are different things, do not mix and match. https://codereview.chromium.org/1399613002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2191: // replace the text with translated text. you will implement this in a follow up cl right after this one right? https://codereview.chromium.org/1399613002/diff/80001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/FullScreenView.java (right): https://codereview.chromium.org/1399613002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/FullScreenView.java:235: // Intentional no-op On 2015/10/14 01:21:32, hush wrote: > We need to support this on FullScreenView, because a dom element can go to > fullscreen mode by calling JS API requestFullscreen(). > > In order to send intents from a FullScreenView(which is a FrameLayout), I think > we can pass AwContents into the constructor of FullScreenView and use AwContents > to send the intent. > > But this will probably be done in a separate CL. Full screen view constructor already receives an AwContents, missed? public FullScreenView(Context context, AwViewMethods awViewMethods, AwContents awContents) { super(context); setAwViewMethods(awViewMethods); mAwContents = awContents; mInternalAccessAdapter = new InternalAccessAdapter(); }
https://codereview.chromium.org/1399613002/diff/80001/android_webview/apk/jav... File android_webview/apk/java/proguard.flags (right): https://codereview.chromium.org/1399613002/diff/80001/android_webview/apk/jav... android_webview/apk/java/proguard.flags:76: #TODO(hush): remove after N release On 2015/10/14 17:40:13, sgurun wrote: > please add a catchall bug for cleanup after N release related to your feature, > and add this clean up as the first item. > > bugs seems to have a better chance of being addressed in future. Done. https://codereview.chromium.org/1399613002/diff/80001/android_webview/apk/jav... android_webview/apk/java/proguard.flags:77: public void onActivityResult(int,int,android.content.Intent); On 2015/10/14 13:42:58, Torne wrote: > Can you add this in a separate block, even though it's on the same class, just > to make the comments make more sense? Done. https://codereview.chromium.org/1399613002/diff/80001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): https://codereview.chromium.org/1399613002/diff/80001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:2260: // TODO(hush): Use mWebViewPrivate.super_startActivityForResult after N release. On 2015/10/14 17:40:13, sgurun wrote: > add this as a second clean up item in the bug I mentioned. Done. https://codereview.chromium.org/1399613002/diff/80001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:2266: throw new RuntimeException("Invalid reflection", e); On 2015/10/14 17:40:13, sgurun wrote: > On 2015/10/14 13:42:58, Torne wrote: > > Maybe you should API-level guard this function for >M, since even if the > > reflection succeeds it's not actually going to work on M devices due to the > > missing onActivityResult plumbing? Do we need a way to check this gracefully > > instead of throwing runtimeexception? > > +1 > also note that, in theory, which api level your frameworks/base changes will go > is not known so >M may not work. but that is the best can be done. View#startActivityForResult will work in M. WebView just does not get notified of the result. So it is the same as calling context#startActivity. https://codereview.chromium.org/1399613002/diff/80001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:2266: throw new RuntimeException("Invalid reflection", e); On 2015/10/14 13:42:58, Torne wrote: > Maybe you should API-level guard this function for >M, since even if the > reflection succeeds it's not actually going to work on M devices due to the > missing onActivityResult plumbing? Do we need a way to check this gracefully > instead of throwing runtimeexception? This is function can only be called on M+ because the menu item to process text is only called when SDK level >= M. https://code.google.com/p/chromium/codesearch#chromium/src/content/public/and... On an M device, you just fire and forget (which is the current implementation in m47 already). This RuntimeException is not supposed to happen (unless OEM changes removes View#startActivityForResult in their M frameworks/base code). So I don't think there is a real need to gracefully handle an invalid reflection here. I can add an assert here about the SDK level though. https://codereview.chromium.org/1399613002/diff/80001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1399613002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2183: context.startActivity(intent); On 2015/10/14 17:40:13, sgurun wrote: > how is this even supposed to work? startActivityForResult and startActivity are > different things, do not mix and match. > Because the view context is not activity, startActivity is the best we can do. This just falls back to the current implementation of m47: fire and forget. https://code.google.com/p/chromium/codesearch#chromium/src/content/public/and... So we are not regressing anything. https://codereview.chromium.org/1399613002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2191: // replace the text with translated text. On 2015/10/14 17:40:13, sgurun wrote: > you will implement this in a follow up cl right after this one right? Yes. I have it in https://codereview.chromium.org/1409443002/. it is not published yet. https://codereview.chromium.org/1399613002/diff/80001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/FullScreenView.java (right): https://codereview.chromium.org/1399613002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/FullScreenView.java:235: // Intentional no-op On 2015/10/14 17:40:13, sgurun wrote: > On 2015/10/14 01:21:32, hush wrote: > > We need to support this on FullScreenView, because a dom element can go to > > fullscreen mode by calling JS API requestFullscreen(). > > > > In order to send intents from a FullScreenView(which is a FrameLayout), I > think > > we can pass AwContents into the constructor of FullScreenView and use > AwContents > > to send the intent. > > > > But this will probably be done in a separate CL. > > > Full screen view constructor already receives an AwContents, missed? > > public FullScreenView(Context context, AwViewMethods awViewMethods, > AwContents awContents) { > super(context); > setAwViewMethods(awViewMethods); > mAwContents = awContents; > mInternalAccessAdapter = new InternalAccessAdapter(); > } > So the goal here is to support "text processing" feature in fullscreen mode by firing the intents through the WebView object, instead of the framelayout object. In order to achieve this, I think AwContents#startActivityForResult should call super_startActivityForResult on the initial InternalAccessDelegate (that is WebViewChromium).
The CQ bit was checked by hush@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1399613002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1399613002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
https://codereview.chromium.org/1399613002/diff/80001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1399613002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2183: context.startActivity(intent); On 2015/10/14 21:33:59, hush wrote: > On 2015/10/14 17:40:13, sgurun wrote: > > how is this even supposed to work? startActivityForResult and startActivity > are > > different things, do not mix and match. > > > > Because the view context is not activity, startActivity is the best we can do. > This just falls back to the current implementation of m47: fire and forget. > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/and... > So we are not regressing anything. Nop, I don't agree. This is a general purpose method in AwContents class interface, which can be used later from other features. The expectation from StartActivityForResult is to start and activity that would return a result. In this case it is creating an unexpected behavior. If your goal is to solve the particular problem for translate, then you can catch the exception from StartActivityForResult, and then call startActivity separately. However, why is somebody calling translate from application context anyway?
https://codereview.chromium.org/1399613002/diff/80001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1399613002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2183: context.startActivity(intent); On 2015/10/14 23:08:01, sgurun wrote: > On 2015/10/14 21:33:59, hush wrote: > > On 2015/10/14 17:40:13, sgurun wrote: > > > how is this even supposed to work? startActivityForResult and startActivity > > are > > > different things, do not mix and match. > > > > > > > Because the view context is not activity, startActivity is the best we can do. > > This just falls back to the current implementation of m47: fire and forget. > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/and... > > So we are not regressing anything. > > Nop, I don't agree. This is a general purpose method in AwContents class > interface, which can be used later from other features. The expectation from > StartActivityForResult is to start and activity that would return a result. In > this case it is creating an unexpected behavior. > > If your goal is to solve the particular problem for translate, then you can > catch the exception from StartActivityForResult, and then call startActivity > separately. > > However, why is somebody calling translate from application context anyway? This if-statement is to address the case of firing text processing intents from non-Activity contexts. Initially I only intended this method to be called by text processing. But it seems this method will be called by other features too (dgn@'s feature). In that case I will make this method for general use. Like we've discussed, it is the caller's responsibility to make sure this method is always called with activity context. It is a programmer error otherwise, and a runtimeexception will be thrown by the Android framework.
https://codereview.chromium.org/1399613002/diff/140001/android_webview/glue/j... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): https://codereview.chromium.org/1399613002/diff/140001/android_webview/glue/j... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:1751: // Overrides WebViewProvider.ViewDelegate.onActivityResult (not in system api jar yet). add this to cleanup crbug. https://codereview.chromium.org/1399613002/diff/140001/android_webview/glue/j... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:2262: assert Build.VERSION.SDK_INT >= Build.VERSION_CODES.M; nit: I don't think java asserts are functional. https://codereview.chromium.org/1399613002/diff/140001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1399613002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:2179: assert ContentViewCore.activityFromContext(mContext) != null; same comment about assert. it will throw an exception anyway. the documentation below, once you move, will serve the same purpose. https://codereview.chromium.org/1399613002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:2181: // A RuntimeException will be thrown by Android framework if move the documentation to method level where it is more visible. https://codereview.chromium.org/1399613002/diff/140001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/FullScreenView.java (right): https://codereview.chromium.org/1399613002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/FullScreenView.java:234: public void super_startActivityForResult(Intent intent, int requestCode) { I think you should call FullScreenView.super.startActivityForResult() here.
lgtm https://codereview.chromium.org/1399613002/diff/140001/android_webview/glue/j... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): https://codereview.chromium.org/1399613002/diff/140001/android_webview/glue/j... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:2262: assert Build.VERSION.SDK_INT >= Build.VERSION_CODES.M; On 2015/10/16 00:06:53, sgurun wrote: > nit: I don't think java asserts are functional. Yeah, don't use asserts, they are off by default but still compiled into the code, so we get no runtime benefit and a binary size cost. :/
Oops, wrong button. I meant: looks okay once selim's remaining comments are addressed.
https://codereview.chromium.org/1399613002/diff/140001/android_webview/glue/j... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): https://codereview.chromium.org/1399613002/diff/140001/android_webview/glue/j... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:1751: // Overrides WebViewProvider.ViewDelegate.onActivityResult (not in system api jar yet). On 2015/10/16 00:06:53, sgurun wrote: > add this to cleanup crbug. Done. https://codereview.chromium.org/1399613002/diff/140001/android_webview/glue/j... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:2262: assert Build.VERSION.SDK_INT >= Build.VERSION_CODES.M; On 2015/10/16 12:49:40, Torne wrote: > On 2015/10/16 00:06:53, sgurun wrote: > > nit: I don't think java asserts are functional. > > Yeah, don't use asserts, they are off by default but still compiled into the > code, so we get no runtime benefit and a binary size cost. :/ Ok. I just deleted it. https://codereview.chromium.org/1399613002/diff/140001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1399613002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:2179: assert ContentViewCore.activityFromContext(mContext) != null; On 2015/10/16 00:06:53, sgurun wrote: > same comment about assert. it will throw an exception anyway. the documentation > below, once you move, will serve the same purpose. Done. https://codereview.chromium.org/1399613002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:2181: // A RuntimeException will be thrown by Android framework if On 2015/10/16 00:06:53, sgurun wrote: > move the documentation to method level where it is more visible. Done. https://codereview.chromium.org/1399613002/diff/140001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/FullScreenView.java (right): https://codereview.chromium.org/1399613002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/FullScreenView.java:234: public void super_startActivityForResult(Intent intent, int requestCode) { On 2015/10/16 00:06:53, sgurun wrote: > I think you should call FullScreenView.super.startActivityForResult() here. We discussed this bit offline. Just recap here: There is no public method FrameLayout#startActivityForResult. I have to get hold of it via reflection. And I can't get rid of the reflection even after N releases. What's more I have to implement onActivityResult for FrameLayout here, which is extra duplicate code with AwContents. That's why I am just using the initial delegate's super_startActivityForResult(that is super_startActivityForResult on WebViewChroium). In this way, WebViewChromium alone is responsible for sending intents and receiving results. And it is also the only place with reflection hack, which will be removed when N is released.
The CQ bit was checked by hush@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org Link to the patchset: https://codereview.chromium.org/1399613002/#ps160001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1399613002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1399613002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hush@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org Link to the patchset: https://codereview.chromium.org/1399613002/#ps180001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1399613002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1399613002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
The CQ bit was checked by hush@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org Link to the patchset: https://codereview.chromium.org/1399613002/#ps200001 (title: "fix test compile")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1399613002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1399613002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
boliu@chromium.org changed reviewers: + boliu@chromium.org
since you asked me to review the second patch.. https://codereview.chromium.org/1399613002/diff/200001/android_webview/glue/j... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): https://codereview.chromium.org/1399613002/diff/200001/android_webview/glue/j... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:2272: } catch (Exception e) { This might catch things not related to reflection? https://codereview.chromium.org/1399613002/diff/200001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1399613002/diff/200001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:2214: mFullScreenTransitionsState.getInitialInternalAccessDelegate() This deserves a comment of why not just mInternalAccessAdapter https://codereview.chromium.org/1399613002/diff/200001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/FullScreenView.java (right): https://codereview.chromium.org/1399613002/diff/200001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/FullScreenView.java:235: // Intentional no-op. startActivityForResult will only go through Could just raise a NotImplemented exception or something? This should be a NOTREACHED, not no-op.
https://codereview.chromium.org/1399613002/diff/200001/android_webview/glue/j... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): https://codereview.chromium.org/1399613002/diff/200001/android_webview/glue/j... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:2272: } catch (Exception e) { On 2015/10/21 00:22:17, boliu wrote: > This might catch things not related to reflection? If we have to be exact, it is going to be a whole bunch of exceptions: NoSuchMethodException, SecurityException, IllegalAccessException, IllegalArgumentException, InvocationTargetException I'm just following the code in WebViewDelegateFactory. This code will go away any way after N. Not really worth it to declare all the exceptions here. https://codereview.chromium.org/1399613002/diff/200001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1399613002/diff/200001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:2214: mFullScreenTransitionsState.getInitialInternalAccessDelegate() On 2015/10/21 00:22:17, boliu wrote: > This deserves a comment of why not just mInternalAccessAdapter Done. https://codereview.chromium.org/1399613002/diff/200001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/FullScreenView.java (right): https://codereview.chromium.org/1399613002/diff/200001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/FullScreenView.java:235: // Intentional no-op. startActivityForResult will only go through On 2015/10/21 00:22:17, boliu wrote: > Could just raise a NotImplemented exception or something? This should be a > NOTREACHED, not no-op. yes this is supposed to be not reached. I can raise a run time exception with a error message.
The CQ bit was checked by hush@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org Link to the patchset: https://codereview.chromium.org/1399613002/#ps220001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1399613002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1399613002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/a58f351e3e6cc0aceac9f46802c7693c6a562557 Cr-Commit-Position: refs/heads/master@{#355390} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
