Handle permission checks graceful considering that the user may have revoked some permissions in Android 6.0 GPSScanner.java [1], [2], [3]--> Permission checks are handled in StumberService [4], so add annotation for MissingPermission to the respective method block GekoAppShell.java [5], [6] -> similar to the above, add annotations to method block. The permission are checked for at [7] [1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/scanners/GPSScanner.java?from=GPSScanner.java#85 [2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/scanners/GPSScanner.java?from=GPSScanner.java#136 [3] https://dxr.mozilla.org/mozilla-central/source/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/scanners/GPSScanner.java?from=GPSScanner.java#141 [4] https://dxr.mozilla.org/mozilla-central/source/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/StumblerService.java?from=StumblerService.java#183 [5] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java?from=GeckoAppShell.java#484 [6] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java?from=GeckoAppShell.java#546 [6] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java?from=GeckoAppShell.java#509

Ah, it's great that we get this from lint.

(In reply to Sebastian Kaspari (:sebastian) from comment #1) > Ah, it's great that we get this from lint. Yeah, this is hot. This and making sure we get the version checking correct is my motivation for improving lint support :)

Could you please assign this bug to me? :)

We assign bugs once there is a patch attached. The build instructions for Android are at https://developer.mozilla.org/en-US/docs/Simple_Firefox_for_Android_build and if you need help irc://irc.mozilla.org/mobile is a good resource.

Vivek,I don't get these warnings in the report generated when I run lint (./mach gradle lint). Am I missing something while running lint?

(In reply to Maurya Talisetti from comment #5) > Vivek,I don't get these warnings in the report generated when I run lint > (./mach gradle lint). Am I missing something while running lint? Post Bug 1233882, these may no longer be present. I'll check this and close if necessary. Thanks for digging into it!

@Nick: I remember filing the bug after applying your tentative patch for bug 1233882. And I can reproduce the same warnings with latest fx-team. @Maurya: Can you check the following link after running ./mach gradle lint path to objdir-droid/gradle/build/mobile/android/app/outputs/lint-results.html#MissingPermission

Vivek, can you help me understand what fx-team is? I don't see any branch with that name in http://hg.mozilla.org/mozilla-central/branches. I got the latest code from http://hg.mozilla.org/mozilla-central/ and ran `./mach gradle lint` on the default branch. There's no item classified under MissingPermission. The report is [here](https://pastebin.mozilla.org/8860501) and the command line output is [here](https://pastebin.mozilla.org/8860502).

(In reply to Maurya Talisetti from comment #8) > Vivek, can you help me understand what fx-team is? I don't see any branch > with that name in http://hg.mozilla.org/mozilla-central/branches. Maurya - fx-team is an integration branch. See https://wiki.mozilla.org/Tree_Rules/Integration. > I got the latest code from http://hg.mozilla.org/mozilla-central/ and ran > `./mach gradle lint` on the default branch. I think this is sufficient because the relevant code has merged to mozilla-central, but would you mind reporting what commit your m-c is at? Then we can know for sure. You can find out what your m-c is at by running |hg parent|. > There's no item classified under MissingPermission. The report is > [here](https://pastebin.mozilla.org/8860501) and the command line output is > [here](https://pastebin.mozilla.org/8860502). That would be great! I think this will be correct, due to the new projects "seeing" the full set of permissions now.

Nick, thanks for those suggestions. My parent wasn't at the tip. I've updated it now. However, I'm now not able to run `./mach gradle lint` anymore. Not able to run `./mach gradle <target>:lint` either. The the error I see is here (https://pastebin.mozilla.org/8860609) . Any idea if the command options changed?

Never mind my question about not being able to run `./mach gradle lint`. I figured I had to rebuild after pulling the latest changes. I'm able to see GPSScanner and GeckoAppShell listed in the lint report. I'll add the necessary annotations and regenerate the report to ensure they don't appear again.

Comment on attachment 8721948 [details] [diff] [review] suppress-lint-permission-check Review of attachment 8721948 [details] [diff] [review]: ----------------------------------------------------------------- LGTM

Comment on attachment 8721948 [details] [diff] [review] suppress-lint-permission-check Review of attachment 8721948 [details] [diff] [review]: ----------------------------------------------------------------- Can I get some context on why it's okay to suppress the check? I assume we are requesting the permission (or have it in the manifest) and this is guarded by some feature flag?

Hi Nick, Nick, It is safe to add the suppress lint annotations for both GeckoAppShell and StumblerService. * In GeckoAppShell, we explicitly check for the permission using PermissionBlock [1] * In case of StumblerService, it is not intuitive at first glance, but the permissions are checked for in StumblerService which instantiate the GpsScanner [2]. Please let me know if this needs to be improved [1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java?from=GeckoAppShell.java#509 [2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/StumblerService.java#183

(In reply to Vivek Balakrishnan[:vivek] from comment #15) > Hi Nick, > Nick, > > It is safe to add the suppress lint annotations for both GeckoAppShell and > StumblerService. > * In GeckoAppShell, we explicitly check for the permission using > PermissionBlock [1] > * In case of StumblerService, it is not intuitive at first glance, but the > permissions are checked for in StumblerService which instantiate the > GpsScanner [2]. > > Please let me know if this needs to be improved > > [1] > https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/ > mozilla/gecko/GeckoAppShell.java?from=GeckoAppShell.java#509 > [2] > https://dxr.mozilla.org/mozilla-central/source/mobile/android/stumbler/java/ > org/mozilla/mozstumbler/service/stumblerthread/StumblerService.java#183 Very good -- perhaps add comments to this effect in the patch? Roll on!

Sure I'll add comments and update the patch.

added comments to annotations

Comment on attachment 8726703 [details] [diff] [review] suppress-lint-permission-check Review of attachment 8726703 [details] [diff] [review]: ----------------------------------------------------------------- lgmt. Thanks, Maurya!

Thanks Nick! Would you be able to vouch for me so that I can get Level 1 (try server) access? https://bugzilla.mozilla.org/show_bug.cgi?id=1254409 You'd need to add a comment saying that you support my application. (See https://www.mozilla.org/en-US/about/governance/policies/commit/)

Vouched! Clearing NI.

Nick, would you be able to assign this to me before marking as resolved!

Nick, can you add me to the "Assigned To" field?

Drive-by assign! :)

Thanks Sebastian :)

(In reply to Maurya Talisetti from comment #25) > Nick, can you add me to the "Assigned To" field? Maurya -- thanks for all your help with these lint issues. I've given you canconfirm and editbugs Bugzilla rights. That'll let you assign yourself, and also change most other fields. Use your new powers wisely :)

Awesome, thank you very much Nick :)