Re: webkit-1.8.0-2: Patches for review
On Wed, 2012-04-18 at 09:54 +0200, Pino Toscano wrote:
> Alle martedì 17 aprile 2012, Svante Signell ha scritto:
> The else branch is not needed, resultString is already initialized.
Initialized to the empty string, or?
> C++ (just like C99, FWIW) allows to declare variables inside code
> blocks, and not strictly at the start of a block.
OK!
> > + if (readLinkBuffer == NULL)
> > + return CString();
>
> The return needs 4 spaces of indentation, like the rest of the code.
OK, didn't notice.
> Now that I saw webkit's code, I'm a bit dubious about the code in
> Source/JavaScriptCore/wtf/gobject/GlibUtilities.cpp; it basically does
> something like this (simplified for convenience of email):
>
> | #if OS(LINUX)
> | CString getCurrentExecutablePath()
> | { /* read path from /proc/self/exe */ }
> | #elif OS(UNIX)
> | CString getCurrentExecutablePath()
> | { /* read path from /proc/curproc/file */ }
> | #elif OS(WINDOWS)
> | CString getCurrentExecutablePath()
> | { /* read path using Windows API */ }
> | #endif
>
> Your patch would make use of /proc/curproc/file, which would seem to be
> part of the FreeBSD procfs[1]; our procfs is more oriented to emulate
> Linux's one, so IMHO we should either use the Linux code, or add an
> empty getCurrentExecutablePath() like this:
>
> | #elif OS(HURD)
> | CString getCurrentExecutablePath()
> | { return CString(); }
Looks like neither /proc/curproc/file or /proc/self/exe exists for
GNU/Hurd (any plans to add that??), so an empty function would be best.
Agreed? If so I'll update the patch (and we don't have to care about the
PATH_MAX stuff).
BTW: In my opinion both the Linux and *BSD code should use the code
construct in the patch. But that is another story.... Any link to
upstream, gnome people or?
Reply to: