[webkit-reviews] review denied: [Bug 46092] Web Inspector: display headers actually used by network stack in Resources tab : [Attachment 68104] 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 46092: Web Inspector: display headers actually used by network stack in
Resources tab
https://bugs.webkit.org/show_bug.cgi?id=46092

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

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

A bunch of nits, otherwise looks good.

> WebCore/inspector/InspectorResource.cpp:180
> +	   m_responseHeaderFields = headers->responseHeaders;

Either mark request as changed here or modify WebInspector.updateResource to
re-read request headers from the payload upon response change.

> WebCore/platform/network/ResourceResponseBase.h:117
> +    PassRefPtr<ResourceRawHeaders> resourceRawHeaders() const;

There is a number of header-related methods (get / set individual headers) that
are not going to affect raw headers. So we add a bit of confusion here. Maybe
we should push raw headers straight into the inspector controller. Did you
track usages of these methods?

> WebKit/chromium/public/WebResourceRawHeaders.h:47
> +class WebResourceRawHeaders {

1. We should do WebResourceHeadersMap I guess and have separate setters in the
WebURLRequest
2. Interestingly, we are using headers visitors in the rest of the places. We
should talk to the authors of that code to find out why. Are we hesitant
copying those for the performance reasons?

> WebKit/chromium/public/WebResourceRawHeaders.h:58
> +	   WebCore::HTTPHeaderMap* m_private;

You should use WebPrivatePtr when wrapping WebCore. See the pattern in
WebURLLoadTiming.

> WebKit/chromium/public/WebURLResponse.h:86
> +    WEBKIT_API WebResourceRawHeaders* resourceRawHeaders();

Must have a setter here.

> WebKit/chromium/src/WebDevToolsAgentImpl.cpp:328
> +	   if (ic->hasFrontend() && request.reportLoadTiming())

I think ic->willSendRequest will do this for you.

> WebKit/chromium/src/WebURLResponsePrivate.h:49
> +    OwnPtr<WebResourceRawHeaders> m_resourceRawHeaders;

Why hiding public WebKit API field here? In fact, we don't need it at all. You
should serve both getter and a setter against corresponding ResourceResponse's
fields.