67807 2011-09-08 14:39:05 -0700 Inline DocumentWriter::encoding() into it's only caller: deprecatedFrameEncoding() 2011-09-08 22:58:51 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 eric eric abarth dbates mrobinson oldest_to_newest 464382 0 eric 2011-09-08 14:39:05 -0700 Make DocumentWriter::encoding() private as it's only used by deprecatedFrameEncoding() 464384 1 106791 eric 2011-09-08 14:39:29 -0700 Created attachment 106791 Patch 464387 2 106793 abarth 2011-09-08 14:45:45 -0700 Created attachment 106793 Patch 464388 3 106793 eric 2011-09-08 14:46:59 -0700 Comment on attachment 106793 Patch LGTM. 464430 4 abarth 2011-09-08 15:19:35 -0700 Committed r94810: <http://trac.webkit.org/changeset/94810> 464561 5 dbates 2011-09-08 18:35:16 -0700 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 464580 6 abarth 2011-09-08 18:54:37 -0700 That's probably not the right fix. The right fix is most likely to get the encoding from Document::encoding. 464581 7 abarth 2011-09-08 18:54:53 -0700 With any luck, deprecatedFrameEncoding will be removed shortly as well. 464647 8 ap 2011-09-08 22:18:49 -0700 Perhaps setEncoding needs a more descriptive name - it's not clear what its precedence is, or how it's related to deprecatedFrameEncoding. 464659 9 abarth 2011-09-08 22:58:51 -0700 (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) 106791 2011-09-08 14:39:29 -0700 2011-09-08 14:40:55 -0700 Patch bug-67807-20110908143928.patch text/plain 1532 eric U3VidmVyc2lvbiBSZXZpc2lvbjogOTQ4MDAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCAxYzVmYjQ3ZmJmMzI0MDgw NDE1M2E0YmRiOTYwMDliODllYjJkOWEzLi5kODMzYTAzMDViYWNhZTU0MDIzODFkMDdhYjJiZGRl NWUyNmRjNWUwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMTQgQEAKIDIwMTEtMDktMDggIEVyaWMg U2VpZGVsICA8ZXJpY0B3ZWJraXQub3JnPgogCisgICAgICAgIE1ha2UgRG9jdW1lbnRXcml0ZXI6 OmVuY29kaW5nKCkgcHJpdmF0ZSBhcyBpdCdzIG9ubHkgdXNlZCBieSBkZXByZWNhdGVkRnJhbWVF bmNvZGluZygpCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD02NzgwNworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg ICogbG9hZGVyL0RvY3VtZW50V3JpdGVyLmg6CisKKzIwMTEtMDktMDggIEVyaWMgU2VpZGVsICA8 ZXJpY0B3ZWJraXQub3JnPgorCiAgICAgICAgIFJlbW92ZSBEb2N1bWVudFdyaXRlcjo6c2V0RGVj b2RlciBhcyBhIGdyZXAgb2YgV2ViS2l0IHNob3dzIG5vIGNhbGxlcnMKICAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTY3ODAzCiAKZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJDb3JlL2xvYWRlci9Eb2N1bWVudFdyaXRlci5oIGIvU291cmNlL1dlYkNvcmUvbG9h ZGVyL0RvY3VtZW50V3JpdGVyLmgKaW5kZXggZTk5Mzg2Y2I4NTllODgzMWQ2OWYzNDkwODY1MmJi YmJmYzkxMjNiNi4uMmQzMDQ2OTk5NmVmNzNlNWI2MWM5NjBjMDUzYmRhNDg2Y2RjY2I4NyAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvbG9hZGVyL0RvY3VtZW50V3JpdGVyLmgKKysrIGIvU291 cmNlL1dlYkNvcmUvbG9hZGVyL0RvY3VtZW50V3JpdGVyLmgKQEAgLTU3LDcgKzU3LDYgQEAgcHVi bGljOgogICAgIAogICAgIHZvaWQgc2V0RnJhbWUoRnJhbWUqIGZyYW1lKSB7IG1fZnJhbWUgPSBm cmFtZTsgfQogCi0gICAgU3RyaW5nIGVuY29kaW5nKCkgY29uc3Q7CiAgICAgdm9pZCBzZXRFbmNv ZGluZyhjb25zdCBTdHJpbmcmIGVuY29kaW5nLCBib29sIHVzZXJDaG9zZW4pOwogCiAgICAgLy8g RklYTUU6IEl0J3MgcmVhbGx5IHVuZm9ydW5hdGUgdG8gbmVlZCB0byBleHBvc2UgdGhpcyBwaWVj ZSBvZiBzdGF0ZS4KQEAgLTc1LDYgKzc0LDkgQEAgcHVibGljOgogICAgIHZvaWQgc2V0RG9jdW1l bnRXYXNMb2FkZWRBc1BhcnRPZk5hdmlnYXRpb24oKTsKIAogcHJpdmF0ZToKKyAgICAvLyBGSVhN RTogRGVsZXRlIHRoaXMgb25jZSBkZXByZWNhdGVkRnJhbWVFbmNvZGluZyBpcyByZW1vdmVkLgor ICAgIFN0cmluZyBlbmNvZGluZygpIGNvbnN0OworCiAgICAgUGFzc1JlZlB0cjxEb2N1bWVudD4g Y3JlYXRlRG9jdW1lbnQoY29uc3QgS1VSTCYpOwogICAgIHZvaWQgY2xlYXIoKTsKIAo= 106793 2011-09-08 14:45:45 -0700 2011-09-08 14:46:59 -0700 Patch bug-67807-20110908144543.patch text/plain 2781 abarth SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDk0ODAyKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTEtMDktMDggIEFkYW0gQmFy dGggIDxhYmFydGhAd2Via2l0Lm9yZz4KKworICAgICAgICBJbmxpbmUgRG9jdW1lbnRXcml0ZXI6 OmVuY29kaW5nKCkgaW50byBpdCdzIG9ubHkgY2FsbGVyOiBkZXByZWNhdGVkRnJhbWVFbmNvZGlu ZygpCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02Nzgw NworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoaXMg ZnVuY3Rpb24gaXMgc3VwZXIgbnV0dHkuICBXZSBkb24ndCB3YW50IGFueSBtb3JlIGZvbGtzIHRv IGNhbGwgaXQKKyAgICAgICAgdGhpbmtpbmcgdGhhdCBpdCBkb2VzIHNvbWV0aGluZyBzYW5lLgor CisgICAgICAgICogbG9hZGVyL0RvY3VtZW50V3JpdGVyLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6 OkRvY3VtZW50V3JpdGVyOjpkZXByZWNhdGVkRnJhbWVFbmNvZGluZyk6CisgICAgICAgICogbG9h ZGVyL0RvY3VtZW50V3JpdGVyLmg6CisKIDIwMTEtMDktMDggIEVyaWMgU2VpZGVsICA8ZXJpY0B3 ZWJraXQub3JnPgogCiAgICAgICAgIFJlbW92ZSBEb2N1bWVudFdyaXRlcjo6c2V0RGVjb2RlciBh cyBhIGdyZXAgb2YgV2ViS2l0IHNob3dzIG5vIGNhbGxlcnMKSW5kZXg6IFNvdXJjZS9XZWJDb3Jl L2xvYWRlci9Eb2N1bWVudFdyaXRlci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUv bG9hZGVyL0RvY3VtZW50V3JpdGVyLmNwcAkocmV2aXNpb24gOTQ4MDIpCisrKyBTb3VyY2UvV2Vi Q29yZS9sb2FkZXIvRG9jdW1lbnRXcml0ZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0yMzYsMTYg KzIzNiw2IEBAIHZvaWQgRG9jdW1lbnRXcml0ZXI6OmVuZElmTm90TG9hZGluZ01haW4KICAgICBt X3BhcnNlciA9IDA7CiB9CiAKLVN0cmluZyBEb2N1bWVudFdyaXRlcjo6ZW5jb2RpbmcoKSBjb25z dAotewotICAgIGlmIChtX2VuY29kaW5nV2FzQ2hvc2VuQnlVc2VyICYmICFtX2VuY29kaW5nLmlz RW1wdHkoKSkKLSAgICAgICAgcmV0dXJuIG1fZW5jb2Rpbmc7Ci0gICAgaWYgKG1fZGVjb2RlciAm JiBtX2RlY29kZXItPmVuY29kaW5nKCkuaXNWYWxpZCgpKQotICAgICAgICByZXR1cm4gbV9kZWNv ZGVyLT5lbmNvZGluZygpLm5hbWUoKTsKLSAgICBTZXR0aW5ncyogc2V0dGluZ3MgPSBtX2ZyYW1l LT5zZXR0aW5ncygpOwotICAgIHJldHVybiBzZXR0aW5ncyA/IHNldHRpbmdzLT5kZWZhdWx0VGV4 dEVuY29kaW5nTmFtZSgpIDogU3RyaW5nKCk7Ci19Ci0KIHZvaWQgRG9jdW1lbnRXcml0ZXI6OnNl dEVuY29kaW5nKGNvbnN0IFN0cmluZyYgbmFtZSwgYm9vbCB1c2VyQ2hvc2VuKQogewogICAgIG1f ZnJhbWUtPmxvYWRlcigpLT53aWxsU2V0RW5jb2RpbmcoKTsKQEAgLTI1NSw3ICsyNDUsMTcgQEAg dm9pZCBEb2N1bWVudFdyaXRlcjo6c2V0RW5jb2RpbmcoY29uc3QgUwogCiBTdHJpbmcgRG9jdW1l bnRXcml0ZXI6OmRlcHJlY2F0ZWRGcmFtZUVuY29kaW5nKCkgY29uc3QKIHsKLSAgICByZXR1cm4g bV9mcmFtZS0+ZG9jdW1lbnQoKS0+dXJsKCkuaXNFbXB0eSgpID8gbV9lbmNvZGluZyA6IGVuY29k aW5nKCk7CisgICAgaWYgKG1fZnJhbWUtPmRvY3VtZW50KCktPnVybCgpLmlzRW1wdHkoKSkKKyAg ICAgICAgcmV0dXJuIG1fZW5jb2Rpbmc7CisKKyAgICBpZiAobV9lbmNvZGluZ1dhc0Nob3NlbkJ5 VXNlciAmJiAhbV9lbmNvZGluZy5pc0VtcHR5KCkpCisgICAgICAgIHJldHVybiBtX2VuY29kaW5n OworCisgICAgaWYgKG1fZGVjb2RlciAmJiBtX2RlY29kZXItPmVuY29kaW5nKCkuaXNWYWxpZCgp KQorICAgICAgICByZXR1cm4gbV9kZWNvZGVyLT5lbmNvZGluZygpLm5hbWUoKTsKKworICAgIFNl dHRpbmdzKiBzZXR0aW5ncyA9IG1fZnJhbWUtPnNldHRpbmdzKCk7CisgICAgcmV0dXJuIHNldHRp bmdzID8gc2V0dGluZ3MtPmRlZmF1bHRUZXh0RW5jb2RpbmdOYW1lKCkgOiBTdHJpbmcoKTsKIH0K IAogdm9pZCBEb2N1bWVudFdyaXRlcjo6c2V0RG9jdW1lbnRXYXNMb2FkZWRBc1BhcnRPZk5hdmln YXRpb24oKQpJbmRleDogU291cmNlL1dlYkNvcmUvbG9hZGVyL0RvY3VtZW50V3JpdGVyLmgKPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvbG9hZGVyL0RvY3VtZW50V3JpdGVyLmgJKHJldmlz aW9uIDk0ODAyKQorKysgU291cmNlL1dlYkNvcmUvbG9hZGVyL0RvY3VtZW50V3JpdGVyLmgJKHdv cmtpbmcgY29weSkKQEAgLTU3LDcgKzU3LDYgQEAgcHVibGljOgogICAgIAogICAgIHZvaWQgc2V0 RnJhbWUoRnJhbWUqIGZyYW1lKSB7IG1fZnJhbWUgPSBmcmFtZTsgfQogCi0gICAgU3RyaW5nIGVu Y29kaW5nKCkgY29uc3Q7CiAgICAgdm9pZCBzZXRFbmNvZGluZyhjb25zdCBTdHJpbmcmIGVuY29k aW5nLCBib29sIHVzZXJDaG9zZW4pOwogCiAgICAgLy8gRklYTUU6IEl0J3MgcmVhbGx5IHVuZm9y dW5hdGUgdG8gbmVlZCB0byBleHBvc2UgdGhpcyBwaWVjZSBvZiBzdGF0ZS4K