Bug 109402 – Web Inspector: JavaScript execution disabled by browser/UA should be notified to the front-end

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

[Vivek Galatage]

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.

[Vivek Galatage]

Created attachment 187516 [details]
Patch

[Yury Semikhatsky]

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.

[Vivek Galatage]

Created attachment 187524 [details]
Patch

[Vivek Galatage]

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.

[Build Bot]

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

[Vivek Galatage]

(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?

[Yury Semikhatsky]

(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?

[Yury Semikhatsky]

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.

[Vivek Galatage]

Created attachment 187777 [details]
Patch

[WebKit Review Bot]

Comment on attachment 187777 [details]
Patch

Clearing flags on attachment: 187777

Committed r142654: <http://trac.webkit.org/changeset/142654>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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

[Vivek Galatage]

(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. :-)

[Vsevolod Vlasov]

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