Bug 67807 – Inline DocumentWriter::encoding() into it's only caller: deprecatedFrameEncoding()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Make DocumentWriter::encoding() private as it's only used by deprecatedFrameEncoding()

[Eric Seidel (no email)]

Created attachment 106791 [details]
Patch

[Adam Barth]

Created attachment 106793 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 106793 [details]
Patch

LGTM.

[Adam Barth]

Committed r94810: <http://trac.webkit.org/changeset/94810>

[Daniel Bates]

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

[Adam Barth]

That's probably not the right fix.  The right fix is most likely to get the encoding from Document::encoding.

[Adam Barth]

With any luck, deprecatedFrameEncoding will be removed shortly as well.

[Alexey Proskuryakov]

Perhaps setEncoding needs a more descriptive name - it's not clear what its precedence is, or how it's related to deprecatedFrameEncoding.

[Adam Barth]

(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)