Bug 126571 - [WK2][Gtk] Add support for ENABLE_INSPECTOR_SERVER to the build system
Summary: [WK2][Gtk] Add support for ENABLE_INSPECTOR_SERVER to the build system
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-01-07 03:52 PST by jaybhaskar
Modified: 2015-05-11 03:11 PDT (History)
9 users (show)

See Also:


Attachments
This patch enables build system to disable/enable inspector server (8.16 KB, patch)
2014-01-07 03:59 PST, jaybhaskar
no flags Details | Formatted Diff | Diff
This patch enables build system to disable/enable inspector server (8.20 KB, patch)
2014-01-07 04:42 PST, jaybhaskar
no flags Details | Formatted Diff | Diff
updatedpatchforremotewebinspector (8.21 KB, patch)
2014-01-08 01:08 PST, jaybhaskar
ossy: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jaybhaskar 2014-01-07 03:52:56 PST
By default web inspector server is built with web inspector for remote web inspector.
This patch enables to disable/enable inspector server in build system
Comment 1 Radar WebKit Bug Importer 2014-01-07 03:53:08 PST
<rdar://problem/15761333>
Comment 2 jaybhaskar 2014-01-07 03:59:53 PST
Created attachment 220509 [details]
This patch enables build system to disable/enable inspector server
Comment 3 WebKit Commit Bot 2014-01-07 04:05:23 PST
Attachment 220509 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/config.h', u'Source/autotools/PrintBuildConfiguration.m4', u'Source/autotools/ReadCommandLineArguments.m4', u'Source/autotools/SetupAutomake.m4', u'Source/autotools/SetupWebKitFeatures.m4', '--commit-queue']" exit_code: 1
ERROR: Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 jaybhaskar 2014-01-07 04:42:22 PST
Created attachment 220514 [details]
This patch enables build system to disable/enable inspector server
Comment 5 Joseph Pecoraro 2014-01-07 09:53:48 PST
Comment on attachment 220514 [details]
This patch enables build system to disable/enable inspector server

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

Seems fine to me. I'll let a GTK developer review in the end though.

> Source/WebKit2/ChangeLog:1
> +2014-01-07  jay bhaskar  <jay.bhaskar@samsung.com>

Nit: Is your name normally lowercase, or capitalized? Jay Bhaskar?

I believe prepare-ChangeLog uses the REAL_NAME environment variable.

> Source/WebKit2/ChangeLog:4
> +        patch enables build system to disable/enable inspector server
> +        https://bugs.webkit.org/show_bug.cgi?id=126571

Nit: This should match the format of the other ChangeLog. Essentially what `prepare-ChangeLog -b 126571` would have given you.

> Source/autotools/ReadCommandLineArguments.m4:237
> +            AC_HELP_STRING([--enable-inspector-server], [build Remote Web Inspector for WebKit2 [default=no]]),
> +                           [], [enable_inspector_server="no"])

Nit: Formatting here is harder to read then all the others.
Comment 6 Gustavo Noronha (kov) 2014-01-07 17:51:17 PST
Comment on attachment 220514 [details]
This patch enables build system to disable/enable inspector server

Why allow disabling the inspector server? And if that makes sense, why disable it by default in the build system? =)
Comment 7 jaybhaskar 2014-01-07 20:03:35 PST
(In reply to comment #6)
> (From update of attachment 220514 [details])
> Why allow disabling the inspector server? And if that makes sense, why disable it by default in the build system? =)

generally remote web inspector is used by application developers, with respect to a normal user it is not so useful. with inspector support it is being built by default. developers or user should be given option to build it so by default it should be default no.

