Bug 135896 – Move DiskCacheMonitor to WebCore so that WebKit1 clients can use it as well

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Pratik Solanki]

The DiskCacheMonitor code in WebKit2 doesn't have to be WebKit2 specific. Moving it to WebCore means WebKit1 clients can take advantage of this as well.

[Pratik Solanki]

Created attachment 236564 [details]
Patch

[Pratik Solanki]

Patch won't apply without the patch in bug 135897.

[Sam Weinig]

Comment on attachment 236564 [details]
Patch

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

> Source/WebCore/loader/DiskCacheMonitor.h:33
> +typedef const struct _CFCachedURLResponse* CFCachedURLResponseRef;

This file doesn't seem Apple specific on the face of it, so having this CFNetwork specific type here seems wrong.

> Source/WebCore/loader/SubresourceLoader.h:82
> +#if PLATFORM(COCOA) && !USE(CFNETWORK)
> +    virtual NSCachedURLResponse* willCacheResponse(ResourceHandle*, NSCachedURLResponse*) override;
> +#endif
> +#if PLATFORM(COCOA) && USE(CFNETWORK)
> +    virtual CFCachedURLResponseRef willCacheResponse(ResourceHandle*, CFCachedURLResponseRef) override;
> +#endif

I realize you are just moving this, but do we have a plan to remove these platform specific types with some abstraction at some point?

[Pratik Solanki]

Comment on attachment 236564 [details]
Patch

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

>> Source/WebCore/loader/DiskCacheMonitor.h:33
>> +typedef const struct _CFCachedURLResponse* CFCachedURLResponseRef;
> 
> This file doesn't seem Apple specific on the face of it, so having this CFNetwork specific type here seems wrong.

Ok. I can move it into loader/cocoa directory.

>> Source/WebCore/loader/SubresourceLoader.h:82
>> +#endif
> 
> I realize you are just moving this, but do we have a plan to remove these platform specific types with some abstraction at some point?

So add a CachedResourceResponse wrapper class and pass that to willCacheResponse? I didn't have any plans but could do it next.

[Sam Weinig]

