337970 - nsHTMLDocument doesn't notify the charset resolver of the resolved character set

Status: RESOLVED INVALID

(Core :: Layout, defect)

Description

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

See bug 337962 comment 2. Patch in a bit.

Comment 1

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

Actually, does the charset resolver need to be notified after all other attempts at guessing the charset have been tried (i.e., right before SetDocumentCharacterSet is called in nsHTMLDocument::StartDocumentLoad)? I'm not quite sure how that can be done. I thought I could just call notifyResolvedCharset after requestCharset succeeds in TryBookmarkCharset, but notifying the resolver that we're using the charset it gave us when ultimately we may not be doesn't seem all that useful.

For the nsBookmarkService nsICharsetResolver implementation, I guess it doesn't really matter, since it doesn't ever set aWantCharset... but if we're assuming that it won't ever set aWantCharset, there isn't much point in adding code to check it.

Comment 2

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

Attachment: don't pass null (checked in)

This part needs fixing, regardless. I'm just not sure whether we should actually do something with "closure". I originally had:
+    if (wantCharset) {
+      // Notify the resolver
+      bookmarksResolver->NotifyResolvedCharset(charset, closure);
+    }
in the "RequestCharset succeeded" block, but as I said in the previous comment, that isn't all that useful.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> after all other attempts at guessing the charset have been tried

Yes.

For now, could just add an assert that aWantCharset gets set to false or something?  :(

Comment 4

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

Comment on attachment 222017 [details] [diff] [review]
don't pass null (checked in)

mozilla/content/html/document/src/nsHTMLDocument.cpp 	3.679

Comment 5

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

Attachment: assert that we're not asked to notify (checked in)

Comment 6

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

Comment on attachment 222054 [details] [diff] [review]
assert that we're not asked to notify (checked in)

mozilla/content/html/document/src/nsHTMLDocument.cpp 	3.680

Comment 7

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

Comment on attachment 222054 [details] [diff] [review]
assert that we're not asked to notify (checked in)

Sorry for being bugspammy, here...

Comment 8

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

Checked in both patches, branch and trunk. Unfortunately I think that's about as much as I can do here, at least for now.

Comment 9

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

Bug 582712 made this INVALID, AFAICT.