Bug 97929

Summary: WebKit Doesn't Recognize Content-Language HTTP Header
Product: WebKit Reporter: Brian White <bcwhite>
Component: WebCore Misc.Assignee: Brian White <bcwhite>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, bashi, dglazkov, efidler, falken, fishd, jamesr, japhet, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: Unspecified   
URL: http://www.w3.org/International/tests/html-css/generate?format=h5&test=language-declarations-003
Bug Depends on:    
Bug Blocks: 10874    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Brian White
Reported 2012-09-28 12:22:34 PDT
If a "Content-Language" header is included in the HTTP response, the value is not recorded in the Document. Only http-equiv values from the meta tags are recognized. Also, this information is not exported through WebDocument for outsiders to read. Patch forthcoming... -- Brian
Attachments
Patch (6.13 KB, patch)
2012-09-28 13:26 PDT, Brian White 		no flags
DetailsFormatted DiffDiff
Patch (6.18 KB, patch)
2012-09-28 13:41 PDT, Brian White 		no flags
DetailsFormatted DiffDiff
Patch (1.75 KB, patch)
2012-10-01 14:06 PDT, Brian White 		no flags
DetailsFormatted DiffDiff
Patch (3.19 KB, patch)
2012-10-03 06:31 PDT, Brian White 		no flags
DetailsFormatted DiffDiff
Patch (7.02 KB, patch)
2012-10-09 11:31 PDT, Brian White 		no flags
DetailsFormatted DiffDiff
Patch (7.02 KB, patch)
2012-10-09 12:03 PDT, Brian White 		no flags
DetailsFormatted DiffDiff
Patch (10.01 KB, patch)
2012-10-10 08:43 PDT, Brian White 		no flags
DetailsFormatted DiffDiff
Patch (10.64 KB, patch)
2012-10-10 12:08 PDT, Brian White 		no flags
DetailsFormatted DiffDiff
Patch (10.42 KB, patch)
2012-10-12 14:11 PDT, Brian White 		no flags
DetailsFormatted DiffDiff
Patch (10.71 KB, patch)
2012-10-16 10:47 PDT, Brian White 		no flags
DetailsFormatted DiffDiff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.
Brian White
Comment 1 2012-09-28 13:26:26 PDT
Created attachment 166305 [details] Patch
WebKit Review Bot
Comment 2 2012-09-28 13:29:21 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
WebKit Review Bot
Comment 3 2012-09-28 13:29:40 PDT
Attachment 166305 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/loader/DocumentWriter.cpp:124: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:10: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:11: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:12: Line contains tab character. [whitespace/tab] [5] Total errors found: 5 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Brian White
Comment 4 2012-09-28 13:41:00 PDT
Created attachment 166307 [details] Patch
WebKit Review Bot
Comment 5 2012-09-28 13:44:22 PDT
Attachment 166307 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/loader/DocumentWriter.cpp:124: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Nate Chapin
Comment 6 2012-09-28 13:46:09 PDT
Comment on attachment 166307 [details] Patch Teaching DocumentWriter about ResourceResponse seems undesirable to me, as it currently doesn't have any real concept of how the data is delivered, just that there are bits that need to go into a Document. Perhaps we could just send this data directly from DocumentLoader to Document, similarly to the setBaseURLOveride() call in DocumentLoader::commitData()?
Brian White
Comment 7 2012-09-28 13:50:46 PDT
There's one "style violation" but I'd like to keep it as it indicates that this is where more such extractions can be done in the future.
Adam Barth
Comment 8 2012-09-28 14:13:45 PDT
> There's one "style violation" but I'd like to keep it as it indicates that this is where more such extractions can be done in the future. If people need to add more statements to the body of the if statement, they're smart enough to add the braces themselves. :)
Adam Barth
Comment 9 2012-09-28 14:14:44 PDT
Comment on attachment 166307 [details] Patch This is not the correct way to plumb this information into Document. There are a bunch of other examples of Document learning information from HTTP header fields. They might provide useful templates for this sort of change.
Alexey Proskuryakov
Comment 10 2012-09-28 17:01:25 PDT
Please consider separating cross-platform and Chromium changes into two patches.
Brian White
Comment 11 2012-09-28 17:22:45 PDT
Nate, I wasn't looking at it as "how it was delivered" but rather "what was delivered". Header contents are (sort of) the "beginning" of a web page and so it seemed reasonable to pass it to "begin" in order to include that information. I suppose that the DocumentWriter may be able to extract the embedded Document pointer (since they're both part of WebCore) from the WebDocument reference it has and inject the ContentLanguage value directly. Would you prefer that? -- Brian
Adam Barth
Comment 12 2012-09-28 17:26:04 PDT
DocumentWriter should not be involved in this operation. Please see Comment #9 for advice about how to implement this patch. This is not the first HTTP header that Documents are interested in learning about.
Brian White
Comment 13 2012-09-28 20:20:06 PDT
Adam, if you would provide a direct pointer to where it is being done, I would appreciate it. I looked for quite some time but could not find such examples. Thanks. -- Brian
Adam Barth
Comment 14 2012-09-29 08:53:06 PDT
> Adam, if you would provide a direct pointer to where it is being done, I would appreciate it. I looked for quite some time but could not find such examples. I grepped Source/WebCore for "httpHeaderField" and I found a bunch of calls in FrameLoader::didBeginDocument. That's what I'd try first.
Brian White
Comment 15 2012-10-01 14:06:12 PDT
Created attachment 166538 [details] Patch
WebKit Review Bot
Comment 16 2012-10-01 14:08:14 PDT
Attachment 166538 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Brian White
Comment 17 2012-10-01 14:15:07 PDT
Thanks for the pointer, Adam. I think this is more what you're looking for. Oops... Forgot to add a test... Will do that tomorrow.
Adam Barth
Comment 18 2012-10-01 14:58:33 PDT
Comment on attachment 166538 [details] Patch Yeah, that looks much better. You've still got a style issue (a tab snuck unto your ChangeLog entry). Also, we'll need a test.
Brian White
Comment 19 2012-10-02 08:00:11 PDT
I'm looking at how to write a test for this... I don't see any existing tests for this area of the code. Am I just missing it?
Alexey Proskuryakov
Comment 20 2012-10-02 13:19:08 PDT
The easiest way to make a test is to take any existing test for Content-Language in META tag, copy it to http/tests/misc, and convert that to a PHP file that prints this HTTP header.
Brian White
Comment 21 2012-10-02 13:22:29 PDT
Ah, an end-to-end test rather than a unit-test on that specific block of code. Okay, I'll try that.
Alexey Proskuryakov
Comment 22 2012-10-02 13:32:27 PDT
Once you get this running, please add a number of tests that check parsing correctness (e.g. what if there are multiple values? What if there are disallowed characters?)
Brian White
Comment 23 2012-10-03 06:31:10 PDT
Created attachment 166881 [details] Patch
Brian White
Comment 24 2012-10-03 06:34:12 PDT
Alexey, this is injecting the value just as the http-equiv meta tag would do. Tests for that seem easier and faster as it doesn't need php or an http server to execute. I haven't yet found how .WebkitLocale gets set so don't know if there's any validation at all. Meanwhile... Is this test okay? -- Brian
Adam Barth
Comment 25 2012-10-03 09:48:53 PDT
Comment on attachment 166881 [details] Patch Does this test fail without this patch?
Alexey Proskuryakov
Comment 26 2012-10-03 10:08:11 PDT
> Alexey, this is injecting the value just as the http-equiv meta tag would do. Tests for that seem easier and faster as it doesn't need php or an http server to execute. I do not understand what you are saying. The test included in the latest patch is a PHP script. Also, please do add tests for invalid and multiple values, as suggested previously.
Brian White
Comment 27 2012-10-03 12:19:59 PDT
Alexey, I'm saying that the tests you're requesting (invalid and multiple values) apply equally to an "http-equiv" for "content-language" as both that and this header copy the value to the same place in the Document data structure. However, I can write tests for http-equiv=content-language under fast/css entirely as HTML without need for PHP and a webserver to process it. That will make the tests faster and still yield the same amount of testing. Of course, I'm not yet even sure if there _is_ any parsing to test. Still looking...
Alexey Proskuryakov
Comment 28 2012-10-03 12:42:53 PDT
> Alexey, I'm saying that the tests you're requesting (invalid and multiple values) apply equally to an "http-equiv" for "content-language" as both that and this header copy the value to the same place in the Document data structure. I don't know why this should be the case. These are governed by entirely separate specifications. Code paths for handling these are also not necessarily the same in browsers.
Brian White
Comment 29 2012-10-03 13:01:59 PDT
Adam, yes, the test fails without the patch. The value returned is "auto" if no HTTP header or http-equiv meta tag is present.
Brian White
Comment 30 2012-10-03 13:05:30 PDT
Alexey, I don't follow. "http-equiv" is a way of simulated the presence of a proper HTTP header. Why would the code paths (or tests) be different?
Alexey Proskuryakov
Comment 31 2012-10-03 13:26:41 PDT
> "http-equiv" is a way of simulated the presence of a proper HTTP header. This is the original idea, but in reality, there is no formal connection between http-equiv and HTTP headers syntax. Specifically, rules for parsing HTTP headers are defined in RFC 2616, while HTML has its own rules for Content-Language: <http://www.whatwg.org/specs/web-apps/current-work/#attr-meta-http-equiv-content-language>. Oh, we also need a test for what happens when there are both an http-equiv and an HTTP header field, and they disagree. HTML spec currently says that http-equiv wins.
Alexey Proskuryakov
Comment 32 2012-10-03 13:27:06 PDT
Comment on attachment 166881 [details] Patch r- to get this out of review queue. We clearly need better test coverage.
Brian White
Comment 33 2012-10-03 13:44:10 PDT
Alexey, good point about the header/equiv. The http-equiv wins (I knew that) but I'll create a test for that as well. As I read the docs at that link, it sounds like processing of the actual header should only set the document()->contentLanguage() if the value contains only one "intended language". Do you read that as well? Or would you prefer that I just drop this so that there is zero overlap between the HTTP header and http-equiv for Content-Language? -- Brian
Brian White
Comment 34 2012-10-05 08:03:20 PDT
Alexey, what do you see as the correct way to go forward with this? Only set the document()->contentLanguage() if the value contains only one "intended language" or just drop this so that there is zero overlap between the HTTP header and http-equiv for Content-Language?
Alexey Proskuryakov
Comment 35 2012-10-05 16:52:50 PDT
This primarily depends on what you need this for. As far as heuristics for glyph selection and charset guessing are concerned, using just the first language is fine. Also, I don't know usage patterns on actual web sites.
Matt Falkenhagen
Comment 36 2012-10-09 00:28:16 PDT
Currently the http-equiv for Content Language code in WebKit just uses the verbatim content string to set the "pragma-set default language". So even a comma-separated list like "a, b" is taken as the language "a, b". I had some plans to improve this parsing but haven't changed anything yet, see bug 77724. See also bug 76701, comment 30 for some explanation as to why the spec is the way it is.
Matt Falkenhagen
Comment 37 2012-10-09 00:29:37 PDT
*** Bug 76892 has been marked as a duplicate of this bug. ***
Brian White
Comment 38 2012-10-09 08:02:32 PDT
_My_ interest in this is Chrome's auto-translation feature. It can try to guess the language but it's (presumably) more accurate to use what is explicitly declared. For that purpose, both HTTP header and an http-equiv tag, despite their somewhat different definitions, accomplish the same thing.
Brian White
Comment 40 2012-10-09 11:31:59 PDT
Created attachment 167799 [details] Patch
Brian White
Comment 41 2012-10-09 11:34:22 PDT
Latest patch extracts only the first language from the header to substitute for an http-equiv tag. Added comments with details and more tests.
Build Bot
Comment 42 2012-10-09 11:41:14 PDT
Comment on attachment 167799 [details] Patch Attachment 167799 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14228714
Early Warning System Bot
Comment 43 2012-10-09 11:42:15 PDT
Comment on attachment 167799 [details] Patch Attachment 167799 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14209996
Early Warning System Bot
Comment 44 2012-10-09 11:47:50 PDT
Comment on attachment 167799 [details] Patch Attachment 167799 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14217838
Brian White
Comment 45 2012-10-09 12:03:17 PDT
Created attachment 167809 [details] Patch
Alexey Proskuryakov
Comment 46 2012-10-09 16:04:00 PDT
Comment on attachment 167809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167809&action=review Thank you for updating the patch. I think that it's quite close now. I have a number of nitpicks below, so another iteration would be beneficial. > Source/WebCore/loader/FrameLoader.cpp:678 > + // HTTP-header and http-equiv meta tag do not have exactly the same > + // meaning. The former is the "intended audience" and the latter > + // is the language of the document and must contain only a single > + // entry. If we detect the former with multiple entries separated > + // by commas, we take only the first one. Alternatively we could > + // ignore an HTTP Content-Language header with multiple entries. > + // http://tools.ietf.org/html/rfc2616#page-118 > + // http://www.whatwg.org/specs/web-apps/current-work/#attr-meta-http-equiv-content-language I would drop this whole comment. It is fairly obvious what's being done here, and if details were really important, one could always do an svn blame and find this bug. > Source/WebCore/loader/FrameLoader.cpp:679 > + size_t comma = headerContentLanguage.find(','); The variable holds comma position, not the comma itself, so it should be named accordingly. > Source/WebCore/loader/FrameLoader.cpp:683 > + m_frame->document()->setContentLanguage(headerContentLanguage); So we'll be setting content language to empty string if header field value were ",foo". This seems incorrect, please fix and add a regression test. > LayoutTests/http/tests/misc/extract-http-content-language-expected.txt:4 > +zh-CN > +ar It's not great that these tests don't have explicit pass/fail output. If one opens the test in browser, how would they know if the test passed? > LayoutTests/http/tests/misc/extract-http-content-language-multiple.php:2 > + header("Content-Language: fr, fi "); Could you try something a little more complicated, like maybe "fr \t , fi "?
Brian White
Comment 47 2012-10-09 18:32:14 PDT
Comment on attachment 167809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167809&action=review >> Source/WebCore/loader/FrameLoader.cpp:679 >> + size_t comma = headerContentLanguage.find(','); > > The variable holds comma position, not the comma itself, so it should be named accordingly. I had it named "comma_index" but got a style violation. Would "commaindex" be okay? >> Source/WebCore/loader/FrameLoader.cpp:683 >> + m_frame->document()->setContentLanguage(headerContentLanguage); > > So we'll be setting content language to empty string if header field value were ",foo". This seems incorrect, please fix and add a regression test. My thinking was that if the header was malformed then it's better not to put any additional effort into trying to guess what was intended because it's just as likely to guess wrong. >> LayoutTests/http/tests/misc/extract-http-content-language-expected.txt:4 >> +ar > > It's not great that these tests don't have explicit pass/fail output. If one opens the test in browser, how would they know if the test passed? And then just have "pass" in the -expected.txt file?
Alexey Proskuryakov
Comment 48 2012-10-09 19:20:47 PDT
> I had it named "comma_index" but got a style violation. Would "commaindex" be okay? commaIndex or commaPosition would be fine. > > So we'll be setting content language to empty string if header field value were ",foo". This seems incorrect, please fix and add a regression test. > > My thinking was that if the header was malformed then it's better not to put any additional effort into trying to guess what was intended because it's just as likely to guess wrong. A Web browser typically deals with malformed input, and it's important to deal with it gracefully. This is less about guessing than about maintaining internal invariants - and expecting that a content language member variable holds either a non-empty string or a null string is reasonable. > And then just have "pass" in the -expected.txt file? That would work failure case, it's desirable to have more information about what went wrong.
Alexey Proskuryakov
Comment 49 2012-10-09 19:21:01 PDT
That would work. In failure case, it's desirable to have more information about what went wrong.
Brian White
Comment 50 2012-10-10 08:43:03 PDT
Created attachment 168006 [details] Patch
Brian White
Comment 51 2012-10-10 08:44:18 PDT
It turns out that some processing has already been done on the header value so ",foo" will be just "foo" by the time it gets to this function. It's probably not possible to get an empty string (the header gets removed completely) but I've put the test in anyways so that it won't set an empty value.
Alexey Proskuryakov
Comment 52 2012-10-10 10:44:51 PDT
Comment on attachment 168006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168006&action=review > Source/WebCore/loader/FrameLoader.cpp:673 > + if (commaIndex != notFound) I don't think that this check is necessary - truncate will do nothing if the index is outside bounds. > LayoutTests/http/tests/misc/extract-http-content-language-malformed.php:20 > + if (language == expect) > + element.innerText = 'Pass. Got "' + expect + '" ' + msg + '.'; > + else > + element.innerText = 'Fail! Expected "' + expect + '" ' + msg + '; got "' + language + '".'; This would be much better if existing testing support was used (shouldBe coming from js-test-pre.js).
Brian White
Comment 53 2012-10-10 12:08:03 PDT
Created attachment 168050 [details] Patch
Alexey Proskuryakov
Comment 54 2012-10-10 12:29:32 PDT
Comment on attachment 168050 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168050&action=review Thank you for switching to js-test-pre.js. > LayoutTests/http/tests/misc/extract-http-content-language.php:17 > + if (window.testRunner) > + testRunner.dumpAsText(); These can be removed now - js-test-pre.js does it automatically. > LayoutTests/http/tests/misc/extract-http-content-language.php:22 > + debug('==> All done...'); This is not needed either - there is a TEST COMPLETE message.
Brian White
Comment 55 2012-10-10 13:24:15 PDT
>> LayoutTests/http/tests/misc/extract-http-content-language.php:22 >> + debug('==> All done...'); > >This is not needed either - there is a TEST COMPLETE message. Yes, but there is a "PASS successfullyParsed is true" message (output from the "js-test-post.js" script) which appears to be part of the previous test if there is no separator message.
Build Bot
Comment 56 2012-10-10 21:07:57 PDT
Comment on attachment 168050 [details] Patch Attachment 168050 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14254413 New failing tests: http/tests/misc/extract-http-content-language-malformed.php
Brian White
Comment 57 2012-10-12 13:31:56 PDT
So... Is it acceptable to leave the redundant debug("all done") call in order to separate the extraneous "PASS successfullyParsed is true" message from the actual tests?
Alexey Proskuryakov
Comment 58 2012-10-12 13:54:26 PDT
You still have r+, so yes, it's acceptable. I don't think there's value in that, but it's not important.
Brian White
Comment 59 2012-10-12 14:09:14 PDT
Okay. I've got a new CL that remove the unnecessary calls to testRunner.dumpAsText() and then I think we're all good!
Brian White
Comment 60 2012-10-12 14:11:24 PDT
Created attachment 168478 [details] Patch
Alexey Proskuryakov
Comment 61 2012-10-12 14:57:23 PDT
Did you find out why extract-http-content-language-malformed.php failed on mac-ews?
Brian White
Comment 62 2012-10-12 15:55:06 PDT
No, I haven't. When I looked at the link provided, there's no mention of that test. It lists 9 tests that failed but none of them are "extract-http-content-language-malformed.php". I wanted to see if it failed again with this updated patch to see if it was just a flaky test. If not, perhaps the "mac" version is not cleaning the header value as other builds do; perhaps it's not stripping the initial comma which would mean a different result in the test code. If it persists, I'll have to get someone here with a Mac to try my patch and see what's going on.
Alexey Proskuryakov
Comment 63 2012-10-12 16:09:56 PDT
> If not, perhaps the "mac" version is not cleaning the header value as other builds do I think that only chromium and mac run tests, other EWS bots just build.
Brian White
Comment 64 2012-10-12 19:10:04 PDT
Interesting. I didn't know that. I'm building on Windows and running the new tests locally there with no problems. But the guy next to me has a Mac so I'll just ask him to build and try it. I'll also verify it on my Linux machine.
Build Bot
Comment 65 2012-10-12 23:18:49 PDT
Comment on attachment 168478 [details] Patch Attachment 168478 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14298167 New failing tests: http/tests/misc/extract-http-content-language-malformed.php
Brian White
Comment 66 2012-10-15 12:51:18 PDT
Ah, so the issue is that the "mac" test is a Safari test and apparently Safari sanitizes the HTTP headers differently than Chrome. Specifically, Chrome turns a value of ",foo" into just "foo" while Safari either removes the header completely or (more likely) leaves it as-is. The end result is a language of "foo" known to Chrome but "auto" known to Safari. Should I update the test to detect the browser type and look for different results for the two?
Alexey Proskuryakov
Comment 67 2012-10-15 14:09:59 PDT
No, there shouldn't be browser sniffing in the test. Expected results should match what's correct behavior in our best judgement, and there should be platform specific results from platforms that get it wrong.
Brian White
Comment 68 2012-10-16 05:36:27 PDT
I was thinking the same. Given that there's no absolute defined behavior in the case of malformed input, I'll just modify the test to accept both logical outcomes: it makes a reasonable guess or refuses to make any guess at all.
Brian White
Comment 69 2012-10-16 10:47:50 PDT
Created attachment 168973 [details] Patch
Alexey Proskuryakov
Comment 70 2012-10-16 12:58:34 PDT
Comment on attachment 168973 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168973&action=review > Source/WebCore/loader/FrameLoader.cpp:673 > + headerContentLanguage.truncate(commaIndex); // notFound == -1 == don't truncate This comment is somewhat misleading - notFound is static_cast<size_t>(-1), not -1. I'd either omit the comment, or say something like "won't truncate if comma was not found". > Source/WebCore/loader/FrameLoader.cpp:674 > + headerContentLanguage = headerContentLanguage.stripWhiteSpace(isHTMLSpace); Haven't noticed this before. Why HTML space? We're not dealing with HTML here. > LayoutTests/http/tests/misc/extract-http-content-language.php:7 > +<script src="../../js-test-resources/js-test-pre.js"></script> A trivial nit - no need for relative path in HTTP tests. I think that "/js-test-resources/js-test-pre.js" would work just as well.
Brian White
Comment 71 2012-10-16 13:05:07 PDT
Comment on attachment 168973 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168973&action=review >> Source/WebCore/loader/FrameLoader.cpp:674 >> + headerContentLanguage = headerContentLanguage.stripWhiteSpace(isHTMLSpace); > > Haven't noticed this before. Why HTML space? We're not dealing with HTML here. The default match does not include \t as whitespace. HTMLspace does, as well as \r and \n which don't really apply here since header processing would have taken those. >> LayoutTests/http/tests/misc/extract-http-content-language.php:7 >> +<script src="../../js-test-resources/js-test-pre.js"></script> > > A trivial nit - no need for relative path in HTTP tests. I think that "/js-test-resources/js-test-pre.js" would work just as well. All the other tests in this directory use the ../.. prefix; I just copied them.
Brian White
Comment 72 2012-10-18 11:47:11 PDT
So... Is this good, then? All tests pass and I've given reasoning for the minor comments.
Alexey Proskuryakov
Comment 73 2012-10-18 12:21:36 PDT
Yes, the patch has an r+. I think that my comments still stand, but addressing them is not necessary to have this landed. If you need someone to approve this for commit queue, please set the cq? flag.
WebKit Review Bot
Comment 74 2012-10-18 12:54:06 PDT
Comment on attachment 168973 [details] Patch Clearing flags on attachment: 168973 Committed r131794: <http://trac.webkit.org/changeset/131794>
WebKit Review Bot
Comment 75 2012-10-18 12:54:13 PDT
All reviewed patches have been landed. Closing bug.
Brian White
Comment 76 2012-10-18 13:41:54 PDT
I want to thank everyone, especially Alexey for helping me with this, my first ever real WebKit change. It has been one of the most frustrating, aggravating, discouraging, and painful experiences of my life. :-) I also learned a lot. Thanks for your patience! -- Brian
Adam Barth
Comment 77 2012-10-18 15:53:30 PDT
We aim to please? Hopefully the next one will go smoother. :)
Mark Rowe (bdash)
Comment 78 2012-10-18 16:51:23 PDT
Why did this change set a bunch of files as executable?!
Brian White
Comment 79 2012-10-18 17:31:03 PDT
It did?!? I didn't notice that in the diff. Damn. There was a conflict between CgyWin and Xemacs that was causing edited files to receive permissions of 000 which was then converted to 755 whenever something was accessing the file. I thought I had corrected that before the last patch was submitted for review. I can submit a fix tomorrow.
Alexey Proskuryakov
Comment 80 2012-10-18 17:42:11 PDT
> Why did this change set a bunch of files as executable?! I overlooked HTMLParserIdioms.h in the patch, but are there any others?
Brian White
Comment 81 2012-10-19 07:38:15 PDT
Looks like someone else has already corrected this. Sorry 'bout that.
Note You need to log in before you can comment on or make changes to this bug.