Bug 83557 - Web Inspector: Disabling Inspector causes build failure on Windows
Summary: Web Inspector: Disabling Inspector causes build failure on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Windows 7
: P2 Critical
Assignee: Vivek Galatage
URL:
Keywords:
Depends on: 83705
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-10 02:02 PDT by Vivek Galatage
Modified: 2012-04-18 05:27 PDT (History)
12 users (show)

See Also:


Attachments
Patch (7.26 KB, patch)
2012-04-10 02:30 PDT, Vivek Galatage
pfeldman: review-
pfeldman: commit-queue-
Details | Formatted Diff | Diff
Build error log file (41.94 KB, text/plain)
2012-04-10 04:13 PDT, Vivek Galatage
no flags Details
Patch (12.14 KB, patch)
2012-04-10 13:59 PDT, Vivek Galatage
no flags Details | Formatted Diff | Diff
Patch (10.49 KB, patch)
2012-04-13 12:35 PDT, Vivek Galatage
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vivek Galatage 2012-04-10 02:02:12 PDT
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
Comment 1 Vivek Galatage 2012-04-10 02:04:22 PDT
The calls to inspector APIs are not put under ENABLE(INSPECTOR) guard.
Comment 2 Pavel Feldman 2012-04-10 02:16:39 PDT
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.
Comment 3 Vivek Galatage 2012-04-10 02:24:29 PDT
Yes Pavel, this is related to the calls to InspectorController which are not guarded. 

Uploading the patch now.
Comment 4 Vivek Galatage 2012-04-10 02:30:38 PDT
Created attachment 136414 [details]
Patch
Comment 5 Vivek Galatage 2012-04-10 04:13:17 PDT
Created attachment 136424 [details]
Build error log file

Attached the build-log when the inspector is disabled.
Comment 6 Pavel Feldman 2012-04-10 04:29:30 PDT
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.
Comment 7 Vivek Galatage 2012-04-10 13:46:48 PDT
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.
Comment 8 Vivek Galatage 2012-04-10 13:59:10 PDT
Created attachment 136534 [details]
Patch
Comment 9 WebKit Review Bot 2012-04-11 01:49:38 PDT
Comment on attachment 136534 [details]
Patch

Clearing flags on attachment: 136534

Committed r113836: <http://trac.webkit.org/changeset/113836>
Comment 10 WebKit Review Bot 2012-04-11 01:49:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Simon Fraser (smfr) 2012-04-11 10:25:08 PDT
This is breaking all the inspector tests in WebKit2:
<http://build.webkit.org/builders/Lion%20Debug%20%28WebKit2%20Tests%29?numbuilds=100>
Comment 12 Anders Carlsson 2012-04-11 10:40:50 PDT
I'm rolling this out.
Comment 13 Vivek Galatage 2012-04-13 04:10:16 PDT
Re-opening as the previous commit caused failures to inspector test cases.
Comment 14 Vivek Galatage 2012-04-13 04:18:40 PDT
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
Comment 15 Vivek Galatage 2012-04-13 12:35:25 PDT
Created attachment 137126 [details]
Patch
Comment 16 WebKit Review Bot 2012-04-18 05:26:51 PDT
Comment on attachment 137126 [details]
Patch

Clearing flags on attachment: 137126

Committed r114499: <http://trac.webkit.org/changeset/114499>
Comment 17 WebKit Review Bot 2012-04-18 05:27:06 PDT
All reviewed patches have been landed.  Closing bug.