Closed Bug 539887 Opened 10 years ago Closed 10 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: hsivonen, Assigned: hsivonen)

References

Details

Attachments

(2 files, 1 obsolete file)

content/html/document/test/test_bug255820.html fails. Presumably, this has to do with the HTML5 parser ignoring document.written charset metas.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Attachment #428678 - Flags: review?(bzbarsky)
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.
Attachment #428679 - Flags: review?(bnewman)
Summary: [HTML5] Document.opened docs should save document.written charset on the wyciwyg cache entry → [HTML5][Patch] Document.opened docs should save document.written charset on the wyciwyg cache entry
Filed bug 548232 as a follow-up reminder.
Attachment #428678 - Flags: review?(bzbarsky) → review+
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.
(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.
This addresses the review comment.
Attachment #428679 - Attachment is obsolete: true
Attachment #431873 - Flags: review?(bnewman)
Attachment #428679 - Flags: review?(bnewman)
Blocks: 551344
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.
Attachment #431873 - Flags: review?(bnewman) → review+
Thanks! Pushed:
http://hg.mozilla.org/mozilla-central/rev/28513d79140d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.