Bug 126688

Summary: [GTK] Add an option to enable MiniBrowser for non developer builds and always install it
Product: WebKit Reporter: Bastien Nocera <bugzilla>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, cgarcia, clopez, gustavo, mcatanzaro, mrobinson, pnormand
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 135752    
Bug Blocks:    
Attachments:
Description Flags
Patch gustavo: review+

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.