Closed Bug 869083 Opened 6 years ago Closed 6 years ago

fix OS X apps code to return failure on failure to launch app

Categories

(Core :: Widget: Cocoa, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jaas, Assigned: jaas)

References

Details

Attachments

(1 file)

2.76 KB, patch
Felipe
: review+
smichaud
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
Attached patch fix v1.0Splinter Review
I noticed that the success variable for launching an app is being ignored. Upon further inspection it seems like it shouldn't be.

This patch gets rid of the unused variable warning and properly returns success or failure. Updated the JS consumer to handle exceptions properly.
Comment on attachment 745963 [details] [diff] [review]
fix v1.0

Requesting review from Bill Walker, perhaps Bill can find a proper reviewer for the JS code.
Attachment #745963 - Attachment is patch: true
Attachment #745963 - Attachment mime type: text/x-patch → text/plain
Attachment #745963 - Flags: review?(bwalker)
Also, this patch switches to using stack-based autorelease pools.
Attachment #745963 - Flags: review?(smichaud)
Comment on attachment 745963 [details] [diff] [review]
fix v1.0

This looks fine to me, but have you tested what happens when an app fails to launch?

One test would be to comment out the call to -[NSWorkspace launchAppWithBundleIdentifier:...] and always return NS_ERROR_FAILURE from nsMacWebAppUtils::LaunchAppWithIdentifier().  Another would be to comment out the call to mwaUtils.launchAppWithIdentifier(aData.origin) and fake an exception instead.
Attachment #745963 - Flags: review?(smichaud) → review+
Assignee: nobody → joshmoz
Duplicate of this bug: 780478
Comment on attachment 745963 [details] [diff] [review]
fix v1.0

Felipe reviewed all the changes to this file that weren't platform-wide refactoring efforts, so he would be a good reviewer for these changes to it.
Attachment #745963 - Flags: review?(bwalker) → review?(felipc)
Attachment #745963 - Flags: review?(felipc) → review+
Comment on attachment 745963 [details] [diff] [review]
fix v1.0

Josh, is this patch ready to land? It was r+'d a month ago. <:)
Attachment #745963 - Flags: checkin?(joshmoz)
Comment on attachment 745963 [details] [diff] [review]
fix v1.0

Just set checkin-needed in the future :)
Attachment #745963 - Flags: checkin?(joshmoz) → checkin+
https://hg.mozilla.org/mozilla-central/rev/8e0fdcc36261
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.