Bug 67807

Summary: Inline DocumentWriter::encoding() into it's only caller: deprecatedFrameEncoding()
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: New BugsAssignee: Eric Seidel (no email) <eric>
Severity: Normal CC: abarth, dbates, mrobinson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch eric: review+

Description Eric Seidel (no email) 2011-09-08 14:39:05 PDT
Make DocumentWriter::encoding() private as it's only used by deprecatedFrameEncoding()
Comment 1 Eric Seidel (no email) 2011-09-08 14:39:29 PDT
Created attachment 106791 [details]
Comment 2 Adam Barth 2011-09-08 14:45:45 PDT
Created attachment 106793 [details]
Comment 3 Eric Seidel (no email) 2011-09-08 14:46:59 PDT
Comment on attachment 106793 [details]

Comment 4 Adam Barth 2011-09-08 15:19:35 PDT
Committed r94810: <http://trac.webkit.org/changeset/94810>
Comment 5 Daniel Bates 2011-09-08 18:35:16 PDT
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:

GTK Linux 64-bit Debug:
Comment 6 Adam Barth 2011-09-08 18:54:37 PDT
That's probably not the right fix.  The right fix is most likely to get the encoding from Document::encoding.
Comment 7 Adam Barth 2011-09-08 18:54:53 PDT
With any luck, deprecatedFrameEncoding will be removed shortly as well.
Comment 8 Alexey Proskuryakov 2011-09-08 22:18:49 PDT
Perhaps setEncoding needs a more descriptive name - it's not clear what its precedence is, or how it's related to deprecatedFrameEncoding.
Comment 9 Adam Barth 2011-09-08 22:58:51 PDT
(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)