|
Description
Andrey Kosyakov
2011-04-08 02:34:38 PDT
Created attachment 91665 [details]
Patch implementing some of the missing functionality.
After talking with Andrey earlier in the week, I've implemented some of the required functionality.
This patch really just hits the trivial bits that were recently implemented. Most notably, cache attributes are missing entirely from this patch; it'll take a little more work than I expected to extract them in a reasonable way, and I'd like to break that work out into a separate patch to follow (probably sometime next week).
I didn't see any tests that hit this functionality, and I'm not familiar enough with WebKit to know where to add them myself. If you can point me in the right direction, I'd appreciate it.
Setting r? for feedback on the additions, though I expect r- for the tests, as noted above. :)
You can find existing tests in LayoutTests/http/tests/inspector/, look for resource-har* > Source/WebCore/inspector/front-end/HAREntry.js:86 > bodySize: this._resource.resourceSize Shouldn't bodySize be this._resource.transferSize - this._resource.responseHeadersSize? > Source/WebCore/inspector/front-end/HAREntry.js:94 > + compression: this._resource.resourceSize - this._resource.transferSize, This should probably be this._resource.resourceSize - (this._resource.transferSize - this._resource.responseHeadersSize)? It's not clear from specification to me. Couldn't it become negative if content size is very small? > Source/WebCore/inspector/front-end/HAREntry.js:96 > + text: this._resource.content // TODO: Why isn't this always available? Resource content is immediately available for XHR/script only. Otherwise you should pass your callback to resource.requestContent(). (In reply to comment #1) > I didn't see any tests that hit this functionality, and I'm not familiar enough with WebKit to know where to add them myself. If you can point me in the right direction, I'd appreciate it. See http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/inspector/resource-har-conversion.html We wary of the things that vary (hmm... sorry!) per platform or time: we often have to dump just the field type, not value (see http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/inspector/resources-test.js) Comment on attachment 91665 [details] Patch implementing some of the missing functionality. View in context: https://bugs.webkit.org/attachment.cgi?id=91665&action=review > Source/WebCore/inspector/front-end/HAREntry.js:61 > + httpVersion: this._resource.requestHttpVersion, // FIXME: This still periodically shows up as undefined. If you still see this as undefined, it's probably an indication of bug somewhere. Can you spot any regularity with that? > Source/WebCore/inspector/front-end/HAREntry.js:80 > + httpVersion: this._resource.responseHttpVersion, // FIXME: This still periodically shows up as undefined. ditto > Source/WebCore/inspector/front-end/HAREntry.js:96 > + text: this._resource.content // TODO: Why isn't this always available? This is intentional -- the resource content is likely to be absent, unless it was explicitly requested. Also, the HAR may grow huge. This should probably be a conversion option (and may be exposed in UI as "Copy HAR log with content"). This is also used by extensions, and we don't want every extension to receive every resource content, unless it really wants. > Source/WebCore/inspector/front-end/Resource.js:505 > + var match = firstLine.match(/(HTTP\/\d+\.\d+)$/); I wonder whether this works reliably -- we should normally have \r\n at the end of each header line, which leaves you with the trailing "\r" after split. Note that we'll have lone "\n" in case requestHeadersText is made up from parsed headers for platforms that don't have raw headers (i.e. everyone but chromium). This looks like a bug in requestHeadersText that needs to be fixed. > Source/WebCore/inspector/front-end/Resource.js:506 > + return (match) ? match[1] : undefined; Nit: parenthesis seem redundant. I'd rather do "return match && match[1]" for brevity. Comment on attachment 91665 [details] Patch implementing some of the missing functionality. View in context: https://bugs.webkit.org/attachment.cgi?id=91665&action=review Dropping r? since I got good feedback that I need to work into another patch. >> Source/WebCore/inspector/front-end/HAREntry.js:61 > > If you still see this as undefined, it's probably an indication of bug somewhere. Can you spot any regularity with that? Cached entries are the most obvious, as no request happens at all. I want to rework the handling entirely for those, honestly. I've also seen some strangeness with a few of the requests from, for example, google.com when it redirects to google.de. I'll try to narrow down a more clear test case. >> Source/WebCore/inspector/front-end/HAREntry.js:96 >> + text: this._resource.content // TODO: Why isn't this always available? > > This is intentional -- the resource content is likely to be absent, unless it was explicitly requested. Also, the HAR may grow huge. This should probably be a conversion option (and may be exposed in UI as "Copy HAR log with content"). This is also used by extensions, and we don't want every extension to receive every resource content, unless it really wants. That makes sense, as does splitting into "with content" and without. I'll drop `text` again in this patch, and put together a boolean flag for generation later. >> Source/WebCore/inspector/front-end/Resource.js:505 >> + var match = firstLine.match(/(HTTP\/\d+\.\d+)$/); > > I wonder whether this works reliably -- we should normally have \r\n at the end of each header line, which leaves you with the trailing "\r" after split. Note that we'll have lone "\n" in case requestHeadersText is made up from parsed headers for platforms that don't have raw headers (i.e. everyone but chromium). This looks like a bug in requestHeadersText that needs to be fixed. Breaking on `\n` alone doesn't break either of the request or response regexes, as far as I can tell: `$` captures the carriage return. That said, would changing the split to `\r?\n` work for you? I think you're correct that `\r\n` is dictated in the HTTP spec, but I'm not sure that we can/should rely on servers adhering to that standard. Regarding the parsed headers, I'll add `\r` there; you're correct to say that we should generate headers that match the spec. >> Source/WebCore/inspector/front-end/Resource.js:506 >> + return (match) ? match[1] : undefined; > > Nit: parenthesis seem redundant. I'd rather do "return match && match[1]" for brevity. Parens are redundant, sorry... just debris from refactoring. :) `return match && match[1]` has a different meaning than explicitly returning `undefined`... `undefined` feels more semantically correct: if I can't find a http version, it's undefined, it isn't `false`. :) That said, I'm fine with changing it if it's more in line with webkit style. (In reply to comment #2) > You can find existing tests in LayoutTests/http/tests/inspector/, look for resource-har* > > > Source/WebCore/inspector/front-end/HAREntry.js:86 > > bodySize: this._resource.resourceSize > > Shouldn't bodySize be this._resource.transferSize - this._resource.responseHeadersSize? It's unclear in the spec: "Size of the received response body in bytes." could mean bytes over the wire or total size. I'll dig into some of the examples to see which is meant. > > Source/WebCore/inspector/front-end/HAREntry.js:94 > > + compression: this._resource.resourceSize - this._resource.transferSize, > > This should probably be this._resource.resourceSize - (this._resource.transferSize - this._resource.responseHeadersSize)? It's not clear from specification to me. It wasn't clear to me either. :) I think your adjustment makes sense, though. I'll fix my code. > Couldn't it become negative if content size is very small? > > > Source/WebCore/inspector/front-end/HAREntry.js:96 > > + text: this._resource.content // TODO: Why isn't this always available? > > Resource content is immediately available for XHR/script only. Otherwise you should pass your callback to resource.requestContent(). Ah, makes sense. I'll roll this into a future patch along with making the content requests optional. Created attachment 91802 [details]
Patch after first round of feedback. Broken tests that I need help understanding.
Thanks for the pointer to the tests effected by this code. I've updated `resource-har-conversion.html` and `resource-parameters.html`, both of which will need a little more work before committing. Specifically, the HTTP version doesn't show up in the headers of more than a few requests/responses, so the header sizes are all wrong, and will be fixed in a future patch once the rest is worked out.
@vsevik: Can you take a look at the requests/responses in question? For help with debugging, I've fiddled with `get requestHttpVersion` and `get responseHttpVersion` so that it dumps the first line of headers if it can't find an HTTP version. In each case, the response/request header text is either missing the HTTP line (which could very well be an artifact of my testing environment) or missing the variable entirely, and generating the text based on the headers object. I'm not sure how to handle that case... hardcoding an HTTP/1.1 string would work, but doesn't sound like a good solution. :)
> @vsevik: Can you take a look at the requests/responses in question? For help with debugging, I've fiddled with `get requestHttpVersion` and `get responseHttpVersion` so that it dumps the first line of headers if it can't find an HTTP version. In each case, the response/request header text is either missing the HTTP line (which could very well be an artifact of my testing environment) or missing the variable entirely, and generating the text based on the headers object. I'm not sure how to handle that case... hardcoding an HTTP/1.1 string would work, but doesn't sound like a good solution. :)
The problem with layout tests is that in Chromium port DRT uses different code for network making it impossible to test inspector's network features. I am not sure if other ports support headers text at all (as you have seen sometimes they only generate it from parsed headers).
We should generate request/status lines in headersText getters when this data is not available from network, like you did in the test in the patch.
(In reply to comment #6) > (In reply to comment #2) > > You can find existing tests in LayoutTests/http/tests/inspector/, look for resource-har* > > > > > Source/WebCore/inspector/front-end/HAREntry.js:86 > > > bodySize: this._resource.resourceSize > > > > Shouldn't bodySize be this._resource.transferSize - this._resource.responseHeadersSize? > > It's unclear in the spec: "Size of the received response body in bytes." could mean bytes over the wire or total size. I'll dig into some of the examples to see which is meant. Take a look at response.content spec: size [number] - Length of the returned content in bytes. Should be equal to response.bodySize if there is no compression and bigger when the content has been compressed. Created attachment 91816 [details]
Patch with passing tests.
This patch hard-codes "HTTP/1.1" in the response/request header text if it isn't already present. It feels hacky, I'd love a better solution, but this looks like the best we've got if the network stack doesn't include the information for whatever reason. It also incorporates the remaining feedback regarding `bodySize` and does a little bit of cleanup in `Resource.js` (dropping line-ending whitespace, and adding a semicolon, nothing big...).
Created attachment 91817 [details]
No functional change from previous patch, just dropping FIXME comments that don't apply anymore. Sorry for the spam. :)
Attachment 91817 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/http/tests/inspector/resource-..." exit_code: 1
Source/WebCore/ChangeLog:14: Line contains tab character. [whitespace/tab] [5]
Total errors found: 1 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 91818 [details]
Tabs in ChangeLog. This is just embarrassing.
View in context: https://bugs.webkit.org/attachment.cgi?id=91818&action=review > Source/WebCore/inspector/front-end/Resource.js:528 > + this._responseHeadersText = ""; Why don't you move the default status line here? I guess we can assume that we either have complete headersText from network or do not have it at all. Same for request. > Source/WebCore/inspector/front-end/Resource.js:551 > + this._responseHeadersSize = this._headersSize(this._responseHeaders); _headersSize could probably be removed now that we have generated headersText. At least we should reuse this logic to avoid situations when headers size changes after responseHeadersText getter call as it does now ("\n" vs. "\r\n"). Created attachment 92221 [details]
Patch that drops `_headerSize()` and only adds HTTP info when no network headers are provided.
@vsevik: Both good ideas. The latest patch drops `_headerSize()`, which is trivial now that the header text is being properly generated. Thanks!
Attachment 92221 [details] did not pass chromium-ews: Output: http://queues.webkit.org/results/8552781 New failing tests: http/tests/inspector/resource-har-conversion.html Created attachment 92365 [details]
Archive of layout-test-results from ec2-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02 Port: Chromium Platform: Linux-2.6.35-22-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 92221 [details]
Patch that drops `_headerSize()` and only adds HTTP info when no network headers are provided.
Dropping review request while looking into `cr-linux` bot failures.
Created attachment 92584 [details]
Patch to remove flakeyness.
Worked with @vsevik to identify some flake in the tests. Adjusted the HARNondeterministic list accordingly, and added a test with manually hard-coded values to at least test some of the logic in question.
Comment on attachment 92584 [details] Patch to remove flakeyness. Attachment 92584 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8605033 New failing tests: http/tests/inspector/resource-har-headers.html Created attachment 92596 [details]
Archive of layout-test-results from ec2-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 92584 [details]
Patch to remove flakeyness.
Dropping review request while I fix the patch. Bah.
Created attachment 92743 [details]
Let's see if this takes care of things.
Comment on attachment 92743 [details]
Let's see if this takes care of things.
Bots seem happy, flagging r+.
Comment on attachment 92743 [details] Let's see if this takes care of things. View in context: https://bugs.webkit.org/attachment.cgi?id=92743&action=review One test nit, otherwise looks good. > LayoutTests/http/tests/inspector/resource-har-headers.html:20 > + InspectorTest.reloadPage(step1); We try not to reload pages while in layout tests unless necessary. You can perform navigation in the iframe in case you need the "navigation" action. Comment on attachment 92743 [details]
Let's see if this takes care of things.
Resetting r+ since only reviewers should do that.
Comment on attachment 92743 [details]
Let's see if this takes care of things.
vsevik, caseq, do you want to review this? I'm happy to rubber stamp it if you are happy.
(In reply to comment #27) > (From update of attachment 92743 [details]) > vsevik, caseq, do you want to review this? I'm happy to rubber stamp it if you are happy. Apologies, I've been buried under I/O. I'd simply copied and pasted from one of the other inspector tests (which, I think, probably also doesn't need the reload). I'll fix those tests and upload a new patch tomorrow morning. If anything else jumps out at you, please do let me know. I'm happy to clean things up. :) Created attachment 93911 [details]
Reducing test.
Comment on attachment 93911 [details]
Reducing test.
This takes care of everything I know about, and the bots seem happy. I think it's ready for a final review, please. :)
Comment on attachment 93911 [details]
Reducing test.
Please do not set r+ for yourself, only reviewer can do that :) You should do r?
(In reply to comment #27) > (From update of attachment 92743 [details]) > vsevik, caseq, do you want to review this? I'm happy to rubber stamp it if you are happy. I am happy now. Comment on attachment 93911 [details] Reducing test. View in context: https://bugs.webkit.org/attachment.cgi?id=93911&action=review Generally looks good, but there's a handful of stylistic issues to fix. > LayoutTests/http/tests/inspector/resource-har-headers.html:23 > + "Response": "response-value" Wrong indent. > LayoutTests/http/tests/inspector/resource-har-headers.html:39 > + 'request': { Here and until the end of class, ident is wrong. > LayoutTests/http/tests/inspector/resource-har-headers.html:57 > + "cache": 1, Is it? ;) > LayoutTests/http/tests/inspector/resources-test.js:11 > startedDateTime: 1, duplicate line? > Source/WebCore/inspector/front-end/HAREntry.js:68 > + if (this._resource.requestFormData) { Redundant parenthesis. > Source/WebCore/inspector/front-end/HAREntry.js:185 > + return (!this._resource.requestFormData) ? nit: I'd rather remove redundant () and put it on a single line. > Source/WebCore/inspector/front-end/Resource.js:502 > + this._requestHeadersText = this.requestMethod + " " + this.url + " HTTP/1.1\r\n"; Here and below in the block, indent is wrong. > Source/WebCore/inspector/front-end/Resource.js:520 > + this._requestHeadersSize = this.requestHeadersText.length; Why cache it? Let's just return this.requestHeadersText.length? > Source/WebCore/inspector/front-end/Resource.js:565 > + return (match) ? match[1] : undefined; Redundant parenthesis. > Source/WebCore/inspector/front-end/Resource.js:604 > + this._responseHeadersSize = this.responseHeadersText.length; Ditto, let's not cache this. > Source/WebCore/inspector/front-end/Resource.js:662 > + return (match) ? match[1] : undefined; Redundant parenthesis. Created attachment 94065 [details]
Style fixes.
Comment on attachment 93911 [details] Reducing test. View in context: https://bugs.webkit.org/attachment.cgi?id=93911&action=review New patch should cover these. Thanks for the feedback! >> LayoutTests/http/tests/inspector/resource-har-headers.html:23 >> + "Response": "response-value" > > Wrong indent. Right. I confuse myself flipping between Chromium and WebKit. >> LayoutTests/http/tests/inspector/resource-har-headers.html:57 >> + "cache": 1, > > Is it? ;) Well, no. It's just not implemented. :) Correcting the test. >> Source/WebCore/inspector/front-end/Resource.js:520 >> + this._requestHeadersSize = this.requestHeadersText.length; > > Why cache it? Let's just return this.requestHeadersText.length? You're right; I don't think there's any good reason to cache it. Comment on attachment 94065 [details] Style fixes. View in context: https://bugs.webkit.org/attachment.cgi?id=94065&action=review > Source/WebCore/inspector/front-end/HAREntry.js:68 > + if this._resource.requestFormData { I didn't mean redundant (), I meant redundant {} -- I guess this wouldn't run :-) Comment on attachment 94065 [details] Style fixes. View in context: https://bugs.webkit.org/attachment.cgi?id=94065&action=review >> Source/WebCore/inspector/front-end/HAREntry.js:68 >> + if this._resource.requestFormData { > > I didn't mean redundant (), I meant redundant {} -- I guess this wouldn't run :-) Tests passed locally. :) Ok, that does make more sense. I was assuming that this was some strange WebKit-ism that I hadn't seen yet. I'll change it. Comment on attachment 94065 [details] Style fixes. Attachment 94065 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8709997 New failing tests: http/tests/inspector/resource-tree/resource-tree-frame-add.html http/tests/inspector/console-resource-errors.html http/tests/inspector/console-xhr-logging.html http/tests/inspector/resource-har-headers.html inspector/console/command-line-api-inspect.html http/tests/inspector/network/download.html http/tests/inspector/network-preflight-options.html inspector/console/alert-toString-exception.html http/tests/inspector-enabled/console-log-before-frame-navigation.html http/tests/inspector/network/ping.html inspector/debugger/debugger-autocontinue-on-syntax-error.html http/tests/inspector/change-iframe-src.html http/tests/inspector/network/x-frame-options-deny.html http/tests/inspector-enabled/dom-storage-open.html http/tests/inspector/resource-har-conversion.html http/tests/inspector/inspect-iframe-from-different-domain.html http/tests/inspector/resource-parameters.html http/tests/inspector/resource-tree/resource-tree-document-url.html inspector/debugger/debugger-activation-crash.html http/tests/inspector-enabled/database-open.html Created attachment 94066 [details]
Archive of layout-test-results from ec2-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
(In reply to comment #37) > (From update of attachment 94065 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94065&action=review > > >> Source/WebCore/inspector/front-end/HAREntry.js:68 > >> + if this._resource.requestFormData { > > > > I didn't mean redundant (), I meant redundant {} -- I guess this wouldn't run :-) > > Tests passed locally. :) > > Ok, that does make more sense. I was assuming that this was some strange WebKit-ism that I hadn't seen yet. I'll change it. *ahem* By "tests passed locally" I obviously mean "I just ran the tests and didn't recompile." Gah. Created attachment 94078 [details]
I'm not usually an idiot, but when I am, I don't compile.
Comment on attachment 94078 [details]
I'm not usually an idiot, but when I am, I don't compile.
LGTM. Pavel, I think it's your turn now. I can land it once you r+.
Comment on attachment 94078 [details] I'm not usually an idiot, but when I am, I don't compile. View in context: https://bugs.webkit.org/attachment.cgi?id=94078&action=review > LayoutTests/http/tests/inspector/resource-har-headers.html:36 > + var testResource = new WebInspector.Resource('testResource', 'http://example.com/inspector-test.js', 1); On a closer look, we normally use double quotes on strings, unless there are strong reasons not to (i.e. nested double quotes). Ditto for a dozen of lines below. Created attachment 94083 [details]
Single quotes
Comment on attachment 94078 [details]
I'm not usually an idiot, but when I am, I don't compile.
LGTM. Pavel?
Comment on attachment 94078 [details]
I'm not usually an idiot, but when I am, I don't compile.
oops, sorry, wrong attachment.
Comment on attachment 94083 [details]
Single quotes
LGTM.
Comment on attachment 94083 [details] Single quotes Rejecting attachment 94083 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2 Last 500 characters of output: tests/xmlhttprequest ................................................................................................................................................................................ http/tests/xmlhttprequest/web-apps ............... http/tests/xmlhttprequest/workers ........... http/tests/xmlviewer . http/tests/xmlviewer/dumpAsText ........... 729.53s total testing time 23583 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 15 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/8718450 Created attachment 94206 [details]
Archive of layout-test-results from cr-jail-8
The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: cr-jail-8 Port: Mac Platform: Mac OS X 10.6.7
The failing test is `http/tests/inspector/network/network-size.html`, which is currently being skipped via `LayoutTests/platform/chromium/test_expectations.txt` (line 676). I can replicate the error by removing the SKIP line, but before I dive into the root cause, is this a test that we expect to be passing? The bug noted in that file ( http://webk.it/56602 ) is marked as fixed... This test failed on mac. It's very platform specific so you don't need to look on the chromium results at all. Ping me if you need any help fixing it on mac. (In reply to comment #51) > This test failed on mac. It's very platform specific so you don't need to look on the chromium results at all. > Ping me if you need any help fixing it on mac. Thanks, I got hung up on the `cr-jail` bot name, and didn't catch the platform. I think I've tracked it down, though; the test was using `_responseHeaderSize` directly, which I removed in response to https://bugs.webkit.org/show_bug.cgi?id=58127#c33 Replacing that with the public getter should fix the problem. Compilation on my macbook is going to take a little while though... I'll upload a patch in a few hours assuming all goes well. :) Created attachment 94234 [details]
Using public getters in `http/test/inspector/network/network-size`
(In reply to comment #53) > Created an attachment (id=94234) [details] > Using public getters in `http/test/inspector/network/network-size` Sounds like a bad rebase/merge. Created attachment 94337 [details]
Correcting bad merge.
(In reply to comment #55) > Created an attachment (id=94337) [details] > Correcting bad merge. Looks good overall, but I just realized that the patch is missing ChangeLog for LayoutTests. Created attachment 94410 [details]
LayoutTest changelog.
Attachment 94410 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1
LayoutTests/ChangeLog:8: Line contains tab character. [whitespace/tab] [5]
LayoutTests/ChangeLog:9: Line contains tab character. [whitespace/tab] [5]
Total errors found: 2 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 94410 [details]
LayoutTest changelog.
Please fix style issues.
Created attachment 94412 [details]
I hate tabs.
Comment on attachment 94412 [details] I hate tabs. Clearing flags on attachment: 94412 Committed r87070: <http://trac.webkit.org/changeset/87070> All reviewed patches have been landed. Closing bug. (In reply to comment #62) > All reviewed patches have been landed. Closing bug. Since they've now been rolled back, we should probably reopen this. :) @vsevik: Can you please take a look at `resource-properties.html` in the Windows 7/WebKit2 build (sorry): http://build.webkit.org/results/Windows%207%20Release%20%28WebKit2%20Tests%29/r87070%20%287156%29.zip I'm confused about two things: 1) Numbers shouldn't be showing up for some of the diffs, they should be `<number>`, and 2) The header size looks simply broken. I don't think that's happening in the JavaScript layer, but I hope you can help me figure out whether that's the case. Created attachment 99046 [details]
Marking more fields as nondeterministic after talking with vsevik@
Attachment 99046 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1
LayoutTests/ChangeLog:8: Line contains tab character. [whitespace/tab] [5]
LayoutTests/ChangeLog:9: Line contains tab character. [whitespace/tab] [5]
Total errors found: 2 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 99047 [details]
Did I mention that I hate tabs?
Created attachment 99069 [details]
Shouldn't add `size` to the nondeterministic bits.
Looks good to me now. Comment on attachment 99069 [details] Shouldn't add `size` to the nondeterministic bits. Clearing flags on attachment: 99069 Committed r90010: <http://trac.webkit.org/changeset/90010> All reviewed patches have been landed. Closing bug. |