kindly suggest better option
Comment 8 Carlos Garcia Campos 2014-01-07 23:31:23 PST
I don't expect web developers to build their own version of webkit just to have remote inspector enabled. The remote inspector needs to be enabled at run time, it's not working unless an env variable is present, so I think it's harmless having it always built, and the benefit is huge.
Comment 9 jaybhaskar 2014-01-08 00:59:04 PST
(In reply to comment #8)
> I don't expect web developers to build their own version of webkit just to have remote inspector enabled. The remote inspector needs to be enabled at run time, it's not working unless an env variable is present, so I think it's harmless having it always built, and the benefit is huge.
in that case ,it is ok and now i am enabling default yes to remote web inspector, during build time we can decide to disable if requires for any case
Comment 10 jaybhaskar 2014-01-08 01:08:54 PST
Created attachment 220608 [details]
updatedpatchforremotewebinspector
Comment 11 Carlos Garcia Campos 2014-01-08 01:14:03 PST
(In reply to comment #9)
> (In reply to comment #8)
> > I don't expect web developers to build their own version of webkit just to have remote inspector enabled. The remote inspector needs to be enabled at run time, it's not working unless an env variable is present, so I think it's harmless having it always built, and the benefit is huge.
> in that case ,it is ok and now i am enabling default yes to remote web inspector, during build time we can decide to disable if requires for any case

I still don't see the benefit of not building remote inspector files, when the remote inspector it not used by default.
Comment 12 Zan Dobersek 2014-01-08 01:26:53 PST
(In reply to comment #9)
> (In reply to comment #8)
> > I don't expect web developers to build their own version of webkit just to have remote inspector enabled. The remote inspector needs to be enabled at run time, it's not working unless an env variable is present, so I think it's harmless having it always built, and the benefit is huge.
> in that case ,it is ok and now i am enabling default yes to remote web inspector, during build time we can decide to disable if requires for any case

Configuration flags are reserved for features that introduce extra dependencies, for instance spellchecking, HTML5 multimedia support, Web Audio etc.

Remote inspector support doesn't introduce any new dependencies, and can be disabled at runtime. Because of that there's no need for an extra configuration flag -- the feature should be enabled at build-time.

The only thing I'd recommend changing is to move ENABLE_INSPECTOR_SERVER definition to SetupWebKitFeatures.m4 and define it to 1 unconditionally.
Comment 13 jaybhaskar 2014-01-08 01:45:10 PST
(In reply to comment #12)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > I don't expect web developers to build their own version of webkit just to have remote inspector enabled. The remote inspector needs to be enabled at run time, it's not working unless an env variable is present, so I think it's harmless having it always built, and the benefit is huge.
> > in that case ,it is ok and now i am enabling default yes to remote web inspector, during build time we can decide to disable if requires for any case
> 
> Configuration flags are reserved for features that introduce extra dependencies, for instance spellchecking, HTML5 multimedia support, Web Audio etc.
> 
> Remote inspector support doesn't introduce any new dependencies, and can be disabled at runtime. Because of that there's no need for an extra configuration flag -- the feature should be enabled at build-time.
> 
> The only thing I'd recommend changing is to move ENABLE_INSPECTOR_SERVER definition to SetupWebKitFeatures.m4 and define it to 1 unconditionally.

The non intentional users who are not aware of remote web inspector will not set env variable to run inspector server i agree, but it is possibility that  env variable can be set before ruing webkit based browser in shell and without user permission user's webpage may be inspected remotely,
by giving no support to build system for remote web inspector,if we do not want to run inspector server, we should not build it even if we build inspector by default
Comment 14 Zan Dobersek 2014-01-08 03:05:18 PST
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #9)
> > > (In reply to comment #8)
> > > > I don't expect web developers to build their own version of webkit just to have remote inspector enabled. The remote inspector needs to be enabled at run time, it's not working unless an env variable is present, so I think it's harmless having it always built, and the benefit is huge.
> > > in that case ,it is ok and now i am enabling default yes to remote web inspector, during build time we can decide to disable if requires for any case
> > 
> > Configuration flags are reserved for features that introduce extra dependencies, for instance spellchecking, HTML5 multimedia support, Web Audio etc.
> > 
> > Remote inspector support doesn't introduce any new dependencies, and can be disabled at runtime. Because of that there's no need for an extra configuration flag -- the feature should be enabled at build-time.
> > 
> > The only thing I'd recommend changing is to move ENABLE_INSPECTOR_SERVER definition to SetupWebKitFeatures.m4 and define it to 1 unconditionally.
> 
> The non intentional users who are not aware of remote web inspector will not set env variable to run inspector server i agree, but it is possibility that  env variable can be set before ruing webkit based browser in shell and without user permission user's webpage may be inspected remotely,
> by giving no support to build system for remote web inspector,if we do not want to run inspector server, we should not build it even if we build inspector by default

I see what you're aiming at, and I agree that enabling the feature through an environment variable is not ideal.

Though, as explained in comment #8, adding a configuration flag to avoid the problem is not the proper solution. I think the best thing would be to move away from enabling the feature through an env to another mechanism, for instance a new setting.

If you agree, please consider opening a new bug where possible solutions can be discussed and implemented. You're of course also invited to work on the problem.
Comment 15 Martin Robinson 2014-01-08 07:52:24 PST
(In reply to comment #12)

> The only thing I'd recommend changing is to move ENABLE_INSPECTOR_SERVER definition to SetupWebKitFeatures.m4 and define it to 1 unconditionally.

Let's actually put it in Platform.h so that everything is ready to go for the CMake build. I plan to move more flags there soon.
Comment 16 Zan Dobersek 2014-01-08 08:14:34 PST
(In reply to comment #15)
> (In reply to comment #12)
> 
> > The only thing I'd recommend changing is to move ENABLE_INSPECTOR_SERVER definition to SetupWebKitFeatures.m4 and define it to 1 unconditionally.
> 
> Let's actually put it in Platform.h so that everything is ready to go for the CMake build. I plan to move more flags there soon.

Handled in bug #126634.
Comment 17 Carlos Garcia Campos 2014-01-08 08:21:05 PST
(In reply to comment #15)
> (In reply to comment #12)
> 
> > The only thing I'd recommend changing is to move ENABLE_INSPECTOR_SERVER definition to SetupWebKitFeatures.m4 and define it to 1 unconditionally.
> 
> Let's actually put it in Platform.h so that everything is ready to go for the CMake build. I plan to move more flags there soon.

Why platform? isn't it wk2 specific flag? wouldn't it be better to use WebKit2Prefix.h instead?
Comment 18 Martin Robinson 2014-01-08 12:30:19 PST
(In reply to comment #17)
> Why platform? isn't it wk2 specific flag? wouldn't it be better to use WebKit2Prefix.h instead?

Platform.h has a lot of WebCore and JavaScriptCore specific flags, even though it's part of WTF. Perhaps the flags should move to other files in some way.
Comment 19 jaybhaskar 2014-01-09 21:07:43 PST
(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (In reply to comment #9)
> > > > (In reply to comment #8)
> > > > > I don't expect web developers to build their own version of webkit just to have remote inspector enabled. The remote inspector needs to be enabled at run time, it's not working unless an env variable is present, so I think it's harmless having it always built, and the benefit is huge.
> > > > in that case ,it is ok and now i am enabling default yes to remote web inspector, during build time we can decide to disable if requires for any case
> > > 
> > > Configuration flags are reserved for features that introduce extra dependencies, for instance spellchecking, HTML5 multimedia support, Web Audio etc.
> > > 
> > > Remote inspector support doesn't introduce any new dependencies, and can be disabled at runtime. Because of that there's no need for an extra configuration flag -- the feature should be enabled at build-time.
> > > 
> > > The only thing I'd recommend changing is to move ENABLE_INSPECTOR_SERVER definition to SetupWebKitFeatures.m4 and define it to 1 unconditionally.
> > 
> > The non intentional users who are not aware of remote web inspector will not set env variable to run inspector server i agree, but it is possibility that  env variable can be set before ruing webkit based browser in shell and without user permission user's webpage may be inspected remotely,
> > by giving no support to build system for remote web inspector,if we do not want to run inspector server, we should not build it even if we build inspector by default
> 
> I see what you're aiming at, and I agree that enabling the feature through an environment variable is not ideal.
> 
> Though, as explained in comment #8, adding a configuration flag to avoid the problem is not the proper solution. I think the best thing would be to move away from enabling the feature through an env to another mechanism, for instance a new setting.
> 
> If you agree, please consider opening a new bug where possible solutions can be discussed and implemented. You're of course also invited to work on the problem.

yes it is better to discuss with new bug report
https://bugs.webkit.org/show_bug.cgi?id=126733
Comment 20 Csaba Osztrogonác 2015-05-11 03:10:52 PDT
Closed as resolved/wontfix based on comments.
(And it is outdated too, GTK doesn't have autoconf build system.)