Bug 109402

Summary: Web Inspector: JavaScript execution disabled by browser/UA should be notified to the front-end
Product: WebKit Reporter: Vivek Galatage <vivekg>
Component: Web Inspector (Deprecated)Assignee: Vivek Galatage <vivekg>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, buildbot, graouts, joepeck, keishi, loislo, ossy, pfeldman, pmuellr, rniwa, timothy, vsevik, web-inspector-bugs, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Vivek Galatage 2013-02-10 23:03:33 PST
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.
Comment 1 Vivek Galatage 2013-02-10 23:08:53 PST
Created attachment 187516 [details]
Patch
Comment 2 Yury Semikhatsky 2013-02-10 23:47:58 PST
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.
Comment 3 Vivek Galatage 2013-02-11 01:13:10 PST
Created attachment 187524 [details]
Patch
Comment 4 Vivek Galatage 2013-02-11 01:23:54 PST
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 5 Build Bot 2013-02-11 04:38:07 PST
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
Comment 6 Vivek Galatage 2013-02-11 21:12:02 PST
(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?
Comment 7 Yury Semikhatsky 2013-02-11 22:16:14 PST
(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 8 Yury Semikhatsky 2013-02-11 22:22:38 PST
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.
Comment 9 Vivek Galatage 2013-02-11 23:13:32 PST
Created attachment 187777 [details]
Patch
Comment 10 WebKit Review Bot 2013-02-12 12:47:23 PST
Comment on attachment 187777 [details]
Patch

Clearing flags on attachment: 187777

Committed r142654: <http://trac.webkit.org/changeset/142654>
Comment 11 WebKit Review Bot 2013-02-12 12:47:27 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Csaba Osztrogon√°c 2013-02-12 14:05:55 PST
(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
Comment 13 Vivek Galatage 2013-02-12 16:17:38 PST
(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 14 Vsevolod Vlasov 2013-02-13 03:03:22 PST
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