[webkit-reviews] review denied: [Bug 47894] Web Inspector: expose request/response cookies in HAR : [Attachment 71151] patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Andrey Kosyakov
<caseq at chromium.org>'s request for review:
Bug 47894: Web Inspector: expose request/response cookies in HAR
https://bugs.webkit.org/show_bug.cgi?id=47894

Attachment 71151: patch
https://bugs.webkit.org/attachment.cgi?id=71151&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71151&action=review

> LayoutTests/http/tests/inspector/resource-har-conversion.html:18
> +	   var requestHeaders =
JSON.parse(JSON.stringify(resource.requestHeaders));

You should instead make a separate test for cookies only, so that there is no
need to preserve rest of the headers.

> LayoutTests/inspector/cookie-parser-expected.txt:136
> +    httponly : undefined

Is this correct?

> WebCore/inspector/front-end/CookieParser.js:33
> +WebInspector.CookieParser = function()

Two things I don't like there:
1) Cookie parsing is tough and is handled by the WebKit already
2) You make all of our remote debugging clients come up with the code like
this.

Can we introduce an api for cookie parsing instead so that WebKit was doing it
for us? That way we are going to be way more consistent. Ideally, we should get
parsed cookies with raw resource data.

> WebCore/inspector/front-end/Resource.js:539
> +	   return (new
WebInspector.CookieParser()).parseCookie(this.requestHeaderValue("Cookie"));

You could encapsulate this into WebInspector.CookieParser.parseCookie() and
.parseSetCookie() respectively.