Summary: | Inline DocumentWriter::encoding() into it's only caller: deprecatedFrameEncoding() | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||
Component: | New Bugs | Assignee: | Eric Seidel (no email) <eric> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, dbates, mrobinson | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2011-09-08 14:39:05 PDT
Created attachment 106791 [details]
Patch
Created attachment 106793 [details]
Patch
Comment on attachment 106793 [details]
Patch
LGTM.
Committed r94810: <http://trac.webkit.org/changeset/94810> This patch broke the GTK build as webkit_web_view_get_encoding() (in webkitwebview.cpp) calls DocumentWriter::encoding(). Substituted DocumentWriter::deprecatedFrameEncoding() for DocumentWriter::encoding() in webkit_web_view_get_encoding() and committed this change in <http://trac.webkit.org/changeset/94826>. For completeness, the following are the build logs from the bots: GTK Linux 32-bit Release: <http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/17225/steps/compile-webkit/logs/stdio> GTK Linux 64-bit Debug: http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/25866/steps/compile-webkit/logs/stdio That's probably not the right fix. The right fix is most likely to get the encoding from Document::encoding. With any luck, deprecatedFrameEncoding will be removed shortly as well. Perhaps setEncoding needs a more descriptive name - it's not clear what its precedence is, or how it's related to deprecatedFrameEncoding. (In reply to comment #8) > Perhaps setEncoding needs a more descriptive name - it's not clear what its precedence is, or how it's related to deprecatedFrameEncoding. Eric said exactly the same thing. Part of this process is to get our minds wrapped around exactly how the encoding selection works. Once we understand that fully we should be able to pick better names for things. For context, Alexey, we're trying to remove deprecatedFrameEncoding and have FrameLoader get the fallback encoding from the current document (which is conceptually what should be happening). It seems that the document's encoding almost always matches the deprecatedFrameEncoding, except possibly when appcache is involved or when the deprecatedFrameEncoding is empty (in which case the document picks a default encoding). Here's a patch we're suing to experiment with this change: diff --git a/Source/WebCore/loader/FrameLoader.cpp b/Source/WebCore/loader/FrameLoader.cpp index c14ad6b..6a4e4f7 100644 --- a/Source/WebCore/loader/FrameLoader.cpp +++ b/Source/WebCore/loader/FrameLoader.cpp @@ -2543,7 +2543,11 @@ void FrameLoader::addExtraFieldsToRequest(ResourceRequest& request, FrameLoadTyp // Always try UTF-8. If that fails, try frame encoding (if any) and then the default. // For a newly opened frame with an empty URL, encoding() should not be used, because this methods asks decoder, which uses ISO-8859-1. Settings* settings = m_frame->settings(); - request.setResponseContentDispositionEncodingFallbackArray("UTF-8", activeDocumentLoader()->writer()->deprecatedFrameEncoding(), settings ? settings->defaultTextEncodingName() : String()); + String encodingFromDocument = m_frame->document()->decoder() ? m_frame->document()->decoder()->encoding().name() : String(); + String deprecatedEncoding = activeDocumentLoader()->writer()->deprecatedFrameEncoding(); + //printf("deprecated: %s document: %s\n", deprecatedEncoding.ascii().data(), encodingFromDocument.ascii().data()); + ASSERT(deprecatedEncoding == encodingFromDocument || (encodingFromDocument == "ISO-8859-1" && deprecatedEncoding.isEmpty())); + request.setResponseContentDispositionEncodingFallbackArray("UTF-8", deprecatedEncoding, settings ? settings->defaultTextEncodingName() : String()); } void FrameLoader::addHTTPOriginIfNeeded(ResourceRequest& request, const String& origin) |