Bug 83557 – Web Inspector: Disabling Inspector causes build failure on Windows

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Vivek Galatage]

Steps to reproduce:
1. Make sure to set your ENABLE_INSPECTOR to 0 in Source/WTF/wtf/Platform.h
2. Try building the solution either in Visual Studio or on cygwin

Expected Outcome:
Build should be successful 

Actual Outcome:
Build fails with multiple error message and linker errors

[Vivek Galatage]

The calls to inspector APIs are not put under ENABLE(INSPECTOR) guard.

[Pavel Feldman]

Could you provide the build logs? Not all the calls to inspector API should be behind the ENABLE(INSPECTOR). For example, InspectorInstrumentation is safe to use as is.

[Vivek Galatage]

Yes Pavel, this is related to the calls to InspectorController which are not guarded. 

Uploading the patch now.

[Vivek Galatage]

Created attachment 136414 [details]
Patch

[Vivek Galatage]

Created attachment 136424 [details]
Build error log file

Attached the build-log when the inspector is disabled.

[Pavel Feldman]

Comment on attachment 136414 [details]
Patch

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

> Source/WebKit/win/WebNodeHighlight.cpp:163
>      m_inspectedWebView->page()->inspectorController()->drawHighlight(context);

You should wrap m_inspectedWebView in header as well for additional compile-time checks.

> Source/WebKit/win/WebView.cpp:715
>      if (m_webInspector)

Ditto

> Source/WebKit/win/WebView.cpp:2664
>      m_inspectorClient = new WebInspectorClient(this);

Ditto

> Source/WebKit/win/WebView.cpp:5776
>  HRESULT STDMETHODCALLTYPE WebView::inspector(IWebInspector** inspector)

You probably want to wrap the method instead of its body to make sure call sites are not accessing it in the disabled inspector mode.

> Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.cpp:461
>  void LayoutTestController::showWebInspector()

Is there a way to hide methods altogether instead of commenting out their bodies and wrap the call sites? Call site should not mention "inspector" when ENABLE(INSPECTOR) is off.

[Vivek Galatage]

Thank you Pavel for your review comments. I have incorporated these review comments except that for WebView.h I couldn't hide the method inspector() as its declared as pure virtual in IWebViewPrivate interface. 
Also I am not sure how to put something like ENABLE_INSPECTOR conditional statements in Windows IDL file to remove this pure virtual method from getting generated.

Rest of the comments have been incorporated and I will be uploading the patch now.

[Vivek Galatage]

Created attachment 136534 [details]
Patch

[WebKit Review Bot]

Comment on attachment 136534 [details]
Patch

Clearing flags on attachment: 136534

Committed r113836: <http://trac.webkit.org/changeset/113836>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This is breaking all the inspector tests in WebKit2:
<http://build.webkit.org/builders/Lion%20Debug%20%28WebKit2%20Tests%29?numbuilds=100>

[Anders Carlsson]

I'm rolling this out.

[Vivek Galatage]

Re-opening as the previous commit caused failures to inspector test cases.

[Vivek Galatage]

The issue lies in the way the LayoutTestController.idl file is processed.

The Tools/WebKitTestRunner/DerivedSources.make file while calling generate-bindings.pl doesn't pass any feature defines. Hence ENABLE_INSPECTOR is unavailable while parsing the LayoutTestController.idl file. As a result of this the Inspector related functionality is disabled in the JSLayoutTestController.h/cpp

Hence removing the changes in the LayoutTestController.idl and making the relevant changes in the LayoutTestController.cpp file.

Will upload the patch with these changes

[Vivek Galatage]

Created attachment 137126 [details]
Patch

[WebKit Review Bot]

Comment on attachment 137126 [details]
Patch

Clearing flags on attachment: 137126

Committed r114499: <http://trac.webkit.org/changeset/114499>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.