* SUMMARY Separate Debugger enable state from being attached. Currently JSC assumes if a JSC::Debugger is attached the debugger is enabled. We are using the JSC::Debugger to access more debugger properties and we may want a debugger to be attached but disabled.
<rdar://problem/23867520>
This might be less important after some recent changes Saam made to allow the page to inline and use the FTL JIT when the debugger is attached.
(In reply to comment #2) > This might be less important after some recent changes Saam made to allow > the page to inline and use the FTL JIT when the debugger is attached. I'm rolling out my inlining patch. I think this is still important. I'm going to start work on the JSC bits of this.
Created attachment 274804 [details] patch
Comment on attachment 274804 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=274804&action=review Looks good to me from an inspector perspective. > Source/JavaScriptCore/debugger/ScriptProfilingScope.h:58 > - if (!m_globalObject->hasDebugger()) > + if (!m_globalObject->hasInteractiveDebugger()) > return false; This should just stay hasDebugger. The Sampling Profiler just needs globalObject->debugger(), it doesn't care if breakpoints are available or not. > Source/JavaScriptCore/debugger/ScriptProfilingScope.h:77 > // Debugger may have been removed. > - if (!m_globalObject->hasDebugger()) > + if (!m_globalObject->hasInteractiveDebugger()) > return false; Same here. I think as a result of doing this you can now remove the FIXMEs in LayoutTests/inspector/script-profiler tests. Namely things that look like: // FIXME: <https://webkit.org/b/152193> Web Inspector: Separate Debugger enable state from being attached // Debugger should not need to be enabled for profiling to work. InspectorProtocol.sendCommand("Debugger.enable", {});
Comment on attachment 274804 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=274804&action=review >> Source/JavaScriptCore/debugger/ScriptProfilingScope.h:58 >> return false; > > This should just stay hasDebugger. The Sampling Profiler just needs globalObject->debugger(), it doesn't care if breakpoints are available or not. Makes sense. I'll revert this.
Comment on attachment 274804 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=274804&action=review >> Source/JavaScriptCore/debugger/ScriptProfilingScope.h:77 >> return false; > > Same here. > > I think as a result of doing this you can now remove the FIXMEs in LayoutTests/inspector/script-profiler tests. Namely things that look like: > > // FIXME: <https://webkit.org/b/152193> Web Inspector: Separate Debugger enable state from being attached > // Debugger should not need to be enabled for profiling to work. > InspectorProtocol.sendCommand("Debugger.enable", {}); r- to update these tests, because after all they reference this bugzilla bug! script-profiler/event-type-API.html 39: // FIXME: <https://webkit.org/b/152193> Web Inspector: Separate Debugger enable state from being attached script-profiler/event-type-Microtask.html 40: // FIXME: <https://webkit.org/b/152193> Web Inspector: Separate Debugger enable state from being attached script-profiler/event-type-Other.html 57: // FIXME: <https://webkit.org/b/152193> Web Inspector: Separate Debugger enable state from being attached
Created attachment 274854 [details] patch
Comment on attachment 274854 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=274854&action=review r=me > Source/JavaScriptCore/debugger/Debugger.cpp:535 > void Debugger::setBreakpointsActivated(bool activated) > { > + bool stateChanged = activated != m_breakpointsActivated; > m_breakpointsActivated = activated; > + if (stateChanged) > + recompileAllJSFunctions(); > } I think we would normally write this with an early return if the state is not changing: if (m_breakpointsActivated == activated) return; m_breakpointsActivated = activated; recompileAllJSFunctions();
landed in: http://trac.webkit.org/changeset/198648