Bug 140608 – REGRESSION(r178527): It broke the !ENABLE(INSPECTOR) build

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Csaba Osztrogonác]

../../Source/WebCore/page/PageConsoleClient.cpp: In member function 'void WebCore::PageConsoleClient::addMessage(JSC::MessageSource, JSC::MessageLevel, const WTF::String&, const WTF::String&, unsigned int, unsigned int, WTF::RefPtr<Inspe
ctor::ScriptCallStack>&&, JSC::ExecState*, long unsigned int)':
../../Source/WebCore/page/PageConsoleClient.cpp:127:25: error: invalid use of incomplete type 'class Inspector::ConsoleMessage'
In file included from ../../Source/WebCore/inspector/InspectorConsoleInstrumentation.h:34:0,
                 from ../../Source/WebCore/page/PageConsoleClient.cpp:36:
../../Source/WebCore/inspector/InspectorInstrumentation.h:60:7: error: forward declaration of 'class Inspector::ConsoleMessage'
../../Source/WebCore/page/PageConsoleClient.cpp:128:34: error: invalid use of incomplete type 'class Inspector::ConsoleMessage'
In file included from ../../Source/WebCore/inspector/InspectorConsoleInstrumentation.h:34:0,
                 from ../../Source/WebCore/page/PageConsoleClient.cpp:36:
../../Source/WebCore/inspector/InspectorInstrumentation.h:60:7: error: forward declaration of 'class Inspector::ConsoleMessage'
../../Source/WebCore/page/PageConsoleClient.cpp:129:36: error: invalid use of incomplete type 'class Inspector::ConsoleMessage'
In file included from ../../Source/WebCore/inspector/InspectorConsoleInstrumentation.h:34:0,
                 from ../../Source/WebCore/page/PageConsoleClient.cpp:36:
../../Source/WebCore/inspector/InspectorInstrumentation.h:60:7: error: forward declaration of 'class Inspector::ConsoleMessage'
...


The whole ConsoleMessage.h is guarded with ENABLE(INSPECTOR), but it is ununsed without guard in PageConsoleClient.cpp.

[Csaba Osztrogonác]

Does r178527 mean that inspector is mandatory feature since now? 

If yes, let's remove the compile time guards and make it really mandatory.
If no, could you possibly give me any hint how to fix the build properly?

[Alexey Proskuryakov]

Given that we use the logic in Inspector::ConsoleMessage even when inspector is not enabled, yes, this logic needs to be always compiled in. I haven't looked into how much of other Inspector code it uses. Perhaps it can be factored out of Inspector::ConsoleMessage.

What is the rationale for disabling Inspector? The !ENABLE(INSPECTOR) build appears to be very high maintenance.

[Brian Burg]

(In reply to comment #1)
> Does r178527 mean that inspector is mandatory feature since now? 
> 
> If yes, let's remove the compile time guards and make it really mandatory.
> If no, could you possibly give me any hint how to fix the build properly?

It seems that these are the only methods that actually require ENABLE(INSPECTOR), and are entry points from InspectorConsoleAgent:

    void addToFrontend(InspectorConsoleFrontendDispatcher*, InjectedScriptManager*, bool generatePreview);
    void updateRepeatCountInConsole(InspectorConsoleFrontendDispatcher*);

Plus IdentifiersFactory.h which looks to have no other Inspector dependencies. So you would need to remove the guard of the entire file and only surround those methods (and their static helper functions in the .cpp file).

Honestly, someone should request to webkit-dev to remove ENABLE(INSPECTOR) guards. There are no bots running this configuration, and I would be surprised if anyone contributing to trunk depends on it. If such a person exists, I would have expected to hear about these build breaks before Ossy found it via --minimal.

[Csaba Osztrogonác]

(In reply to comment #2)
> Given that we use the logic in Inspector::ConsoleMessage even when inspector
> is not enabled, yes, this logic needs to be always compiled in. I haven't
> looked into how much of other Inspector code it uses. Perhaps it can be
> factored out of Inspector::ConsoleMessage.
> 
> What is the rationale for disabling Inspector? The !ENABLE(INSPECTOR) build
> appears to be very high maintenance.

It can be useful to reduce the binary codesize on embedded 
systems where you don't need inspector at all. 
( https://bugs.webkit.org/show_bug.cgi?id=140098#c5 )

I don't have strong opinion about it, but I don't like broken and unmaintained
ifdef guards. If we want to have a guard, we should maintain it, if we don't
want to maintain it, let's remove the guard.

[Csaba Osztrogonác]

(In reply to comment #3)

> Honestly, someone should request to webkit-dev to remove ENABLE(INSPECTOR)
> guards. There are no bots running this configuration, and I would be
> surprised if anyone contributing to trunk depends on it. If such a person
> exists, I would have expected to hear about these build breaks before Ossy
> found it via --minimal.

Will send soon.

[Csaba Osztrogonác]

webkit-dev mail sent: https://lists.webkit.org/pipermail/webkit-dev/2015-January/027153.html

[Csaba Osztrogonác]

There is no !INSPECTOR build since http://trac.webkit.org/changeset/178820 .