[webkit-reviews] review denied: [Bug 61033] [chromium] add extraData field to resource requests : [Attachment 93978] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied
jochen at chromium.org's request for review:
Bug 61033: [chromium] add extraData field to resource requests
https://bugs.webkit.org/show_bug.cgi?id=61033

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=93978&action=review

I guess ExtraData just won't be something we copy across threads when we copy
a ResourceRequest to another thread.  That is probably OK.

> Source/WebCore/platform/network/chromium/ResourceRequest.h:112
> +	   // pointer will be deleted when the request is destroyed. Setting
the

deleted -> deref'd

> Source/WebCore/platform/network/chromium/ResourceRequest.h:114
> +	   // pointer to be deleted.

this comment is a bit incorrect.  the existing extra data may not be deleted,
it will be deref'd.  that may or may not cause it to be deleted.

> Source/WebKit/chromium/public/WebURLRequest.h:59
> +    class ExtraData {

nit: try not to break up the flow of enum declarations.  grouping enums
separately from inner classes is nicer.
how about putting this below TargetType?

> Source/WebKit/chromium/src/WebURLRequest.cpp:310
> +    RefPtr<ExtraDataContainer> container =
ExtraDataContainer::create(extraData);

nit: no need for the container temp var...

just put it all in a single line:

 
m_private->m_resourceRequest->setExtraData(ExtraDataContainer::create(extraData
));

> Source/WebKit/chromium/tests/WebURLRequestTest.cpp:59
> +	   WebURLRequest urlRequest;

this is a good test, but you should also test that copying urlRequest
works properly too.