Bug 135752 - [GTK] Do not look for child processes in the UI process binary path
Summary: [GTK] Do not look for child processes in the UI process binary path
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 126688
  Show dependency treegraph
 
Reported: 2014-08-08 01:10 PDT by Carlos Garcia Campos
Modified: 2015-03-11 10:48 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.08 KB, patch)
2014-08-08 01:13 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
New patch (4.31 KB, patch)
2015-03-10 04:51 PDT, Carlos Garcia Campos
gns: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2014-08-08 01:10:05 PDT
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.
Comment 1 Carlos Garcia Campos 2014-08-08 01:13:13 PDT
Created attachment 236271 [details]
Patch
Comment 2 Gustavo Noronha (kov) 2014-08-28 05:26:56 PDT
This will mean we need to set WEBKIT_EXEC_PATH for running MiniBrowser directly, right?
Comment 3 Carlos Garcia Campos 2014-08-28 05:41:12 PDT
(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.
Comment 4 Carlos Garcia Campos 2014-08-28 05:42:20 PDT
(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?
Comment 5 Alberto Garcia 2014-08-28 05:57:58 PDT
(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.
Comment 6 Carlos Garcia Campos 2014-08-28 06:13:32 PDT
(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.
Comment 7 Alberto Garcia 2014-08-28 06:44:37 PDT
(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?
Comment 8 Carlos Garcia Campos 2014-08-28 06:50:29 PDT
(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.
Comment 9 Alberto Garcia 2014-08-29 01:00:29 PDT
(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.
Comment 10 Carlos Garcia Campos 2015-03-10 04:51:57 PDT
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.
Comment 11 Carlos Garcia Campos 2015-03-11 10:48:15 PDT
Committed r181392: <http://trac.webkit.org/changeset/181392>