Last Comment Bug 755934 - No feedback on install
: No feedback on install
Status: VERIFIED FIXED
[qa!]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Web Apps (show other bugs)
: unspecified
: x86 Mac OS X
: P1 normal
: Firefox 16
Assigned To: Dan Walkowski
: Jason Smith [:jsmith]
Mentors:
: 762828 (view as bug list)
Depends on: 769095 774363
Blocks: 755539 775703 777517
  Show dependency treegraph
 
Reported: 2012-05-16 15:23 PDT by Irakli Gozalishvili [:irakli] [:gozala] [@gozala]
Modified: 2016-02-04 15:00 PST (History)
15 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch: Notification on App Install. Cross platform where supported. (2.29 KB, patch)
2012-06-13 16:47 PDT, Dan Walkowski
no flags Details | Diff | Splinter Review
separate function, localizable (2.85 KB, patch)
2012-06-14 14:39 PDT, Dan Walkowski
no flags Details | Diff | Splinter Review
removed null check on notifier (2.78 KB, patch)
2012-06-14 15:08 PDT, Dan Walkowski
no flags Details | Diff | Splinter Review
oops. this one is now tested correct. (2.82 KB, patch)
2012-06-14 16:34 PDT, Dan Walkowski
felipc: feedback+
Details | Diff | Splinter Review
second proposed patch (2.50 KB, patch)
2012-06-27 11:08 PDT, Dan Walkowski
felipc: review+
Details | Diff | Splinter Review

Description Irakli Gozalishvili [:irakli] [:gozala] [@gozala] 2012-05-16 15:23:33 PDT
On OSX installing an app does not gives any feedback to a user. While installer page could provide feedback on successful installs it not quite it, as a user I'd expect some UI in browser telling me that something has finished installing. 

Also, if I don't set any oninstall listener there will be no feedback at all. I'd expect browser to have default fallback telling users that app was installed.
Comment 1 Jason Smith [:jsmith] 2012-06-08 08:10:38 PDT
*** Bug 762828 has been marked as a duplicate of this bug. ***
Comment 2 Jason Smith [:jsmith] 2012-06-11 11:04:22 PDT
Desktop notifications may be an option here:

http://dougturner.wordpress.com/2010/10/14/desktop-notifications-in-fennec/
Comment 3 Jason Smith [:jsmith] 2012-06-11 11:12:46 PDT
Brian - Here's an idea for a short-term solution that falls in line with your "first run experience" idea:

We do a one-time desktop notification for a web app install that provides a short summary on how to get started and points the user to some MDN doc on where they can get more information.