(In reply to comment #4)
> (From update of attachment 236564 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236564&action=review
> 
> >> Source/WebCore/loader/SubresourceLoader.h:82
> >> +#endif
> > 
> > I realize you are just moving this, but do we have a plan to remove these platform specific types with some abstraction at some point?
> 
> So add a CachedResourceResponse wrapper class and pass that to willCacheResponse? I didn't have any plans but could do it next.

I'm not sure, we should discuss it further.

[Pratik Solanki]

Created attachment 236666 [details]
Patch v2

Note that this patch depends on the one in bug 135897 so it won't apply cleanly for bots.

[Pratik Solanki]

Created attachment 236722 [details]
Patch

[Pratik Solanki]

Created attachment 236731 [details]
Patch with build fixes

[Pratik Solanki]

Created attachment 236774 [details]
Patch with more build fixes

[Andreas Kling]

Comment on attachment 236774 [details]
Patch with more build fixes

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

r=me

> Source/WebCore/ChangeLog:17
> +            CFNetwork callback block, it calls a virtual function that is overriden by

Typo, overridden.

> Source/WebKit2/NetworkProcess/mac/NetworkDiskCacheMonitor.h:43
> +class NetworkDiskCacheMonitor : public WebCore::DiskCacheMonitor, IPC::MessageSender {

This class should be marked "final".

[Darin Adler]

Comment on attachment 236774 [details]
Patch with more build fixes

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

> Source/WebCore/loader/SubresourceLoader.h:78
> +    virtual NSCachedURLResponse* willCacheResponse(ResourceHandle*, NSCachedURLResponse*) override;

Formatting not right here for NSCachedURLResponse *.

> Source/WebCore/loader/cocoa/DiskCacheMonitor.h:33
> +typedef const struct _CFCachedURLResponse* CFCachedURLResponseRef;

Is this a CF-only header? I’m really confused that the header doesn’t have the Cocoa suffix in it, if it’s truly a platform-specific header.

> Source/WebCore/loader/cocoa/DiskCacheMonitor.h:61
> +void DiskCacheMonitor::monitorFileBackingStoreCreation(const ResourceRequest&, SessionID, CFCachedURLResponseRef) { }
> +PassRefPtr<SharedBuffer> DiskCacheMonitor::tryGetFileBackedSharedBufferFromCFURLCachedResponse(CFCachedURLResponseRef) { return nullptr; }

Those should be marked inline if they are going to be in a header file. Otherwise we’ll get linker errors if we use this header in more than one place.

I also don’t think they should be clumped up all in one line like this.

> Source/WebCore/loader/cocoa/DiskCacheMonitor.h:64
> +#endif
> +} // namespace WebKit

Should have a blank line here.

> Source/WebKit2/NetworkProcess/mac/NetworkDiskCacheMonitor.h:43
> +class NetworkDiskCacheMonitor : public WebCore::DiskCacheMonitor, IPC::MessageSender {

This makes derivation from MessageSender private instead of public. If it can be private, that’s good, but it looks like you did this by accident; we should explicitly state either private or public. I prefer private, though.

> Source/WebKit2/NetworkProcess/mac/NetworkDiskCacheMonitor.h:46
> +    ~NetworkDiskCacheMonitor() { }

This is not needed, and if we did want to explicitly declare the destructor I would suggest marking it virtual explicitly. I suggest just removing this line entirely.

> Source/WebKit2/NetworkProcess/mac/NetworkDiskCacheMonitor.mm:53
> +    new NetworkDiskCacheMonitor(cachedResponse, loader); // Balanced by adoptPtr in the blocks setup in the DiskCacheMonitor constructor, one of which is guaranteed to run.

Should be "set up" rather than "setup".

This is a bad situation; code to implement this unconventional storage management is now spread across two classes, with the new call in the derived class and the adoptPtr in the base class. It’s really easy to get that wrong. Can we find a more straightforward idiom for this? I think we should consider moving the logic from the DiskCacheMonitor constructor into a separate function that takes a std::unique_ptr to get rid of this strangeness.

> Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:31
> +#import "NetworkConnectionToWebProcess.h"

Why is this needed?

[Pratik Solanki]

Comment on attachment 236774 [details]
Patch with more build fixes

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

>> Source/WebCore/loader/cocoa/DiskCacheMonitor.h:33
>> +typedef const struct _CFCachedURLResponse* CFCachedURLResponseRef;
> 
> Is this a CF-only header? I’m really confused that the header doesn’t have the Cocoa suffix in it, if it’s truly a platform-specific header.

Yes, this is CF only. I'll rename it to DiskCacheMonitorCocoa.h since it's in the cocoa directory anyway.

>> Source/WebKit2/NetworkProcess/mac/NetworkDiskCacheMonitor.mm:53
>> +    new NetworkDiskCacheMonitor(cachedResponse, loader); // Balanced by adoptPtr in the blocks setup in the DiskCacheMonitor constructor, one of which is guaranteed to run.
> 
> Should be "set up" rather than "setup".
> 
> This is a bad situation; code to implement this unconventional storage management is now spread across two classes, with the new call in the derived class and the adoptPtr in the base class. It’s really easy to get that wrong. Can we find a more straightforward idiom for this? I think we should consider moving the logic from the DiskCacheMonitor constructor into a separate function that takes a std::unique_ptr to get rid of this strangeness.

Agreed. The memory management now spread across two classes is bad. I'll try to fix this.

>> Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:31
>> +#import "NetworkConnectionToWebProcess.h"
> 
> Why is this needed?

I was getting compile errors because I had an inline destructor for NetworkDiskCacheMonitor which wanted to clear out the connection. I removed the dtor and don't seem to need this include anymore.

[Pratik Solanki]

(In reply to comment #12)
> (From update of attachment 236774 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236774&action=review
> 
> > This is a bad situation; code to implement this unconventional storage management is now spread across two classes, with the new call in the derived class and the adoptPtr in the base class. It’s really easy to get that wrong. Can we find a more straightforward idiom for this? I think we should consider moving the logic from the DiskCacheMonitor constructor into a separate function that takes a std::unique_ptr to get rid of this strangeness.
> 
> Agreed. The memory management now spread across two classes is bad. I'll try to fix this.

I'll do this separately from this patch. Filed bug 136112.

[Pratik Solanki]

Committed r172811: <http://trac.webkit.org/changeset/172811>