[webkit-reviews] review denied: [Bug 80992] Web Inspector: provide a way to reload page with given script preprocessor. : [Attachment 167091] Patch

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has denied Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 80992: Web Inspector: provide a way to reload page with given script
preprocessor.
https://bugs.webkit.org/show_bug.cgi?id=80992

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

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=167091&action=review


> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:107
> +	   RecursionScopeSuppression suppressionScope;

Please use MicrotaskSuppression instead.

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:149
> +    v8::Isolate* m_utilityIsolate;

You don't seem to use the isolate anymore, please remove this.

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:447
> +	       if (!m_scriptPreprocessor)

You need to protect against evals in the preprocessor script that would trigger
recursive BeforeCompile events.

> Source/WebCore/bindings/v8/ScriptDebugServer.h:109
> +    ~ScriptDebugServer();

Please make destructor virtual.

> Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:99
> +    v8::TryCatch tryCatch;

Please add tryCatch.SetVerbose(true) so that the errors are propagated to the
console.

> Source/WebCore/inspector/InspectorDebuggerAgent.h:80
> +    virtual void didClearMainFrameWindowObject();

This method should probably be moved to PageDebuggerAgent completely as there
is no such thing as main frame in worker.

> LayoutTests/inspector/debugger/debugger-script-preprocessor.html:23
> +		   return script + "//@ sourceURL=window_should_not_be_there";

Please add FAIL word somewhere in this line to make it clear that it is a
failure case.