Closed Bug 337962 Opened 15 years ago Closed 15 years ago

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

Categories

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

1.8 Branch
defect

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey1.1alpha

People

(Reporter: Gavin, Assigned: Gavin)

References

Details

(Keywords: fixed-seamonkey1.1a, fixed1.8.1)

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #222010 - Flags: superreview?(bzbarsky)
Attachment #222010 - Flags: review?(neil)
Priority: -- → P5
> 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.
Attachment #222010 - Attachment is obsolete: true
Attachment #222010 - Flags: superreview?(bzbarsky)
Attachment #222010 - Flags: review?(neil)
Depends on: 337970
(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
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+
Attached patch fix QI (obsolete) — Splinter Review
(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+
Attachment #222048 - Attachment is obsolete: true
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
Flags: blocking-seamonkey1.1a?
Attachment #222056 - Flags: approval-branch-1.8.1?(neil) → approval-branch-1.8.1+
mozilla/xpfe/components/bookmarks/src/nsBookmarksService.cpp 	1.336.2.1
Flags: blocking-seamonkey1.1a?
You need to log in before you can comment on or make changes to this bug.