https://developer.mozilla.org/en/OpenWebApps/The_JavaScript_API describes the API. The implementation is split in 3 parts: - an xpcom component that exposes the API to web content - a js module that manages the repository implementation - some application specific glue to link the the xpcom to the js module. We have an e10s compatible implementation in fennec/xul, for which we landed the xpcom and js module in toolkit/mozapps/webapps. The plan here is to implement the desktop specific shim with a tab-modal installation prompt.

WIP implementation of the UI part for desktop, using a tab modal prompt. Applications are opened in a pinned tab that is reused if already opened.

Implementation of the desktop UI for the navigator.mozApps API (DOM part in bug 697383). This uses a tab modal prompt, so I did reuse most of the functionality provided by the prompts modal dialogs.

Has this had any kind of UX input? The tab-modal permissions prompt seems kind of odd, and inconsistent with our other permissions prompts (which have mostly been moving towards using the PopupNotification API).

We had no UX input on this. I choose to use a prompt since I don't think it's similar to a permission prompt in terms of use case.

(In reply to Fabrice Desré [:fabrice] from comment #7) > We had no UX input on this. I think we should, before diving any deeper into implementation details. Can you get in touch with Limi and see what kind of help his team can provide? > I choose to use a prompt since I don't think it's similar to a permission > prompt in terms of use case. Isn't it similar to our addon install dialog? I'm not entirely familiar with the apps proposals so I'm just going off of the screenshot. The "Grant Dashboard Privileges" checkbox is particularly confusing, since it's not clear what kind of implications that has.

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #8) > I think we should, before diving any deeper into implementation details. Can > you get in touch with Limi and see what kind of help his team can provide? Sure. > > I choose to use a prompt since I don't think it's similar to a permission > > prompt in terms of use case. > > Isn't it similar to our addon install dialog? I'm not entirely familiar with > the apps proposals so I'm just going off of the screenshot. The "Grant > Dashboard Privileges" checkbox is particularly confusing, since it's not > clear what kind of implications that has. It is kind of similar to the add-on install flow (only simpler), so this dialog is the equivalent of the "Install add-ons only from authors whom you trust" modal dialog. I agree that the "Grant Dashboard Privileges" checkbox is confusing. We should have some manifest permission support to only show this when it makes sense, and use better wording (like: "this app ask for elevated privileges, do you agree to grant them?")

Comment on attachment 576535 [details] screenshot : tab modal install dialog based on http://people.mozilla.com/~faaborg/files/projects/apps/creation-i1/permissions.html

(In reply to Fabrice Desré [:fabrice] from comment #9) > (In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment > #8) > > Isn't it similar to our addon install dialog? I'm not entirely familiar with > > the apps proposals so I'm just going off of the screenshot. The "Grant > > Dashboard Privileges" checkbox is particularly confusing, since it's not > > clear what kind of implications that has. > > It is kind of similar to the add-on install flow (only simpler), so this > dialog is the equivalent of the "Install add-ons only from authors whom you > trust" modal dialog. I agree that the "Grant > Dashboard Privileges" checkbox is confusing. We should have some manifest > permission support to only show this when it makes sense, and use better > wording (like: "this app ask for elevated privileges, do you agree to grant > them?") Without knowing more about what "Dashboard privileges" are, it's hard for me to give any useful input here. :)

The navigator.mozApps API exposes some calls that are available to any page (like .install(), .launch() and .enumerate()) and some calls only to pages that have a "webapps management" permission set. These pages can uninstall apps, enumerate apps installed from any store, and attach event listeners to be notified when an application is installed or uninstalled. So this permission is mostly useful for dashboards. What we plan to have is that apps will specify in their installation manifest which permission they need and include here this "web apps management" permission. This would go along with geolocation and offline storage, and allow users to grant permissions when installing. I welcome any way to convey this meaningfully to the users ;)

I still don't get what we're trying to do here, sorry. :/ Can you give me a step-by-step example of how this would work for an end-user (and don't worry about it not being optimal, figuring that out is what we can help with), so I can see what we're trying to accomplish here? On a related note, there should never be doorhangers unless the user initiated an action — just making sure this isn't related to the autodiscovery stuff — I know I commented on that in another bug.

The use case is: - A user goes to apps.mozilla.org, chooses an app. - He clicks on the "Install" button for the app. - this calls navigator.mozApps.install(), which pop ups the install dialog to get user confirmation. I added the "grant privileges" checkbox as a poor man version of the permission section from this mockup: http://people.mozilla.com/~faaborg/files/projects/apps/creation-i1/permissions.html The relevant permission here is one that allows the installed app to uninstall apps etc. I expect this to be better when we'll have permissions requests in the installation manifest, so we'll only show relevant permission checkboxes. There is no auto-discovery or doorhanger here. It's rather similar to installing an add-on.

Rebased and updated patch. Gavin, can you check it and give feedback, even if we may need some minor UI changes?

Comment on attachment 576535 [details] screenshot : tab modal install dialog It doesn't make a lot of sense to me to do a ui-review on this one if it isn't finished yet, and things like "Grant Dashboard Privileges" makes no sense to a user. How about we do another round of reviews once this one actually has permissions that make sense to the user? :) If you want conceptual or visual feedback outside of the actual functionality, the main things I'd point out is that you should respect the left/right positioning of the buttons and default ordering, e.g: Windows: [Install] [Cancel] Mac: [Cancel] [Install] Linux: (I assume it's still the same as on Windows)

