[webkit-reviews] review denied: [Bug 93865] Break on inline scripts blocked by CSP. : [Attachment 158271] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Mike West
<mkwst at chromium.org>'s request for review:
Bug 93865: Break on inline scripts blocked by CSP.
https://bugs.webkit.org/show_bug.cgi?id=93865

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=158271&action=review


Overall looks good.

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:582
> +	   breakProgram(InspectorFrontend::Debugger::Reason::Exception,
desc.release());

Reason::Exception should have exception in the aux data, while you only pass
plain text (see ScopeChainSidebarPane.js:83). I think you should create a new
reason "CSPViolation" and pass 0 as the aux data string (it does not add much
info). You would need to change ScriptsPanel around line 288 to format it
properly as well.

> Source/WebCore/inspector/InspectorDebuggerAgent.h:117
> +    virtual void scriptExecutionBlockedByCSP();

While you are here, could you move schedulePauseOnNextStatement,
cancelPauseOnNextStatement, breakProgram to above this line as well? They are a
part of the instrumentation API, but squeezed into the middle of the protocol
methods.

> Source/WebCore/page/ContentSecurityPolicy.cpp:598
> +    bool checkInlineAndReportViolation(CSPDirective*, const String&
consoleMessage, const String& contextURL, const WTF::OrdinalNumber&
contextLine, bool isInlineScript = true) const;

checkInline receiving isInlineScript confuses me. Leaving this up to abarth at .