Bug 53784 – Application Cache should save audio/ and video/ mime types as flat files

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Jer Noble]

Various platform media engines do not support client-generated byte streams.  As a work around, and to avoid saving potentially huge BLOBs in the Application Cache database, WebKit should store audio/ and video/ resources as flat files.

[Jer Noble]

rdar://problem/8932473

[Jer Noble]

Created attachment 81292 [details]
Patch

[Jer Noble]

Comment on attachment 81292 [details]
Patch

Removing r? for now until I can get more eyeballs on this diff.

[Build Bot]

Attachment 81292 [details] did not build on win:
Build output: http://queues.webkit.org/results/7705667

[Jer Noble]

Created attachment 81539 [details]
Patch

[Jer Noble]

Modified the previous patch to implement quota checking when saving flat files to the Application Cache.  In order to test these changes, WebKit has also been modified to pass the maximum total- and per-origin-quota values from WebKitPreferences through to ApplicationCacheStorage upon startup.  These values can be modified via:                                                                            
  defaults write com.apple.Safari WebKitApplicationCacheTotalQuota -int <new quota>
  defaults write com.apple.Safari WebKitApplicationCacheDefaultOriginQuota -int <new quota>
then restarting Safari.

[Jer Noble]

Again, clearing the r? flag until more reviewers look at this patch.

[Jer Noble]

Created attachment 81545 [details]
Patch

[Jer Noble]

Comment on attachment 81545 [details]
Patch

Clarified some comments, and added an additional error check when writing flat files.

[Jer Noble]

Splitting the patch into three parts, only one of which changes the filesystem reading/wrtiting behavior of the Application Cache.

[Jer Noble]

Created attachment 81700 [details]
[1/3] Add "path" ivar to ApplicationCacheResource.

[Jer Noble]

Created attachment 81701 [details]
[2/3] Store media files as flat files within the Application Cache area.

[Jer Noble]

Created attachment 81702 [details]
[3/3] Set the maximum total- and quota-specific Application Cache sizes from WebKitPreferences.

[Build Bot]

Attachment 81701 [details] did not build on win:
Build output: http://queues.webkit.org/results/7819224

[Early Warning System Bot]

Attachment 81701 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7791202

[Gustavo Noronha (kov)]

Attachment 81701 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7801222

[WebKit Review Bot]

Attachment 81701 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7819297

[Maciej Stachowiak]

Comment on attachment 81700 [details]
[1/3] Add "path" ivar to ApplicationCacheResource.

r=me

[Maciej Stachowiak]

Comment on attachment 81702 [details]
[3/3] Set the maximum total- and quota-specific Application Cache sizes from WebKitPreferences.

r=me

Does this need to be done for WebKit2 as well?

[Maciej Stachowiak]

Comment on attachment 81701 [details]
[2/3] Store media files as flat files within the Application Cache area.

View in context: https://bugs.webkit.org/attachment.cgi?id=81701&action=review

Overall comment: where does "path" come from in the first place? Is it based on the URL of the resource? If so, we need to make sure to sanitize it to avoid dangerous characters like / or null. I see that you try to avoid escaping the flat file directory, but I don't see that in store().

I would like to see path sanitization code or an explanation of why it's not needed. The style comments are purely optional, but I think this part is required to fix, so r-.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:564
> +    // Allow the normal flat-file cleanup process to execute before clearing all the tables:

I suggest removing this comment or add an inline wrapper for empty() called cleanUpFlatFiles() if it's important to state the purpose.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:648
> +    // When a cache resource is deleted, if it contains a non-empty path, that path should
> +    // be added to the DeletedCacheResources table so the flat file at that path can
> +    // be deleted at a later time.
> +    executeSQLCommand("CREATE TRIGGER IF NOT EXISTS CacheResourceDataDeleted AFTER DELETE ON CacheResourceData"
> +                      " FOR EACH ROW"
> +                      " WHEN OLD.path NOT NULL BEGIN"
> +                      "  INSERT INTO DeletedCacheResources (path) values (OLD.path);"
> +                      " END");

