Bug 58127

Summary: Web Inspector: missing fields in HAR
Product: WebKit Reporter: Andrey Kosyakov <caseq>
Component: Web Inspector (Deprecated)Assignee: Andrey Kosyakov <caseq>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, commit-queue, dglazkov, joepeck, keishi, loislo, mkwst, pfeldman, pmuellr, rik, vsevik, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 58259, 61285    
Bug Blocks:    
Attachments:
Description Flags
Patch implementing some of the missing functionality.
none
Patch after first round of feedback. Broken tests that I need help understanding.
none
Patch with passing tests.
none
No functional change from previous patch, just dropping FIXME comments that don't apply anymore. Sorry for the spam. :)
none
Tabs in ChangeLog. This is just embarrassing.
none
Patch that drops `_headerSize()` and only adds HTTP info when no network headers are provided.
none
Archive of layout-test-results from ec2-cr-linux-02
none
Patch to remove flakeyness.
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-03
none
Let's see if this takes care of things.
none
Reducing test.
none
Style fixes.
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-02
none
I'm not usually an idiot, but when I am, I don't compile.
none
Single quotes
pfeldman: review+, commit-queue: commit-queue-
Archive of layout-test-results from cr-jail-8
none
Using public getters in `http/test/inspector/network/network-size`
none
Correcting bad merge.
none
LayoutTest changelog.
yurys: review-
I hate tabs.
none
Marking more fields as nondeterministic after talking with vsevik@
none
Did I mention that I hate tabs?
none
Shouldn't add `size` to the nondeterministic bits. none

