Upstream the iOS-specific disk image cache feature.
Created attachment 212755 [details] Patch
Attachment 212755 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/loader/DiskImageCache.cpp', u'Source/WebCore/loader/DiskImageCache.h', u'Source/WebCore/loader/DiskImageCacheClient.h', u'Source/WebCore/loader/ResourceBuffer.cpp', u'Source/WebCore/loader/ResourceBuffer.h', u'Source/WebCore/loader/cache/CachedImage.cpp', u'Source/WebCore/loader/cache/CachedImage.h', u'Source/WebCore/loader/cache/CachedResource.cpp', u'Source/WebCore/loader/cache/CachedResource.h', u'Source/WebCore/loader/cache/MemoryCache.cpp', u'Source/WebCore/loader/cache/MemoryCache.h', u'Source/WebCore/loader/ios/DiskImageCacheIOS.mm', u'Source/WebCore/platform/Logging.h', u'Source/WebCore/platform/SharedBuffer.cpp', u'Source/WebCore/platform/SharedBuffer.h', u'Source/WebCore/platform/cf/SharedBufferCF.cpp', u'Source/WebKit/ChangeLog', u'Source/WebKit/WebKit.xcodeproj/project.pbxproj', u'Source/WebKit/ios/WebCoreSupport/WebDiskImageCacheClientIOS.h', u'Source/WebKit/ios/WebCoreSupport/WebDiskImageCacheClientIOS.mm', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/Misc/WebCache.mm', u'Source/WebKit/mac/WebView/WebDataSource.mm', u'Source/WebKit/mac/WebView/WebDataSourcePrivate.h', u'Source/WebKit/mac/WebView/WebPreferenceKeysPrivate.h', u'Source/WebKit/mac/WebView/WebPreferences.mm', u'Source/WebKit/mac/WebView/WebView.mm']" exit_code: 1 Source/WebCore/loader/DiskImageCache.cpp:116: Missing space before { [whitespace/braces] [5] Source/WebCore/loader/DiskImageCache.cpp:128: Missing space before { [whitespace/braces] [5] Source/WebCore/loader/DiskImageCache.cpp:228: Missing space before { [whitespace/braces] [5] Source/WebCore/loader/DiskImageCache.cpp:231: Missing space before { [whitespace/braces] [5] Source/WebCore/loader/DiskImageCache.cpp:266: Missing space before { [whitespace/braces] [5] Source/WebCore/loader/DiskImageCache.cpp:269: More than one command on the same line [whitespace/newline] [4] Source/WebCore/loader/DiskImageCache.cpp:269: Missing space before { [whitespace/braces] [5] Total errors found: 7 in 30 files If any of these errors are false positives, please file a bug against check-webkit-style.
I am looking to clean up the WebPreference code so as to work for both iOS and Mac. For now, I added an implementation, but didn't declare the following methods in WebPreferencesPrivate.h: diskImageCacheEnabled, setDiskImageCacheEnabled, diskImageCacheMinimumImageSize, diskImageCacheMaximumCacheSize, setDiskImageCacheMaximumCacheSize, _diskImageCacheSavedCacheDirectory, _setDiskImageCacheSavedCacheDirectory. Let me know if it would be preferred to omit the implementations as well or if we should look to clean up WebPreferences before landing this patch.
Comment on attachment 212755 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212755&action=review A few drive by comments, but this looks fine to me. Ideally this code will go away eventually; in favor of better shared cache between the lower network layer and WebCore. > Source/WebCore/loader/DiskImageCache.cpp:29 > +#if ENABLE(DISK_IMAGE_CACHE) This file openly uses WebThreadRun and includes WebCoreThread* files. So perhaps this should be #if PLATFORM(IOS) && ENABLE(DISK_IMAGE_CACHE)? > Source/WebCore/loader/DiskImageCache.cpp:42 > + DEFINE_STATIC_LOCAL(DiskImageCache, staticCache, ()); Newer code has been moving away from DEFINE_STATIC_LOCAL. It seems NeverDestroyed<> or just using a static (like WebCore::memoryCache()) would be fine. > Source/WebCore/loader/cache/CachedResource.h:255 > + virtual bool canUseDiskImageCache() const { return false; } > + virtual void useDiskImageCache() { ASSERT(canUseDiskImageCache()); } These could be OVERRIDE.
(In reply to comment #4) > [...] > > Source/WebCore/loader/DiskImageCache.cpp:29 > > +#if ENABLE(DISK_IMAGE_CACHE) > > This file openly uses WebThreadRun and includes WebCoreThread* files. So perhaps this should be #if PLATFORM(IOS) && ENABLE(DISK_IMAGE_CACHE)? I can do this. What are your thoughts about moving files Source/WebCore/loader/DiskImageCache.{cpp, h}, and Source/WebCore/loader/DiskImageCacheClient.h to directory Source/WebCore/loader/ios? > > > Source/WebCore/loader/DiskImageCache.cpp:42 > > + DEFINE_STATIC_LOCAL(DiskImageCache, staticCache, ()); > > Newer code has been moving away from DEFINE_STATIC_LOCAL. It seems NeverDestroyed<> or just using a static (like WebCore::memoryCache()) would be fine. Will use NeverDestroyed<>. > > > Source/WebCore/loader/cache/CachedResource.h:255 > > + virtual bool canUseDiskImageCache() const { return false; } > > + virtual void useDiskImageCache() { ASSERT(canUseDiskImageCache()); } > > These could be OVERRIDE. These are the base class definitions; => we can't mark them OVERRIDE.
(In reply to comment #5) > (In reply to comment #4) > > [...] > > > Source/WebCore/loader/DiskImageCache.cpp:29 > > > +#if ENABLE(DISK_IMAGE_CACHE) > > > > This file openly uses WebThreadRun and includes WebCoreThread* files. So perhaps this should be #if PLATFORM(IOS) && ENABLE(DISK_IMAGE_CACHE)? > > I can do this. What are your thoughts about moving files Source/WebCore/loader/DiskImageCache.{cpp, h}, and Source/WebCore/loader/DiskImageCacheClient.h to directory Source/WebCore/loader/ios? That sounds fine. > > > Source/WebCore/loader/cache/CachedResource.h:255 > > > + virtual bool canUseDiskImageCache() const { return false; } > > > + virtual void useDiskImageCache() { ASSERT(canUseDiskImageCache()); } > > > > These could be OVERRIDE. > > These are the base class definitions; => we can't mark them OVERRIDE. Ahh, good point!
Created attachment 212839 [details] Patch
Comment on attachment 212839 [details] Patch r=me
Committed r156918: <http://trac.webkit.org/changeset/156918>
(In reply to comment #4) > […] Ideally this code will go away eventually; in favor of better shared cache between the lower network layer and WebCore. Is that really possible? To me it looks like this is being done at the proper level, since it's actually decoded data that's being file-backed, rather than the bytes that were downloaded.
hrm, no, I misunderstood, it's caching the data it got from the network, so I think your comment makes sense, it would be great to make it integrate with the lower level.