[webkit-reviews] review denied: [Bug 109402] Web Inspector: JavaScript execution disabled by browser/UA should be notified to the front-end : [Attachment 187516] Patch

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has denied Vivek Galatage
<vivekg at webkit.org>'s request for review:
Bug 109402: Web Inspector: JavaScript execution disabled by browser/UA should
be notified to the front-end
https://bugs.webkit.org/show_bug.cgi?id=109402

Attachment 187516: Patch
https://bugs.webkit.org/attachment.cgi?id=187516&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
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.