[webkit-reviews] review canceled: [Bug 58127] Web Inspector: missing fields in HAR : [Attachment 91665] Patch implementing some of the missing functionality.

[bugzilla-daemon at webkit.org]

Mike West <mkwst at chromium.org> has canceled Mike West <mkwst at chromium.org>'s
request for review:
Bug 58127: Web Inspector: missing fields in HAR
https://bugs.webkit.org/show_bug.cgi?id=58127

Attachment 91665: Patch implementing some of the missing functionality.
https://bugs.webkit.org/attachment.cgi?id=91665&action=review

------- Additional Comments from Mike West <mkwst at chromium.org>
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.