Bug 121985

Summary: [iOS] Upstream disk image cache
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebCore Misc.Assignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, commit-queue, darin, ddkilzer, gns, japhet, joepeck
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch joepeck: review+

Description Daniel Bates 2013-09-26 14:52:28 PDT
Upstream the iOS-specific disk image cache feature.
Comment 1 Daniel Bates 2013-09-26 14:59:59 PDT
Created attachment 212755 [details]
Patch
Comment 2 WebKit Commit Bot 2013-09-26 15:01:55 PDT
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.
Comment 3 Daniel Bates 2013-09-26 15:06:18 PDT
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 4 Joseph Pecoraro 2013-09-26 15:44:08 PDT
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.
Comment 5 Daniel Bates 2013-09-26 16:58:44 PDT
(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.
Comment 6 Joseph Pecoraro 2013-09-26 18:13:54 PDT
(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!
Comment 7 Daniel Bates 2013-09-27 13:39:47 PDT
Created attachment 212839 [details]
Patch
Comment 8 Joseph Pecoraro 2013-10-04 13:01:28 PDT
Comment on attachment 212839 [details]
Patch

r=me
Comment 9 Daniel Bates 2013-10-04 15:39:53 PDT
Committed r156918: <http://trac.webkit.org/changeset/156918>
Comment 10 Gustavo Noronha (kov) 2013-10-23 11:59:42 PDT
(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.
Comment 11 Gustavo Noronha (kov) 2013-10-28 04:34:15 PDT
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.