539887 - [HTML5][Patch] Document.opened docs should save document.written charset on the wyciwyg cache entry

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P1)

Description

[Henri Sivonen (:hsivonen)]

content/html/document/test/test_bug255820.html fails. Presumably, this has to do with the HTML5 parser ignoring document.written charset metas.

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Comment out a test assertion per IRC discussion

Comment 2

[Henri Sivonen (:hsivonen)]

Attachment: Partial fix for making document.written meta charset on document.opened docs set the charset on the document object

This patch is incomplete: the charset doesn't get properly recorded in the wyciwyg channel's cache entry. Per IRC discussion with Boris, I'll start discussion on the WHATWG list about what the complete right fix should be. Still, I'm pretty convinced the right fix will require the components that are already in this patch, and I'd like to get these bit out of the way so that they don't cause merge conflicts with other fixes.

Comment 3

[Henri Sivonen (:hsivonen)]

Filed bug 548232 as a follow-up reminder.

Comment 4

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 428679 [details] [diff] [review]
Partial fix for making document.written meta charset on document.opened docs set the charset on the document object

>   mStreamParser->SetDocumentCharset(aCharset, aCharsetSource);
>+  mExecutor->SetDocumentCharsetSource(aCharsetSource);
>   mExecutor->SetDocumentCharset((nsACString&)aCharset);

Why not make these interfaces more similar; i.e., add aCharsetSource parameter to the existing nsHtml5TreeOpExecutor::SetDocumentCharset method?  You set both in every instance that you set either, as far as I can tell from this patch.

Comment 5

[Henri Sivonen (:hsivonen)]

Test pushed:
http://hg.mozilla.org/mozilla-central/rev/dd43b45fac42

Comment 6

[Henri Sivonen (:hsivonen)]

(In reply to comment #4)
> (From update of attachment 428679 [details] [diff] [review])
> >   mStreamParser->SetDocumentCharset(aCharset, aCharsetSource);
> >+  mExecutor->SetDocumentCharsetSource(aCharsetSource);
> >   mExecutor->SetDocumentCharset((nsACString&)aCharset);
> 
> Why not make these interfaces more similar; i.e., add aCharsetSource parameter
> to the existing nsHtml5TreeOpExecutor::SetDocumentCharset method?  You set both
> in every instance that you set either, as far as I can tell from this patch.

SetDocumentCharset comes from nsIContentSink. That's why it is a separate method.

I quick look at the callers on MXR suggests that I could turn the method from the interface into an unimplemented stub and add a new method that combines the current two.

Comment 7

[Henri Sivonen (:hsivonen)]

Attachment: Combine charset and charset source setting

This addresses the review comment.

Comment 8

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 431873 [details] [diff] [review]
Combine charset and charset source setting

Looks good.  Thanks for revisiting this, and my apologies for the delayed review.

Comment 9

[Henri Sivonen (:hsivonen)]

Thanks! Pushed:
http://hg.mozilla.org/mozilla-central/rev/28513d79140d