[webkit-reviews] review denied: [Bug 136542] Web Inspector: breakpoint actions should work regardless of Content Security Policy : [Attachment 237664] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied Matt Baker <mattbaker at apple.com>'s
request for review:
Bug 136542: Web Inspector: breakpoint actions should work regardless of Content
Security Policy
https://bugs.webkit.org/show_bug.cgi?id=136542

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

------- Additional Comments from Mark Lam <mark.lam at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=237664&action=review


> Source/JavaScriptCore/ChangeLog:9
> +	   JSGlobalObject for the duration of scope, returning the eval enabled
state to its

/duration of scope/duration of a scope/

> Source/JavaScriptCore/ChangeLog:11
> +	   to allow breakpoint actions to execute JS in pages with CSP that
would normally

Would you mind spelling out Content Security Policy instead of the CSP
abbreviation?  Thanks.

> Source/JavaScriptCore/ChangeLog:14
> +	   Refactored Inspector::InjectedScriptBase to use RAII object instead
of manually

/use RAII/use the RAII/

> Source/JavaScriptCore/ChangeLog:15
> +	   setting/resetting eval enabled state.

/...ing eval enabled/...ing the eval enabled/

> Source/JavaScriptCore/debugger/DebuggerEvalEnabler.h:46
> +    explicit DebuggerEvalEnabler(const ExecState* exec)
> +	   : m_globalObject(nullptr)
> +	   , m_evalEnabled(false)
> +    {
> +	   if (exec) {
> +	       m_globalObject = exec->lexicalGlobalObject();
> +	       m_evalEnabled = m_globalObject->evalEnabled();
> +	   }
> +	   if (m_globalObject && !m_evalEnabled)
> +	       m_globalObject->setEvalEnabled(true,
m_globalObject->evalDisabledErrorMessage());
> +    }

I suggest renaming m_evalEnabled to m_evalWasEnabled to indicate that it is
referring to the original state.  Actually, I think you should use
m_evalwasDisabled instead because that would remove the need for a lot of
negation in the code (making the logic simpler).

I think it's weird that the exec can be NULL, but that appears to be a due to
InjectedScriptBase getting the exec from ScriptObject::scriptState() which can
currently be NULL.  I suggest adding a comment in the ChangeLog to document
this so that we'll have a record of why this NULL check exists, and can remove
it in the future if we fix ScriptObject::scriptState() to never be NULL (at
least for the duration when the DebuggerEvalEnabler is called).

Why not just cache the ExecState, and do a conditional on that?  I suggest the
following:

    explicit DebuggerEvalEnabler(const ExecState* exec)
	: m_exec(exec)
	, m_evalWasDisabled(false)
    {
	if (exec) {
	    JSGlobalObject* globalObject = exec->lexicalGlobalObject();
	    m_evalWasDisabled = !m_globalObject->evalEnabled();
	    if (m_evalWasDisabled)
		globalObject->setEvalEnabled(true,
globalObject->evalDisabledErrorMessage());
	}
    }

    ~DebuggerEvalEnabler()
    {
	if (m_evalWasDisabled) {
	    ASSERT(m_exec);
	    JSGlobalObject* globalObject = exec->lexicalGlobalObject();
	    globalObject()->setEvalEnabled(false,
globalObject->evalDisabledErrorMessage());
	}
    }

I think the code is a little simpler this way.