Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2760543002:
Add a new class as an entry-point for Java reflection (Closed)
|
Created:
3 years, 9 months ago by Nate Fischer Modified:
3 years, 9 months ago CC:
agrieve+watch_chromium.org, android-webview-reviews_chromium.org, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a new class as an entry-point for Java reflection
This adds a new class named ReflectionEndPoint in the glue layer. This
class is intended to serve as a stable entry-point for Java reflection.
This adds one method to the class for disabling WebView Safe Browsing.
BUG=701189
Review-Url: https://codereview.chromium.org/2760543002
Cr-Commit-Position: refs/heads/master@{#458855}
Committed: https://chromium.googlesource.com/chromium/src/+/e5e53cb898e3f482f53043cdef84e2ae0129f035
Patch Set 1 #
Total comments: 8
Patch Set 2 : Moving to a new package and updating the class comment #
Messages
Total messages: 22 (15 generated)
ntfschr@chromium.org changed reviewers: + torne@chromium.org
The CQ bit was checked by ntfschr@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...
PTAL. Let me know if there's a better place for this, or if I can improve this in any way. I've manually verified this works on release & debug builds of WebView (proguard won't remove the class). My demo app can successfully call the method and disable Safe Browsing. I wasn't sure what we wanted to name the class, so I gave it a pretty simple name. If you have better suggestions, I'll gladly make the change.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ntfschr@chromium.org changed reviewers: + sgurun@chromium.org
torne@chromium.org changed reviewers: + tobiasjs@chromium.org
+toby who has been thinking about support lib stuff as well https://codereview.chromium.org/2760543002/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/ReflectionEndPoint.java (right): https://codereview.chromium.org/2760543002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/ReflectionEndPoint.java:5: package com.android.webview.chromium; Let's introduce a new package for this since I suspect we'll end up with more than one class (as soon as we need something non-static). Maybe com.android.webview.chromium.reflection ? https://codereview.chromium.org/2760543002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/ReflectionEndPoint.java:13: * Methods in this class are not guaranteed to remain, and may be removed when we no longer wish to Might want to be a bit more explicit in the comment here: we will be able to remove stuff from here, but we shouldn't change things in any other incompatible way (i.e. behaviour shouldn't become drastically different), because we don't want anyone doing version-based logic here. https://codereview.chromium.org/2760543002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/ReflectionEndPoint.java:16: @UsedByReflection("") Since we want to keep everything here, it might be better to just add a wildcard based on the class/package to the proguard config for webview, rather than using @UsedByReflection which will get pretty repetitive. We can just keep all public members here. https://codereview.chromium.org/2760543002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/ReflectionEndPoint.java:17: public class ReflectionEndPoint { If we introduce a dedicated package for this then probably this class could be called something more specific; maybe something like WebViewConfig (if we're just going to have static configuration methods on it) Not sure what's ideal though.
ntfschr@chromium.org changed reviewers: - sgurun@chromium.org
PTAL https://codereview.chromium.org/2760543002/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/ReflectionEndPoint.java (right): https://codereview.chromium.org/2760543002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/ReflectionEndPoint.java:5: package com.android.webview.chromium; On 2017/03/20 at 13:35:49, Torne wrote: > Let's introduce a new package for this since I suspect we'll end up with more than one class (as soon as we need something non-static). Maybe com.android.webview.chromium.reflection ? done https://codereview.chromium.org/2760543002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/ReflectionEndPoint.java:13: * Methods in this class are not guaranteed to remain, and may be removed when we no longer wish to On 2017/03/20 at 13:35:49, Torne wrote: > Might want to be a bit more explicit in the comment here: we will be able to remove stuff from here, but we shouldn't change things in any other incompatible way (i.e. behaviour shouldn't become drastically different), because we don't want anyone doing version-based logic here. done https://codereview.chromium.org/2760543002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/ReflectionEndPoint.java:16: @UsedByReflection("") On 2017/03/20 at 13:35:49, Torne wrote: > Since we want to keep everything here, it might be better to just add a wildcard based on the class/package to the proguard config for webview, rather than using @UsedByReflection which will get pretty repetitive. We can just keep all public members here. While I see the advantages to that, I'd still like to argue in favor of the explicit @UsedByReflection: - Without the annotation, someone may be enticed to add their new static method (perhaps a new API) to this class, simply to avoid proguard stripping it. The annotation discourages this behavior, and encourages the programmer to find a more appropriate class for their method. - Without the annotation, someone reading the code may be confused why the method is never called. Best case, they waste time being needlessly confused; worse case, they remove the method to "fix" the code. Reflection should be limited, so I don't think the burden of @UsedByReflection will be too great. I don't think we can rely on the package name here, as programmers often overlook package names (I know I often do). Perhaps I'm underestimating the scale of static reflection methods we'll need. If you or Toby feel strongly about this, I will make the change. https://codereview.chromium.org/2760543002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/ReflectionEndPoint.java:17: public class ReflectionEndPoint { On 2017/03/20 at 13:35:49, Torne wrote: > If we introduce a dedicated package for this then probably this class could be called something more specific; maybe something like WebViewConfig (if we're just going to have static configuration methods on it) > > Not sure what's ideal though. WebViewConfig SGTM. I'm open to other suggestions from Toby
The CQ bit was checked by ntfschr@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2017/03/20 19:57:48, Nate Fischer wrote: > https://codereview.chromium.org/2760543002/diff/1/android_webview/glue/java/s... > android_webview/glue/java/src/com/android/webview/chromium/ReflectionEndPoint.java:16: > @UsedByReflection("") > On 2017/03/20 at 13:35:49, Torne wrote: > > Since we want to keep everything here, it might be better to just add a > wildcard based on the class/package to the proguard config for webview, rather > than using @UsedByReflection which will get pretty repetitive. We can just keep > all public members here. > > While I see the advantages to that, I'd still like to argue in favor of the > explicit @UsedByReflection: > > - Without the annotation, someone may be enticed to add their new static method > (perhaps a new API) to this class, simply to avoid proguard stripping it. The > annotation discourages this behavior, and encourages the programmer to find a > more appropriate class for their method. I don't think that's a real concern; the class pretty clearly explains what it's for, and we review code. We shouldn't be letting anyone make that kind of change :) > - Without the annotation, someone reading the code may be confused why the > method is never called. Best case, they waste time being needlessly confused; > worse case, they remove the method to "fix" the code. > > Reflection should be limited, so I don't think the burden of @UsedByReflection > will be too great. I don't think we can rely on the package name here, as > programmers often overlook package names (I know I often do). > > Perhaps I'm underestimating the scale of static reflection methods we'll need. > If you or Toby feel strongly about this, I will make the change. I think it's fine to leave it as it is for now. I'm guessing the support library implementation is likely to lead to having many more things exposed by reflection here, but if that starts to be cumbersome we can change it then. Thanks; LGTM.
On 2017/03/21 at 10:45:12, torne wrote: > On 2017/03/20 19:57:48, Nate Fischer wrote: > > https://codereview.chromium.org/2760543002/diff/1/android_webview/glue/java/s... > > android_webview/glue/java/src/com/android/webview/chromium/ReflectionEndPoint.java:16: > > @UsedByReflection("") > > On 2017/03/20 at 13:35:49, Torne wrote: > > > Since we want to keep everything here, it might be better to just add a > > wildcard based on the class/package to the proguard config for webview, rather > > than using @UsedByReflection which will get pretty repetitive. We can just keep > > all public members here. > > > > While I see the advantages to that, I'd still like to argue in favor of the > > explicit @UsedByReflection: > > > > - Without the annotation, someone may be enticed to add their new static method > > (perhaps a new API) to this class, simply to avoid proguard stripping it. The > > annotation discourages this behavior, and encourages the programmer to find a > > more appropriate class for their method. > > I don't think that's a real concern; the class pretty clearly explains what it's for, and we review code. We shouldn't be letting anyone make that kind of change :) > > > - Without the annotation, someone reading the code may be confused why the > > method is never called. Best case, they waste time being needlessly confused; > > worse case, they remove the method to "fix" the code. > > > > Reflection should be limited, so I don't think the burden of @UsedByReflection > > will be too great. I don't think we can rely on the package name here, as > > programmers often overlook package names (I know I often do). > > > > Perhaps I'm underestimating the scale of static reflection methods we'll need. > > If you or Toby feel strongly about this, I will make the change. > > I think it's fine to leave it as it is for now. I'm guessing the support library implementation is likely to lead to having many more things exposed by reflection here, but if that starts to be cumbersome we can change it then. > > Thanks; LGTM. Ok, I'll land this. If Toby has any comments, we can do a follow-up CL to fix.
The CQ bit was checked by ntfschr@chromium.org
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": 20001, "attempt_start_ts": 1490207587706320,
"parent_rev": "2cd67b5b3f57bd69c5a88e927852600f7b956912", "commit_rev":
"e5e53cb898e3f482f53043cdef84e2ae0129f035"}
Message was sent while issue was closed.
Description was changed from ========== Add a new class as an entry-point for Java reflection This adds a new class named ReflectionEndPoint in the glue layer. This class is intended to serve as a stable entry-point for Java reflection. This adds one method to the class for disabling WebView Safe Browsing. BUG=701189 ========== to ========== Add a new class as an entry-point for Java reflection This adds a new class named ReflectionEndPoint in the glue layer. This class is intended to serve as a stable entry-point for Java reflection. This adds one method to the class for disabling WebView Safe Browsing. BUG=701189 Review-Url: https://codereview.chromium.org/2760543002 Cr-Commit-Position: refs/heads/master@{#458855} Committed: https://chromium.googlesource.com/chromium/src/+/e5e53cb898e3f482f53043cdef84... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/e5e53cb898e3f482f53043cdef84... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
