Bug 135752 – [GTK] Do not look for child processes in the UI process binary path

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

I think this is only useful for internal tools and tests, but never when installed, since we don't install the processes in the bin dir but in the libexec dir. We already have an env var to cover the cases of the internal tools and tests, so I think it would be better to use that variable only.

[Carlos Garcia Campos]

Created attachment 236271 [details]
Patch

[Gustavo Noronha (kov)]

This will mean we need to set WEBKIT_EXEC_PATH for running MiniBrowser directly, right?

[Carlos Garcia Campos]

(In reply to comment #2)
> This will mean we need to set WEBKIT_EXEC_PATH for running MiniBrowser directly, right?

Yes, that's what the patch does. And I don't like it, because downstream need to patch it to remove that. I think we could pass DEVELOPER_MODE to the compiler and only set the envs for developer builds

#ifdef DEVELOPER_MODE
     g_setenv("WEBKIT_EXEC_PATH", WEBKIT_EXEC_PATH, FALSE);
     g_setenv("WEBKIT_INJECTED_BUNDLE_PATH", WEBKIT_INJECTED_BUNDLE_PATH, FALSE);
#endif

Or only pass WEBKIT_EXEC_PATH and WEBKIT_INJECTED_BUNDLE_PATH for developer builds and check those directly.

[Carlos Garcia Campos]

(In reply to comment #3)
> (In reply to comment #2)
> > This will mean we need to set WEBKIT_EXEC_PATH for running MiniBrowser directly, right?
> 
> Yes, that's what the patch does. And I don't like it, because downstream need to patch it to remove that. I think we could pass DEVELOPER_MODE to the compiler and only set the envs for developer builds
> 
> #ifdef DEVELOPER_MODE
>      g_setenv("WEBKIT_EXEC_PATH", WEBKIT_EXEC_PATH, FALSE);
>      g_setenv("WEBKIT_INJECTED_BUNDLE_PATH", WEBKIT_INJECTED_BUNDLE_PATH, FALSE);
> #endif
> 
> Or only pass WEBKIT_EXEC_PATH and WEBKIT_INJECTED_BUNDLE_PATH for developer builds and check those directly.

hmm, wait, but Tools (including MB) are not build on non developer builds, or does debian also patch that? Berto?

[Alberto Garcia]

(In reply to comment #4)
> hmm, wait, but Tools (including MB) are not build on non developer
> builds, or does debian also patch that? Berto?

In Debian I'm patching CMakeLists.txt to build the MiniBrowser, the
rest of the tools we don't need.

I also remove the g_setenv("WEBKIT_INJECTED_BUNDLE_PATH", ...) call
from main.c.

[Carlos Garcia Campos]

(In reply to comment #5)
> (In reply to comment #4)
> > hmm, wait, but Tools (including MB) are not build on non developer
> > builds, or does debian also patch that? Berto?
> 
> In Debian I'm patching CMakeLists.txt to build the MiniBrowser, the
> rest of the tools we don't need.
> 
> I also remove the g_setenv("WEBKIT_INJECTED_BUNDLE_PATH", ...) call
> from main.c.

Ok, so you will have to keep patching it anyway, so we either leave it as it is, or build MB unconditionally. MB doesn't use any internal symbol, so it shouldn't be a problem.

[Alberto Garcia]

(In reply to comment #6)
> Ok, so you will have to keep patching it anyway

Yeah, that's not a problem, it's a trivial patch anyway.

The only alternative would be that we want to install MiniBrowser by
default upstream, which I don't think it's the case?

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #6)
> > Ok, so you will have to keep patching it anyway
> 
> Yeah, that's not a problem, it's a trivial patch anyway.
> 
> The only alternative would be that we want to install MiniBrowser by
> default upstream, which I don't think it's the case?

I don't see why not. It's like gtk3-demo, for example, or poppler-glib-demo. We can add a specific compile option to disable it (not the installation, but the whole MB), and enable it always for developer builds, since it's required/expected by the layout tests.

[Alberto Garcia]

(In reply to comment #8)
> > The only alternative would be that we want to install MiniBrowser
> > by default upstream, which I don't think it's the case?
> I don't see why not. It's like gtk3-demo, for example, or
> poppler-glib-demo.

Ok, why are not we installing it then?

Anyway, if we add an option to install it I think we should disable
the hardcoded WEBKIT_INJECTED_BUNDLE_PATH somehow.

[Carlos Garcia Campos]

Created attachment 248327 [details]
New patch

This patch introduces DEVELOPMENT_BUILD, and only when defined we look for child processes in UI process bin dir, or even in WEBKIT_EXEC_PATH. And the same for the WEBKIT_INJECTED_BUNDLE_PATH env var.

[Carlos Garcia Campos]

Committed r181392: <http://trac.webkit.org/changeset/181392>