Closed Bug 777517 Opened 8 years ago Closed 7 years ago

Webapp installation alert should launch app on click

Categories

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

defect

Tracking

(firefox16 wontfix)

RESOLVED FIXED
Firefox 25
Tracking Status
firefox16 --- wontfix

People

(Reporter: dwalkowski, Assigned: marco)

References

Details

Attachments

(1 file, 2 obsolete files)

The alert displayed when installing a new Webapp is not responsive.
Clicking on it should launch the app.
OS: Mac OS X → All
Hardware: x86 → All
Attached patch enable webapp launch from alert (obsolete) — Splinter Review
This patch enables clicking in the Webapp installation alert to launch the app.
Attachment #645898 - Flags: review?(felipc)
Priority: -- → P2
Comment on attachment 645898 [details] [diff] [review]
enable webapp launch from alert

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

Looks good! just some small suggestions

Hopefully we won't even need the forceAppRegistration before landing this, as you told me the ".app" fix might make it unecessary

::: browser/modules/webappsUI.jsm
@@ +145,5 @@
> +                let data = JSON.parse(launchData);
> +                WebappOSUtils.launch(data.app);
> +              }
> +            }
> +          }

you should create this listener inside installationSuccessNotification itself, it does not need to be outside. With that you also won't need to pass the data.app json as the parameter, you can just directly use it from the aData argument in the function.

@@ +150,5 @@
> +
> +
> +function installationSuccessNotification(app, aData, aWindow) {
> +  let mwaUtils = Cc["@mozilla.org/widget/mac-web-app-utils;1"].
> +  createInstance(Ci.nsIMacWebAppUtils);

mwaUtils is not used here, no need to add this part

@@ +168,3 @@
>  
> +    } catch (ex) {
> +      dump(" Notifier Exception: " + ex + "\n");

debugging code
Attachment #645898 - Flags: review?(felipc) → feedback+
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → mcastelluccio
Attachment #645898 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #782040 - Flags: review?(myk)
Comment on attachment 782040 [details] [diff] [review]
Patch

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

r=myk with conflict fixed!

::: browser/modules/webappsUI.jsm
@@ +116,5 @@
>  
>            DOMApplicationRegistry.confirmInstall(aData, false, localDir, null,
>              function (aManifest) {
>                if (WebappsInstaller.install(aData, aManifest)) {
> +                installationSuccessNotification(aData, app, aWindow);

This now fails to apply, but the conflict is trivial.
Attachment #782040 - Flags: review?(myk) → review+
Attached patch PatchSplinter Review
Carrying r+
Attachment #782040 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3b9baad91485
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.