[webkit-reviews] review denied: [Bug 99736] Refactor CachedResourceLoader: add CachedResourceRequest : [Attachment 169450] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Marja Hölttä <marja at chromium.org>'s
request for review:
Bug 99736: Refactor CachedResourceLoader: add CachedResourceRequest
https://bugs.webkit.org/show_bug.cgi?id=99736

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=169450&action=review


This looks great.  Some comments below.  Also, it looks like your patch is
causing a large number of test failures.

> Source/WebCore/ChangeLog:8
> +	   This is needed for fixing bugs 84883 and 92761.

Please add more information to the ChangeLog that explains why we're making
this change.

>> Source/WebCore/loader/cache/CachedResourceLoader.h:72
>> +	class CachedResourceRequest {
> 
> what's the reason for making this a sub class? Why not even put it into a
separate file?

Yeah, we should put it in a separate CachedResourceRequest.h and in the WebCore
namespace directly.

> Source/WebCore/loader/cache/CachedResourceLoader.h:82
> +	   ResourceRequest& resourceRequest() { return m_resourceRequest; }

resourceRequest -> mutableResourceRequest to emphasize that this is a non-const
reference.

> Source/WebCore/loader/cache/CachedResourceLoader.h:89
> +    private:

Please add a blank line above this line.