Description Andrey Kosyakov 2011-04-08 02:34:38 PDT
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
Comment 1 Mike West 2011-04-29 04:51:43 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. :)
Comment 2 Vsevolod Vlasov 2011-04-29 05:11:56 PDT
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 3 Andrey Kosyakov 2011-04-29 06:09:24 PDT
(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 4 Andrey Kosyakov 2011-04-29 06:46:53 PDT
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 5 Mike West 2011-04-30 02:48:42 PDT
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.
Comment 6 Mike West 2011-04-30 03:23:55 PDT
(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.
Comment 7 Mike West 2011-04-30 06:04:22 PDT
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. :)
Comment 8 Vsevolod Vlasov 2011-04-30 06:54:04 PDT
> @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.
Comment 9 Vsevolod Vlasov 2011-04-30 07:04:13 PDT
(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.
Comment 10 Mike West 2011-05-01 02:19:22 PDT
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...).
Comment 11 Mike West 2011-05-01 02:22:43 PDT
Created attachment 91817 [details]
No functional change from previous patch, just dropping FIXME comments that don't apply anymore. Sorry for the spam. :)
Comment 12 WebKit Review Bot 2011-05-01 02:25:19 PDT
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.
Comment 13 Mike West 2011-05-01 02:26:38 PDT
Created attachment 91818 [details]
Tabs in ChangeLog.  This is just embarrassing.
Comment 14 Vsevolod Vlasov 2011-05-03 09:48:27 PDT
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").
Comment 15 Mike West 2011-05-04 05:42:40 PDT
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!
Comment 16 WebKit Review Bot 2011-05-04 20:04:11 PDT
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
Comment 17 WebKit Review Bot 2011-05-04 20:04:21 PDT
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 18 Mike West 2011-05-06 04:23:09 PDT
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.
Comment 19 Mike West 2011-05-06 08:02:54 PDT
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 20 WebKit Review Bot 2011-05-06 10:07:38 PDT
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
Comment 21 WebKit Review Bot 2011-05-06 10:07:48 PDT
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 22 Mike West 2011-05-08 11:22:54 PDT
Comment on attachment 92584 [details]
Patch to remove flakeyness.

Dropping review request while I fix the patch.  Bah.
Comment 23 Mike West 2011-05-08 13:16:24 PDT
Created attachment 92743 [details]
Let's see if this takes care of things.
Comment 24 Mike West 2011-05-08 17:42:02 PDT
Comment on attachment 92743 [details]
Let's see if this takes care of things.

Bots seem happy, flagging r+.
Comment 25 Pavel Feldman 2011-05-08 20:02:17 PDT
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 26 Pavel Feldman 2011-05-08 20:04:41 PDT
Comment on attachment 92743 [details]
Let's see if this takes care of things.

Resetting r+ since only reviewers should do that.
Comment 27 Pavel Feldman 2011-05-13 11:24:49 PDT
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.
Comment 28 Mike West 2011-05-13 11:42:03 PDT
(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 29 Mike West 2011-05-18 07:28:20 PDT
Created attachment 93911 [details]
Reducing test.
Comment 30 Mike West 2011-05-18 10:48:44 PDT
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 31 Pavel Feldman 2011-05-19 04:33:38 PDT
Comment on attachment 93911 [details]
Reducing test.

Please do not set r+ for yourself, only reviewer can do that :) You should do r?
Comment 32 Vsevolod Vlasov 2011-05-19 05:52:58 PDT
(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 33 Andrey Kosyakov 2011-05-19 06:17:03 PDT
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.
Comment 34 Mike West 2011-05-19 07:04:55 PDT
Created attachment 94065 [details]
Style fixes.
Comment 35 Mike West 2011-05-19 07:09:36 PDT
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 36 Andrey Kosyakov 2011-05-19 07:12:55 PDT
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 37 Mike West 2011-05-19 07:16:19 PDT
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 38 WebKit Review Bot 2011-05-19 07:19:22 PDT
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
Comment 39 WebKit Review Bot 2011-05-19 07:19:28 PDT
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
Comment 40 Mike West 2011-05-19 07:20:53 PDT
(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 41 Mike West 2011-05-19 09:06:07 PDT
Created attachment 94078 [details]
I'm not usually an idiot, but when I am, I don't compile.
Comment 42 Andrey Kosyakov 2011-05-19 09:20:55 PDT
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 43 Andrey Kosyakov 2011-05-19 09:29:59 PDT
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 44 Mike West 2011-05-19 10:12:12 PDT
Created attachment 94083 [details]
Single quotes
Comment 45 Andrey Kosyakov 2011-05-19 10:14:47 PDT
Comment on attachment 94078 [details]
I'm not usually an idiot, but when I am, I don't compile.

LGTM. Pavel?
Comment 46 Andrey Kosyakov 2011-05-19 10:15:23 PDT
Comment on attachment 94078 [details]
I'm not usually an idiot, but when I am, I don't compile.

oops, sorry, wrong attachment.
Comment 47 Andrey Kosyakov 2011-05-19 10:16:04 PDT
Comment on attachment 94083 [details]
Single quotes

LGTM.
Comment 48 WebKit Commit Bot 2011-05-20 06:20:21 PDT
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
Comment 49 WebKit Commit Bot 2011-05-20 06:20:26 PDT
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
Comment 50 Mike West 2011-05-20 07:31:11 PDT
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...
Comment 51 Vsevolod Vlasov 2011-05-20 07:43:16 PDT
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.
Comment 52 Mike West 2011-05-20 08:21:49 PDT
(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. :)
Comment 53 Mike West 2011-05-20 09:22:55 PDT
Created attachment 94234 [details]
Using public getters in `http/test/inspector/network/network-size`
Comment 54 Pavel Feldman 2011-05-20 09:45:28 PDT
(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.
Comment 55 Mike West 2011-05-22 02:29:26 PDT
Created attachment 94337 [details]
Correcting bad merge.
Comment 56 Andrey Kosyakov 2011-05-23 05:11:35 PDT
(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.
Comment 57 Mike West 2011-05-23 06:02:59 PDT
Created attachment 94410 [details]
LayoutTest changelog.
Comment 58 WebKit Review Bot 2011-05-23 06:06:01 PDT
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 59 Yury Semikhatsky 2011-05-23 06:07:18 PDT
Comment on attachment 94410 [details]
LayoutTest changelog.

Please fix style issues.
Comment 60 Mike West 2011-05-23 06:09:46 PDT
Created attachment 94412 [details]
I hate tabs.
Comment 61 Andrey Kosyakov 2011-05-23 06:29:36 PDT
Comment on attachment 94412 [details]
I hate tabs.

Clearing flags on attachment: 94412

Committed r87070: <http://trac.webkit.org/changeset/87070>
Comment 62 Andrey Kosyakov 2011-05-23 06:30:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 63 Mike West 2011-05-23 08:16:19 PDT
(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.
Comment 64 Mike West 2011-06-29 00:24:33 PDT
Created attachment 99046 [details]
Marking more fields as nondeterministic after talking with vsevik@
Comment 65 WebKit Review Bot 2011-06-29 00:26:56 PDT
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.
Comment 66 Mike West 2011-06-29 00:31:58 PDT
Created attachment 99047 [details]
Did I mention that I hate tabs?
Comment 67 Mike West 2011-06-29 04:04:31 PDT
Created attachment 99069 [details]
Shouldn't add `size` to the nondeterministic bits.
Comment 68 Vsevolod Vlasov 2011-06-29 05:39:49 PDT
Looks good to me now.
Comment 69 Pavel Feldman 2011-06-29 05:43:57 PDT
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>
Comment 70 Pavel Feldman 2011-06-29 05:45:06 PDT
All reviewed patches have been landed.  Closing bug.