[GTK] WebKit does not compile without JAVASCRIPT_DEBUGGER

[Gyuyoung Kim]

Created attachment 44085 [details]
patch for the build error when JAVASCRIPT_DEBUGGER is disabled on gtk

When I try to build webkit without ENABLE(JAVASCRIPT_DEBUGGER), there are some problems. 

First, even though I run './configure --enable-javascript-debugger=no', the ENABLE(JAVASCRIPT_DEBUGGER) is still enabled. I'm going to report the reason.

Second, when I defined 'ENABLE_JAVASCRIPT_DEBUGGER 0' as IPHONE, ANDROID in wtf/Platform.h, there are two build errors as below, 

-f "WebKit/gtk/webkit/.deps/libwebkit_1_0_la-webkitwebinspector.Tpo"; exit 1; fi
WebKit/gtk/webkit/webkitwebinspector.cpp: In function 'void webkit_web_inspector_set_property(GObject*, guint, const GValue*, GParamSpec*)':
WebKit/gtk/webkit/webkitwebinspector.cpp:353: error: 'class WebCore::InspectorController' has no member named 'enableProfiler'
WebKit/gtk/webkit/webkitwebinspector.cpp:355: error: 'class WebCore::InspectorController' has no member named 'disableProfiler'
WebKit/gtk/webkit/webkitwebinspector.cpp: In function 'void webkit_web_inspector_get_property(GObject*, guint, GValue*, GParamSpec*)':
WebKit/gtk/webkit/webkitwebinspector.cpp:388: error: 'class WebCore::InspectorController' has no member named 'profilerEnabled'
make[1]: *** [WebKit/gtk/webkit/libwebkit_1_0_la-webkitwebinspector.lo] Error 1
make[1]: Leaving directory `/root/workspace/WebKit'
make: *** [all] Error 2

Tpo" "WebKit/gtk/webkit/.deps/libwebkit_1_0_la-webkitwebinspector.Plo"; else rm -f "WebKit/gtk/webkit/.deps/libwebkit_1_0_la-webkitwebinspector.Tpo"; exit 1; fi
WebKit/gtk/webkit/webkitwebinspector.cpp: In function 'void webkit_web_inspector_get_property(GObject*, guint, GValue*, GParamSpec*)':
WebKit/gtk/webkit/webkitwebinspector.cpp:388: error: 'class WebCore::InspectorController' has no member named 'profilerEnabled'
make[1]: *** [WebKit/gtk/webkit/libwebkit_1_0_la-webkitwebinspector.lo] Error 1
make[1]: Leaving directory `/root/workspace/WebKit'
make: *** [all] Error 2

The reason of upper build errors is that webkitwebinspector.cpp still invokes the enableProfiler(), disableProfiler() and profilerEnabled() defined in WebCore/inspector/InspectorController.h.
In my opinion, it seems embedded device based on gtk doesn't need to use JAVASCRIPT_DEBUGGER feature.

I make a try a patch for webkitwebinspector.cpp. If there are any nits or problems, please let me know.

Thanks.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44085 [details] without any errors.

[Eric Seidel (no email)]

Comment on attachment 44085 [details]
patch for the build error when JAVASCRIPT_DEBUGGER is disabled on gtk

I'm surprised that these case statements would be outisde the #if.  Does the UI still show JS profiling stuff, and does the console object still expose profiling JS commands?

This looks OK as a build fix, but is probably not the full fix.

[Eric Seidel (no email)]

Comment on attachment 44085 [details]
patch for the build error when JAVASCRIPT_DEBUGGER is disabled on gtk

Actually, nevermind.  This has no ChangeLog.  see http://webkit.org/coding/contributing.html

[Gyuyoung Kim]

Created attachment 44126 [details]
 patch with Changelog

Thank you for your comment. When I define "ENABLE_JAVASCRIPT_DEBUGGER 0" in Platform.h, there are no build errors on this patch. However, I also think that this patch is not full fix. I will report full patch to this bug.

[Gustavo Noronha (kov)]

Comment on attachment 44126 [details]
 patch with Changelog

Hrm. Yeah, I think the case statements should indeed be outside the #if clauses, because it would be kind of an API breakage otherwise, but you should perhaps add a g_message to the #else case, and make sure get_property already sets false to the value, just to be sure.

[Gyuyoung Kim]

Created attachment 44192 [details]
Patch with g_message

I add a g_message to #else case. However, I'm not sure it is you want. If there are problems, please let me know.
Thanks.

