Bug 54683 – WebKit2: The CFNetwork Cache should be shared between the UI Process and the Web Process on Windows

• Component: WebKit2
• Status: RESOLVED FIXED

[Jessie Berlin]

There is already code to share the CFNetwork Cache between the UI Process and the Web Process on Mac. We should do the same on Windows.

<rdar://problem/8882342>

[Jessie Berlin]

Created attachment 82852 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 82852 [details]
Patch

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

> Source/WebKit2/ChangeLog:22
> +        Add the parameters for the path to the cache, the disk capacity, and the memory capacity:
> +        * Shared/WebProcessCreationParameters.cpp:
> +        (WebKit::WebProcessCreationParameters::encode):
> +        (WebKit::WebProcessCreationParameters::decode):
> +        * Shared/WebProcessCreationParameters.h:
> + 
> +        * UIProcess/win/WebContextWin.cpp:
> +        (WebKit::WebContext::platformInitializeWebProcess):
> +        Make sure to remove the ending slash, as CFNetwork does not recognize the directory with
> +        that slash.
> +
> +        * WebProcess/win/WebProcessWin.cpp:
> +        (WebKit::WebProcess::platformInitializeWebProcess):
> +        Create a cache using the path, disk capacity, and memory capacity and set it as default.

Having some comments above the items they apply to and some below confuses me!

> Source/WebKit2/Shared/WebProcessCreationParameters.cpp:72
>      encoder->encode(shouldPaintNativeControls);
> +    encoder->encode(cfURLCachePath);
> +    encoder->encode(cfURLCacheMemoryCapacity);
> +    encoder->encode(cfURLCacheDiskCapacity);

It would be nice to match the order in which these are declared in the struct.

> Source/WebKit2/Shared/WebProcessCreationParameters.cpp:128
>      if (!decoder->decode(parameters.shouldPaintNativeControls))
>          return false;
> +    if (!decoder->decode(parameters.cfURLCachePath))
> +        return false;
> +    if (!decoder->decode(parameters.cfURLCacheMemoryCapacity))
> +        return false;
> +    if (!decoder->decode(parameters.cfURLCacheDiskCapacity))
> +        return false;

Ditto.

> Source/WebKit2/UIProcess/win/WebContextWin.cpp:64
> +    parameters.cfURLCachePath = wkCopyFoundationCacheDirectory();

Presumably wkCopyFoundationCacheDirectory returns a CFStringRef, which you're leaking?

[Jessie Berlin]

(In reply to comment #2)
> (From update of attachment 82852 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82852&action=review
> 
> > Source/WebKit2/ChangeLog:22
> > +        Add the parameters for the path to the cache, the disk capacity, and the memory capacity:
> > +        * Shared/WebProcessCreationParameters.cpp:
> > +        (WebKit::WebProcessCreationParameters::encode):
> > +        (WebKit::WebProcessCreationParameters::decode):
> > +        * Shared/WebProcessCreationParameters.h:
> > + 
> > +        * UIProcess/win/WebContextWin.cpp:
> > +        (WebKit::WebContext::platformInitializeWebProcess):
> > +        Make sure to remove the ending slash, as CFNetwork does not recognize the directory with
> > +        that slash.
> > +
> > +        * WebProcess/win/WebProcessWin.cpp:
> > +        (WebKit::WebProcess::platformInitializeWebProcess):
> > +        Create a cache using the path, disk capacity, and memory capacity and set it as default.
> 
> Having some comments above the items they apply to and some below confuses me!

Removed the comment above the items it applied to, since it was not really helpful anyways.

> 
> > Source/WebKit2/Shared/WebProcessCreationParameters.cpp:72
> >      encoder->encode(shouldPaintNativeControls);
> > +    encoder->encode(cfURLCachePath);
> > +    encoder->encode(cfURLCacheMemoryCapacity);
> > +    encoder->encode(cfURLCacheDiskCapacity);
> 
> It would be nice to match the order in which these are declared in the struct.

Done.

> 
> > Source/WebKit2/Shared/WebProcessCreationParameters.cpp:128
> >      if (!decoder->decode(parameters.shouldPaintNativeControls))
> >          return false;
> > +    if (!decoder->decode(parameters.cfURLCachePath))
> > +        return false;
> > +    if (!decoder->decode(parameters.cfURLCacheMemoryCapacity))
> > +        return false;
> > +    if (!decoder->decode(parameters.cfURLCacheDiskCapacity))
> > +        return false;
> 
> Ditto.

Done.

> 
> > Source/WebKit2/UIProcess/win/WebContextWin.cpp:64
> > +    parameters.cfURLCachePath = wkCopyFoundationCacheDirectory();
> 
> Presumably wkCopyFoundationCacheDirectory returns a CFStringRef, which you're leaking?

Whoops! Fixed.

Thanks for the review!

[Jessie Berlin]

Comment on attachment 82852 [details]
Patch

Committed in r78916
http://trac.webkit.org/changeset/78916