[webkit-reviews] review denied: [Bug 55775] Web Inspector: introduce RuntimeAgent::evaluateAsJSON, implement completion, audits and image hover on its basis. : [Attachment 89008] [PATCH] Function detection comment addressed

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 55775: Web Inspector: introduce RuntimeAgent::evaluateAsJSON, implement
completion, audits and image hover on its basis.
https://bugs.webkit.org/show_bug.cgi?id=55775

Attachment 89008: [PATCH] Function detection comment addressed
https://bugs.webkit.org/attachment.cgi?id=89008&action=review

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

> Source/WebCore/bindings/js/JSInjectedScriptHostCustom.cpp:121
> +	   ASSERT_UNUSED(objectError, objectError.isEmpty());

Why ASSERT_UNUSED?

> Source/WebCore/bindings/js/ScriptValue.cpp:150
> +	   if (getCallData(value, callData) != CallTypeNone) {

You should mimic JSON behavior and silently skip functions or call the original
method other than evaluateAsJSON. Rationale: others can add functions to your
prototype.

> Source/WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:107
> +    ASSERT_UNUSED(objectError, objectError.isEmpty());

ditto

> Source/WebCore/inspector/InjectedScript.cpp:212
> +	       String resultMessage = "Failed to construct resulting value";

You should already have a valid error message, result should be empty.

> Source/WebCore/inspector/InjectedScript.cpp:218
> +	   *result = InspectorString::create("Exception while making a call");

ditto. You should not touch result in case or error.

> Source/WebCore/inspector/InjectedScriptSource.js:253
> +	       return false;

Silently ignoring user exceptions is bad, r- is primarily for this. Please wrap
the error as it is done in the other evaluates.

> Source/WebCore/inspector/InspectorRuntimeAgent.cpp:62
> +    InjectedScript injectedScript =
m_injectedScriptManager->injectedScriptFor(getDefaultInspectedState());

This way it won't work for iframes where you probably want it to work. You
might want to move this method to DOM agent and pass context node for injected
script lookup.