[webkit-reviews] review denied: [Bug 133483] [Curl] Empty headers in request response. : [Attachment 232573] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied peavo at outlook.com's request for
review:
Bug 133483: [Curl] Empty headers in request response.
https://bugs.webkit.org/show_bug.cgi?id=133483

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

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=232573&action=review


I think this looks good, but it could be made a bit tighter.

> Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:48
> +CurlCacheEntry::CurlCacheEntry(const String& url, ResourceHandle* job, const
String& cacheDir)

This could be a reference...

> Source/WebCore/platform/network/curl/CurlCacheEntry.h:64
> +    const ResourceHandle* getJob() const { return m_job; }

This could then return a reference...

> Source/WebCore/platform/network/curl/CurlCacheEntry.h:80
> +    ResourceHandle* m_job;

This could just be a const reference, since m_job doesn't appear to be allowed
to be null, does it?

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:142
> +	   std::unique_ptr<CurlCacheEntry> cacheEntry =
std::make_unique<CurlCacheEntry>(url, nullptr, m_cacheDir);

auto cacheEntry!

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:213
> +	   HashMap<String, std::unique_ptr<CurlCacheEntry>>::iterator it =
m_index.find(url);

This would be better as auto to avoid having to type all that stuff out.

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:219
> +	   std::unique_ptr<CurlCacheEntry> cacheEntry =
std::make_unique<CurlCacheEntry>(url, job, m_cacheDir);

auto cacheEntry

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:230
> +void CurlCacheManager::didFinishLoading(ResourceHandle* job)

Make this a ResourceHandle& ...

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:236
> +	   return;

... and get rid of this null check!

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:276
> +void CurlCacheManager::didReceiveData(ResourceHandle* job, const char* data,
size_t size)

Make this a reference ...

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:282
> +	   return;

... and get rid of this null check.

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:331
> +void CurlCacheManager::didFail(ResourceHandle *job)

Make this a reference ...

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:334
> +	   return;

... and get rid of this null check.

> Source/WebCore/platform/network/curl/CurlCacheManager.h:55
> +    void didFail(ResourceHandle*);

These could all be (const ResourceHandle&), since 'job' is never NULL in the
use cases you are changing.

> Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:328
> +	   CurlCacheManager::getInstance().didReceiveData(job,
static_cast<char*>(ptr), totalSize);

'job' was always non-null here, so why not pass it as a reference?

> Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:692
> +		   CurlCacheManager::getInstance().didFinishLoading(job);

ditto: 'job' was always non-null here, so why not pass it as a reference?

> Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:704
> +		   CurlCacheManager::getInstance().didFail(job);

ditto: 'job' was always non-null here, so why not pass it as a reference?