[Gyuyoung Kim]

Created attachment 44323 [details]
 patch for the build error when JAVASCRIPT_DEBUGGER is disabled on gtk

I have tried to solve the build breaks. As I mentioned in description, there are two cases of error. First, there are build errors as below when the javascript-debugger is disabled in configure(./configure --enable-javascript-debugger=no).

WebCore/bindings/js/JSConsoleCustom.cpp:42: error: no ‘JSC::JSValue WebCore::JSConsole::profiles(JSC::ExecState*) const’ member function declared in class ‘WebCore::JSConsole’
WebCore/bindings/js/JSConsoleCustom.cpp:54: error: no ‘JSC::JSValue WebCore::JSConsole::profile(JSC::ExecState*, const JSC::ArgList&)’ member function declared in class ‘WebCore::JSConsole’
WebCore/bindings/js/JSConsoleCustom.cpp:62: error: no ‘JSC::JSValue WebCore::JSConsole::profileEnd(JSC::ExecState*, const JSC::ArgList&)’ member function declared in class ‘WebCore::JSConsole’
make[1]: *** [WebCore/bindings/js/libWebCore_la-JSConsoleCustom.lo] Error 1
make[1]: Leaving directory `/home/gyuyoung/workspace/WebKit'
make: *** [all] Error 2

I think that JSConsole.h of DerivedSources doesn't make the JSConsole::profiles() because of the configure setting, but JSConsoleCustom.cpp still compiles the JSConsole::profiles(), which are wrapped by JAVASCRIPT_DEBUGGER as below,
 
#if ENABLE(JAVASCRIPT_DEBUGGER)

typedef Vector<RefPtr<JSC::Profile> > ProfilesArray;

JSValue JSConsole::profiles(ExecState* exec) const
{
    const ProfilesArray& profiles = impl()->profiles();
    MarkedArgumentBuffer list;

    ProfilesArray::const_iterator end = profiles.end();
    for (ProfilesArray::const_iterator iter = profiles.begin(); iter != end; ++iter)
        list.append(toJS(exec, iter->get()));

    return constructArray(exec, list);
}
...
#endif

The reason is that JAVASCRIPT_DEBUGGER macro is enabled by Platform.h as below,

679 #if !defined(ENABLE_JAVASCRIPT_DEBUGGER)
680 #define ENABLE_JAVASCRIPT_DEBUGGER 1
681 #endif

Upper condition means that if ENABLE_JAVASCRIPT_DEBUGGER is not defined, the ENABLE_JAVASCRIPT_DEBUGGER should be enabled.
(It seems the ENABLE_JAVASCRIPT_DEBUGGER is enabled by WebCore/GNUmakefile.am when it is set as "yes" in configure) 
I think this make the build errors even though the "javascript-debugger" is disabled by configure.

IMO, why don't we make the ENABLE_JAVASCRIPT_DEBUGGER set as 0(disable) when it is not defined ? If user sets the javascript-debugger as "no", it means that user doesn't want to use it. So, I think the feature can be disabled as default.

BTW, even if the macro is set as 0 basically, as mentioned in description, there are still build breaks as below,

-f "WebKit/gtk/webkit/.deps/libwebkit_1_0_la-webkitwebinspector.Tpo"; exit 1;
fi
WebKit/gtk/webkit/webkitwebinspector.cpp: In function 'void
webkit_web_inspector_set_property(GObject*, guint, const GValue*,
GParamSpec*)':
WebKit/gtk/webkit/webkitwebinspector.cpp:353: error: 'class
WebCore::InspectorController' has no member named 'enableProfiler'
WebKit/gtk/webkit/webkitwebinspector.cpp:355: error: 'class
WebCore::InspectorController' has no member named 'disableProfiler'
WebKit/gtk/webkit/webkitwebinspector.cpp: In function 'void
webkit_web_inspector_get_property(GObject*, guint, GValue*, GParamSpec*)':
WebKit/gtk/webkit/webkitwebinspector.cpp:388: error: 'class
WebCore::InspectorController' has no member named 'profilerEnabled'
make[1]: *** [WebKit/gtk/webkit/libwebkit_1_0_la-webkitwebinspector.lo] Error 1
make[1]: Leaving directory `/root/workspace/WebKit'
make: *** [all] Error 2

