[Webkit-unassigned] [Bug 123333] [curl] Add file cache

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Nov 6 09:43:25 PST 2013 

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


Brent Fulgham <bfulgham at webkit.org> changed:

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




--- Comment #5 from Brent Fulgham <bfulgham at webkit.org>  2013-11-06 09:42:12 PST ---
(From update of attachment 216164)
View in context: https://bugs.webkit.org/attachment.cgi?id=216164&action=review

A very nice start!  There seems to be a bit of excess copying going on, as well as a few coding standard violations in this patch.  Please revise and resubmit so we can push it forward.

> Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:47
> +CurlCacheEntry::CurlCacheEntry(String url, String cacheDir)

These should be passed by const ref to avoid copying.

> Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:87
> +    return &m_requestHeaders;

If the m_requestHeaders member always exists, why do we return as a pointer?  Why not return it by reference?

> Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:90
> +bool CurlCacheEntry::saveCachedData(const char *data, int size)

We write this as "const char* data"

It seems like size should be a "size_t" type.

> Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:92
> +    PlatformFileHandle contentFile = openFile(m_contentFilename, OpenForWrite);

It's too bad we don't have some kind of smart pointer for file operations (to ensure it is closed for any return state).

> Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:105
> +bool CurlCacheEntry::loadCachedData(ResourceHandle *job)

We write this as "ResourceHandle* job"

> Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:117
> +bool CurlCacheEntry::saveResponseHeaders(ResourceResponse &response)

We write this as "ResourceResponse& response".

> Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:142
> +// load the headers from cache file into memory

This comment isn't really needed.

> Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:149
> +    // create strings from buffer

This comment doesn't really add anything.

> Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:167
> +void CurlCacheEntry::setResponseFromCachedHeaders(ResourceResponse &response)

We write this as "ResourceResponse& response".

> Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:173
> +    HTTPHeaderMap cachedResponseHeaders = m_cachedResponse.httpHeaderFields();

This is making a copy; is that intentional?

> Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:207
> +void CurlCacheEntry::generateBaseFilename(CString url)

Cstring might be fairly big; shouldn't this be a const reference?

> Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:220
> +bool CurlCacheEntry::loadFileToBuffer(String filepath, Vector<char> &buffer)

Strings can be big; should probably pass by const reference.

We write this as "Vector<char>& buffer"

> Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:265
> +bool CurlCacheEntry::parseResponseHeaders(ResourceResponse &response)

We write this as "ResourceResponse& response"

> Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:271
> +    bool maxAgeIsValid = false;

In general, we prefer to define values as close to the point they are used as possible.  Several of these could be located further down in the method implementation.

> Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:338
> +

Extra space should be removed.

> Source/WebCore/platform/network/curl/CurlCacheEntry.h:42
> +private:

We put private stuff at the end of our header files, not the top.

> Source/WebCore/platform/network/curl/CurlCacheEntry.h:54
> +    bool loadFileToBuffer(String filepath, Vector<char> &buffer);

See my comments about using const references and proper syntax (e.g., use "Vector<char>& buffer", etc.)

> Source/WebCore/platform/network/curl/CurlCacheEntry.h:58
> +    CurlCacheEntry(String url, String cacheDir);

Ditto const references.

> Source/WebCore/platform/network/curl/CurlCacheEntry.h:62
> +    bool isInMemory() { return m_headerInMemory; }

This should be const.

> Source/WebCore/platform/network/curl/CurlCacheEntry.h:65
> +    bool saveCachedData(const char* data, int size);

It seems like size should be a "size_t" type.

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:54
> +void CurlCacheManager::setCacheDirectory(String directory)

Pass by const reference.

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:90
> +    long long filesize = -1;

You might be better off using one of the C99 size types, such as "int64_t" to represent a long-long.  What should this be for a 64-bit build, for example? Should it be a 128-bit value?

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:100
> +    int bufferReadSize = 4096;

The "4096" magic number should be declared as a constant somewhere and given a meaningful name.

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:158
> +void CurlCacheManager::didReceiveResponse(ResourceHandle *job, ResourceResponse& response)

We write this as "ResourceHandle* job".

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:167
> +

Remove this blank line.

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:182
> +

Remove this blank line.

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:185
> +void CurlCacheManager::didFinishLoading(String url)

Pass by const reference.

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:195
> +bool CurlCacheManager::isCached(String url)

Pass by const reference.

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:210
> +HTTPHeaderMap* CurlCacheManager::requestHeaders(String url)

Pass by const reference.

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:222
> +void CurlCacheManager::didReceiveData(String url, const char *data, int size)

Pass by const reference.

We write this as "const char* data"

"size" should probably be a size_t, unless you are using -1 as a sentinel value.

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:233
> +void CurlCacheManager::saveResponseHeaders(String url, ResourceResponse& response)

Pass url as const reference.

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:244
> +void CurlCacheManager::invalidateCacheEntry(String url)

Pass url as const reference.  Why copy a few hundred bytes if you don't need to?

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:256
> +void CurlCacheManager::didFail(String url)

Pass by const reference.

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:261
> +void CurlCacheManager::loadCachedData(String url, ResourceHandle *job, ResourceResponse& response)

Pass by const reference.

We write this as "ResourceHandle* job"

> Source/WebCore/platform/network/curl/CurlCacheManager.h:40
> +private:

Private stuff goes at the bottom of the file.

> Source/WebCore/platform/network/curl/CurlCacheManager.h:43
> +    CurlCacheManager(CurlCacheManager const &);

We write this as "CurlCacheManager const&"

> Source/WebCore/platform/network/curl/CurlCacheManager.h:44
> +    void operator=(CurlCacheManager const &);

Ditto.

> Source/WebCore/platform/network/curl/CurlCacheManager.h:55
> +    void loadCachedData(String, ResourceHandle*, ResourceResponse&);

The above should be passing by const reference.

> Source/WebCore/platform/network/curl/CurlCacheManager.h:58
> +    static CurlCacheManager & getInstance()

This should be written as "CurlCacheManager& getInstance".

Move this implementation into the cpp file. You don't want this being emitted into every compilation unit, do you?

> Source/WebCore/platform/network/curl/CurlCacheManager.h:64
> +    String getCacheDirectory() { return m_cacheDir; }

Return as const reference. Make the method const.

> Source/WebCore/platform/network/curl/CurlCacheManager.h:65
> +    void setCacheDirectory(String);

Pass a const reference.

> Source/WebCore/platform/network/curl/CurlCacheManager.h:68
> +    HTTPHeaderMap* requestHeaders(String); // load headers

Pass the above two Strings as const references.

> Source/WebCore/platform/network/curl/CurlCacheManager.h:71
> +    void didReceiveData(String, const char*, int); // save data

Consider making this 'int' type a size_t.

Pass String by const reference.

> Source/WebCore/platform/network/curl/CurlCacheManager.h:73
> +    void didFinishLoading(String);

Pass the above strings as const references.

> Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:953
> +            HTTPHeaderMap* requestHeaders = CurlCacheManager::getInstance().requestHeaders(url);

It seems like requestHeaders should be returned by reference.

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

More information about the webkit-unassigned mailing list