991766 - Webapp uninstallation on Mac through mozapps uninstall function

Status: RESOLVED FIXED

(Firefox Graveyard :: Web Apps, defect)

Description

[Marco Castelluccio [:marco]]

Like bug 774142 and bug 774144. I think on Mac we should move the application to the trash.

Comment 1

[Marco Castelluccio [:marco]]

Attachment: Patch

r?myk for the toolkit/webapps/ changes.
r?josh for the widget/cocoa/ changes.

Comment 2

[Josh Aas]

Comment on attachment 8401919 [details] [diff] [review]
Patch

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

::: widget/cocoa/nsMacWebAppUtils.mm
@@ +60,5 @@
> +
> +NS_IMETHODIMP nsMacWebAppUtils::TrashApp(const nsAString& path, nsITrashAppCallback* aCallback) {
> +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
> +
> +  NS_ENSURE_ARG_POINTER(aCallback);

I think someone mentioned that we're not supposed to be using this NS_ENSURE... stuff any more. Maybe just do a normal value check.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

> I think someone mentioned that we're not supposed to be using this NS_ENSURE... stuff any more.

I guess you're referring to bug 672843.  MDN doesn't yet have any doc on this (that I can find).  But the following phrase is repeated several times in the bug, and seems to be a good description:

Create the new macro NS_WARN_IF and deprecate NS_ENSURE_* in favor of the explicit warning/return style.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8401919 [details] [diff] [review]
Patch

This looks fine to me, with the following caveat:

Apple's doc on -[NSWorkplace recycleURLs:...] has this odd comment:

> In OS X v10.6, this method requires that the main run loop be run in
> a common mode to facilitate the display of any user interface
> elements. It is safe to call this method from any thread of your
> app.

I assume this means that, at least on OS X 10.6, it'd be better to call -[NSWorkspace recycleURLs:...] on the main thread.  But I also assume that's what this code will do.

Comment 5

[Marco Castelluccio [:marco]]

(Myk, this applies on top of bug 774144)

(In reply to Steven Michaud from comment #4)
> I assume this means that, at least on OS X 10.6, it'd be better to call
> -[NSWorkspace recycleURLs:...] on the main thread.  But I also assume that's
> what this code will do.

Thank you for the quick review! Yes, it'll run on the main thread.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8401919 [details] [diff] [review]
Patch

This looks reasonable to me, although it doesn't apply to my current trunk, and the conflict is non-trivial.  Did you base it on another change?

Comment 7

[Marco Castelluccio [:marco]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #6)
> Comment on attachment 8401919 [details] [diff] [review]
> Patch
> 
> This looks reasonable to me, although it doesn't apply to my current trunk,
> and the conflict is non-trivial.  Did you base it on another change?

Yes, see comment 5. I'll admit the note was difficult to see because it has the same format of a "(In reply to ..." :P

Comment 8

[Myk Melez [:myk] [@mykmelez]]

(In reply to Marco Castelluccio [:marco] from comment #7)
> Yes, see comment 5. I'll admit the note was difficult to see because it has
> the same format of a "(In reply to ..." :P

Aha!  Setting dependency accordingly.

Comment 9

[Marco Castelluccio [:marco]]

Attachment: Patch

Carrying r+.

Comment 10

[Marco Castelluccio [:marco]]

Please land bug 774144 before this one.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0012e4d8a9a

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out because bug 774144 was backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/93caa1abf5a3

Comment 13

[Marco Castelluccio [:marco]]

Retrying to land bug 774144 and this one (see bug 774144 to understand why).

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05d9e7977182

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/05d9e7977182