Bug 67807 - Inline DocumentWriter::encoding() into it's only caller: deprecatedFrameEncoding()
Summary: Inline DocumentWriter::encoding() into it's only caller: deprecatedFrameEncod...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-08 14:39 PDT by Eric Seidel (no email)
Modified: 2011-09-08 22:58 PDT (History)
3 users (show)

See Also:


Attachments
Patch (1.50 KB, patch)
2011-09-08 14:39 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (2.72 KB, patch)
2011-09-08 14:45 PDT, Adam Barth
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
Patch
Comment 2 Adam Barth 2011-09-08 14:45:45 PDT
Created attachment 106793 [details]
Patch
Comment 3 Eric Seidel (no email) 2011-09-08 14:46:59 PDT
Comment on attachment 106793 [details]
Patch

LGTM.
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:
<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
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)