I suggest instead of the comment you factor this out as a helper function: recordResourcePathForLaterDeletion() or something like that.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:786
> +    if (resource->response().mimeType().startsWith("audio/", false) || resource->response().mimeType().startsWith("video/", false)) {

The chunk of code added in this if statement is pretty big to bury in an if statement. I suggest factoring it out into a separate function.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1076
> +        String path = cacheStatement.getColumnText(6);

Consider making a named constant instead of using 6 directly in code.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1383
> +        ASSERT(directoryName(fullPath) == flatFileDirectory);

I don't think it's right to ASSERT for something based on a file on disk. You should always assume that files on disk could be corrupted or even malicious. I suggest you leve it at just the if check below.

[Jer Noble]

(In reply to comment #20)
> (From update of attachment 81701 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81701&action=review
> 
> Overall comment: where does "path" come from in the first place? Is it based on the URL of the resource? If so, we need to make sure to sanitize it to avoid dangerous characters like / or null. I see that you try to avoid escaping the flat file directory, but I don't see that in store().
> 
> I would like to see path sanitization code or an explanation of why it's not needed. The style comments are purely optional, but I think this part is required to fix, so r-.

Sure, the "path" is a generated canonical UUID, so as to be sure that a) the filename is unique and b) it doesn't contain any path characters on any platform.  However, since you suggested a self-documenting function name below, perhaps that could be encapsulated in a function called "bool writeDataToUniqueFileInDirectory(inData, inDirectory, outFileName)" or similar.

The reason why it's sanitized in checkForDeletedResources() but not in store() is that we completely control the filename in the second case, but not in the first.  But to advance the cause of filesystem paranoia, I'll add the same check inside the new writeData...() function.

> > Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:564
> > +    // Allow the normal flat-file cleanup process to execute before clearing all the tables:
> 
> I suggest removing this comment or add an inline wrapper for empty() called cleanUpFlatFiles() if it's important to state the purpose.

Sure.  Perhaps a helper function called "deleteTables()" that calls both empty() and m_dataBase.clearAllTables().

> > Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:648
> > +    // When a cache resource is deleted, if it contains a non-empty path, that path should
> > +    // be added to the DeletedCacheResources table so the flat file at that path can
> > +    // be deleted at a later time.
> > +    executeSQLCommand("CREATE TRIGGER IF NOT EXISTS CacheResourceDataDeleted AFTER DELETE ON CacheResourceData"
> > +                      " FOR EACH ROW"
> > +                      " WHEN OLD.path NOT NULL BEGIN"
> > +                      "  INSERT INTO DeletedCacheResources (path) values (OLD.path);"
> > +                      " END");
> 
> I suggest instead of the comment you factor this out as a helper function: recordResourcePathForLaterDeletion() or something like that.

I was just following convention; the rest of the "CREATE TRIGGER" statements contain comments explaining the SQL.  I don't think the function name is quite self-documenting enough either, and it would remove the trigger creation statement from it's peers rendering it both lonely and difficult to find.  

> > Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:786
> > +    if (resource->response().mimeType().startsWith("audio/", false) || resource->response().mimeType().startsWith("video/", false)) {
> 
> The chunk of code added in this if statement is pretty big to bury in an if statement. I suggest factoring it out into a separate function.

Sure thing.

> > Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1076
> > +        String path = cacheStatement.getColumnText(6);
> 
> Consider making a named constant instead of using 6 directly in code.
> 
> > Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1383
> > +        ASSERT(directoryName(fullPath) == flatFileDirectory);
> 
> I don't think it's right to ASSERT for something based on a file on disk. You should always assume that files on disk could be corrupted or even malicious. I suggest you leve it at just the if check below.

Sure thing.

[Jer Noble]

Created attachment 82004 [details]
[2/3] Store media files as flat files within the Application Cache area.

[Collabora GTK+ EWS bot]

Attachment 82004 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7870394

[Build Bot]

Attachment 82004 [details] did not build on win:
Build output: http://queues.webkit.org/results/7869411

[Jer Noble]

Created attachment 82022 [details]
[2/3] Store media files as flat files within the Application Cache area.

Whoops, discovered a compile error in the previous patch.  This latest one will apply and build cleanly.

[Jer Noble]

(In reply to comment #19)
> (From update of attachment 81702 [details])
> r=me
> 
> Does this need to be done for WebKit2 as well?

Yes, it probably does. Filed: https://bugs.webkit.org/show_bug.cgi?id=54237

[Build Bot]

Attachment 82022 [details] did not build on win:
Build output: http://queues.webkit.org/results/7870426

[Collabora GTK+ EWS bot]

Attachment 82022 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7867462

[Early Warning System Bot]

Attachment 82022 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7884426

[WebKit Review Bot]

Attachment 82022 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7875487

[Jer Noble]

Created attachment 82169 [details]
[2/3] Store media files as flat files within the Application Cache area.

Fixed a few bugs that would leave the path field of ApplicationCacheResource empty, and save 0-length files to the ApplicationCache/ area.

[Darin Adler]

Comment on attachment 82169 [details]
[2/3] Store media files as flat files within the Application Cache area.

View in context: https://bugs.webkit.org/attachment.cgi?id=82169&action=review

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:47
> +static const char* const kFlatFileSubdirectory = "ApplicationCache";

We don’t use “k” for these. This should be inside the WebCore namespace. Also perhaps better to have this be an array rather than a constant pointer:

    static const char flatFileSubdirectory[] = "ApplicationCache";

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:786
> +    if (!resource->path().isEmpty()) {
> +        dataStatement.bindText(2, pathGetFileName(resource->path()));
> +    } else if (shouldStoreResourceAsFlatFile(resource)) {

No braces around single line if bodies.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:788
> +        // to check the per-origin quote here, as it was already checked in storeNewestCache().

quota, not quote

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:789
> +        if (m_database.totalSize() + flatFileAreaSize() + resource->data()->size() > m_maximumSize) {

Any possibility of overflow here?

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:794
> +        String path;

I’d move this definition down to just before the writeDataToUniqueFileInDirectory call, since it’s an out argument.

[Jer Noble]

Comment on attachment 82169 [details]
[2/3] Store media files as flat files within the Application Cache area.

View in context: https://bugs.webkit.org/attachment.cgi?id=82169&action=review

>> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:47
>> +static const char* const kFlatFileSubdirectory = "ApplicationCache";
> 
> We don’t use “k” for these. This should be inside the WebCore namespace. Also perhaps better to have this be an array rather than a constant pointer:
> 
>     static const char flatFileSubdirectory[] = "ApplicationCache";

Sure thing! Changed.

>> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:786
>> +    } else if (shouldStoreResourceAsFlatFile(resource)) {
> 
> No braces around single line if bodies.

Removed.

>> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:788
>> +        // to check the per-origin quote here, as it was already checked in storeNewestCache().
> 
> quota, not quote

Changed.

>> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:789
>> +        if (m_database.totalSize() + flatFileAreaSize() + resource->data()->size() > m_maximumSize) {
> 
> Any possibility of overflow here?

m_database.totalSize(), flatFileAreaSize(), m_maximumSize are all int64_t, and resource->data()->size() is an unsigned.  Since the first two are queried from the filesystem itself, I believe an overflow is pretty much impossible.

>> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:794
>> +        String path;
> 
> I’d move this definition down to just before the writeDataToUniqueFileInDirectory call, since it’s an out argument.

Sure thing!  Moved.

[Darin Adler]

Comment on attachment 82169 [details]
[2/3] Store media files as flat files within the Application Cache area.

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.

[Jer Noble]

Comment on attachment 82169 [details]
[2/3] Store media files as flat files within the Application Cache area.

View in context: https://bugs.webkit.org/attachment.cgi?id=82169&action=review

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

Removed.

>> 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.

I only used "long long" here because that's what ResourceResponse expects.  I believe this is functionally identical to a int64_t, and I use it in other places in this patch, so I might as well use it here too.
b
>> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1234
>> +    } while (directoryName(fullPath) != directory || fileExists(fullPath));
> 
> Under what circumstances will directoryName(fullPath) be != directory?

This is a paranoia check.  Canonical UUIDs should never have a path component in them, but for the sake of extreme overprotection, this check guarantees that we will never write to "ApplicationCache/../../etc/passwd" or any other such nonsense.

I guess this could be an "ASSERT(directoryName(fullPath) == directory)" instead, if you prefer.

>> 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?

Looking at the POSIX implementation, -1 is the error code return value, but you're right in that other implementations might use different error codes, and that != data->size() would be a more universal check.

>> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1408
>> +                                "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.

I did my best to use only a single indent in these cases, but this one must have escaped. :)  Undented.

>> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1425
>> +        // 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.

7th grade typing class strikes again (with the double-space after end-of-sentence punctuation).

How about: "Don't exit the flatFileDirectory! This should only happen if the "path" entry contains a directory component, but protect against it regardless."

In the case of a very long string, deleteFile() would silently fail.

[Jer Noble]

Created attachment 86806 [details]
[2/3] Store media files as flat files within the Application Cache area.

Uploading a new version of [2/3], even thought Darin r+'d it already, so that the EWS bots can chew on it.

[Jer Noble]

Comment on attachment 86806 [details]
[2/3] Store media files as flat files within the Application Cache area.

Looks like this patch didn't apply cleanly either.  I'll rebase and re-upload a new patch.

[Jer Noble]

Created attachment 86830 [details]
[2/3] Store media files as flat files within the Application Cache area.

[Jer Noble]

Well, shoot.  Looks like the [2/3] patch will never fully apply cleanly because it depends on a piece of [1/3].

[Maciej Stachowiak]

(In reply to comment #39)
> Well, shoot.  Looks like the [2/3] patch will never fully apply cleanly because it depends on a piece of [1/3].

You could check in 1/3 by itself to get EWS rolling on this, assuming there is no harm to landing just 1/3.

[Darin Adler]

(In reply to comment #40)
> You could check in 1/3 by itself to get EWS rolling on this, assuming there is no harm to landing just 1/3.

You should definitely do this if you can.

[Darin Adler]

(In reply to comment #35)
> This is a paranoia check.  Canonical UUIDs should never have a path component in them, but for the sake of extreme overprotection, this check guarantees that we will never write to "ApplicationCache/../../etc/passwd" or any other such nonsense.
> 
> I guess this could be an "ASSERT(directoryName(fullPath) == directory)" instead, if you prefer.

An assert plus a runtime check together would give you safety in the case where we are wrong in our believe that this should never happen, plus clarity that this should actually never happen. So that's OK.

> How about: "Don't exit the flatFileDirectory! This should only happen if the "path" entry contains a directory component, but protect against it regardless."

Seems OK.

[Joseph Pecoraro]

Comment on attachment 86830 [details]
[2/3] Store media files as flat files within the Application Cache area.

View in context: https://bugs.webkit.org/attachment.cgi?id=86830&action=review

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1069
> +    String flatFileDirectory = pathByAppendingComponent(m_cacheDirectory, flatFileSubdirectory);

Nit: You calculate this a few times. Once in loadCache, and again in store. It might be nice to put this into a function, "one place".

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1418
> +    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");

The WHERE statement here has an added "subquery" which seems
like unnecessary complexity. Can you instead just check if
CacheResourceData.path is NULL?

    WHERE CacheResourceData.path IS NULL

Since your trigger does not put NULL paths into DeletedCacheResources,
you can always assume that DeletedCacheResources.path is NOT NULL,
and you want to find the DeletedCacheResources which do not have
a corresponding entry in CacheResourceData, so the left join in
those cases produce 1 row with null values for CacheResourceData.

Let me know if there is a case this wouldn't handle.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.h:116
> +    bool shouldStoreResourceAsFlatFile(ApplicationCacheResource*);

Nit: I think this can be marked const

[Joseph Pecoraro]

Comment on attachment 81702 [details]
[3/3] Set the maximum total- and quota-specific Application Cache sizes from WebKitPreferences.

Was this part (part 3) so that you could test with quotas? The
following code already sets the default origin quota in
-[WebView _preferencesChanged]:

    // Application Cache Preferences are stored on the global cache storage manager, not in Settings.
    [WebApplicationCache setDefaultOriginQuota:[preferences applicationCacheDefaultOriginQuota]];

There could just be another line added to set the maximum storage.

I think the default quotas on PLATFORM(MAC) are "noQuota", so I
don't think this part actually changes anything. Am I missing
its usefulness?

[Jer Noble]

(In reply to comment #42)
> (In reply to comment #35)
> > This is a paranoia check.  Canonical UUIDs should never have a path component in them, but for the sake of extreme overprotection, this check guarantees that we will never write to "ApplicationCache/../../etc/passwd" or any other such nonsense.
> > 
> > I guess this could be an "ASSERT(directoryName(fullPath) == directory)" instead, if you prefer.
> 
> An assert plus a runtime check together would give you safety in the case where we are wrong in our believe that this should never happen, plus clarity that this should actually never happen. So that's OK.

Okay, I'll add the ASSERT as well.  Thanks!

[Jer Noble]

Committed r81919: <http://trac.webkit.org/changeset/81919>

[Jer Noble]

Committed part [1/3], so that the EWS bots can successfully apply and test [2/3].

[Jer Noble]

(In reply to comment #44)
> (From update of attachment 81702 [details])
> Was this part (part 3) so that you could test with quotas? The
> following code already sets the default origin quota in
> -[WebView _preferencesChanged]:
> 
>     // Application Cache Preferences are stored on the global cache storage manager, not in Settings.
>     [WebApplicationCache setDefaultOriginQuota:[preferences applicationCacheDefaultOriginQuota]];
> 
> There could just be another line added to set the maximum storage.

Indeed!  No need to add a new function.  Changed.

> I think the default quotas on PLATFORM(MAC) are "noQuota", so I
> don't think this part actually changes anything. Am I missing
> its usefulness?

It doesn't change anything directly, but it does allow the maximum cache size to be changed via user defaults, thus allowing the maximum cache behavior to be tested.

[Jer Noble]

Committed r81939: <http://trac.webkit.org/changeset/81939>

[Jer Noble]

(In reply to comment #43)
> (From update of attachment 86830 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86830&action=review
> 
> > Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1069
> > +    String flatFileDirectory = pathByAppendingComponent(m_cacheDirectory, flatFileSubdirectory);
> 
> Nit: You calculate this a few times. Once in loadCache, and again in store. It might be nice to put this into a function, "one place".

I can't figure out a way to do this which won't end up with a net increase of NLOCs.

> > Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1418
> > +    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");
> 
> The WHERE statement here has an added "subquery" which seems
> like unnecessary complexity. Can you instead just check if
> CacheResourceData.path is NULL?
> 
>     WHERE CacheResourceData.path IS NULL
> 
> Since your trigger does not put NULL paths into DeletedCacheResources,
> you can always assume that DeletedCacheResources.path is NOT NULL,
> and you want to find the DeletedCacheResources which do not have
> a corresponding entry in CacheResourceData, so the left join in
> those cases produce 1 row with null values for CacheResourceData.
> 
> Let me know if there is a case this wouldn't handle.

No, I think this works really well.  Changed.

> > Source/WebCore/loader/appcache/ApplicationCacheStorage.h:116
> > +    bool shouldStoreResourceAsFlatFile(ApplicationCacheResource*);
> 
> Nit: I think this can be marked const

Done.  Thanks!

[Jer Noble]

Created attachment 86888 [details]
[2/3] Store media files as flat files within the Application Cache area.

[Jer Noble]

(In reply to comment #48)
> (In reply to comment #44)
> > (From update of attachment 81702 [details] [details])
> > Was this part (part 3) so that you could test with quotas? The
> > following code already sets the default origin quota in
> > -[WebView _preferencesChanged]:
> > 
> >     // Application Cache Preferences are stored on the global cache storage manager, not in Settings.
> >     [WebApplicationCache setDefaultOriginQuota:[preferences applicationCacheDefaultOriginQuota]];
> > 
> > There could just be another line added to set the maximum storage.
> 
> Indeed!  No need to add a new function.  Changed.

Unfortunately, using the above approach causes a couple of test failures (see: http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r81939%20(28013)/results.html).  I haven't quite figured out why yet, but the original patch doesn't cause the same errors.

[Build Bot]

Attachment 86888 [details] did not build on win:
Build output: http://queues.webkit.org/results/8233947

[Jer Noble]

Looks like the EWS bot doesn't like the int64_t change; and the Windows bot can't find WebCore::openFile() during linking.

[Jer Noble]

(In reply to comment #54)
> Looks like the EWS bot doesn't like the int64_t change; and the Windows bot can't find WebCore::openFile() during linking.

Gah! It appears openFile() isn't even implemented on Windows.

[Jer Noble]

Created attachment 86967 [details]
[2/3] Store media files as flat files within the Application Cache area.

[Brian Weinstein]

Comment on attachment 86967 [details]
[2/3] Store media files as flat files within the Application Cache area.

View in context: https://bugs.webkit.org/attachment.cgi?id=86967&action=review

> Source/WebCore/platform/win/FileSystemWin.cpp:241
> +    return CreateFile(path.characters(), desiredAccess, 0, 0, creationDisposition, FILE_ATTRIBUTE_NORMAL, 0);

This should be charactersWithNullTermination, I believe.

[Jer Noble]

(In reply to comment #57)
> (From update of attachment 86967 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86967&action=review
> 
> > Source/WebCore/platform/win/FileSystemWin.cpp:241
> > +    return CreateFile(path.characters(), desiredAccess, 0, 0, creationDisposition, FILE_ATTRIBUTE_NORMAL, 0);
> 
> This should be charactersWithNullTermination, I believe.

Indeed it should!  Changed.

[Eric Carlson]

Comment on attachment 86967 [details]
[2/3] Store media files as flat files within the Application Cache area.

View in context: https://bugs.webkit.org/attachment.cgi?id=86967&action=review

> Source/WebCore/ChangeLog:9
> +        No new tests.

It would be nice to have an explanation of why there aren't any new tests.

[Alexey Proskuryakov]

Comment on attachment 86967 [details]
[2/3] Store media files as flat files within the Application Cache area.

View in context: https://bugs.webkit.org/attachment.cgi?id=86967&action=review

Looks good to me, too.

I think that it would be very useful to add tests checking that updating an appcache with media resources works. That probably doesn't need to block landing this patch.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1253
> +    if (writtenBytes != (int64_t)data->size()) {

We prefer static_cast to C-style casts.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1454
> +    if (selectPaths.prepare() != SQLResultOk)
> +        return 0;

Should we log an error?

> Source/WebCore/platform/win/FileSystemWin.cpp:239
> +    DWORD desiredAccess = mode == OpenForRead ? GENERIC_READ : GENERIC_WRITE;
> +    DWORD creationDisposition = mode == OpenForRead ? OPEN_EXISTING : CREATE_ALWAYS;

Perhaps this would be safer as a switch - that way, the compiler will warn if other modes are added later.

[Jer Noble]

Comment on attachment 86967 [details]
[2/3] Store media files as flat files within the Application Cache area.

View in context: https://bugs.webkit.org/attachment.cgi?id=86967&action=review

>> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1253
>> +    if (writtenBytes != (int64_t)data->size()) {
> 
> We prefer static_cast to C-style casts.

Sure thing.  Changed.

>> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1454
>> +        return 0;
> 
> Should we log an error?

We don't currently log errors for most other equivalent select statements elsewhere in this class, but I'll add one here.

>> Source/WebCore/platform/win/FileSystemWin.cpp:239
>> +    DWORD creationDisposition = mode == OpenForRead ? OPEN_EXISTING : CREATE_ALWAYS;
> 
> Perhaps this would be safer as a switch - that way, the compiler will warn if other modes are added later.

Sure thing.  Changed.

Thanks!

[Jer Noble]

The [2/3] patch has passed the style, qt, mac, win, and efl EWS bots, so I'm preparing to land this patch now.

[Jer Noble]

Committed r82000: <http://trac.webkit.org/changeset/82000>

[Jer Noble]

Comment on attachment 81702 [details]
[3/3] Set the maximum total- and quota-specific Application Cache sizes from WebKitPreferences.

This change causes a number of broken (and some crashed) LayoutTests, so I'm going to obsolete it for now.  This patch should not affect blocked bugs, so I'm going to mark this bug as closed, and file a new bug for this obsolete patch.