Add support for custom confirmation UI for Fennec

VERIFIED FIXED in mozilla1.9.3a5

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: mfinkle, Assigned: mfinkle)

Tracking

Trunk
mozilla1.9.3a5
Points:
---
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Created attachment 445432 [details] [diff] [review]
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.
Attachment #445432 - Flags: review?(dtownsend)
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?
Attachment #445432 - Flags: review?(dtownsend) → review-
(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?
Created attachment 445499 [details] [diff] [review]
patch 2

Updated for review comments
Assignee: nobody → mark.finkle
Attachment #445432 - Attachment is obsolete: true
Attachment #445499 - Flags: review?(dtownsend)
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.
Attachment #445499 - Flags: review?(dtownsend) → review-
Blocks: 562495
(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.
(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 ?
Created attachment 445724 [details] [diff] [review]
patch 3

Updated patch
Attachment #445499 - Attachment is obsolete: true
Attachment #445724 - Flags: review?(dtownsend)
Created attachment 445970 [details] [diff] [review]
patch 4 (after a hg qref)

Forgot to 'hg gref'
Attachment #445724 - Attachment is obsolete: true
Attachment #445970 - Flags: review?(dtownsend)
Attachment #445724 - Flags: review?(dtownsend)
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);
>   },
Attachment #445970 - Flags: review?(dtownsend) → review+
pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/6108e2debf9e
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
How can this be manually verified? Worth having any kind of tests for that change?
Flags: in-testsuite?
Flags: in-litmus?
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.3a5
Version: unspecified → Trunk
(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.
(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?
Summary: Add support for custom confirmation UI → Add support for custom confirmation UI for Fennec
Flags: in-litmus? → in-litmus?(vlad.maniac)
Verified fixed with Fennec/4.0b4pre.

Aakash, if you wanna have a manual test for this bug please take care of it. Thanks.
Status: RESOLVED → VERIFIED
Flags: in-litmus?(vlad.maniac) → in-litmus?
Ioana, can you please take care of this Litmus test for Fennec? Thanks.
Flags: in-litmus? → in-litmus?(ioana.chiorean)
Mark, does Fennec have tests that verify this?
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.
Guess I'd consider that tested then
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus?(ioana.chiorean)
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.