Closed Bug 755934 Opened 12 years ago Closed 12 years ago

No feedback on install

Categories

(Firefox Graveyard :: Web Apps, defect, P1)

x86
macOS
defect

Tracking

(blocking-kilimanjaro:+, firefox16-)

VERIFIED FIXED
Firefox 16
blocking-kilimanjaro +
Tracking Status
firefox16 - ---

People

(Reporter: irakli, Assigned: dwalkowski)

References

Details

(Whiteboard: [qa!])

Attachments

(1 file, 4 obsolete files)

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.
Component: General → Web Apps
Product: Web Apps → Firefox
QA Contact: general → webapps
Blocks: 755539
blocking-kilimanjaro: --- → ?
blocking-kilimanjaro: ? → +
Priority: -- → P1
Target Milestone: --- → Firefox 16
Target Milestone: Firefox 16 → ---
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?
Keywords: uiwanted
(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.
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.
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.
(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?).
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?
Assignee: nobody → dwalkowski
Target Milestone: --- → Firefox 16
Summary: No feedback on install → No feedback on install - OS X
Keywords: uiwanted
(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
Exactly, that is what I am using.
(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.
(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?
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.
Attachment #632960 - Flags: review?
(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.
So this isn't only for Mac.
Summary: No feedback on install - OS X → No feedback on install
(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?
(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.
(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?
(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).
Attachment #632960 - Flags: review? → review?(felipc)
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.
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
Any reason not to have this code in WebappsInstaller? It seems to also already have code that calls getBiggestIconURL included from WebappsIconHelpers.js
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.
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.
Attached patch separate function, localizable (obsolete) — Splinter Review
moved the code out into separate function, and added description string to browser.properties for localization purposes.
Attachment #632960 - Attachment is obsolete: true
Attachment #632960 - Flags: review?(felipc)
Attachment #633283 - Flags: review?(felipc)
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?
Attachment #633283 - Flags: review?(felipc)
Attached patch removed null check on notifier (obsolete) — Splinter Review
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.
Attachment #633283 - Attachment is obsolete: true
Attachment #633295 - Flags: feedback?
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.
Forgot to pass a ref to the window.  that's what I get for doing a trivial refactor without testing afterward.
Attachment #633295 - Attachment is obsolete: true
Attachment #633295 - Flags: feedback?
Attachment #633312 - Flags: feedback?(gavin.sharp)
Attachment #633312 - Flags: feedback?(gavin.sharp) → feedback?(felipc)
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)
Attachment #633312 - Flags: feedback?(felipc) → feedback+
This patch incorporates the various suggestions provided
Attachment #633312 - Attachment is obsolete: true
Attachment #637189 - Flags: review?(felipc)
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
Attachment #637189 - Flags: review?(felipc) → review+
Status: NEW → ASSIGNED
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.
If this doesn't block the feature, no need to track. We'd definitely consider taking an uplift if/when ready, however.
QA Contact: jsmith
Whiteboard: [qa+]
http://hg.mozilla.org/mozilla-central/rev/eda29d325cea
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
Whiteboard: [qa+] → [qa verification failed]
Depends on: 774363
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.
Whiteboard: [qa verification failed] → [qa+]
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?
(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.
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.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
Blocks: 775703
Blocks: 777517
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: