>>>> 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.
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().
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/ MediaPlayerPriv ateGStreamer. cpp:1348 CLASS_NAME( G_OBJECT_ GET_CLASS( G_OBJECT( element) )); className, "GstDownloadBuf fer")) {
>> + const gchar* className = G_OBJECT_
>> + if (!g_strcmp0(
>
> 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/ MediaPlayerPriv ateGStreamer. cpp:1353 gchar> oldDownloadTemp late;
> + GUniqueOutPtr<
char
> Source/ WebCore/ platform/ graphics/ gstreamer/ MediaPlayerPriv ateGStreamer. cpp:1356
> + const gchar* processName = g_get_prgname();
char
>>>> Source/ WebCore/ platform/ graphics/ gstreamer/ MediaPlayerPriv ateGStreamer. cpp:1358 /github. com/GStreamer/ gst-plugins- base/blob/ 1.8.0/gst/ playback/ gsturidecodebin .c#L1951
>>>> + 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:/
>
> 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/ MediaPlayerPriv ateGStreamer. cpp:1360 late(g_ strdup_ printf( "/var/tmp/ %s-XXXXXX" , processName));
>> + GUniquePtr<gchar> newDownloadTemp
>
> 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/ MediaPlayerPriv ateGStreamer. cpp:1362 "Reconfiguring file download template from '%s' to '%s'\n", oldDownloadTemp late.get( ), newDownloadTemp late.get( ));
> + GST_TRACE(
The \n is not advisable.
>>>> Source/ WebCore/ platform/ graphics/ gstreamer/ MediaPlayerPriv ateGStreamer. cpp:1364 >purgeOldDownlo adFiles( oldDownloadTemp late.get( ));
>>>> + player-
>>>
>>> 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/ MediaPlayerPriv ateGStreamer. cpp:1374 gchar> downloadFile;
> + GUniqueOutPtr<
char
> Source/ WebCore/ platform/ graphics/ gstreamer/ MediaPlayerPriv ateGStreamer. cpp:1382 ateGStreamer: :purgeOldDownlo adFiles( const gchar* downloadFileTem plate)
> +void MediaPlayerPriv
char
> Source/ WebCore/ platform/ graphics/ gstreamer/ MediaPlayerPriv ateGStreamer. cpp:1388 g_path_ get_dirname( downloadFileTem plate)) ; g_path_ get_basename( downloadFileTem plate)) ;
> + GUniquePtr<gchar> templatePath(
> + GUniquePtr<gchar> templateFile(
char
> Source/ WebCore/ platform/ graphics/ gstreamer/ MediaPlayerPriv ateGStreamer. cpp:1391 templateFile. get(), "-XX");
> + gchar* found = g_strrstr(
char
> Source/ WebCore/ platform/ graphics/ gstreamer/ MediaPlayerPriv ateGStreamer. cpp:1393
> + if (!found)
This might be UNLIKELY too.
> Source/ WebCore/ platform/ graphics/ gstreamer/ MediaPlayerPriv ateGStreamer. cpp:1405 name(directory) ; fileName; fileName = g_dir_read_ name(directory) ) { has_prefix( fileName, templateFile. get())) { g_build_ filename( templatePath. get(), fileName, nullptr));
> + for (const gchar* fileName = g_dir_read_
> + if (g_str_
> + GUniquePtr<gchar> filePath(
If you use FileSystem: :listDirectory you won't probably use this, but char.
> Source/ WebCore/ platform/ graphics/ gstreamer/ MediaPlayerPriv ateGStreamer. h:171 dFiles( const gchar* downloadFileTem plate);
> + void purgeOldDownloa
char, remove parameter name