Comment on attachment 579577 [details] [diff] [review] updated patch Sorry for the feedback request delay. Still seems like the real blocker here is sorting out exactly what the UI should be, though. >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml I don't think we want to add an entirely new prompting system just for this unless it's actually required. This is duplicating a lot of the existing tab-modal prompt stuff, and that seems unnecessary. We should be able to just use doorhangers for this. >diff --git a/browser/base/content/webapps.js b/browser/base/content/webapps.js This code looks window-agnostic, so it probably shouldn't live in global-scripts.inc, but rather in a JS module.

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #17) > Comment on attachment 579577 [details] [diff] [review] > updated patch > > Sorry for the feedback request delay. Still seems like the real blocker here > is sorting out exactly what the UI should be, though. Sure, but I still don't understand why using faaborgs mockups is a bad idea. > >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml > > I don't think we want to add an entirely new prompting system just for this > unless it's actually required. This is duplicating a lot of the existing > tab-modal prompt stuff, and that seems unnecessary. We should be able to > just use doorhangers for this. Note that we'll need a tab-modal prompt for the camera preview UI (bug 692955) that would also benefit from the small refactoring in tabbrowser.xml. If we use doorhangers, can we change dynamically the icon? PopupNotifications.jsm uses an id + css. > >diff --git a/browser/base/content/webapps.js b/browser/base/content/webapps.js > > This code looks window-agnostic, so it probably shouldn't live in > global-scripts.inc, but rather in a JS module. Good point, I'll move this in a jsm. What would be the best location?

For the permissions checkbox, what about this solution until we take a clear decision: - no checkbox at all. - a white list of sites (eg. AMO) that are automatically granted the privileges. This would be applicable both for desktop and fennec, but also for b2g where we would whitelist the homescreen application.

Gavin: - I can't actually move to a .jsm since we check that the dialog is targeted at the current window in line 143 - I removed the checkbox and corresponding entity. - The order of buttons is similar to tabprompts, so this shouhld be fine.

Is the install request API sync? I bet somebody made sure it's async; a non-modal doorhanger is the tool specifically designed for this kind of use case.

It's asynchronous. If you are ok with a really fat door hanger with tons of content and an icon/image, we can do that. Can you please also comment on any other change you want. We dont have time for many iterations. Trying to make the next train. Thanks!

