The following fields of HAR are not yet provided due to lack of back-end support:
- entry.cache
- entry.request.httpVersion
- entry.request.headersSize
- entry.request.bodySize
- entry.response.httpVersion
- entry.response.headersSize
- entry.response.content.compression
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().
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.
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!
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.
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 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.
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. :)
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. :)
(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
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 :-)
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.
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.
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.
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. :)
(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.
(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.
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.
(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.
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.
2011-04-29 04:51 PDT, Mike West
2011-04-30 06:04 PDT, Mike West
2011-05-01 02:19 PDT, Mike West
2011-05-01 02:22 PDT, Mike West
2011-05-01 02:26 PDT, Mike West
2011-05-04 05:42 PDT, Mike West
2011-05-04 20:04 PDT, WebKit Review Bot
2011-05-06 08:02 PDT, Mike West
2011-05-06 10:07 PDT, WebKit Review Bot
2011-05-08 13:16 PDT, Mike West
2011-05-18 07:28 PDT, Mike West
2011-05-19 07:04 PDT, Mike West
2011-05-19 07:19 PDT, WebKit Review Bot
2011-05-19 09:06 PDT, Mike West
2011-05-19 10:12 PDT, Mike West
commit-queue: commit-queue-
2011-05-20 06:20 PDT, WebKit Commit Bot
2011-05-20 09:22 PDT, Mike West
2011-05-22 02:29 PDT, Mike West
2011-05-23 06:02 PDT, Mike West
2011-05-23 06:09 PDT, Mike West
2011-06-29 00:24 PDT, Mike West
2011-06-29 00:31 PDT, Mike West
2011-06-29 04:04 PDT, Mike West