Make nsIHelperAppLauncher inherit from nsICancelable

RESOLVED FIXED in mozilla1.8beta2

Status

Core Graveyard
File Handling
P1
normal
RESOLVED FIXED
13 years ago
a year ago

People

(Reporter: Biesinger, Assigned: Biesinger)

Tracking

({arch})

Trunk
mozilla1.8beta2
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Note: This will affect embeddors (launcher->Cancel() calls need to be replaced
with launcher->Cancel(NS_BINDING_ABORTED) or another error code)


nsIHelperAppLauncher should implement nsICancelable. this is part of the work to
make all the download-related interfaces implemented by mozilla code implement
nsICancelable.
Created attachment 180333 [details] [diff] [review]
patch
Attachment #180333 - Flags: superreview?(darin)
Attachment #180333 - Flags: review?(bzbarsky)
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta2
Attachment #180333 - Flags: review?(bzbarsky) → review+

Comment 2

13 years ago
Comment on attachment 180333 [details] [diff] [review]
patch

>Index: embedding/components/ui/helperAppDlg/nsHelperAppDlg.js

>+            const NS_BINDING_ABORTED = 0x80020006;
>+            this.mLauncher.cancel(NS_BINDING_ABORTED);

This is not NS_BINDING_ABORTED.  I think you want 0x804b0002


>Index: uriloader/exthandler/nsExternalHelperAppService.cpp

>+NS_IMETHODIMP nsExternalAppHandler::Cancel(nsresult aReason)
> {
>+  // XXX should not ignore the reason

How about adding a NS_ERROR_ARG(NS_FAILED(aReason)) at least
to ensure that people use this API correctly even if we aren't
going to use aReason for anything yet.


sr=darin with those changes
Attachment #180333 - Flags: superreview?(darin) → superreview+
> This is not NS_BINDING_ABORTED.  I think you want 0x804b0002

huh, true, I copied that from
http://lxr.mozilla.org/seamonkey/source/browser/components/nsBrowserContentHandler.js#62.
I filed bug 289981

Created attachment 180430 [details] [diff] [review]
patch v2
Attachment #180333 - Attachment is obsolete: true
Attachment #180430 - Flags: approval1.8b2?
justification for approval request:
this is an api change, and thus it'd be nice to get it into the developer
preview. it should also be comparatively low risk, since the main effect is that
a function got an additional parameter.

Comment 6

13 years ago
Comment on attachment 180430 [details] [diff] [review]
patch v2

a=asa
Attachment #180430 - Flags: approval1.8b2? → approval1.8b2+
thanks, checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.