[Webkit-unassigned] [Bug 133483] [Curl] Empty headers in request response.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=133483


Brent Fulgham <bfulgham at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #232573|review?                     |review-
               Flag|                            |




--- Comment #9 from Brent Fulgham <bfulgham at webkit.org>  2014-06-05 22:32:55 PST ---
(From update of attachment 232573)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.