Closed Bug 337962 Opened 15 years ago Closed 15 years ago
Bookmarks Service implements ns ICharset Resolver, but forgets to tell anyone that it does (xpfe bookmarks don't provide a charset to layout code)
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.
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.
> 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... :(
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 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.
(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.
No longer depends on: 337970
Comment on attachment 222010 [details] [diff] [review] patch We still want the QI change here, right? sr=bzbarsky on that part of the patch.
Attachment #222010 - Flags: superreview+
(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.
Attachment #222048 - Flags: review?(neil)
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.
Attachment #222048 - Flags: review?(neil) → review+
Checked in on the trunk. mozilla/xpfe/components/bookmarks/src/nsBookmarksService.cpp 1.340
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment on attachment 222056 [details] [diff] [review] fix QI, for checkin I'd like to fix this on the branch, too.
Attachment #222056 - Flags: approval-branch-1.8.1?(neil)
Target Milestone: --- → seamonkey1.1alpha
Attachment #222056 - Flags: approval-branch-1.8.1?(neil) → approval-branch-1.8.1+
You need to log in before you can comment on or make changes to this bug.