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.
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.
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?
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.
(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.
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.
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.
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.
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.
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.
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.
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.
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.
(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.
(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.
(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.
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
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?
(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!
(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.
(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!
(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.
(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.
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.
(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.
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.
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!
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.
2011-02-04 14:51 PST, Jer Noble
2011-02-07 15:16 PST, Jer Noble
2011-02-07 15:55 PST, Jer Noble
2011-02-08 14:57 PST, Jer Noble
2011-02-08 14:58 PST, Jer Noble
2011-02-08 14:59 PST, Jer Noble
2011-02-10 10:55 PST, Jer Noble
2011-02-10 12:18 PST, Jer Noble
2011-02-11 13:56 PST, Jer Noble
2011-03-24 11:52 PDT, Jer Noble
2011-03-24 14:04 PDT, Jer Noble
2011-03-24 22:54 PDT, Jer Noble
2011-03-25 11:44 PDT, Jer Noble