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 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?

(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?

Updated for review comments

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.

(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 ?

Updated patch

Forgot to 'hg gref'

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); > },

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

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

(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?

Verified fixed with Fennec/4.0b4pre. Aakash, if you wanna have a manual test for this bug please take care of it. Thanks.

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

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