Bug 126173 – [GTK][WK2] Simplify ProcessExecutablePathGtk

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

[GTK][WK2] Simplify ProcessExecutablePathGtk

[Zan Dobersek]

Created attachment 219923 [details]
Patch

[Martin Robinson]

Comment on attachment 219923 [details]
Patch

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

> Source/WebKit2/Shared/gtk/ProcessExecutablePathGtk.cpp:51
> +    static CString executablePath = getCurrentExecutablePath();
>      if (!executablePath.isNull()) {
> -        String processPath = pathByAppendingComponent(executablePath, processName);
> +        String processPath = pathByAppendingComponent(directoryName(filenameToString(executablePath.data())), processName);
>          if (fileExists(processPath))
>              return processPath;

This is a little slower, because directoryName and filenameToString runs for each path, but not much.

[Zan Dobersek]

Comment on attachment 219923 [details]
Patch

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

>> Source/WebKit2/Shared/gtk/ProcessExecutablePathGtk.cpp:51
>>              return processPath;
> 
> This is a little slower, because directoryName and filenameToString runs for each path, but not much.

I can further store return values of findWebKitProcess in static variables in executablePathOf*Process. This would limit the number of times that findWebKitProcess is called to 3.

If that sounds OK I can upload a new patch.

[Martin Robinson]

(In reply to comment #3)
> (From update of attachment 219923 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219923&action=review
> 
> >> Source/WebKit2/Shared/gtk/ProcessExecutablePathGtk.cpp:51
> >>              return processPath;
> > 
> > This is a little slower, because directoryName and filenameToString runs for each path, but not much.
> 
> I can further store return values of findWebKitProcess in static variables in executablePathOf*Process. This would limit the number of times that findWebKitProcess is called to 3.
> 
> If that sounds OK I can upload a new patch.

It seems simpler to cache the return value of directoryName(filenameToString(...)), but if this function is called only a few times, I don't think it's a big deal.

[Zan Dobersek]

Comment on attachment 219923 [details]
Patch

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

>>>> Source/WebKit2/Shared/gtk/ProcessExecutablePathGtk.cpp:51
>>>>              return processPath;
>>> 
>>> This is a little slower, because directoryName and filenameToString runs for each path, but not much.
>> 
>> I can further store return values of findWebKitProcess in static variables in executablePathOf*Process. This would limit the number of times that findWebKitProcess is called to 3.
>> 
>> If that sounds OK I can upload a new patch.
> 
> It seems simpler to cache the return value of directoryName(filenameToString(...)), but if this function is called only a few times, I don't think it's a big deal.

This will now be called whenever a new process is being created. It's unnecessary work, really.

I can offload all of this (getCurrentExecutablePath, filenameToString and directoryName calls) into a small helper static function. Its return value would then be stored in the static variable, so it would only get called once.

[Zan Dobersek]

Created attachment 219929 [details]
Patch

[Zan Dobersek]

Comment on attachment 219929 [details]
Patch

Clearing flags on attachment: 219929

Committed r161020: <http://trac.webkit.org/changeset/161020>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Bear Travis]

Reopening to attach new patch.

[Bear Travis]

Created attachment 220760 [details]
Initial patch

[Martin Robinson]

(In reply to comment #10)
> Created an attachment (id=220760) [details]
> Initial patch

Wrong bug?

[Bear Travis]

Yes, apologies. Was wondering where webkit-patch sent this one.