[Curl] There is no way to specify cache folder.

For a WebKit client app, there is no way to specify where the cache folder should be.

Created attachment 218091 [details]
Patch

[Mátyás Mustoha]

Hi,

Instead of WebCore, the Cache Manager should be turned on in WebKit, as the cache folder parameter may vary between different platforms.

For example, you can see how this is solved in Nix here:
https://github.com/WebKitNix/webkitnix/blob/master/Source/WebKit2/WebProcess/curl/WebProcessCurl.cpp

Created attachment 221747 [details]
Patch

(In reply to comment #2)
> Hi,
> 
> Instead of WebCore, the Cache Manager should be turned on in WebKit, as the cache folder parameter may vary between different platforms.
> 
> For example, you can see how this is solved in Nix here:
> https://github.com/WebKitNix/webkitnix/blob/master/Source/WebKit2/WebProcess/curl/WebProcessCurl.cpp

Thanks for looking into this :)

I updated the patch, added methods in the IWebCache interface to set and get cache folder location.
I also enabled the disc cache in WinLauncher by setting it's cache folder.

[Brent Fulgham]

Comment on attachment 221747 [details]
Patch

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

> Tools/WinLauncher/WinLauncher.cpp:394
> +static bool getAppDataFolder(std::wstring& directory)

Why not take a _bstr_t intend? Then you don't have to do all the conversion to/from std::wsting?

> Tools/WinLauncher/WinLauncher.cpp:448
> +    BSTR location = SysAllocString(appDataFolder.data());

If you use a _bstr_t you don't need to SysAlloc and SysFree manually. This is already used in WInLauncher.cpp, so it should build on all of our Windows targets.

Created attachment 221782 [details]
Patch

(In reply to comment #5)
> (From update of attachment 221747 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221747&action=review
> 
> > Tools/WinLauncher/WinLauncher.cpp:394
> > +static bool getAppDataFolder(std::wstring& directory)
> 
> Why not take a _bstr_t intend? Then you don't have to do all the conversion to/from std::wsting?
> 

Thanks, good point, updated patch :)

[Brent Fulgham]

Comment on attachment 221782 [details]
Patch

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

Minor nit-pick about prefixing with ::.  Could you please fix that, and I'll r+ it?

> Tools/WinLauncher/WinLauncher.cpp:402
> +    PathRemoveExtensionW(executablePath);

We prefix C API calls with ::, so these should be ::GetModuleFileNameW and ::PathRemoveExtensionW.

> Tools/WinLauncher/WinLauncher.cpp:404
> +    directory = _bstr_t(appDataDirectory) + L"\\" + PathFindFileNameW(executablePath);

This should be ::PathFindFileNameW, too.

(In reply to comment #8)
> (From update of attachment 221782 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221782&action=review
> 
> Minor nit-pick about prefixing with ::.  Could you please fix that, and I'll r+ it?
> 
> > Tools/WinLauncher/WinLauncher.cpp:402
> > +    PathRemoveExtensionW(executablePath);
> 
> We prefix C API calls with ::, so these should be ::GetModuleFileNameW and ::PathRemoveExtensionW.

I would, but I'm getting style errors when I add the prefix:

ERROR: Tools/WinLauncher/WinLauncher.cpp:402:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]

Is there a bug in the style checker?

[Brent Fulgham]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 221782 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=221782&action=review
> > 
> > Minor nit-pick about prefixing with ::.  Could you please fix that, and I'll r+ it?
> > 
> > > Tools/WinLauncher/WinLauncher.cpp:402
> > > +    PathRemoveExtensionW(executablePath);
> > 
> > We prefix C API calls with ::, so these should be ::GetModuleFileNameW and ::PathRemoveExtensionW.
> 
> I would, but I'm getting style errors when I add the prefix:
> 
> ERROR: Tools/WinLauncher/WinLauncher.cpp:402:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
> 
> Is there a bug in the style checker?

Yes, it looks like there is.  I've filed a few bugs recently against it. Something changed that regressed a few things regarding whitespace.

Go ahead and propose the patch with the incorrect style warning and I'll approve it.

Created attachment 221990 [details]
Patch

(In reply to comment #10)
> Yes, it looks like there is.  I've filed a few bugs recently against it. Something changed that regressed a few things regarding whitespace.
> 
> Go ahead and propose the patch with the incorrect style warning and I'll approve it.

Thanks, updated patch :)

[WebKit Commit Bot]

Attachment 221990 [details] did not pass style-queue:


ERROR: Tools/WinLauncher/WinLauncher.cpp:401:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Tools/WinLauncher/WinLauncher.cpp:402:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Tools/WinLauncher/WinLauncher.cpp:404:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Tools/WinLauncher/WinLauncher.cpp:402:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Tools/WinLauncher/WinLauncher.cpp:404:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 5 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Comment on attachment 221990 [details]
Patch

r=me. Stupid stylebot!

(In reply to comment #14)
> (From update of attachment 221990 [details])
> r=me. Stupid stylebot!

:) Thanks!

[WebKit Commit Bot]

Comment on attachment 221990 [details]
Patch

Clearing flags on attachment: 221990

Committed r162623: <http://trac.webkit.org/changeset/162623>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.