Bug 121985 – [iOS] Upstream disk image cache

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

Upstream the iOS-specific disk image cache feature.

[Daniel Bates]

Created attachment 212755 [details]
Patch

[WebKit Commit Bot]

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.

[Daniel Bates]

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.

[Joseph Pecoraro]

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.

[Daniel Bates]

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

[Joseph Pecoraro]

(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!

[Daniel Bates]

Created attachment 212839 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 212839 [details]
Patch

r=me

[Daniel Bates]

Committed r156918: <http://trac.webkit.org/changeset/156918>

[Gustavo Noronha (kov)]

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

[Gustavo Noronha (kov)]

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.