Closed
Bug 991766
Opened 10 years ago
Closed 10 years ago
Webapp uninstallation on Mac through mozapps uninstall function
Categories
(Firefox Graveyard :: Web Apps, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 31
People
(Reporter: marco, Assigned: marco)
References
Details
Attachments
(1 file, 1 obsolete file)
5.84 KB,
patch
|
marco
:
review+
|
Details | Diff | Splinter Review |
Like bug 774142 and bug 774144. I think on Mac we should move the application to the trash.
Assignee | ||
Comment 1•10 years ago
|
||
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)
Comment 3•10 years ago
|
||
> 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•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
(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•10 years ago
|
||
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+
Assignee | ||
Comment 7•10 years ago
|
||
(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•10 years ago
|
||
(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
Assignee | ||
Comment 9•10 years ago
|
||
Carrying r+.
Attachment #8401919 -
Attachment is obsolete: true
Attachment #8405319 -
Flags: review+
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0012e4d8a9a
Keywords: checkin-needed
Comment 12•10 years ago
|
||
Backed out because bug 774144 was backed out. https://hg.mozilla.org/integration/mozilla-inbound/rev/93caa1abf5a3
Assignee | ||
Comment 13•10 years ago
|
||
Retrying to land bug 774144 and this one (see bug 774144 to understand why).
Keywords: checkin-needed
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/05d9e7977182
Keywords: checkin-needed
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/05d9e7977182
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Updated•8 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•