Bug 135896 - Move DiskCacheMonitor to WebCore so that WebKit1 clients can use it as well
Summary: Move DiskCacheMonitor to WebCore so that WebKit1 clients can use it as well
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pratik Solanki
URL:
Keywords:
Depends on: 135897
Blocks:
  Show dependency treegraph
 
Reported: 2014-08-13 13:02 PDT by Pratik Solanki
Modified: 2014-08-20 14:08 PDT (History)
7 users (show)

See Also:


Attachments
Patch (32.57 KB, patch)
2014-08-13 16:46 PDT, Pratik Solanki
no flags Details | Formatted Diff | Diff
Patch v2 (37.00 KB, patch)
2014-08-15 12:07 PDT, Pratik Solanki
no flags Details | Formatted Diff | Diff
Patch (34.49 KB, patch)
2014-08-16 13:40 PDT, Pratik Solanki
no flags Details | Formatted Diff | Diff
Patch with build fixes (35.18 KB, patch)
2014-08-17 11:42 PDT, Pratik Solanki
no flags Details | Formatted Diff | Diff
Patch with more build fixes (34.91 KB, patch)
2014-08-18 12:13 PDT, Pratik Solanki
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pratik Solanki 2014-08-13 13:02:31 PDT
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.
Comment 1 Pratik Solanki 2014-08-13 16:46:13 PDT
Created attachment 236564 [details]
Patch
Comment 2 Pratik Solanki 2014-08-13 16:46:44 PDT
Patch won't apply without the patch in bug 135897.
Comment 3 Sam Weinig 2014-08-13 22:05:11 PDT
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?
Comment 4 Pratik Solanki 2014-08-13 22:40:22 PDT
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.
Comment 5 Sam Weinig 2014-08-14 08:32:14 PDT
(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.
Comment 6 Pratik Solanki 2014-08-15 12:07:02 PDT
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.
Comment 7 Pratik Solanki 2014-08-16 13:40:22 PDT
Created attachment 236722 [details]
Patch
Comment 8 Pratik Solanki 2014-08-17 11:42:20 PDT
Created attachment 236731 [details]
Patch with build fixes
Comment 9 Pratik Solanki 2014-08-18 12:13:04 PDT
Created attachment 236774 [details]
Patch with more build fixes
Comment 10 Andreas Kling 2014-08-18 14:59:23 PDT
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".
Comment 11 Darin Adler 2014-08-18 15:04:27 PDT
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?
Comment 12 Pratik Solanki 2014-08-18 17:32:23 PDT
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.
Comment 13 Pratik Solanki 2014-08-20 11:21:18 PDT
(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.
Comment 14 Pratik Solanki 2014-08-20 14:08:01 PDT
Committed r172811: <http://trac.webkit.org/changeset/172811>