Bug 126688 – [GTK] Add an option to enable MiniBrowser for non developer builds and always install it

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

[Bastien Nocera]

So that distributions can ship it in the -devel package.

It's useful for debugging bugs in other users of webkitgtk such as epiphany or gnome-online-accounts.

[Carlos Alberto Lopez Perez]

Seems that Debian has a patch for this: http://anonscm.debian.org/gitweb/?p=pkg-webkit/webkit.git;a=blob;f=debian/patches/install-minibrowser.patch

[Alberto Garcia]

(In reply to comment #1)
> Seems that Debian has a patch for this

Yes, I actually wrote that patch :)

I see how shipping the MiniBrowser is useful, I'm not so sure about
the best way to do it upstream, though.

Would you install it in all cases? Would you put it under /usr/bin or
somewhere else?

[Gustavo Noronha (kov)]

How about on libexec?

[Alberto Garcia]

(In reply to comment #3)
> How about on libexec?

That's what we do in Debian, and I have to say that I'm not completely convinced, but whatever.

There's one more thing: we're hardcoding WEBKIT_INJECTED_BUNDLE_PATH to the build directory via g_setenv() so the user can test the MiniBrowser without having to install webkitgtk.

If we are going to to install the MiniBrowser we don't want to hardcode that, but we still want users to be able to run it from the build directory. So we have to come up with a solution.

[Carlos Garcia Campos]

(In reply to comment #2)
> (In reply to comment #1)
> > Seems that Debian has a patch for this
> 
> Yes, I actually wrote that patch :)
> 
> I see how shipping the MiniBrowser is useful, I'm not so sure about
> the best way to do it upstream, though.
> 
> Would you install it in all cases? Would you put it under /usr/bin or
> somewhere else?

gtk3-demo, for example, is provided in debian by gtk-3-examples and it's installed in /usr/bin. So, I think we could install it in /usr/bin by default, and distros can patch that if the want.

[Carlos Garcia Campos]

Created attachment 248329 [details]
Patch

So, in the end I've added an option ENABLE_MINIBROWSER, that is enabled by default on development builds and disable for production builds to keep backwards compatibility. It can be enabled for production builds by passing -DENABLE_MINIBROWSER=ON. And it's always installed when enabled.

[Zan Dobersek]

Comment on attachment 248329 [details]
Patch

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

> Tools/MiniBrowser/gtk/CMakeLists.txt:-48
> -add_definitions(-DWEBKIT_EXEC_PATH="${CMAKE_RUNTIME_OUTPUT_DIRECTORY}")

Is this not needed anymore?

> Tools/MiniBrowser/gtk/main.c:259
> +#if defined(DEVELOPMENT_BUILD)

Where is DEVELOPMENT_BUILD defined? should this also check that DEVELOPMENT_BUILD is defined to a non-null value?

[Zan Dobersek]

Comment on attachment 248329 [details]
Patch

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

>> Tools/MiniBrowser/gtk/main.c:259
>> +#if defined(DEVELOPMENT_BUILD)
> 
> Where is DEVELOPMENT_BUILD defined? should this also check that DEVELOPMENT_BUILD is defined to a non-null value?

Just saw, it's defined in bug #135752.

[Carlos Garcia Campos]

(In reply to comment #7)
> Comment on attachment 248329 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248329&action=review
> 
> > Tools/MiniBrowser/gtk/CMakeLists.txt:-48
> > -add_definitions(-DWEBKIT_EXEC_PATH="${CMAKE_RUNTIME_OUTPUT_DIRECTORY}")
> 
> Is this not needed anymore?

No, I guess we removed that at some point and forgot to remove this from the makefile. The binaries are all in bin, including MiniBroeser, so for development builds, those will be used.

[Carlos Garcia Campos]

(In reply to comment #8)
> Comment on attachment 248329 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248329&action=review
> 
> >> Tools/MiniBrowser/gtk/main.c:259
> >> +#if defined(DEVELOPMENT_BUILD)
> > 
> > Where is DEVELOPMENT_BUILD defined? should this also check that DEVELOPMENT_BUILD is defined to a non-null value?
> 
> Just saw, it's defined in bug #135752.

Yes, that's why I made this bug depend on #135752

[Carlos Garcia Campos]

Committed r181395: <http://trac.webkit.org/changeset/181395>

[Michael Catanzaro]

(In reply to comment #4)
> (In reply to comment #3)
> > How about on libexec?
> 
> That's what we do in Debian, and I have to say that I'm not completely
> convinced, but whatever.

It should go into pkglibexecdir (i.e. $(libexecdir)/webkit2gtk-4.0) so that it can be parallel-installable. Bug #162602.

P.S. Berto, you should really drop the patch in the Debian package and pass -DENABLE_MINIBROWSER=ON in the Debian rules file instead.

[Michael Catanzaro]

(In reply to comment #12)
> It should go into pkglibexecdir (i.e. $(libexecdir)/webkit2gtk-4.0) so that
> it can be parallel-installable. Bug #162602.

That's actually what the Debian patch does, the LIBEXEC_INSTALL_DIR CMake variable is actually set to pkglibexecdir.

[Alberto Garcia]

(In reply to comment #12)

> P.S. Berto, you should really drop the patch in the Debian package
> and pass -DENABLE_MINIBROWSER=ON in the Debian rules file instead.

Yeah, now after r206434 I'll drop the patch.