Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
273313003:
Fix lint issues with Chromoting for Android. (Closed)
|
Created:
6 years, 7 months ago by Sungmann Cho Modified:
6 years, 7 months ago CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionFix lint issues with Chromoting for Android.
Currently, the lint warns at:
Error: Field requires API level 18 (current min is 14):
android.provider.Settings#EXTRA_ACCOUNT_TYPES [InlinedApi]
Error: Field requires API level 16 (current min is 14):
android.view.View#SYSTEM_UI_FLAG_FULLSCREEN [InlinedApi]
So we need to add @TargetApi to suppress these lint error.
BUG=327768
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269995
Patch Set 1 #
Total comments: 3
Messages
Total messages: 11 (0 generated)
Please take a look. Thanks!
https://codereview.chromium.org/273313003/diff/1/remoting/android/java/src/or... File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): https://codereview.chromium.org/273313003/diff/1/remoting/android/java/src/or... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:107: intent.putExtra(Settings.EXTRA_ACCOUNT_TYPES, Doesn't this code need a if (Build.VERSION.SDK_INT >= ... check around the EXTRA_ACCOUNT_TYPES as well?
lgtm https://codereview.chromium.org/273313003/diff/1/remoting/android/java/src/or... File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): https://codereview.chromium.org/273313003/diff/1/remoting/android/java/src/or... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:107: intent.putExtra(Settings.EXTRA_ACCOUNT_TYPES, On 2014/05/12 17:37:14, garykac wrote: > Doesn't this code need a > if (Build.VERSION.SDK_INT >= ... > check around the EXTRA_ACCOUNT_TYPES as well? > > I'm not sure. It works fine on an older device with Android 4.0.3, and there's no sign of any crash in the log. I guess this is because EXTRA_ACCOUNT_TYPES is a compile-time integer constant, and so the generated bytecode doesn't need to do a runtime field lookup, which would throw an exception on older versions. All that happens is that the Intent ends up with extra information that the older Android OS doesn't know about so it is simply ignored. If the check is needed, it is only for style reasons. I'm happy to defer judgement on this.
On 2014/05/12 20:39:29, Lambros wrote: > lgtm > > https://codereview.chromium.org/273313003/diff/1/remoting/android/java/src/or... > File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): > > https://codereview.chromium.org/273313003/diff/1/remoting/android/java/src/or... > remoting/android/java/src/org/chromium/chromoting/Chromoting.java:107: > intent.putExtra(Settings.EXTRA_ACCOUNT_TYPES, > On 2014/05/12 17:37:14, garykac wrote: > > Doesn't this code need a > > if (Build.VERSION.SDK_INT >= ... > > check around the EXTRA_ACCOUNT_TYPES as well? > > > > > I'm not sure. It works fine on an older device with Android 4.0.3, and there's > no sign of any crash in the log. > > I guess this is because EXTRA_ACCOUNT_TYPES is a compile-time integer constant, > and so the generated bytecode doesn't need to do a runtime field lookup, which > would throw an exception on older versions. All that happens is that the Intent > ends up with extra information that the older Android OS doesn't know about so > it is simply ignored. > > If the check is needed, it is only for style reasons. I'm happy to defer > judgement on this. sgtm
The CQ bit was checked by sungmann.cho@navercorp.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sungmann.cho@navercorp.com/273313003/1
Message was sent while issue was closed.
Change committed as 269995
Message was sent while issue was closed.
drive-by after-the-fact review :) https://codereview.chromium.org/273313003/diff/1/remoting/android/java/src/or... File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): https://codereview.chromium.org/273313003/diff/1/remoting/android/java/src/or... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:107: intent.putExtra(Settings.EXTRA_ACCOUNT_TYPES, On 2014/05/12 20:39:29, Lambros wrote: > On 2014/05/12 17:37:14, garykac wrote: > > Doesn't this code need a > > if (Build.VERSION.SDK_INT >= ... > > check around the EXTRA_ACCOUNT_TYPES as well? > > > > > I'm not sure. It works fine on an older device with Android 4.0.3, and there's > no sign of any crash in the log. > > I guess this is because EXTRA_ACCOUNT_TYPES is a compile-time integer constant, > and so the generated bytecode doesn't need to do a runtime field lookup, which > would throw an exception on older versions. All that happens is that the Intent > ends up with extra information that the older Android OS doesn't know about so > it is simply ignored. Correct. Constants on Android are typically static final, so their values are inlined at compile time. This means this code won't crash at runtime on old versions of Android, but could -- in theory -- fail in mysterious ways. To suppress the warning, you can either use the @TargetApi annotation if the code is only run on higher versions of Android, or if the warning doesn't actually indicate a problem (which often seems to be the case with InlinedApi), you can use @SuppressLint("InlinedApi"). I think the latter would actually be appropriate here... unless this code path never runs on Android devices running JB MR1 and below. > > If the check is needed, it is only for style reasons. I'm happy to defer > judgement on this.
Message was sent while issue was closed.
On 2014/05/13 04:04:47, newt wrote: > drive-by after-the-fact review :) > > https://codereview.chromium.org/273313003/diff/1/remoting/android/java/src/or... > File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): > > https://codereview.chromium.org/273313003/diff/1/remoting/android/java/src/or... > remoting/android/java/src/org/chromium/chromoting/Chromoting.java:107: > intent.putExtra(Settings.EXTRA_ACCOUNT_TYPES, > On 2014/05/12 20:39:29, Lambros wrote: > > On 2014/05/12 17:37:14, garykac wrote: > > > Doesn't this code need a > > > if (Build.VERSION.SDK_INT >= ... > > > check around the EXTRA_ACCOUNT_TYPES as well? > > > > > > > > I'm not sure. It works fine on an older device with Android 4.0.3, and there's > > no sign of any crash in the log. > > > > I guess this is because EXTRA_ACCOUNT_TYPES is a compile-time integer > constant, > > and so the generated bytecode doesn't need to do a runtime field lookup, which > > would throw an exception on older versions. All that happens is that the > Intent > > ends up with extra information that the older Android OS doesn't know about so > > it is simply ignored. > > Correct. Constants on Android are typically static final, so their values are > inlined at compile time. This means this code won't crash at runtime on old > versions of Android, but could -- in theory -- fail in mysterious ways. > > To suppress the warning, you can either use the @TargetApi annotation if the > code is only run on higher versions of Android, or if the warning doesn't > actually indicate a problem (which often seems to be the case with InlinedApi), > you can use @SuppressLint("InlinedApi"). I think the latter would actually be > appropriate here... unless this code path never runs on Android devices running > JB MR1 and below. > > > > > > If the check is needed, it is only for style reasons. I'm happy to defer > > judgement on this. Thanks for the nice comments! I learn something new everyday through reviews. :) As you mentioned, @SuppressLint("InlinedApi") would be more appropriate here. And I think we could apply this to Desktop.java in the same vein. What do you think?
Message was sent while issue was closed.
On 2014/05/13 05:32:58, Sungmann Cho wrote: > On 2014/05/13 04:04:47, newt wrote: > > drive-by after-the-fact review :) > > > > > https://codereview.chromium.org/273313003/diff/1/remoting/android/java/src/or... > > File remoting/android/java/src/org/chromium/chromoting/Chromoting.java > (right): > > > > > https://codereview.chromium.org/273313003/diff/1/remoting/android/java/src/or... > > remoting/android/java/src/org/chromium/chromoting/Chromoting.java:107: > > intent.putExtra(Settings.EXTRA_ACCOUNT_TYPES, > > On 2014/05/12 20:39:29, Lambros wrote: > > > On 2014/05/12 17:37:14, garykac wrote: > > > > Doesn't this code need a > > > > if (Build.VERSION.SDK_INT >= ... > > > > check around the EXTRA_ACCOUNT_TYPES as well? > > > > > > > > > > > I'm not sure. It works fine on an older device with Android 4.0.3, and > there's > > > no sign of any crash in the log. > > > > > > I guess this is because EXTRA_ACCOUNT_TYPES is a compile-time integer > > constant, > > > and so the generated bytecode doesn't need to do a runtime field lookup, > which > > > would throw an exception on older versions. All that happens is that the > > Intent > > > ends up with extra information that the older Android OS doesn't know about > so > > > it is simply ignored. > > > > Correct. Constants on Android are typically static final, so their values are > > inlined at compile time. This means this code won't crash at runtime on old > > versions of Android, but could -- in theory -- fail in mysterious ways. > > > > To suppress the warning, you can either use the @TargetApi annotation if the > > code is only run on higher versions of Android, or if the warning doesn't > > actually indicate a problem (which often seems to be the case with > InlinedApi), > > you can use @SuppressLint("InlinedApi"). I think the latter would actually be > > appropriate here... unless this code path never runs on Android devices > running > > JB MR1 and below. > > > > > > > > > > If the check is needed, it is only for style reasons. I'm happy to defer > > > judgement on this. > > Thanks for the nice comments! I learn something new everyday through reviews. :) > As you mentioned, @SuppressLint("InlinedApi") would be more appropriate here. > And I think we could apply this to Desktop.java in the same vein. What do you > think? Yes, please send a patch to newt@ for review (and myself for OWNERS approval). He understands these annotations much better than I do - I need to do some reading! I don't know which is the right way to annotate Desktop.java, hopefully newt@ will be able to tell us.
Message was sent while issue was closed.
On 2014/05/13 16:43:08, Lambros wrote: > On 2014/05/13 05:32:58, Sungmann Cho wrote: > > On 2014/05/13 04:04:47, newt wrote: > > > drive-by after-the-fact review :) > > > > > > > > > https://codereview.chromium.org/273313003/diff/1/remoting/android/java/src/or... > > > File remoting/android/java/src/org/chromium/chromoting/Chromoting.java > > (right): > > > > > > > > > https://codereview.chromium.org/273313003/diff/1/remoting/android/java/src/or... > > > remoting/android/java/src/org/chromium/chromoting/Chromoting.java:107: > > > intent.putExtra(Settings.EXTRA_ACCOUNT_TYPES, > > > On 2014/05/12 20:39:29, Lambros wrote: > > > > On 2014/05/12 17:37:14, garykac wrote: > > > > > Doesn't this code need a > > > > > if (Build.VERSION.SDK_INT >= ... > > > > > check around the EXTRA_ACCOUNT_TYPES as well? > > > > > > > > > > > > > > I'm not sure. It works fine on an older device with Android 4.0.3, and > > there's > > > > no sign of any crash in the log. > > > > > > > > I guess this is because EXTRA_ACCOUNT_TYPES is a compile-time integer > > > constant, > > > > and so the generated bytecode doesn't need to do a runtime field lookup, > > which > > > > would throw an exception on older versions. All that happens is that the > > > Intent > > > > ends up with extra information that the older Android OS doesn't know > about > > so > > > > it is simply ignored. > > > > > > Correct. Constants on Android are typically static final, so their values > are > > > inlined at compile time. This means this code won't crash at runtime on old > > > versions of Android, but could -- in theory -- fail in mysterious ways. > > > > > > To suppress the warning, you can either use the @TargetApi annotation if the > > > code is only run on higher versions of Android, or if the warning doesn't > > > actually indicate a problem (which often seems to be the case with > > InlinedApi), > > > you can use @SuppressLint("InlinedApi"). I think the latter would actually > be > > > appropriate here... unless this code path never runs on Android devices > > running > > > JB MR1 and below. > > > > > > > > > > > > > > If the check is needed, it is only for style reasons. I'm happy to defer > > > > judgement on this. > > > > Thanks for the nice comments! I learn something new everyday through reviews. > :) > > As you mentioned, @SuppressLint("InlinedApi") would be more appropriate here. > > And I think we could apply this to Desktop.java in the same vein. What do you > > think? > > Yes, please send a patch to newt@ for review (and myself for OWNERS approval). > He understands these annotations much better than I do - I need to do some > reading! I don't know which is the right way to annotate Desktop.java, hopefully > newt@ will be able to tell us. I got it. Thanks! :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
