Bug 126688 - [GTK] Add an option to enable MiniBrowser for non developer builds and always install it
Summary: [GTK] Add an option to enable MiniBrowser for non developer builds and always...
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:
Depends on: 135752
Blocks:
  Show dependency treegraph
 
Reported: 2014-01-09 03:02 PST by Bastien Nocera
Modified: 2016-09-29 02:30 PDT (History)
7 users (show)

See Also:


Attachments
Patch (5.37 KB, patch)
2015-03-10 05:36 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 Bastien Nocera 2014-01-09 03:02:21 PST
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.
Comment 1 Carlos Alberto Lopez Perez 2014-05-13 14:51:53 PDT
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
Comment 2 Alberto Garcia 2014-05-14 03:35:31 PDT
(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?
Comment 3 Gustavo Noronha (kov) 2014-05-14 05:08:53 PDT
How about on libexec?
Comment 4 Alberto Garcia 2014-05-14 05:16:17 PDT
(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.
Comment 5 Carlos Garcia Campos 2015-03-10 03:43:03 PDT
(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.
Comment 6 Carlos Garcia Campos 2015-03-10 05:36:12 PDT
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.
Comment 7 Zan Dobersek 2015-03-10 11:01:53 PDT
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?
Comment 8 Zan Dobersek 2015-03-10 11:03:39 PDT
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.
Comment 9 Carlos Garcia Campos 2015-03-10 11:06:52 PDT
(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.
Comment 10 Carlos Garcia Campos 2015-03-10 11:07:33 PDT
(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
Comment 11 Carlos Garcia Campos 2015-03-11 11:09:16 PDT
Committed r181395: <http://trac.webkit.org/changeset/181395>
Comment 12 Michael Catanzaro 2016-09-27 03:14:18 PDT
(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.
Comment 13 Michael Catanzaro 2016-09-27 03:23:15 PDT
(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.
Comment 14 Alberto Garcia 2016-09-29 02:30:54 PDT
(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.