Tpo" "WebKit/gtk/webkit/.deps/libwebkit_1_0_la-webkitwebinspector.Plo"; else rm
-f "WebKit/gtk/webkit/.deps/libwebkit_1_0_la-webkitwebinspector.Tpo"; exit 1;
fi
WebKit/gtk/webkit/webkitwebinspector.cpp: In function 'void
webkit_web_inspector_get_property(GObject*, guint, GValue*, GParamSpec*)':
WebKit/gtk/webkit/webkitwebinspector.cpp:388: error: 'class
WebCore::InspectorController' has no member named 'profilerEnabled'
make[1]: *** [WebKit/gtk/webkit/libwebkit_1_0_la-webkitwebinspector.lo] Error 1
make[1]: Leaving directory `/root/workspace/WebKit'
make: *** [all] Error 2

The webkitwebinspector.cpp invokes some functions related to JAVASCRIPT_DEBUGGER macro regardless of enabling/disabling it.

I make a patch for it. Please review it.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44323 [details] without any errors.

[Gyuyoung Kim]

Dear Eric :
Could you review this patch ?

(In reply to comment #7)
> Created an attachment (id=44323) [details]
>  patch for the build error when JAVASCRIPT_DEBUGGER is disabled on gtk
> 
> I have tried to solve the build breaks. As I mentioned in description, there
> are two cases of error. First, there are build errors as below when the
> javascript-debugger is disabled in configure(./configure
> --enable-javascript-debugger=no).
> 
> WebCore/bindings/js/JSConsoleCustom.cpp:42: error: no ‘JSC::JSValue
> WebCore::JSConsole::profiles(JSC::ExecState*) const’ member function declared
> in class ‘WebCore::JSConsole’
> WebCore/bindings/js/JSConsoleCustom.cpp:54: error: no ‘JSC::JSValue
> WebCore::JSConsole::profile(JSC::ExecState*, const JSC::ArgList&)’ member
> function declared in class ‘WebCore::JSConsole’
> WebCore/bindings/js/JSConsoleCustom.cpp:62: error: no ‘JSC::JSValue
> WebCore::JSConsole::profileEnd(JSC::ExecState*, const JSC::ArgList&)’ member
> function declared in class ‘WebCore::JSConsole’
> make[1]: *** [WebCore/bindings/js/libWebCore_la-JSConsoleCustom.lo] Error 1
> make[1]: Leaving directory `/home/gyuyoung/workspace/WebKit'
> make: *** [all] Error 2
> 
> I think that JSConsole.h of DerivedSources doesn't make the
> JSConsole::profiles() because of the configure setting, but JSConsoleCustom.cpp
> still compiles the JSConsole::profiles(), which are wrapped by
> JAVASCRIPT_DEBUGGER as below,
> 
> #if ENABLE(JAVASCRIPT_DEBUGGER)
> 
> typedef Vector<RefPtr<JSC::Profile> > ProfilesArray;
> 
> JSValue JSConsole::profiles(ExecState* exec) const
> {
>     const ProfilesArray& profiles = impl()->profiles();
>     MarkedArgumentBuffer list;
> 
>     ProfilesArray::const_iterator end = profiles.end();
>     for (ProfilesArray::const_iterator iter = profiles.begin(); iter != end;
> ++iter)
>         list.append(toJS(exec, iter->get()));
> 
>     return constructArray(exec, list);
> }
> ...
> #endif
> 
> The reason is that JAVASCRIPT_DEBUGGER macro is enabled by Platform.h as below,
> 
> 679 #if !defined(ENABLE_JAVASCRIPT_DEBUGGER)
> 680 #define ENABLE_JAVASCRIPT_DEBUGGER 1
> 681 #endif
> 
> Upper condition means that if ENABLE_JAVASCRIPT_DEBUGGER is not defined, the
> ENABLE_JAVASCRIPT_DEBUGGER should be enabled.
> (It seems the ENABLE_JAVASCRIPT_DEBUGGER is enabled by WebCore/GNUmakefile.am
> when it is set as "yes" in configure) 
> I think this make the build errors even though the "javascript-debugger" is
> disabled by configure.
> 
> IMO, why don't we make the ENABLE_JAVASCRIPT_DEBUGGER set as 0(disable) when it
> is not defined ? If user sets the javascript-debugger as "no", it means that
> user doesn't want to use it. So, I think the feature can be disabled as
> default.
> 
> BTW, even if the macro is set as 0 basically, as mentioned in description,
> there are still build breaks as below,
> 
> -f "WebKit/gtk/webkit/.deps/libwebkit_1_0_la-webkitwebinspector.Tpo"; exit 1;
> fi
> WebKit/gtk/webkit/webkitwebinspector.cpp: In function 'void
> webkit_web_inspector_set_property(GObject*, guint, const GValue*,
> GParamSpec*)':
> WebKit/gtk/webkit/webkitwebinspector.cpp:353: error: 'class
> WebCore::InspectorController' has no member named 'enableProfiler'
> WebKit/gtk/webkit/webkitwebinspector.cpp:355: error: 'class
> WebCore::InspectorController' has no member named 'disableProfiler'
> WebKit/gtk/webkit/webkitwebinspector.cpp: In function 'void
> webkit_web_inspector_get_property(GObject*, guint, GValue*, GParamSpec*)':
> WebKit/gtk/webkit/webkitwebinspector.cpp:388: error: 'class
> WebCore::InspectorController' has no member named 'profilerEnabled'
> make[1]: *** [WebKit/gtk/webkit/libwebkit_1_0_la-webkitwebinspector.lo] Error 1
> make[1]: Leaving directory `/root/workspace/WebKit'
> make: *** [all] Error 2
> 
> Tpo" "WebKit/gtk/webkit/.deps/libwebkit_1_0_la-webkitwebinspector.Plo"; else rm
> -f "WebKit/gtk/webkit/.deps/libwebkit_1_0_la-webkitwebinspector.Tpo"; exit 1;
> fi
> WebKit/gtk/webkit/webkitwebinspector.cpp: In function 'void
> webkit_web_inspector_get_property(GObject*, guint, GValue*, GParamSpec*)':
> WebKit/gtk/webkit/webkitwebinspector.cpp:388: error: 'class
> WebCore::InspectorController' has no member named 'profilerEnabled'
> make[1]: *** [WebKit/gtk/webkit/libwebkit_1_0_la-webkitwebinspector.lo] Error 1
> make[1]: Leaving directory `/root/workspace/WebKit'
> make: *** [all] Error 2
> 
> The webkitwebinspector.cpp invokes some functions related to
> JAVASCRIPT_DEBUGGER macro regardless of enabling/disabling it.
> 
> I make a patch for it. Please review it.

