[webkit-reviews] review granted: [Bug 186841] [WPE] Pass the backend library name as command line parameter to the web process : [Attachment 344682] Patch

[bugzilla-daemon at webkit.org]

Zan Dobersek <zan at falconsigh.net> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 186841: [WPE] Pass the backend library name as command line parameter to
the web process
https://bugs.webkit.org/show_bug.cgi?id=186841

Attachment 344682: Patch

https://bugs.webkit.org/attachment.cgi?id=344682&action=review




--- Comment #20 from Zan Dobersek <zan at falconsigh.net> ---
Comment on attachment 344682
  --> https://bugs.webkit.org/attachment.cgi?id=344682
Patch

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

>>>>> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:106
>>>>> +#endif
>>>> 
>>>> Do we need the --backend-library flag?
>>> 
>>> Comment #8
>>> 
>>> "Yes, probably, I'm trying to avoid passing other stuff to wpe loader if we
mess it up with the order of the arguments."
>>> 
>>> I can remove it if you don't like it.
>> 
>> It's out-of-place in that argument array, IMO.
>> 
>> Regardless of that, should the library name be escaped somehow before it's
included in the arguments? Is there any way in which the impl library name
alone could cause trouble? Should we use FileSystem::fileSystemRepresentation()
like we do for other filenames of executables?
> 
> That's a good point, I'll use fileSystemRepresantation.

r=me with this change.

>>>>> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:137
>>>>> +        argv[i++] = wpeBackendLibraryParameter ?
wpeBackendLibraryParameter.get() : "-";
>>>> 
>>>> I don't think wpeBackendLibraryParameter can be null if you use
g_strdup_printf()? Unless we can remove the --backend-library flag and just
strdup the impl library name, if any.
>>> 
>>> It can be nullptr when building with previous versions of WPEBackend.
>> 
>> Let's ensure it's non-null by assigning it the "-" token by default, and
override it when compiling against the newer WPEBackend lib.
> 
> I don't see the point here. We would be heap allocating "-", just to free and
allocate again in the case of new wpebackend, which will be the common one. I
don't see the problem with the check here and using a static string "-".

Makes sense.