There's a lot of random permissions code relating to permissions we no longer grant, or checks that are no longer called. These can be cleaned up.

Comment on attachment 8968312 [details] Bug 1452826 - Remove unused permissions code from Navigator; https://reviewboard.mozilla.org/r/236986/#review243036

Comment on attachment 8968313 [details] Bug 1452826 - Simplify vibrate permission handling; https://reviewboard.mozilla.org/r/236988/#review243040 ::: dom/base/Navigator.cpp:853 (Diff revision 1) > return true; > } > > mRequestedVibrationPattern.SwapElements(pattern); > - uint32_t permission = GetPermission(mWindow, kVibrationPermissionType); > + uint32_t permission = nsContentUtils::IsExactSitePermAllow(doc->NodePrincipal(), > + kVibrationPermissionType); nsContentUtils::IsExactSitePermAllow returns a bool, so shouldn't this get coerced to 0 or 1? That isn't a problem for ALLOW_ACTION (which is 1), but DENY_ACTION is 2, so we'd break denying the permission here. It's probably easiest to just use permMgr as above... Technically checking for exact domain (without subdomains) also isn't the original behavior, though I wonder if it was intended to match subdomains... Bug 1243431 doesn't have any obvious mentions whether a discussion happened.

Comment on attachment 8968314 [details] Bug 1452826 - Simplify sensor event check; https://reviewboard.mozilla.org/r/236990/#review243044 Refactoring these if-conditions looks a little scary but I think you got it right! Note that I'll trust you on the "there is no place in the code that grants that permission anymore" part :) Thanks for the patches!

Comment on attachment 8968313 [details] Bug 1452826 - Simplify vibrate permission handling; https://reviewboard.mozilla.org/r/236988/#review243260 You need an integer, not a boolean, for this code to work. The different permission levels are: 0 - permission not set/always ask (UNKNOWN_ACTION) 1 - Always Allow (ALLOW_ACTION) 2 - Always Deny (DENY_ACTION) Currently you are just checking whether "Always Allow" is set, which means that the user will never get asked for permission because the 0 and 2 cases are treated the same. ::: dom/base/Navigator.cpp:872 (Diff revision 2) > SetVibrationPermission(false /* permitted */, false /* persistent */); > return true; > } > > // Request user permission. > obs->NotifyObservers(ToSupports(this), "Vibration:Request", nullptr); It's interesting that we don't have static analysis that catches this as dead code, we have that in JS land though I guess the condition is quite complex and I'm not sure whether it would have caught it...

Comment on attachment 8968313 [details] Bug 1452826 - Simplify vibrate permission handling; https://reviewboard.mozilla.org/r/236988/#review243260 Ah, thanks, completely misread the logic the first time I changed it. Just using perm manager now and testing things like they were.

Comment on attachment 8968313 [details] Bug 1452826 - Simplify vibrate permission handling; https://reviewboard.mozilla.org/r/236988/#review243840 That seems good, thank you!

Pushed by kmachulis@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3c6921d4e65f Remove unused permissions code from Navigator; r=johannh https://hg.mozilla.org/integration/autoland/rev/60659125bdd3 Simplify vibrate permission handling; r=johannh https://hg.mozilla.org/integration/autoland/rev/8908bab5b9d2 Simplify sensor event check; r=johannh

https://hg.mozilla.org/mozilla-central/rev/3c6921d4e65f https://hg.mozilla.org/mozilla-central/rev/60659125bdd3 https://hg.mozilla.org/mozilla-central/rev/8908bab5b9d2