[Eric Seidel (no email)]

Comment on attachment 44323 [details]
 patch for the build error when JAVASCRIPT_DEBUGGER is disabled on gtk

The platform change is wrong.  I do not think you should change the default value for ENABLE_JAVASCRIPT_DEBUGGER.

[Gyuyoung Kim]

Created attachment 44345 [details]
patch for the build error when JAVASCRIPT_DEBUGGER is disabled on gtk-2

I add '-DENABLE_JAVASCRIPT_DEBUGGER 0' to WebCore/GNUmakefile.am as below,  
===========================================================
if ENABLE_JAVASCRIPT_DEBUGGER
FEATURE_DEFINES_JAVASCRIPT += ENABLE_JAVASCRIPT_DEBUGGER=1

webcore_cppflags += \
        -DENABLE_JAVASCRIPT_DEBUGGER=1

else
webcore_cppflags += \
        -DENABLE_JAVASCRIPT_DEBUGGER=0

endif # END ENABLE_JAVASCRIPT_DEBUGGER
===========================================================

I find that some features ,for example ENABLE_DATABASE, ENABLE_ICONDATABASE, ENABLE_WEB_SOCKETS, already use this way.

[Gyuyoung Kim]

Created attachment 44348 [details]
patch for the build error when JAVASCRIPT_DEBUGGER is disabled on gtk-3

Oops, I added an unnecessary blank line to Changelog. I make a patch again.
Thanks.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44348 [details] without any errors.

[Gyuyoung Kim]

Dear Eric,
Sorry for frequent review request. Could you review my latest patch again ? I want to fix this defect.

[Eric Seidel (no email)]

Comment on attachment 44348 [details]
patch for the build error when JAVASCRIPT_DEBUGGER is disabled on gtk-3

I don't know really anything about the GNUmakefile.am, but this looks sane enough.

Seems strange that whatever build system is using that file doesn't have a cleaner/more consistent way to set feature defines.

[WebKit Commit Bot]

Comment on attachment 44348 [details]
patch for the build error when JAVASCRIPT_DEBUGGER is disabled on gtk-3

Clearing flags on attachment: 44348

Committed r51783: <http://trac.webkit.org/changeset/51783>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.