895638 - Reinstall permissions when the runtime is upgraded

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P2)

Description

[Marco Castelluccio [:marco]]

Attachment: Patch

I think we should reinstall permissions when the runtime is upgraded and just install them (not reinstall) on first run.
Also, on upgrades we should do the same things we do on first run, as the preferences could change.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 778073 [details] [diff] [review]
Patch

># HG changeset patch
># Parent 4e163cc4cbc912ec621511b767d47d8ae63229cd
># User Marco Castelluccio <[e-mail]>
>Reinstall permissions when the runtime is upgraded (and not reinstall permissions, but just install, on first run)
>
>diff --git a/mobile/android/chrome/content/WebAppRT.js b/mobile/android/chrome/content/WebAppRT.js
>--- a/mobile/android/chrome/content/WebAppRT.js
>+++ b/mobile/android/chrome/content/WebAppRT.js
>@@ -33,31 +33,31 @@ let WebAppRT = {
>     pref("media.useAudioChannelService", true)
>   ],
> 
>   init: function(aStatus, aUrl, aCallback) {
>     this.deck = document.getElementById("browsers");
>     this.deck.addEventListener("click", this, false, true);
> 
>     // on first run, update any prefs
>-    if (aStatus == "new") {
>+    if (aStatus == "new" || aStatus == "upgrade") {
>       this.getDefaultPrefs().forEach(this.addPref);
> 
>-      // prevent offering to use helper apps for things that this app handles
>-      // i.e. don't show the "Open in market?" popup when we're showing the market app
>-      let uri = Services.io.newURI(aUrl, null, null);
>-
>       // update the blocklist url to use a different app id
>       let blocklist = Services.prefs.getCharPref("extensions.blocklist.url");
>       blocklist = blocklist.replace(/%APP_ID%/g, "[e-mail]");
>       Services.prefs.setCharPref("extensions.blocklist.url", blocklist);
> 
>       this.getManifestFor(aUrl, function (aManifest, aApp) {
>         if (aManifest) {
>-          PermissionsInstaller.installPermissions(aApp, true);
>+          if (aStatus == "new") {
>+            PermissionsInstaller.installPermissions(aApp, false);
>+          } else if (aStatus == "upgrade") {
>+            PermissionsInstaller.installPermissions(aApp, true);
>+          }
>         }
>       });
>     }
> 
>     this.findManifestUrlFor(aUrl, aCallback);
>   },
> 
>   getManifestFor: function (aUrl, aCallback) {

Comment 2

[Wesley Johnston (:wesj)]

Comment on attachment 778073 [details] [diff] [review]
Patch

Review of attachment 778073 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/WebAppRT.js
@@ +42,1 @@
>        this.getDefaultPrefs().forEach(this.addPref);

I don't think we really need to do this on upgrades. These prefs are unrelated to the app...

@@ +44,4 @@
>        // update the blocklist url to use a different app id
>        let blocklist = Services.prefs.getCharPref("extensions.blocklist.url");
>        blocklist = blocklist.replace(/%APP_ID%/g, "[e-mail]");
>        Services.prefs.setCharPref("extensions.blocklist.url", blocklist);

Same for this probably. If the user has somehow figured out how to flip this, I don't really want to flip it back on them. The permissions are a bit different in that the app itself specifies them, and I think we need to respect its wishes.

Comment 3

[Marco Castelluccio [:marco]]

Attachment: Patch v2

Comment 4

[Marco Castelluccio [:marco]]

I completely forgot about this bug. I'll modify the patch because we're basically calling |PermissionsInstaller.installPermission| with the second parameter as true everywhere.
But we still need to reinstall permissions on update.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Marco: can you rebase the patch to apply to the current tip?

Comment 6

[Marco Castelluccio [:marco]]

Attachment: Patch v3

I'm always passing |true| as the second argument to installPermissions because, even if |aStatus| is new, it could still be a reinstallation.

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 8390106 [details] [diff] [review]
Patch v3

Review of attachment 8390106 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/WebappRT.js
@@ +55,5 @@
>        Services.prefs.setCharPref("extensions.blocklist.url", blocklist);
>  
> +      this.installPermissions(aUrl);
> +    } else if (aStatus == "upgrade") {
> +      this.installPermissions(aUrl);

Since this is repeated, can you move it out of the if-else block.

Comment 8

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/56fcaf175edd

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/56fcaf175edd