Summary: | Web Inspector: Disabling Inspector causes build failure on Windows | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Vivek Galatage <vivekgalatage> | ||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Vivek Galatage <vivekgalatage> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Critical | CC: | andersca, apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, simon.fraser, webkit.review.bot, yurys | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | Windows 7 | ||||||||||||
Bug Depends on: | 83705 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Vivek Galatage
2012-04-10 02:02:12 PDT
The calls to inspector APIs are not put under ENABLE(INSPECTOR) guard. 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. Yes Pavel, this is related to the calls to InspectorController which are not guarded. Uploading the patch now. Created attachment 136414 [details]
Patch
Created attachment 136424 [details]
Build error log file
Attached the build-log when the inspector is disabled.
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. 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. Created attachment 136534 [details]
Patch
Comment on attachment 136534 [details] Patch Clearing flags on attachment: 136534 Committed r113836: <http://trac.webkit.org/changeset/113836> All reviewed patches have been landed. Closing bug. This is breaking all the inspector tests in WebKit2: <http://build.webkit.org/builders/Lion%20Debug%20%28WebKit2%20Tests%29?numbuilds=100> I'm rolling this out. Re-opening as the previous commit caused failures to inspector test cases. 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 Created attachment 137126 [details]
Patch
Comment on attachment 137126 [details] Patch Clearing flags on attachment: 137126 Committed r114499: <http://trac.webkit.org/changeset/114499> All reviewed patches have been landed. Closing bug. |