(In reply to Andreas Gal :gal from comment #22) > It's asynchronous. If you are ok with a really fat door hanger with tons of > content and an icon/image, we can do that. Why do we need "tons of content"? Why does this prompt need to be significantly different than e.g. the geolocation permission prompt? The mockup has a different visual appearance, but appears to be functionally the same as a doorhanger. That system already exists and works, so if your intent is to get something in that works as soon as possible, that's the approach we should be taking. It would be significantly simpler than the current patch, AFAICT. This patch duplicates a bunch of existing code, and adds app-specific workarounds to generic toolkit code for this specific use case. Like I said in comment 17, I don't think this approach is suitable as the first step, particularly not if you want it in the next train.

Comment on attachment 581081 [details] [diff] [review] updated patch Fabrice: I can help if you have any questions about the doorhanger API, feel free to come find me.

Comment on attachment 581081 [details] [diff] [review] updated patch >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js > hideChromeForLocation: function(aLocation) { >+ let ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore); >+ if (ss.getTabValue(gBrowser.selectedTab, "appOrigin")) >+ return true; This assumes that this function is only called for the currently selected tab, which isn't a valid assumption since this method can be overridden/used by addons (that's its purpose). Just add this check next to the current hideChromeForLocation caller (or put it in a separate hideChromeForTab helper). >diff --git a/browser/base/content/webapps.js b/browser/base/content/webapps.js Needs a license header. >+var webappsUI = { >+ init: function() { >+ Components.utils.import("resource://gre/modules/Services.jsm"); >+ Services.obs.addObserver(webappsUI, "webapps-ask-install", false); >+ Services.obs.addObserver(webappsUI, "webapps-launch", false); >+ }, Rather than having observers in every window that return early, you could have a single observer set up in a module, and have that observer find the right window to prompt in. >+ _getBrowserForId: function(aId) { This looks like it doesn't deal with subframes. What sets .appId on content windows?

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #23) > Why do we need "tons of content"? Why does this prompt need to be > significantly different than e.g. the geolocation permission prompt? The > mockup has a different visual appearance, but appears to be functionally the > same as a doorhanger. That system already exists and works, so if your > intent is to get something in that works as soon as possible, that's the > approach we should be taking. It would be significantly simpler than the > current patch, AFAICT. It's not a permission prompt, it's an installation prompt. Very similar to what happens when you click "Add to firefox" on AMO. > This patch duplicates a bunch of existing code, and adds app-specific > workarounds to generic toolkit code for this specific use case. Like I said > in comment 17, I don't think this approach is suitable as the first step, > particularly not if you want it in the next train. Are you refering to the tab-modal prompts? I'd be happy to turn it into something reusable for something else than the alert()/confirm()/prompt() that it supports currently, to make it actually generic. Anyway, I'm gonna switch to a doorhanger, and we'll revise the UX later.

(In reply to Fabrice Desré [:fabrice] from comment #26) > (In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment > #23) > It's not a permission prompt, it's an installation prompt. Very similar to > what happens when you click "Add to firefox" on AMO. FYI, we are handling installations with doorhangers as well, at least from non-whitelisted sites.

(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #27) > (In reply to Fabrice Desré [:fabrice] from comment #26) > > (In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment > > #23) > > It's not a permission prompt, it's an installation prompt. Very similar to > > what happens when you click "Add to firefox" on AMO. > > FYI, we are handling installations with doorhangers as well, at least from > non-whitelisted sites. Not exactly. You first get a doorhanger saying "Firefox prevented (foo.com) to install software on your computer", and then a download progress doorhanger, and then a modal installation dialog. For webapps we only need this last dialog.

New patch using a notification box implemented as an XBL override to display the app icon. I also addressed the other comments from Gavin. Frames are now handled correctly (appId is set on the content window by the js xpcom component when instanciated as a navigator property).

Comment on attachment 581490 [details] [diff] [review] patch >+webapps.requestInstall = Do you want to install (%S) from (%S)? What's the point of these parentheses?

(In reply to Dão Gottwald [:dao] from comment #31) > Comment on attachment 581490 [details] [diff] [review] > patch > > >+webapps.requestInstall = Do you want to install (%S) from (%S)? > > What's the point of these parentheses? I just tried being consistent with : http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#18 http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#69 http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#188 http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#196

(In reply to Fabrice Desré [:fabrice] from comment #32) The parenthesized stuff annotates "this site" or "this website" there. You're not doing this in your sentence. Basically, you want your sentence to make sense with all parenthesized text removed, but "Do you want to install from?" obviously doesn't work out grammatically.

Point taken. Care to suggest a solution?

Obvious choices for fixing up the sentence without changing it to something else entirely: Do you want to install %S from this site (%S)? Do you want to install %S from %S? Do you want to install the “%S” Web application from this site (%S)?

Comment on attachment 581490 [details] [diff] [review] patch >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js >+ if (ss.getTabValue(gBrowser.selectedTab, "appOrigin")) >+ document.documentElement.setAttribute("disablechrome", "true"); > document.documentElement.removeAttribute("disablechrome"); This isn't going to work :) >diff --git a/browser/base/content/global-scripts.inc b/browser/base/content/global-scripts.inc >+<script type="application/javascript" src="chrome://browser/content/webapps.js"/> Don't add this code to global-scripts.inc. Generally we'd just put this into browser.js, but it might be nice to try putting this code in a module and instantiating a copy for each window. That might not be necessary if it's small enough after my modifications suggested below to just dump in browser.js. >diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml >+ <binding id="webapps-notification" extends="chrome://global/content/bindings/notification.xml#popup-notification"> This will do for now, but can you file a followup to add support for the "icon" param to PopupNotifications.show(), so that this extra binding isn't needed? Should be pretty easy. >diff --git a/browser/base/content/webapps.js b/browser/base/content/webapps.js >+var webappsUI = { >+ openURL: function(aUrl, aOrigin) { This function should go in the global JSM, since it's not tied to a given window. >+ let numTabs = tabbrowser.browsers.length; nit: use tabbrowser.tabs.length for consistency. >+ let currentBrowser = tabbrowser.getBrowserAtIndex(index); This is unused. >+ doInstall: function(aData, aChromeWin, aBrowser) { >+ let browserBundle = Services.strings.createBundle("chrome://browser/locale/browser.properties"); gNavigatorBundle >+ let manifest = new DOMApplicationManifest(aData.app.manifest, aData.app.origin); Seems odd that oyu need to re-create an object here. Why can't the notification just pass the relevant information directly? >+ let message = browserBundle.formatStringFromName("webapps.requestInstall", >+ [manifest.name, requestingURI.host], 2); requestingURI.host can throw for non-nsStandardURL nsIURIs (e.g. data: URIs), need to handle that. >diff --git a/browser/base/content/webappsUI.jsm b/browser/base/content/webappsUI.jsm >+let webappsUI = { >+ observe: function webappsUI_observe(aSubject, aTopic, aData) { >+ case "webapps-ask-install": >+ chromeWin.webappsUI.doInstall(data, chromeWin, browser); You don't need to pass in chromeWin if you're calling the method on the chromeWindow, it can just use its "this" (implicitly). >+ _getBrowserForId: function(aId) { Rather than creating a new window ID and sticking it as an expando on the window, why not use our existing window IDs? You can use nsIDOMWindowUtils.getOuterWindowWithId to do the lookup, and then get from that content window to a chrome window via the docshell tree (and avoid the need to iterate over all tabs/windows). See ConsoleAPI.js and HUDService.jsm's "console-api-log-event" observer for an example. >diff --git a/toolkit/locales/en-US/chrome/global/webapps.dtd b/toolkit/locales/en-US/chrome/global/webapps.dtd >diff --git a/toolkit/locales/jar.mn b/toolkit/locales/jar.mn This looks unused.

All comments from Gavin addressed, and using one the strings suggested by Dao. (In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #36) > This will do for now, but can you file a followup to add support for the > "icon" param to PopupNotifications.show(), so that this extra binding isn't > needed? Should be pretty easy. I filed bug 711291 to address this. > >+ let manifest = new DOMApplicationManifest(aData.app.manifest, aData.app.origin); > > Seems odd that oyu need to re-create an object here. Why can't the > notification just pass the relevant information directly? DOMApplicationManifest() provides a helper that deals with locale resolution for manifest parameters, and that would be a bit wasteful to generate a "localized manifest" with all properties when we don't now which ones will be used.

Comment on attachment 582151 [details] [diff] [review] updated patch >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js >+Cu.import("resource://gre/modules/webappsUI.jsm"); resource:///modules/webappsUI.jsm >+XPCOMUtils.defineLazyGetter(this, "DOMApplicationManifest", function() { >+XPCOMUtils.defineLazyGetter(this, "DOMApplicationRegistry", function() { I don't really like these generic names being added to the global browser window scope. Can you encapsulate them in a "Webapps" object? (It would be nicer for Webapps.jsm to export a single object with these properties, but I don't know offhand how much trouble that is to change). >+var webappsUI = { >+ doInstall: function(aData, aBrowser) { >+ // let browserBundle = Services.strings.createBundle("chrome://browser/locale/browser.properties"); should remove this >+ let requestingURI = Services.io.newURI(aData.from, null, null); You can use "makeURI(aData.from)". >diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml >+ <binding id="webapps-notification" extends="chrome://global/content/bindings/notification.xml#popup-notification"> >+ if (this.notification.options.iconURL) >+ document.getAnonymousElementByAttribute(this, "anonid", "appIcon").setAttribute("src", this.notification.options.iconURL); nit: local variables for "this.notification.options.iconURL" and the icon element would make this nicer to read IMO. You don't need to do this, but it would be nice to just fix bug 711291 first to avoid needing this binding at all - it should be very easy. >diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties >+webapps.requestInstall = Do you want to install â%Sâ from this site (%S)? You should probably use ASCII quotes ("") here, and you'll want to use positional arguments and add a localization note (see http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties#18 for an example). Can web sites provide arbitrary strings for the app name? That can run into spoofing issues, if the app names include quotes or very long strings (you can make the notification look like it's saying something else entirely). Can we sanitize these strings? I think this needs to be addressed before landing this. >diff --git a/browser/modules/webappsUI.jsm b/browser/modules/webappsUI.jsm >+let webappsUI = { >+ observe: function webappsUI_observe(aSubject, aTopic, aData) { >+ let manifest = new DOMApplicationManifest(aManifest, data.origin); You create a manifest by passing in... a manifest? This is a really confusing API :( >+ _getBrowserForId: function(aId) { >+ let browser = content.QueryInterface(Ci.nsIInterfaceRequestor) >+ .getInterface(Ci.nsIWebNavigation) >+ .QueryInterface(Ci.nsIDocShell).chromeEventHandler; You can just use: let win = browser.ownerDocument.defaultView; >diff --git a/dom/base/Webapps.js b/dom/base/Webapps.js > init: function(aWindow) { > let from = Services.io.newURI(this._window.location.href, null, null); Not relevant to this bug, but this should be using aWindow.document.documentURIObject. In general, it's fragile to be using URIs for security/origin checks, you generally should be using nsIPrincipals for that. This is probably worth filing a followup on to investigate. >+ this._id = util.outerWindowID; this._outerWindowID ? r=me with those addressed, but this also needs tests and it wouldn't hurt to get a final ui-review. You should probably also run the entire feature through a security review if it hasn't already had one.

This is on top of bug 711291. It also wraps DOMApplicationRegistry and DOMApplicationManifest in a Webapps object. Please test these changes, because I didn't!

fixed a whitespace screw-up while I was at it

rebased and cleaned up version.

Comment on attachment 602494 [details] [diff] [review] rebased patch Rebased patch.

Comment on attachment 602494 [details] [diff] [review] rebased patch Looks like most of the comments from comment 38 haven't been addressed. >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js >+var webappsUI = { >+ doInstall: function(aData, aBrowser) { It looks like this code could actually live in the module - rather than that code calling window.webappsUI.doInstall, it can refer to window.gNavigatorBundle and window.PopupNotifications, etc. That means you could avoid importing the Webapps module in the window too, right? If you do still need it for some reason, use defineLazyModuleGetter. >diff --git a/browser/modules/webappsUI.jsm b/browser/modules/webappsUI.jsm >+/* ***** BEGIN LICENSE BLOCK ***** Probably want to use the new MPL 2 header: https://www.mozilla.org/MPL/headers/. I think it would be more intuitive to have webappsUI.jsm be initialized by nsBrowserGlue, since it only needs to happen once, and it doesn't need to be added to all browser windows. It might also be clearer to have it export a symbol and explicitly have nsBrowserGlue call an init() method that adds the observers, rather than that happening "magically" on import().

Comment on attachment 602495 [details] [diff] [review] enable mozApps in desktop firefox just remove the ifdefs entirely instead of commenting them out, as mentioned on IRC.

Patch updated to address comment #38 and #45. loading webappsUI.jsm fails in nsBrowserGlue.js but I don't know yet why. The error reported is: JS Component Loader: ERROR (null):0 uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: resource://gre/modules/FileUtils.jsm :: FileUtils_getDir :: line 60" data: no] WARNING: Cannot create startup observer : service,@mozilla.org/browser/browserglue;1: file /home/fabrice/dev/fennec/inbound/mozilla-central/embedding/components/appstartup/src/nsAppStartupNotifier.cpp, line 119

The problem was happening because webappsUI.jsm was being imported too early, in a moment where gDirService.get("ProfD") wouldn't work. I moved it to a later step in the initialization process and it fixed the prob. Fabrice, two questions: - when I install an app for the first time, the doorhanger appears pointing to the tab instead of the url bar.. It looks like the icon in the urlbar hasn't been constructed yet. Did you see this happening before? - Do you really need to import dom/base/Webapps.jsm into webappsUI? I don't see it being used there. Or is it for initialization purposes? It would be nice if we could completely avoid initilizating that before it's used.

updated patch with felipe's solution. > Fabrice, two questions: > - when I install an app for the first time, the doorhanger appears pointing to the tab instead of the url bar.. It looks like the icon in the urlbar hasn't been constructed yet. Did you see this happening before? I've seen it a couple of times, but can't reproduce it consistently. > - Do you really need to import dom/base/Webapps.jsm into webappsUI? I don't see it being used there. Or is it for initialization purposes? It would be nice if we could completely avoid initilizating that before it's used. yes, I need it there. webappsUI uses DOMApplicationRegistry and DOMApplicationManifest from Webapps.jsm

Comment on attachment 604131 [details] [diff] [review] updated patch v2 >diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js >+ webappsUI.init(); nit: you could also just import the module here, rather than adding the lazy getter (I don't feel strongly either way) >diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties >+#LOCALIZATION NOTE (webapps.requestInstall) %1$S is the application name, %2$S is the site from which the app is installed >+webapps.requestInstall = Do you want to install "%1$S" from this site (%2$S)? "%1$S is the name of the web application being installed", lest someone think you are referring to "Firefox" (which is pretty common in these strings). Did we sort out the issues with ensuring that app name has a maximum length? >diff --git a/browser/modules/webappsUI.jsm b/browser/modules/webappsUI.jsm >+ doInstall: function(aData, aBrowser, aWindow) { >+ let bundle = aWindow.gNavigatorBundle; nit: fix indent >+ let secondaryAction = { >+ label: bundle.getString("webapps.noinstall"), >+ accessKey: bundle.getString("webapps.noinstall.accesskey"), Hmm, actually, don't all notifications with a secondary action have a built-in "Not Now" button? It doesn't really make sense to have both "Not Now" and "Don't Install" in the dropdown. Could we just remove the "Don't Install" button? The webapps code would need to handle denyInstall never being called, but it kind of needs to handle that anyways, since these notifications are very easy to dismiss and be forgotten. r=me with those addressed, but I think this still needs ui-review, right? Previous ui-review looks like it was for the previous UI.

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #50) > Comment on attachment 604131 [details] [diff] [review] > >+#LOCALIZATION NOTE (webapps.requestInstall) %1$S is the application name, %2$S is the site from which the app is installed > >+webapps.requestInstall = Do you want to install "%1$S" from this site (%2$S)? > > "%1$S is the name of the web application being installed", lest someone > think you are referring to "Firefox" (which is pretty common in these > strings). Did we sort out the issues with ensuring that app name has a > maximum length? We want to do that as part of the manifest validation (early in the install process) since we also have constraints for the native application generation. I'm following up with Felipe on this. > >+ let secondaryAction = { > >+ label: bundle.getString("webapps.noinstall"), > >+ accessKey: bundle.getString("webapps.noinstall.accesskey"), > > Hmm, actually, don't all notifications with a secondary action have a > built-in "Not Now" button? It doesn't really make sense to have both "Not > Now" and "Don't Install" in the dropdown. > > Could we just remove the "Don't Install" button? The webapps code would need > to handle denyInstall never being called, but it kind of needs to handle > that anyways, since these notifications are very easy to dismiss and be > forgotten. The main issue there is that web sites except to receive either the success or error callback to ba called when installing. So if we remove the "Don't install" option there, when do we choose to fire onerror?

Alex, feel free to move the review to someone else if don't have time.

(In reply to Fabrice Desré [:fabrice] from comment #51) > The main issue there is that web sites except to receive either the success > or error callback to ba called when installing. So if we remove the "Don't > install" option there, when do we choose to fire onerror? After a timeout? Like I said, users of these kinds of notification can never rely on "cancel" notifications being sent, because these notifications can be dismissed without either option being activated. Generally, I think all new web API prompts should take this into account. Can we make webapps handle this correctly?

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #53) > > After a timeout? Like I said, users of these kinds of notification can never > rely on "cancel" notifications being sent, because these notifications can > be dismissed without either option being activated. Generally, I think all > new web API prompts should take this into account. Can we make webapps > handle this correctly? Sure, we can add a timeout. That looks really bad UX to me, but I won't fight this battle again for the desktop.

I removed the secondary action and associated l10n strings, and added an delay to automatically fire uninstall. The webapps.no-install.timeout preference is used to set the delay, and defaults at 30 seconds currently.

As per IRC discussion, the delay is now hardcoded at 30 seconds

Comment on attachment 605234 [details] [diff] [review] updated patch >diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties >+#LOCALIZATION NOTE (webapps.requestInstall) %1$S is the application name, %2$S is the site from which the web app is installed This still hasn't been updated per comment 50 (though you did mention on IRC that you'd fixed it - just not in this patch for some reason?)

Also there appears to be a leftover "installDone" variable in that patch (remnant from the timeout patch, I guess)

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #58) > Comment on attachment 605234 [details] [diff] [review] > updated patch > > >diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties > > >+#LOCALIZATION NOTE (webapps.requestInstall) %1$S is the application name, %2$S is the site from which the web app is installed > > This still hasn't been updated per comment 50 (though you did mention on IRC > that you'd fixed it - just not in this patch for some reason?) Right, I fixed it for the %2$S paramater only.

https://hg.mozilla.org/integration/mozilla-inbound/rev/17de225179ac https://hg.mozilla.org/integration/mozilla-inbound/rev/374977a5f8c6

https://hg.mozilla.org/mozilla-central/rev/17de225179ac https://hg.mozilla.org/mozilla-central/rev/374977a5f8c6

Jason, I'm not sure what you're trying to do here, but please be more careful.

Sorry. Was trying to migrate bugs over to the Web Apps component. Didn't realize a bug is unassigned when it switches components. Need to determine if this belongs in firefox general or web apps.

The webapp icon needs to be enforced a maximum size: .popup-notification-icon{ max-width:64px; max-height:64px} Now webapps can have very large icons and thereby blowing up the notification panel.

(In reply to Alfred Kayser from comment #66) > The webapp icon needs to be enforced a maximum size: Alfred: Could you file a bug for this if you haven't already?

Is this safe to leave active in FF 13 Aurora (see bug 741415)? The web apps integration into desktop feature got moved to FF 14. A partial implementation of the feature in aurora may create confusion. There's also two bugs this will affect if this enabled on Aurora (bug 743702, bug 740922).

Per a discussion with Asa & Ragavan, this code should not be active in Firefox 13. Please get the code out of Firefox 13 per bug 741415. When the bug is fixed, this bug can be marked as resolved fixed again with the FF 14 target milestone.

Looks like bug 741415 tracks the follow up work, no need to reopen this bug.

FYI to Fabrice - See the depends list for bugs that need to be fixed.

Verified that this feature has landed. Follow-up bugs will track any additional fixes that need to be made to improve the feature quality.