312241 - nsIconURI::Clone and ::Resolve are unimplemented

Status: RESOLVED FIXED

Description

[Christian Persch (GNOME) (away; not receiving bug mail)]

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.

Comment 1

[Christian Persch (GNOME) (away; not receiving bug mail)]

Created attachment 199359 [details] [diff] [review]
the easy way: throw NS_ERROR_NOT_IMPLEMENTED

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 3

[Steve England [:stevee]]

Can this bug now be RESOLVED FIXED?

Comment 4

[Christian Persch (GNOME) (away; not receiving bug mail)]

(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. 

Comment 5

[Steve England [:stevee]]

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.

Comment 6

[Steve England [:stevee]]

Or maybe i'm totally wrong and should shut up :)

Comment 7

[Reed Loden [:reed] (use needinfo?)]

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.

Comment 8

[Mike Schroepfer]

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.

Comment 9

[Benjamin Smedberg]

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.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 11

[Christian Persch (GNOME) (away; not receiving bug mail)]

Created attachment 211984 [details] [diff] [review]
implement nsMozIconURI::Clone

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 13

[Daniel Veditz [:dveditz]]

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

Comment 14

[Christian Persch (GNOME) (away; not receiving bug mail)]

Created attachment 213082 [details] [diff] [review]
updated patch, addresses reviewer's comments

Comment 15

[Daniel Veditz [:dveditz]]

fix checked into 1.8.0 branch

Comment 16

[Boris Zbarsky [:bz] (still digging out from vacation mail)]

Did that patch ever get checked in?  If not, do you need someone to do that or something?

Comment 17

[Christian Persch (GNOME) (away; not receiving bug mail)]

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 :)

Comment 18

[Boris Zbarsky [:bz] (still digging out from vacation mail)]

Fixed on trunk.