Steps: 1. Open the page with Inspector/DevTools open. 2. Open "Settings Panel" with "General" tab visible. See the "Disable JavaScript" setting. 3. From the Browser/UA (QtTestBrowser etc.), disable the JavaScript. Expected outcome: "Disable JavaScript" setting reflecting the action performed by the UA/Browser. Actual outcome: "Disable JavaScript" remains unchanged Patch follows.
Created attachment 187516 [details] Patch
Comment on attachment 187516 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187516&action=review > Source/WebCore/inspector/InspectorPageAgent.cpp:817 > + m_state->setBoolean(PageAgentState::scriptExecutionStateChangeNotifierDisabled, true); You should use a field in the agent instead as you never need to pass the state of the option to another instance of the agent. > Source/WebCore/page/Settings.cpp:419 > + InspectorInstrumentation::didScriptExecutionStateChange(m_page, m_isScriptEnabled); We use did prefix only in cases when there is matching will* method, for events like this I think scriptExecutionStateChange or shorter scriptsEnabled would be a better name. > LayoutTests/inspector/script-execution-state-change-notification.html:26 > + InspectorTest.runTestSuite([ Looks like the suite only complicates the code in this case. Rewriting it without the suite and passing callbacks directly to the InspectorTest.evaluateInPage calls would make the code clearer and less verbose, e.g.: function disableJavaScript(next) { function done(result) { next(); } InspectorTest.evaluateInPage("disableJavaScript();", done ); }, would become function disableJavaScript() { InspectorTest.evaluateInPage("disableJavaScript();", enableJavaScript); }, > LayoutTests/inspector/script-execution-state-change-notification.html:30 > + pageDispatcher.scriptExecutionStateChanged = function(isEnabled) Please use InspectorTest.addSniffer instead.
Created attachment 187524 [details] Patch
Comment on attachment 187516 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187516&action=review Thank you for the review. >> Source/WebCore/inspector/InspectorPageAgent.cpp:817 >> + m_state->setBoolean(PageAgentState::scriptExecutionStateChangeNotifierDisabled, true); > > You should use a field in the agent instead as you never need to pass the state of the option to another instance of the agent. Sure. I will make the corrections. >> Source/WebCore/page/Settings.cpp:419 >> + InspectorInstrumentation::didScriptExecutionStateChange(m_page, m_isScriptEnabled); > > We use did prefix only in cases when there is matching will* method, for events like this I think scriptExecutionStateChange or shorter scriptsEnabled would be a better name. Changed. >> LayoutTests/inspector/script-execution-state-change-notification.html:26 >> + InspectorTest.runTestSuite([ > > Looks like the suite only complicates the code in this case. Rewriting it without the suite and passing callbacks directly to the InspectorTest.evaluateInPage calls would make the code clearer and less verbose, e.g.: > > function disableJavaScript(next) > { > function done(result) > { > next(); > } > InspectorTest.evaluateInPage("disableJavaScript();", done ); > }, > > would become > > function disableJavaScript() > { > InspectorTest.evaluateInPage("disableJavaScript();", enableJavaScript); > }, Modified it according to the comments. Thank you. >> LayoutTests/inspector/script-execution-state-change-notification.html:30 >> + pageDispatcher.scriptExecutionStateChanged = function(isEnabled) > > Please use InspectorTest.addSniffer instead. Done Uploaded a new patch with all the comments incorporated.
Comment on attachment 187524 [details] Patch Attachment 187524 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16487264 New failing tests: inspector/audits/audits-panel-functional.html inspector/script-execution-state-change-notification.html
(In reply to comment #5) > New failing tests: > inspector/script-execution-state-change-notification.html There seems to be a bug in the TestRunner on mac platform as the call to testRunner.overridePreference("WebKitJavaScriptEnabled", true) has no effect. The scripts are disabled always ignoring the second parameter "true". I verified this change with safari and its working as expected. So shall we put this test case as skipped for mac and raise a bug for testRunner.overridePreference?
(In reply to comment #6) > (In reply to comment #5) > > New failing tests: > > inspector/script-execution-state-change-notification.html > > There seems to be a bug in the TestRunner on mac platform as the call to testRunner.overridePreference("WebKitJavaScriptEnabled", true) has no effect. The scripts are disabled always ignoring the second parameter "true". > This one should be easy to fix unless there were some reasons for not supporting it on Mac. > I verified this change with safari and its working as expected. > > So shall we put this test case as skipped for mac and raise a bug for testRunner.overridePreference?
Comment on attachment 187524 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187524&action=review > Source/WebCore/inspector/InspectorPageAgent.cpp:340 > + , m_enabledScriptsEnabledNotification(true) The name sounds a bit awkward, maybe rename it to something like m_ignoreScriptsEnabledNotification? > Source/WebCore/inspector/InspectorPageAgent.cpp:1038 > + if (!m_frontend || !m_enabledScriptsEnabledNotification) You don't need to check for m_frontend since control flow reaches this method iff there is a front-end and the agent is enabled.
Created attachment 187777 [details] Patch
Comment on attachment 187777 [details] Patch Clearing flags on attachment: 187777 Committed r142654: <http://trac.webkit.org/changeset/142654>
All reviewed patches have been landed. Closing bug.
(In reply to comment #10) > (From update of attachment 187777 [details]) > Clearing flags on attachment: 187777 > > Committed r142654: <http://trac.webkit.org/changeset/142654> and buildfix landed in https://trac.webkit.org/changeset/142669 for !ENABLE(INSPECTOR) platforms
(In reply to comment #12) > and buildfix landed in https://trac.webkit.org/changeset/142669 for !ENABLE(INSPECTOR) platforms Thank you Ossy, I will take care of this case hence forth. :-)
Comment on attachment 187777 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187777&action=review > LayoutTests/inspector/script-execution-state-change-notification.html:46 > + InspectorTest.addResult("Finished verification.") This line should have been before completeTest() call, I fixed it. (I admit it did work, but such a code does not make much sense) > LayoutTests/inspector/script-execution-state-change-notification.html:47 > + InspectorTest.evaluateInPage("completeTest();"); This was causing the next test to timeout. You didn't need to call testRunner.notifyDone(), this is done implicitly in InspectorTest.completeTest. Fixed in http://trac.webkit.org/changeset/142733