337962 - nsBookmarksService implements nsICharsetResolver, but forgets to tell anyone that it does (xpfe bookmarks don't provide a charset to layout code)

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect, P5)

Description

[:Gavin Sharp [email: [e-mail]]]

I found this while I was trying to steal the code to fix bug 284660.

Fixing that revealed that RequestCharset also does not handle a null aClosure, which is what the only current caller passes.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Note: I haven't tested this patch, but I've tested the equivalent patch to my code, which is ported to the Firefox bookmarks code.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> which is what the only current caller passes.

That's a bug in the caller; looks like the first checkin for bug 101995.

In fact, the caller doesn't really fulfill the contract here (which includes notifying the resolver of the final resolved charset as needed).

We should at least fix the null thing.  Want to do it?  Or should I when I get back?

I sorta wonder whether anyone tested the patches in bug 101995.  :(  I apparently did not... :(

Comment 3

[Boris Zbarsky [:bzbarsky]]

I'd really rather we fixed the nsHTMLDocument caller, since this "null out param" crap can't be implemented in general (e.g. in a JS impl of nsICharsetResolver).

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 222010 [details] [diff] [review]
patch

I guess I can do that. I'll port that change to my patch for bug 284660, too.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #2)
> In fact, the caller doesn't really fulfill the contract here (which includes
> notifying the resolver of the final resolved charset as needed

I'll fix this in bug 337970.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 222010 [details] [diff] [review]
patch

We still want the QI change here, right?  sr=bzbarsky on that part of the patch.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Attachment: fix QI

(In reply to comment #6)
> We still want the QI change here, right?

Yeah, we do. I'll check this in after I check in the patch for bug 337970, to avoid any crashing.

Comment 8

[[e-mail]]

Comment on attachment 222048 [details] [diff] [review]
fix QI

Hey, only three years late ;-)

>-NS_IMPL_QUERY_INTERFACE9(nsBookmarksService, 
>+NS_IMPL_QUERY_INTERFACE10(nsBookmarksService,
>              nsIBookmarksService,
>              nsIRDFDataSource,
>              nsIRDFRemoteDataSource,
>              nsIRDFPropagatableDataSource,
>              nsIRDFObserver,
>              nsIStreamListener,
>+             nsICharsetResolver,
>              nsIRequestObserver,
>              nsIObserver,
>              nsISupportsWeakReference)
Nit: nsIRequestObserver is the base class of nsIStreamListener, so nsICharsetResolver needs to go just before nsIObserver instead.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Attachment: fix QI, for checkin

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Checked in on the trunk.
mozilla/xpfe/components/bookmarks/src/nsBookmarksService.cpp 	1.340

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 222056 [details] [diff] [review]
fix QI, for checkin

I'd like to fix this on the branch, too.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

mozilla/xpfe/components/bookmarks/src/nsBookmarksService.cpp 	1.336.2.1