Bug 126571 – [WK2][Gtk] Add support for ENABLE_INSPECTOR_SERVER to the build system

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Web Inspector

[jaybhaskar]

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

[Radar WebKit Bug Importer]

<rdar://problem/15761333>

[jaybhaskar]

Created attachment 220509 [details]
This patch enables build system to disable/enable inspector server

[WebKit Commit Bot]

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.

[jaybhaskar]

Created attachment 220514 [details]
This patch enables build system to disable/enable inspector server

[Joseph Pecoraro]

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
> [phone]  jay bhaskar  <[e-mail]>

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.

[Gustavo Noronha (kov)]

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? =)

[jaybhaskar]

(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

[Carlos Garcia Campos]

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.

[jaybhaskar]

(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

[jaybhaskar]

Created attachment 220608 [details]
updatedpatchforremotewebinspector

[Carlos Garcia Campos]

(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.

[Zan Dobersek]

(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.

[jaybhaskar]

(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

[Zan Dobersek]

(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.

[Martin Robinson]

(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.

[Zan Dobersek]

(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.

[Carlos Garcia Campos]

(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?

[Martin Robinson]

(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.

[jaybhaskar]

(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

[Csaba Osztrogonác]

Closed as resolved/wontfix based on comments.
(And it is outdated too, GTK doesn't have autoconf build system.)