566020 - Add support for custom confirmation UI for Fennec

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

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

Attachment: patch

amIWebInstallListener is hard coded to use xpinstallConfirm.xul, which is not helpful for Fennec. The attached patch allows applications to override the confirmation UI.

Comment 1

[Dave Townsend [:mossop]]

Comment on attachment 445432 [details] [diff] [review]
patch

>diff --git a/toolkit/mozapps/extensions/amIWebInstallListener.idl b/toolkit/mozapps/extensions/amIWebInstallListener.idl
>--- a/toolkit/mozapps/extensions/amIWebInstallListener.idl
>+++ b/toolkit/mozapps/extensions/amIWebInstallListener.idl
>@@ -98,8 +98,34 @@ interface amIWebInstallListener : nsISup
>    * @param  aCount
>    *         The number of AddonInstalls
>    * @return true if the caller should start the installs
>    */
>   boolean onWebInstallRequested(in nsIDOMWindowInternal aWindow, in nsIURI aUri,
>                                 [array, size_is(aCount)] in nsIVariant aInstalls,
>                                 [optional] in PRUint32 aCount);
> };
>+
>+/**
>+ * amIWebInstallConfirPrompt is used by the default implementation of 

Typo. I think we should just go with amIWebInstallPrompt too.

>+ * amIWebInstallInfo to display a confirmation UI to the user before running
>+ * installs.
>+ */
>+[scriptable, uuid(c5529918-4291-4b56-bd46-e9268900f2a3)]
>+interface amIWebInstallConfirmPrompt : nsISupports
>+{
>+  /**
>+   * Get a confimration that the user wants to start the installs.

Typo

>+   *
>+   * @param  aWindow
>+   *         The window that triggered the installs
>+   * @param  aUri
>+   *         The URI of the site that triggered the installs
>+   * @param  aInstalls
>+   *         The AddonInstalls that were requested
>+   * @param  aCount
>+   *         The number of AddonInstalls
>+   * @return true if the caller should start the installs

This doesn't seem to be the case, though I think it probably should be, unless we want the prompter to be able to individually start/cancel the installs?

>+   */
>+  void confirm(in nsIDOMWindowInternal aWindow, in nsIURI aUri,
>+               [array, size_is(aCount)] in nsIVariant aInstalls,
>+               [optional] in PRUint32 aCount);
>+};
>diff --git a/toolkit/mozapps/extensions/amWebInstallListener.js b/toolkit/mozapps/extensions/amWebInstallListener.js
>--- a/toolkit/mozapps/extensions/amWebInstallListener.js
>+++ b/toolkit/mozapps/extensions/amWebInstallListener.js
>@@ -106,16 +106,22 @@ Installer.prototype = {
>   checkAllDownloaded: function() {
>     if (--this.count > 0)
>       return;
> 
>     // Maybe none of the downloads were sucessful
>     if (this.installs.length == 0)
>       return;
> 
>+    let prompt = Cc["@mozilla.org/addons/web-install-confirm;1"].getService(Ci.amIWebInstallConfirmPrompt);

This will throw in any app that doesn't define this component. Test that |"@mozilla.org/addons/web-install-prompt;1" in Cc| first (note the different contract ID).

>+    if (prompt) {
>+      prompt.confirm(this.window, this.url, this.installs, this.installs.length);
>+      return;
>+    }
>+
>     let args = {};
>     args.url = this.url;
>     args.installs = this.installs;
>     args.wrappedJSObject = args;
> 
>     Services.ww.openWindow(this.window, "chrome://mozapps/content/xpinstall/xpinstallConfirm.xul",
>                            null, "chrome,modal,centerscreen", args);
>   },

Is there a way we can test this?

Comment 2

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

(In reply to comment #1)
> (From update of attachment 445432 [details] [diff] [review])
> >diff --git a/toolkit/mozapps/extensions/amIWebInstallListener.idl b/toolkit/mozapps/extensions/amIWebInstallListener.idl

> Typo. I think we should just go with amIWebInstallPrompt too.

Typos fixed and interface changed to amIWebInstallPrompt

> >+   * @param  aWindow
> >+   *         The window that triggered the installs
> >+   * @param  aUri
> >+   *         The URI of the site that triggered the installs
> >+   * @param  aInstalls
> >+   *         The AddonInstalls that were requested
> >+   * @param  aCount
> >+   *         The number of AddonInstalls
> >+   * @return true if the caller should start the installs
> 
> This doesn't seem to be the case, though I think it probably should be, unless
> we want the prompter to be able to individually start/cancel the installs?

I removed the @return for now and just made the prompter have to deal with the install or cancel, which is just like the xpinstallConfirm.js

> >+    let prompt = Cc["@mozilla.org/addons/web-install-confirm;1"].getService(Ci.amIWebInstallConfirmPrompt);
> 
> This will throw in any app that doesn't define this component. Test that
> |"@mozilla.org/addons/web-install-prompt;1" in Cc| first (note the different
> contract ID).

Added the check for the contract and changed the contract to "@mozilla.org/addons/web-install-prompt;1"

> Is there a way we can test this?

I was going to add a browser-chrome test for this in Fennec. Is that enough?

Comment 3

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

Attachment: patch 2

Updated for review comments

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 445499 [details] [diff] [review]
patch 2

>diff --git a/toolkit/mozapps/extensions/amIWebInstallListener.idl b/toolkit/mozapps/extensions/amIWebInstallListener.idl

>+/**
>+ * amIWebInstallPrompt is used by the default implementation of 

"is used if available by the default"

>diff --git a/toolkit/mozapps/extensions/amWebInstallListener.js b/toolkit/mozapps/extensions/amWebInstallListener.js

>+    if ("@mozilla.org/addons/web-install-prompt;1" in Cc) {
>+      let prompt = Cc["@mozilla.org/addons/web-install-prompt;1"].getService(Ci.amIWebInstallPrompt);

Wrap this into two lines, break after the ".".

>+      if (prompt) {

prompt is never going to be null at this point, either the getService will succeed or it will throw so wrap it in a try...catch block instead.

Sorry, picky, should be an r+ next time.

Comment 5

[Dave Townsend [:mossop]]

(In reply to comment #2)
> > Is there a way we can test this?
> 
> I was going to add a browser-chrome test for this in Fennec. Is that enough?

Probably for now, I may end up switching the toolkit test harness for
InstallTrigger to use this in the end though, may make for less problems than
opening windows.

Comment 6

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

(In reply to comment #4)

> >+/**
> >+ * amIWebInstallPrompt is used by the default implementation of 
> 
> "is used if available by the default"

Fixed

> >diff --git a/toolkit/mozapps/extensions/amWebInstallListener.js b/toolkit/mozapps/extensions/amWebInstallListener.js
> 
> >+    if ("@mozilla.org/addons/web-install-prompt;1" in Cc) {
> >+      let prompt = Cc["@mozilla.org/addons/web-install-prompt;1"].getService(Ci.amIWebInstallPrompt);
> 
> Wrap this into two lines, break after the ".".

Fixed

> >+      if (prompt) {
> 
> prompt is never going to be null at this point, either the getService will
> succeed or it will throw so wrap it in a try...catch block instead.

Added the try/catch, but now the if("" in Cc) check seems redundant. Should I just remove it and keep the try/catch ?

Comment 7

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

Attachment: patch 3

Updated patch

Comment 8

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

Attachment: patch 4 (after a hg qref)

Forgot to 'hg gref'

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 445970 [details] [diff] [review]
patch 4 (after a hg qref)

>diff --git a/toolkit/mozapps/extensions/amIWebInstallListener.idl b/toolkit/mozapps/extensions/amIWebInstallListener.idl
>--- a/toolkit/mozapps/extensions/amIWebInstallListener.idl
>+++ b/toolkit/mozapps/extensions/amIWebInstallListener.idl
>@@ -98,8 +98,33 @@ interface amIWebInstallListener : nsISup
>    * @param  aCount
>    *         The number of AddonInstalls
>    * @return true if the caller should start the installs
>    */
>   boolean onWebInstallRequested(in nsIDOMWindowInternal aWindow, in nsIURI aUri,
>                                 [array, size_is(aCount)] in nsIVariant aInstalls,
>                                 [optional] in PRUint32 aCount);
> };
>+
>+/**
>+ * amIWebInstallPrompt is used, if available, by the default implementation of 
>+ * amIWebInstallInfo to display a confirmation UI to the user before running
>+ * installs.
>+ */
>+[scriptable, uuid(c5529918-4291-4b56-bd46-e9268900f2a3)]
>+interface amIWebInstallPrompt : nsISupports
>+{
>+  /**
>+   * Get a confirmation that the user wants to start the installs.
>+   *
>+   * @param  aWindow
>+   *         The window that triggered the installs
>+   * @param  aUri
>+   *         The URI of the site that triggered the installs
>+   * @param  aInstalls
>+   *         The AddonInstalls that were requested
>+   * @param  aCount
>+   *         The number of AddonInstalls
>+   */
>+  void confirm(in nsIDOMWindowInternal aWindow, in nsIURI aUri,
>+               [array, size_is(aCount)] in nsIVariant aInstalls,
>+               [optional] in PRUint32 aCount);
>+};
>diff --git a/toolkit/mozapps/extensions/amWebInstallListener.js b/toolkit/mozapps/extensions/amWebInstallListener.js
>--- a/toolkit/mozapps/extensions/amWebInstallListener.js
>+++ b/toolkit/mozapps/extensions/amWebInstallListener.js
>@@ -106,16 +106,26 @@ Installer.prototype = {
>   checkAllDownloaded: function() {
>     if (--this.count > 0)
>       return;
> 
>     // Maybe none of the downloads were sucessful
>     if (this.installs.length == 0)
>       return;
> 
>+    if ("@mozilla.org/addons/web-install-prompt;1" in Cc) {
>+      try {
>+        let prompt = Cc["@mozilla.org/addons/web-install-prompt;1"].
>+                     getService(Ci.amIWebInstallPrompt);
>+        prompt.confirm(this.window, this.url, this.installs, this.installs.length);
>+        return;
>+      }
>+      catch (e) {}
>+    }
>+
>     let args = {};
>     args.url = this.url;
>     args.installs = this.installs;
>     args.wrappedJSObject = args;
> 
>     Services.ww.openWindow(this.window, "chrome://mozapps/content/xpinstall/xpinstallConfirm.xul",
>                            null, "chrome,modal,centerscreen", args);
>   },

Comment 10

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

pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/6108e2debf9e

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

How can this be manually verified? Worth having any kind of tests for that change?

Comment 12

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

(In reply to comment #11)
> How can this be manually verified? Worth having any kind of tests for that
> change?

Only Fennec trunk uses the custom confirmation dialog. Use a Fennec trunk nightly to install an add-on from a webpage, like AMO. You should see a Fennec-friendly dialog appear.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #12)
> (In reply to comment #11)
> > How can this be manually verified? Worth having any kind of tests for that
> > change?
> 
> Only Fennec trunk uses the custom confirmation dialog. Use a Fennec trunk
> nightly to install an add-on from a webpage, like AMO. You should see a
> Fennec-friendly dialog appear.

Thanks Mark. Aakash, can you take care of it?

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Fennec/4.0b4pre.

Aakash, if you wanna have a manual test for this bug please take care of it. Thanks.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ioana, can you please take care of this Litmus test for Fennec? Thanks.

Comment 16

[Dave Townsend [:mossop]]

Mark, does Fennec have tests that verify this?

Comment 17

[Wesley Johnston (:wesj)]

We have some basic tests that install addons, and check that all the prompts and notifications that should appear do, including this one. They are a bit messy, and could certainly be fleshed out more.

Comment 18

[Dave Townsend [:mossop]]

Guess I'd consider that tested then