Closed Bug 991766 Opened 6 years ago Closed 5 years ago

Webapp uninstallation on Mac through mozapps uninstall function

Categories

(Firefox Graveyard :: Web Apps, defect)

x86_64
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(1 file, 1 obsolete file)

Like bug 774142 and bug 774144. I think on Mac we should move the application to the trash.
Attached patch Patch (obsolete) — Splinter Review
r?myk for the toolkit/webapps/ changes.
r?josh for the widget/cocoa/ changes.
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Attachment #8401919 - Flags: review?(myk)
Attachment #8401919 - Flags: review?(joshmoz)
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.
Attachment #8401919 - Flags: review?(joshmoz) → review?(smichaud)
> 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 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.
Attachment #8401919 - Flags: review?(smichaud) → review+
(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 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?
Attachment #8401919 - Flags: review?(myk) → review+
(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
(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.
Depends on: 774144
Attached patch PatchSplinter Review
Carrying r+.
Attachment #8401919 - Attachment is obsolete: true
Attachment #8405319 - Flags: review+
Please land bug 774144 before this one.
Keywords: checkin-needed
Retrying to land bug 774144 and this one (see bug 774144 to understand why).
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/05d9e7977182
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
QA Whiteboard: [qa-]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.