nsIconURI::Clone and ::Resolve are unimplemented

RESOLVED FIXED

Status

()

Core
ImageLib
--
critical
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Christian Persch (GNOME) (away; not receiving bug mail), Assigned: Christian Persch (GNOME) (away; not receiving bug mail))

Tracking

({fixed1.8.0.2, fixed1.8.1})

Trunk
x86
Linux
fixed1.8.0.2, fixed1.8.1
Points:
---
Bug Flags:
blocking1.8.0.2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nvn-dl])

Attachments

(2 attachments, 1 obsolete attachment)

http://lxr.mozilla.org/seamonkey/source/modules/libpr0n/decoders/icon/nsIconURI.cpp#434
:

434 NS_IMETHODIMP
435 nsMozIconURI::Clone(nsIURI **result)
436 {
437   return NS_OK;
438 }
439 
440 NS_IMETHODIMP
441 nsMozIconURI::Resolve(const nsACString &relativePath, nsACString &result)
442 {
443   return NS_OK;
444 }

They should throw NS_ERROR_NOT_IMPLEMENTED, but I'd really like at least ::Clone
to be implemented.
Created attachment 199359 [details] [diff] [review]
the easy way: throw NS_ERROR_NOT_IMPLEMENTED
Attachment #199359 - Flags: superreview?(bryner)
Attachment #199359 - Flags: review?(cbiesinger)
Attachment #199359 - Flags: review?(cbiesinger) → review+
Attachment #199359 - Flags: superreview?(bryner) → superreview+
Checking in modules/libpr0n/decoders/icon/nsIconURI.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/nsIconURI.cpp,v  <--  nsIconURI.cpp
new revision: 1.20; previous revision: 1.19
done
Can this bug now be RESOLVED FIXED?
(In reply to comment #3)
> Can this bug now be RESOLVED FIXED?

I still want at least ::Clone to be really implemented; attachment 199359 [details] [diff] [review] was just the minimal fix. 
Summary: nsIconURI::Clone and ::Resolve return NS_OK but don't do anything → nsIconURI::Clone and ::Resolve are unimplemented
I think filing another bug for that would be the right approach. This one seemed to be about NS_OK being returned when it should really have been NS_ERROR_NOT_IMPLEMENTED; which has now been fixed.
Or maybe i'm totally wrong and should shut up :)

Updated

12 years ago
Blocks: 320621
Comment on attachment 199359 [details] [diff] [review]
the easy way: throw NS_ERROR_NOT_IMPLEMENTED

Requesting approval1.8.0.1 and approval1.8.1 for the patch because biesi says it will fix the crash in bug 320621.
Attachment #199359 - Flags: approval1.8.1?
Attachment #199359 - Flags: approval1.8.0.1?

Comment 8

12 years ago
Comment on attachment 199359 [details] [diff] [review]
the easy way: throw NS_ERROR_NOT_IMPLEMENTED

Please consider for 1.8.1 - 1.8.0.1 is for major security and crash issues only.
Attachment #199359 - Flags: approval1.8.0.1? → approval1.8.0.1-
Comment on attachment 199359 [details] [diff] [review]
the easy way: throw NS_ERROR_NOT_IMPLEMENTED

Renominating for 1.8.0.2 based on the reproducible crash this fixes.
Attachment #199359 - Flags: branch-1.8.1+
Attachment #199359 - Flags: approval1.8.1?
Attachment #199359 - Flags: approval1.8.0.2?
checked in on MOZILLA_1_8_BRANCH
Checking in modules/libpr0n/decoders/icon/nsIconURI.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/nsIconURI.cpp,v  <--  nsIconURI.cpp
new revision: 1.19.8.1; previous revision: 1.19
done
Keywords: fixed1.8.1
Keywords: fixed1.8.1
Created attachment 211984 [details] [diff] [review]
implement nsMozIconURI::Clone
Attachment #211984 - Flags: review?(cbiesinger)
Comment on attachment 211984 [details] [diff] [review]
implement nsMozIconURI::Clone

+    rv = mFileIcon->Clone (getter_AddRefs (newFileIcon));

please remove the space before the (, that's not the style of this file

+  nsMozIconURI *uri = new nsMozIconURI();
+  if (uri)

why not make this an early return as well, like for mFileIcon->Clone?

+    NS_ADDREF (uri);

again a space before (

+    uri->mFileIcon = newFileIcon;

might want to use .swap here, to avoid a useless addref/release pair
Attachment #211984 - Flags: review?(cbiesinger) → review+
Comment on attachment 199359 [details] [diff] [review]
the easy way: throw NS_ERROR_NOT_IMPLEMENTED

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #199359 - Flags: approval1.8.0.2? → approval1.8.0.2+
Flags: blocking1.8.0.2+
Keywords: fixed1.8.1
Created attachment 213082 [details] [diff] [review]
updated patch, addresses reviewer's comments
Attachment #211984 - Attachment is obsolete: true
Attachment #213082 - Flags: superreview?(bryner)
Attachment #213082 - Flags: review+
fix checked into 1.8.0 branch
Keywords: fixed1.8.0.2
Attachment #213082 - Flags: superreview?(bryner) → superreview+

Updated

12 years ago
Whiteboard: [nvn-dl]
Did that patch ever get checked in?  If not, do you need someone to do that or something?
Flags: blocking1.9a2?
Comment on attachment 213082 [details] [diff] [review]
updated patch, addresses reviewer's comments

> Did that patch ever get checked in?
Not checked in, no, since I have no cvs access and forgot about asking someone else...
> If not, do you need someone to do that or
something?
Yes :)
Assignee: pavlov → chpe
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Flags: blocking1.9a2?
You need to log in before you can comment on or make changes to this bug.