Thoughts?
Comment 4 Marco Castelluccio [:marco] 2012-06-11 11:26:56 PDT
(In reply to Jason Smith [:jsmith] from comment #2)
> Desktop notifications may be an option here:
> 
> http://dougturner.wordpress.com/2010/10/14/desktop-notifications-in-fennec/

Is there a bug for the implementation of this API? This would be the best solution for this bug, in my opinion.
Comment 5 Jason Smith [:jsmith] 2012-06-11 11:39:01 PDT
More information here as well:

https://developer.mozilla.org/en/DOM/Displaying_notifications

It's currently only enabled for mobile, blocked on waiting for UX input for desktop.
Comment 6 Dan Walkowski 2012-06-12 13:11:59 PDT
In the Mac implementation, we could post notifications to Growl, which is a very commonly installed notification system.  In OSX 10.8 and beyond, there is a system-level notification system.
Comment 7 Jason Smith [:jsmith] 2012-06-13 01:25:18 PDT
(In reply to Dan Walkowski from comment #6)
> In the Mac implementation, we could post notifications to Growl, which is a
> very commonly installed notification system.  In OSX 10.8 and beyond, there
> is a system-level notification system.

That's a good idea - and it's native like too. Combining that with Dils's original proposal for a one-time notification, maybe an idea to implement this could be:

A one-time notification for a web app install that indicates that your first web app was installed, where it was installed, and how to launch it.

Bryan Clark - What do you think of this idea? Would one-time notification be acceptable? Or would a more persistent notification scheme be better (notify every post-install?).
Comment 8 Dan Walkowski 2012-06-13 10:46:55 PDT
I have a working but unfinished Growl notification on install.  I'll post the patch here today.  A small, unobtrusive, and auto-timeout growl notification appears each time you natively install an app, and only then. The notification includes the app name, and I am currently working on getting the icon in there.  If that becomes an issue, would it be acceptable to have the generic rocketship icon?
Comment 9 Ed Lee :Mardak 2012-06-13 11:18:33 PDT
(In reply to Dan Walkowski from comment #8)
> I have a working but unfinished Growl notification on install.
There's nsIAlertsService that does this cross platform:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/nsIAlertsService.idl#11
Comment 10 Dan Walkowski 2012-06-13 11:27:37 PDT
Exactly, that is what I am using.
Comment 11 Ed Lee :Mardak 2012-06-13 11:28:56 PDT
(In reply to Dan Walkowski from comment #10)
> Exactly, that is what I am using.
Sounds good. It just seemed odd that there's separate bugs filed for each platform when the fix is cross platform.
Comment 12 Jason Smith [:jsmith] 2012-06-13 11:35:48 PDT
(In reply to Edward Lee :Mardak from comment #11)
> (In reply to Dan Walkowski from comment #10)
> > Exactly, that is what I am using.
> Sounds good. It just seemed odd that there's separate bugs filed for each
> platform when the fix is cross platform.

Oh, that was my doing (didn't know the fix would be cross-platform). I'm looking at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/nsIAlertsService.idl#11 and noticing that it's supported for OS X and Android right now, correct? Or is it supported on other platforms too?
Comment 13 Dan Walkowski 2012-06-13 16:47:14 PDT
Created attachment 632960 [details] [diff] [review]
Patch: Notification on App Install.  Cross platform where supported.

This uses showAlertNotification(), which is (incompletely?) cross-platform. A Notification is presented indicating that an app was installed, with the name and icon of the app.
Comment 14 Jason Smith [:jsmith] 2012-06-13 16:48:25 PDT
(In reply to Dan Walkowski from comment #13)
> Created attachment 632960 [details] [diff] [review]
> Patch: Notification on App Install.  Cross platform where supported.
> 
> This uses showAlertNotification(), which is (incompletely?) cross-platform.
> A Notification is presented indicating that an app was installed, with the
> name and icon of the app.

Could get a screenshot of this in action and attach it to this bug? Curious to see what this looks like.
Comment 15 Marco Castelluccio [:marco] 2012-06-13 17:57:41 PDT
So this isn't only for Mac.
Comment 16 Jason Smith [:jsmith] 2012-06-13 19:07:47 PDT
(In reply to Marco Castelluccio from comment #15)
> So this isn't only for Mac.

Right, although the API though isn't implemented on Windows & Linux. We'll need followup bugs to implement support on Windows & Mac. I'll modify bug 764493 to reflect this for Windows. We'll need an equivalent bug for linux - can you file a bug for it?
Comment 17 Jason Smith [:jsmith] 2012-06-13 19:16:14 PDT
(In reply to Jason Smith [:jsmith] from comment #16)
> (In reply to Marco Castelluccio from comment #15)
> > So this isn't only for Mac.
> 
> Right, although the API though isn't implemented on Windows & Linux. We'll
> need followup bugs to implement support on Windows & Mac. I'll modify bug
> 764493 to reflect this for Windows. We'll need an equivalent bug for linux -
> can you file a bug for it?

Actually already took care of that - filed bug 764687.
Comment 18 Ed Lee :Mardak 2012-06-13 19:21:15 PDT
(In reply to Jason Smith [:jsmith] from comment #16)
> Right, although the API though isn't implemented on Windows & Linux.
What makes you think that? Does Windows not show a notification when you finish downloading files?
Comment 19 Jason Smith [:jsmith] 2012-06-13 19:28:34 PDT
(In reply to Edward Lee :Mardak from comment #18)
> (In reply to Jason Smith [:jsmith] from comment #16)
> > Right, although the API though isn't implemented on Windows & Linux.
> What makes you think that? Does Windows not show a notification when you
> finish downloading files?

Actually it does. Disregard my comment then (I misread the comments in the API).
Comment 20 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-14 11:41:03 PDT
Comment on attachment 632960 [details] [diff] [review]
Patch: Notification on App Install.  Cross platform where supported.

>diff --git a/browser/modules/webappsUI.jsm b/browser/modules/webappsUI.jsm

>+                notifier.showAlertNotification(iconURI.spec, 
>+                                              "Application Installed",

This will need to be a localized string, by adding a string to browser.properties and using bundle.getString() as used elsewhere in this method.
Comment 21 :Felipe Gomes (needinfo me!) 2012-06-14 11:51:01 PDT
Yeah, and please add all this code as a separate function in this module that can just be called from the successful `if (WebappsInstaller.install(aData))`  branch
Comment 22 Ed Lee :Mardak 2012-06-14 11:56:03 PDT
Any reason not to have this code in WebappsInstaller? It seems to also already have code that calls getBiggestIconURL included from WebappsIconHelpers.js
Comment 23 :Felipe Gomes (needinfo me!) 2012-06-14 12:15:53 PDT
This is UI-related so it fits better in webappsUI.jsm than WebappsInstaller, which I'd like to keep UI-free.

If WebappsInstaller.install needs to return any more data to make things easier that is fine, it could return an object with a property to describe success and failure + extra data.
Comment 24 Jason Smith [:jsmith] 2012-06-14 12:21:42 PDT
Btw, thanks a lot Dan for putting out a first revision here (this is a top pain point in the web apps experience right now, so getting traction here is very helpful).

My main feedback is that when an app is successfully installed, we need to provide context similar to what marketplace shows right now on what a user needs to do post-install on a per-OS basis. For example, on windows this contextual information would be needed:

Launch this app from your Windows desktop or Start ► All Programs.
Comment 25 Dan Walkowski 2012-06-14 14:39:33 PDT
Created attachment 633283 [details] [diff] [review]
separate function, localizable

moved the code out into separate function, and added description string to browser.properties for localization purposes.
Comment 26 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-14 14:58:46 PDT
Comment on attachment 633283 [details] [diff] [review]
separate function, localizable

This looks good, there are just a couple of nit-picky things...

>diff --git a/browser/modules/webappsUI.jsm b/browser/modules/webappsUI.jsm

>+function installationSuccessNotification(aData) {
>+  // System-level Notifications, which I believe are implemented cross-platform.
>+  if (("@mozilla.org/alerts-service;1" in Cc)) {
>+    let notifier;
>+    try {
>+      notifier = Cc["@mozilla.org/alerts-service;1"].
>+                 getService(Ci.nsIAlertsService);
>+
>+      if (notifier) {   

getService will either throw or return the service, it can't return null. So the null check is unnecessary. You could also short-circuit much of this with an early return to avoid extra indentation:

let notifier;
try {
  notifier = Cc["@mozilla.org/alerts-service;1"].
             getService(Ci.nsIAlertsService);
} catch (ex) {
  return;
}

etc...

>+        // Is it a data URI?
>+        try {
>+          let tempURI = Services.io.newURI(biggestIcon, null, null);
>+          if (tempURI.scheme == "data") {
>+            iconURI = tempURI;
>+          }
>+        } catch (ex) {}
>+
>+        // It's an HTTP URI
>+        if (!iconURI) {
>+          try {
>+            let origin = Services.io.newURI(aData.app.origin, null, null);
>+            iconURI = Services.io.newURI(origin.resolve(biggestIcon), null, null);
>+          }
>+          catch (ex) {}
>+        }

This code appears to be copied from the NativeApp constructor in WebappsInstaller.jsm - can we put this code in a common place and avoid the duplication?

>+        notifier.showAlertNotification(iconURI.spec, 
>+                                      bundle.getString("webapps.install.success"),
>+                                      aData.app.manifest.name,

Do we need more text in the notification?
Comment 27 Dan Walkowski 2012-06-14 15:08:10 PDT
Created attachment 633295 [details] [diff] [review]
removed null check on notifier

That code is indeed copied from WebappsInstaller.jsm, as is getBiggestIcon.
They are not exported, and I didn't want to alter someone's API.
I would be happy if they were in a shared place.
Comment 28 Dan Walkowski 2012-06-14 15:12:13 PDT
Oh, and the text can be changed.  I chose to make it minimally intrusive.
I was partway through implementing click-to-launch in the growl notification, but remembered that .app registration often takes a few seconds, and thus the launch could fail.  An update could be forced on install, which would require changes to Felipe's install code.
Comment 29 Dan Walkowski 2012-06-14 16:34:02 PDT
Created attachment 633312 [details] [diff] [review]
oops. this one is now tested correct.

Forgot to pass a ref to the window.  that's what I get for doing a trivial refactor without testing afterward.
Comment 30 :Felipe Gomes (needinfo me!) 2012-06-19 19:18:48 PDT
Comment on attachment 633312 [details] [diff] [review]
oops. this one is now tested correct.

Review of attachment 633312 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Take a look at gavin's suggestion to return the function early instead of indenting an if() + try {} blocks

We should also move the data:/http duplicated code inside the getBiggestIconURL function that will be just #included over (see next point), but that can be done as a separate bug

::: browser/modules/webappsUI.jsm
@@ +177,5 @@
> +  } // End Notification
> +}
> +
> +
> +function getBiggestIconURL(aIcons) {

Instead of copying this function over, just do a

#include WebappsIconHelpers.js

and move webappsUI.jsm to the EXTRA_PP_JS_MODULES section in this file: http://mxr.mozilla.org/mozilla-central/source/browser/modules/Makefile.in#22

(the pre-processor only runs in the files in the EXTRA_PP_ section)
Comment 31 Dan Walkowski 2012-06-27 11:08:56 PDT
Created attachment 637189 [details] [diff] [review]
second proposed patch

This patch incorporates the various suggestions provided
Comment 32 :Felipe Gomes (needinfo me!) 2012-06-28 02:26:25 PDT
Comment on attachment 637189 [details] [diff] [review]
second proposed patch

(nit: Makefile change no longer necessary)

We will wait for a solution on bug 769095 before landing this one
Comment 33 Jason Smith [:jsmith] 2012-07-03 12:20:25 PDT
Not blocking currently for this release, but this is strongly desired, especially since UX feedback has reported many times this to be an issue. Flagging for tracking-firefox16.
Comment 34 Alex Keybl [:akeybl] 2012-07-05 09:56:14 PDT
If this doesn't block the feature, no need to track. We'd definitely consider taking an uplift if/when ready, however.
Comment 35 :Felipe Gomes (needinfo me!) 2012-07-14 01:12:14 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a405768bd32a
Comment 37 :Felipe Gomes (needinfo me!) 2012-07-16 00:07:40 PDT
http://hg.mozilla.org/mozilla-central/rev/eda29d325cea
Comment 38 Jason Smith [:jsmith] 2012-07-16 10:41:30 PDT
Test Results:

- Windows 7: Pass
- OS X 10.7: Fail
- Ubuntu 12: Pass

I'll file a followup bug for OS X 10.7 not working.
Comment 39 Jason Smith [:jsmith] 2012-07-16 11:01:01 PDT
My bad. For OS X 10.7, there's a requirement to have Growl installed to see the notifications. Will re-test on OS X 10.7.
Comment 40 Jason Smith [:jsmith] 2012-07-16 12:25:58 PDT
For some reason, I'm having a lot of trouble getting notifications to work with Growl in general. I installed Growl on a OS X 10.6.8 machine and made sure it was running. I tried installing apps, but notification was fired. I even tried downloading files, but notifications were fired. Perhaps I have something misconfigured. Any ideas?
Comment 41 Jason Smith [:jsmith] 2012-07-16 12:34:02 PDT
(In reply to Jason Smith [:jsmith] from comment #40)
> For some reason, I'm having a lot of trouble getting notifications to work
> with Growl in general. I installed Growl on a OS X 10.6.8 machine and made
> sure it was running. I tried installing apps, but notification was fired. I
> even tried downloading files, but notifications were fired. Perhaps I have
> something misconfigured. Any ideas?

Wording fix - I mean "no notifications fired," not the opposite.
Comment 42 Jason Smith [:jsmith] 2012-07-16 13:10:52 PDT
I figured out what wasn't working with Felipe. After walking away from the machine for an hour and retrying, it worked fine. This sounds like something finder does for timing when growl is registered. Anyways, this verifies for OS X.

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