Bug 58127 – Web Inspector: missing fields in HAR

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Andrey Kosyakov]

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

[Mike West]

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. :)

[Vsevolod Vlasov]

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().

[Andrey Kosyakov]

(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)

[Andrey Kosyakov]

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.

[Mike West]

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.

[Mike West]

(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.

[Mike West]

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. :)

[Vsevolod Vlasov]

> @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.

[Vsevolod Vlasov]

(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.

[Mike West]

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...).

[Mike West]

Created attachment 91817 [details]
No functional change from previous patch, just dropping FIXME comments that don't apply anymore. Sorry for the spam. :)

[WebKit Review Bot]

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.

[Mike West]

Created attachment 91818 [details]
Tabs in ChangeLog.  This is just embarrassing.

[Vsevolod Vlasov]

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").

[Mike West]

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!

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Mike West]

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.

[Mike West]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Mike West]

Comment on attachment 92584 [details]
Patch to remove flakeyness.

Dropping review request while I fix the patch.  Bah.

[Mike West]

Created attachment 92743 [details]
Let's see if this takes care of things.

[Mike West]

Comment on attachment 92743 [details]
Let's see if this takes care of things.

Bots seem happy, flagging r+.

[Pavel Feldman]

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.

[Pavel Feldman]

Comment on attachment 92743 [details]
Let's see if this takes care of things.

Resetting r+ since only reviewers should do that.

[Pavel Feldman]

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.

[Mike West]

(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. :)

[Mike West]

Created attachment 93911 [details]
Reducing test.

[Mike West]

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. :)

[Pavel Feldman]

Comment on attachment 93911 [details]
Reducing test.

Please do not set r+ for yourself, only reviewer can do that :) You should do r?

[Vsevolod Vlasov]

(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.

[Andrey Kosyakov]

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.

[Mike West]

Created attachment 94065 [details]
Style fixes.

[Mike West]

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.

[Andrey Kosyakov]

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 :-)

[Mike West]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Mike West]

(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.

[Mike West]

Created attachment 94078 [details]
I'm not usually an idiot, but when I am, I don't compile.

[Andrey Kosyakov]

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+.

[Andrey Kosyakov]

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.

[Mike West]

Created attachment 94083 [details]
Single quotes

[Andrey Kosyakov]

Comment on attachment 94078 [details]
I'm not usually an idiot, but when I am, I don't compile.

LGTM. Pavel?

[Andrey Kosyakov]

Comment on attachment 94078 [details]
I'm not usually an idiot, but when I am, I don't compile.

oops, sorry, wrong attachment.

[Andrey Kosyakov]

Comment on attachment 94083 [details]
Single quotes

LGTM.

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[Mike West]

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...

[Vsevolod Vlasov]

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.

[Mike West]

(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. :)

[Mike West]

Created attachment 94234 [details]
Using public getters in `http/test/inspector/network/network-size`

[Pavel Feldman]

(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.

[Mike West]

Created attachment 94337 [details]
Correcting bad merge.

[Andrey Kosyakov]

(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.

[Mike West]

Created attachment 94410 [details]
LayoutTest changelog.

[WebKit Review Bot]

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.

[Yury Semikhatsky]

Comment on attachment 94410 [details]
LayoutTest changelog.

Please fix style issues.

[Mike West]

Created attachment 94412 [details]
I hate tabs.

[Andrey Kosyakov]

Comment on attachment 94412 [details]
I hate tabs.

Clearing flags on attachment: 94412

Committed r87070: <http://trac.webkit.org/changeset/87070>

[Andrey Kosyakov]

All reviewed patches have been landed.  Closing bug.

[Mike West]

(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.

[Mike West]

Created attachment 99046 [details]
Marking more fields as nondeterministic after talking with vsevik@

[WebKit Review Bot]

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.

[Mike West]

Created attachment 99047 [details]
Did I mention that I hate tabs?

[Mike West]

Created attachment 99069 [details]
Shouldn't add `size` to the nondeterministic bits.

[Vsevolod Vlasov]

Looks good to me now.

[Pavel Feldman]

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>

[Pavel Feldman]

All reviewed patches have been landed.  Closing bug.