[webkit-reviews] review granted: [Bug 53784] Application Cache should save audio/ and video/ mime types as flat files : [Attachment 82169] [2/3] Store media files as flat files within the Application Cache area.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 53784: Application Cache should save audio/ and video/ mime types as flat
files
https://bugs.webkit.org/show_bug.cgi?id=53784

Attachment 82169: [2/3] Store media files as flat files within the Application
Cache area.
https://bugs.webkit.org/attachment.cgi?id=82169&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=82169&action=review

This patch didn’t apply, so you probably need to upload a new one to get EWS
checking on it.

> Source/WebCore/loader/appcache/ApplicationCacheResource.h:60
> +    void setPath(const String& path) { m_path = path; };

Extra semicolon here is not needed.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1073
> +	   long long size = 0;

I’m never sure what the best types are to use. For a long time I liked to use
int and unsigned, but Maciej suggested we might want to move to int32_t and
uint32_t. Similarly for 64-bit we have used long long for a while, but maybe
int64_t is better.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1234
> +    } while (directoryName(fullPath) != directory || fileExists(fullPath));

Under what circumstances will directoryName(fullPath) be != directory?

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1243
> +    if (writtenBytes == -1) {

Is -1 the only value we should check for? Maybe != data->size() is what we want
to check?

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1408
> +    SQLiteStatement selectPaths(m_database, "SELECT
DeletedCacheResources.path "
> +				   "FROM DeletedCacheResources "
> +				   "LEFT JOIN CacheResourceData "
> +				   "ON DeletedCacheResources.path =
CacheResourceData.path "
> +				   "WHERE (SELECT DeletedCacheResources.path ==
CacheResourceData.path) IS NULL");

This file uses this style of alignment everywhere, but in the past we frowned
on that in WebKit. We want to be able to rename.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1425
> +	   // Don't exit the flatFileDirectory!  This should only happen if
someone tampers 
> +	   // with the ApplicationCache database, but protect against it
regardless.

One space after the !, not two.

The comment “tampers with” is a little scary. I think you could word this in a
more specific way that could make things clearer. I guess the idea is that if
the path has a "/" character in it, this could happen. It seems like there
could be other kinds of bad paths, such as very long strings. What would this
code do in a case like that? I suppose the deleteFile function would just fail
and do nothing.