Created attachment 360804 [details] [review]
SoupCache: fix setting default value for cache dir

The documentation mentions that a value of NULL can be used to let
SoupCache use its default, but code currently tries to call
g_file_test() on NULL in that case, which will cause a critical warning
to be generated.

Refactor how the cache dir default value is initialized to avoid that
issue.

Review of attachment 360804 [details] [review]:

FWIW this looks fine to me – neater and fixes the bug.

Comment on attachment 360804 [details] [review]
SoupCache: fix setting default value for cache dir

Hm... I never noticed that "feature" of the code. But the cache isn't safe for multiple processes to access at once, so no one should ever be using this.

I guess having a critical is bad, but can you at least also update the docs to say that people shouldn't use this feature? And maybe spew a g_warning if they do?

Dan, thanks for the review. Would it not make more sense to deprecate the SoupCache type/methods then? I would be happy to open a separate issue to track that, but I'm afraid that actually implementing the deprecation is more work than I wanted to do for this, which is really just a bugfix...

Deprecating the whole type just because of a bad default value is a little extreme. I guess we could deprecate that property and replace it with a nearly-identical one, but, whatever. Just document it for now.

Oh, I misunderstood what you meant by "no one should ever be using this" :-)
Unfortunately, the warning would not help either with my use case; some code ends up calling in the default constructor inside tests, which turn warnings into aborts.

I will definitely update the patch to change the docs too though.

Created attachment 360877 [details] [review]
SoupCache: fix setting default value for cache dir

--

Updated patch.

Thanks, pushed to master.

Attachment 360877 [details] pushed as 2f2681f - SoupCache: fix setting default value for cache dir