Last Comment Bug 697006 - Add desktop support for the Open Web Apps API
: Add desktop support for the Open Web Apps API
Status: VERIFIED FIXED
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 13
Assigned To: [:fabrice] Fabrice Desré
:
:
Mentors:
: 683635 731545 (view as bug list)
Depends on: 731720 697383 706634 707277 711291 720415 733937 734016 735517 740922 741415 745435
Blocks: 735571 708462 735555
  Show dependency treegraph
 
Reported: 2011-10-24 21:08 PDT by [:fabrice] Fabrice Desré
Modified: 2013-01-08 11:49 PST (History)
26 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
disabled


Attachments
wip part 1 - Domify the owa error object (5.02 KB, patch)
2011-10-28 16:46 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Splinter Review
wip part 2 - desktop UI (41.68 KB, patch)
2011-10-28 16:47 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Splinter Review
screenshot : tab modal install dialog (167.88 KB, image/png)
2011-10-28 16:51 PDT, [:fabrice] Fabrice Desré
no flags Details
desktop UI patch (33.84 KB, patch)
2011-11-23 10:18 PST, [:fabrice] Fabrice Desré
no flags Details | Diff | Splinter Review
screenshot : tab modal install dialog (119.83 KB, image/png)
2011-11-23 10:18 PST, [:fabrice] Fabrice Desré
limi: ui‑review+
Details
updated patch (33.91 KB, patch)
2011-12-06 20:19 PST, [:fabrice] Fabrice Desré
gavin.sharp: feedback-
Details | Diff | Splinter Review
updated patch (33.05 KB, patch)
2011-12-12 16:10 PST, [:fabrice] Fabrice Desré
no flags Details | Diff | Splinter Review
patch (28.72 KB, patch)
2011-12-13 17:25 PST, [:fabrice] Fabrice Desré
gavin.sharp: review-
Details | Diff | Splinter Review
screenshot : webapps notification (59.67 KB, image/png)
2011-12-13 17:25 PST, [:fabrice] Fabrice Desré
no flags Details
updated patch (27.00 KB, patch)
2011-12-15 17:12 PST, [:fabrice] Fabrice Desré
gavin.sharp: review+
Details | Diff | Splinter Review
patch with some review comments addressed (32.77 KB, patch)
2011-12-17 08:15 PST, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch with some review comments addressed (32.77 KB, patch)
2011-12-17 08:17 PST, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
rebased patch (22.85 KB, patch)
2012-03-02 14:30 PST, [:fabrice] Fabrice Desré
gavin.sharp: review-
Details | Diff | Splinter Review
enable mozApps in desktop firefox (1.63 KB, patch)
2012-03-02 14:31 PST, [:fabrice] Fabrice Desré
gavin.sharp: review+
Details | Diff | Splinter Review
updated patch (21.19 KB, patch)
2012-03-06 16:49 PST, [:fabrice] Fabrice Desré
no flags Details | Diff | Splinter Review
updated patch (4.79 KB, patch)
2012-03-07 15:39 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
updated patch v2 (21.75 KB, patch)
2012-03-08 11:08 PST, [:fabrice] Fabrice Desré
gavin.sharp: review+
Details | Diff | Splinter Review
install doorhanger (168.10 KB, image/png)
2012-03-12 09:46 PDT, [:fabrice] Fabrice Desré
no flags Details
patch with timeout (22.60 KB, patch)
2012-03-12 14:22 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Splinter Review
updated timeout patch (21.57 KB, patch)
2012-03-12 14:47 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Splinter Review
updated patch (21.59 KB, patch)
2012-03-12 17:08 PDT, [:fabrice] Fabrice Desré
gavin.sharp: review+
Details | Diff | Splinter Review

