Comment on attachment 300331
Patch

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

Lots of comments and suggestions, let's see this patch again.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1348
>> +    const gchar* className = G_OBJECT_CLASS_NAME(G_OBJECT_GET_CLASS(G_OBJECT(element)));
>> +    if (!g_strcmp0(className, "GstDownloadBuffer")) {
> 
> Early return here instead.

Remove the variable because it is only used at the if. Besides, type should have been const char*.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1353
> +        GUniqueOutPtr<gchar> oldDownloadTemplate;

char

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1356
> +        const gchar* processName = g_get_prgname();

char

>>>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1358
>>>> +            processName = "GStreamer";
>>> 
>>> shouldn't it be WebKitWebProcess by default?
>> 
>> That's what g_get_prgname() should return, yes. In the very unlikely case that it returns NULL, I'm just defaulting to the same value that GstUriDecodeBin would:
>> 
>> https://github.com/GStreamer/gst-plugins-base/blob/1.8.0/gst/playback/gsturidecodebin.c#L1951
> 
> Because gst can't know in which process is running, but we know it.

I am with Carlos here. We don't need the process name actually to name the temp file. I'd go for something like WebKit-Media-xxxxxx. This would be positive for not having to rename in case in the future we can do these downloads thru the network process.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1360
>> +        GUniquePtr<gchar> newDownloadTemplate(g_strdup_printf("/var/tmp/%s-XXXXXX", processName));
> 
> Use g_build_filename

char.

Another thing is that I think we should ensure the files are created in a different directory for each user and ensure that directory has the proper permissions so that it cannot be access by anyone else other than the user. I think media people watch online can vary a lot from cats to other things. I'd write a function to return the template and ensure it is placed in the proper directory.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1362
> +        GST_TRACE("Reconfiguring file download template from '%s' to '%s'\n", oldDownloadTemplate.get(), newDownloadTemplate.get());

The \n is not advisable.

>>>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1364
>>>> +        player->purgeOldDownloadFiles(oldDownloadTemplate.get());
>>> 
>>> So, if we purge the old files, then we don't really need to use /var/tmp, no?
>> 
>> The "$HOME/.cache" vs. "/var/tmp" discussion was about if the files were considered as "cache" or as "temporary". It's the second, so they belong to a temporary directory.
>> 
>> This purging is here just for legacy files generated by WebKit prior to this patch. Michael told me that leaking those legacy files wasn't unacceptable. That's why I added the purge method.
>> 
>> Ideally, when all the users have migrated to a WebKit version having this patch, the purge shouldn't be needed anymore and might be removed.
> 
> Right, ok.

Yep, this will purge the old files. I have a concern though for the files that fall under the GST_WARNING of line 1377 of not being able to unlink. Should we assume that is is very unlikely and forget (and trust distros to purge this from time to time)? Btw, if it is so unlikely, we can tag the decission as UNLIKELY().

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1374
> +    GUniqueOutPtr<gchar> downloadFile;

char

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1382
> +void MediaPlayerPrivateGStreamer::purgeOldDownloadFiles(const gchar* downloadFileTemplate)

char

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1388
> +    GUniquePtr<gchar> templatePath(g_path_get_dirname(downloadFileTemplate));
> +    GUniquePtr<gchar> templateFile(g_path_get_basename(downloadFileTemplate));

char

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1391
> +    gchar* found = g_strrstr(templateFile.get(), "-XX");

char

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1393
> +    if (!found)

This might be UNLIKELY too.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1405
> +    for (const gchar* fileName = g_dir_read_name(directory); fileName; fileName = g_dir_read_name(directory)) {
> +        if (g_str_has_prefix(fileName, templateFile.get())) {
> +            GUniquePtr<gchar> filePath(g_build_filename(templatePath.get(), fileName, nullptr));

If you use FileSystem::listDirectory you won't probably use this, but char.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:171
> +    void purgeOldDownloadFiles(const gchar* downloadFileTemplate);

char, remove parameter name