Description [:fabrice] Fabrice Desré 2011-10-24 21:08:18 PDT
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.
Comment 1 [:fabrice] Fabrice Desré 2011-10-28 16:46:06 PDT
Created attachment 570413 [details] [diff] [review]
wip part 1 - Domify the owa error object
Comment 2 [:fabrice] Fabrice Desré 2011-10-28 16:47:45 PDT
Created attachment 570416 [details] [diff] [review]
wip part 2 - desktop UI

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.
Comment 3 [:fabrice] Fabrice Desré 2011-10-28 16:51:53 PDT
Created attachment 570420 [details]
screenshot : tab modal install dialog
Comment 4 [:fabrice] Fabrice Desré 2011-11-23 10:18:05 PST
Created attachment 576533 [details] [diff] [review]
desktop UI patch

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.
Comment 5 [:fabrice] Fabrice Desré 2011-11-23 10:18:33 PST
Created attachment 576535 [details]
screenshot : tab modal install dialog
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-11-23 10:20:30 PST
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).
Comment 7 [:fabrice] Fabrice Desré 2011-11-23 10:25:39 PST
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.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-11-23 10:31:34 PST
(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.
Comment 9 [:fabrice] Fabrice Desré 2011-11-23 10:41:16 PST
(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 10 [:fabrice] Fabrice Desré 2011-11-29 08:54:37 PST
Comment on attachment 576535 [details]
screenshot : tab modal install dialog

based on
http://people.mozilla.com/~faaborg/files/projects/apps/creation-i1/permissions.html
Comment 11 Alex Limi (:limi) — Firefox UX Team 2011-11-30 14:41:20 PST
(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. :)
Comment 12 [:fabrice] Fabrice Desré 2011-11-30 14:52:06 PST
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 ;)
Comment 13 Alex Limi (:limi) — Firefox UX Team 2011-12-02 12:54:18 PST
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.
Comment 14 [:fabrice] Fabrice Desré 2011-12-02 13:04:30 PST
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.
Comment 15 [:fabrice] Fabrice Desré 2011-12-06 20:19:35 PST
Created attachment 579577 [details] [diff] [review]
updated patch

Rebased and updated patch.

Gavin, can you check it and give feedback, even if we may need some minor UI changes?
Comment 16 Alex Limi (:limi) — Firefox UX Team 2011-12-09 15:28:38 PST
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 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-09 16:27:28 PST
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.
Comment 18 [:fabrice] Fabrice Desré 2011-12-09 16:43:52 PST
(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?
Comment 19 [:fabrice] Fabrice Desré 2011-12-09 18:04:36 PST
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.
Comment 20 [:fabrice] Fabrice Desré 2011-12-12 16:10:24 PST
Created attachment 581081 [details] [diff] [review]
updated patch

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.
Comment 21 Dão Gottwald [:dao] 2011-12-12 17:34:02 PST
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.
Comment 22 Andreas Gal :gal 2011-12-12 18:21:34 PST
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!
Comment 23 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-13 11:45:13 PST
(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 24 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-13 11:46:54 PST
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 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-13 11:54:41 PST
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?
Comment 26 [:fabrice] Fabrice Desré 2011-12-13 12:36:21 PST
(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.
Comment 27 Robert Kaiser 2011-12-13 12:59:13 PST
(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.
Comment 28 [:fabrice] Fabrice Desré 2011-12-13 13:04:35 PST
(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.
Comment 29 [:fabrice] Fabrice Desré 2011-12-13 17:25:23 PST
Created attachment 581490 [details] [diff] [review]
patch

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 30 [:fabrice] Fabrice Desré 2011-12-13 17:25:58 PST
Created attachment 581491 [details]
screenshot : webapps notification
Comment 31 Dão Gottwald [:dao] 2011-12-14 01:38:44 PST
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?
Comment 33 Dão Gottwald [:dao] 2011-12-14 09:16:44 PST
(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.
Comment 34 [:fabrice] Fabrice Desré 2011-12-14 09:35:50 PST
Point taken. Care to suggest a solution?
Comment 35 Dão Gottwald [:dao] 2011-12-14 09:53:20 PST
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 36 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-14 17:56:49 PST
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.
Comment 37 [:fabrice] Fabrice Desré 2011-12-15 17:12:14 PST
Created attachment 582151 [details] [diff] [review]
updated patch

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 38 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-16 17:02:07 PST
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.
Comment 39 Dão Gottwald [:dao] 2011-12-17 08:15:04 PST
Created attachment 582536 [details] [diff] [review]
patch with some review comments addressed

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!
Comment 40 Dão Gottwald [:dao] 2011-12-17 08:17:45 PST
Created attachment 582537 [details] [diff] [review]
patch with some review comments addressed

fixed a whitespace screw-up while I was at it
Comment 41 [:fabrice] Fabrice Desré 2012-03-02 07:45:53 PST
*** Bug 731545 has been marked as a duplicate of this bug. ***
Comment 42 [:fabrice] Fabrice Desré 2012-03-02 14:30:43 PST
Created attachment 602494 [details] [diff] [review]
rebased patch

rebased and cleaned up version.
Comment 43 [:fabrice] Fabrice Desré 2012-03-02 14:31:12 PST
Created attachment 602495 [details] [diff] [review]
enable mozApps in desktop firefox
Comment 44 [:fabrice] Fabrice Desré 2012-03-06 11:55:35 PST
Comment on attachment 602494 [details] [diff] [review]
rebased patch

Rebased patch.
Comment 45 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-06 12:16:38 PST
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 46 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-06 12:37:30 PST
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.
Comment 47 [:fabrice] Fabrice Desré 2012-03-06 16:49:48 PST
Created attachment 603519 [details] [diff] [review]
updated patch

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
Comment 48 :Felipe Gomes (needinfo me!) 2012-03-07 15:39:17 PST
Created attachment 603898 [details] [diff] [review]
updated patch

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.
Comment 49 [:fabrice] Fabrice Desré 2012-03-08 11:08:43 PST
Created attachment 604131 [details] [diff] [review]
updated patch v2

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 50 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-08 18:00:22 PST
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.
Comment 51 [:fabrice] Fabrice Desré 2012-03-12 09:36:42 PDT
(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?
Comment 52 [:fabrice] Fabrice Desré 2012-03-12 09:46:21 PDT
Created attachment 604986 [details]
install doorhanger

Alex, feel free to move the review to someone else if don't have time.
Comment 53 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-12 12:34:33 PDT
(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?
Comment 54 [:fabrice] Fabrice Desré 2012-03-12 13:18:58 PDT
(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.
Comment 55 [:fabrice] Fabrice Desré 2012-03-12 14:22:32 PDT
Created attachment 605126 [details] [diff] [review]
patch with timeout

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.
Comment 56 [:fabrice] Fabrice Desré 2012-03-12 14:47:54 PDT
Created attachment 605140 [details] [diff] [review]
updated timeout patch

As per IRC discussion, the delay is now hardcoded at 30 seconds
Comment 57 [:fabrice] Fabrice Desré 2012-03-12 17:08:31 PDT
Created attachment 605234 [details] [diff] [review]
updated patch
Comment 58 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-12 17:18:31 PDT
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?)
Comment 59 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-12 17:19:30 PDT
Also there appears to be a leftover "installDone" variable in that patch (remnant from the timeout patch, I guess)
Comment 60 [:fabrice] Fabrice Desré 2012-03-12 17:23:34 PDT
(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.
Comment 63 Tim Abraldes [:TimAbraldes] [:tabraldes] 2012-03-14 13:21:53 PDT
*** Bug 683635 has been marked as a duplicate of this bug. ***
Comment 64 Dão Gottwald [:dao] 2012-03-14 15:00:36 PDT
Jason, I'm not sure what you're trying to do here, but please be more careful.
Comment 65 Jason Smith [:jsmith] 2012-03-14 15:01:47 PDT
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.
Comment 66 Alfred Kayser 2012-03-15 04:56:29 PDT
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.
Comment 67 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-18 18:26:56 PDT
(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?
Comment 68 Jason Smith [:jsmith] 2012-04-09 09:31:59 PDT
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).
Comment 69 Jason Smith [:jsmith] 2012-04-12 10:44:59 PDT
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.
Comment 70 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-12 11:33:18 PDT
Looks like bug 741415 tracks the follow up work, no need to reopen this bug.
Comment 71 Jason Smith [:jsmith] 2012-04-13 09:42:59 PDT
FYI to Fabrice - See the depends list for bugs that need to be fixed.
Comment 72 Jason Smith [:jsmith] 2012-05-04 16:27:59 PDT
Verified that this feature has landed. Follow-up bugs will track any additional fixes that need to be made to improve the feature quality.

Note You need to log in before you can comment on or make changes to this bug.