[webkit-reviews] review denied: [Bug 193149] Web Inspector: Audit: create new IDL type for exposing special functionality in test context : [Attachment 359007] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jan 14 13:51:26 PST 2019


Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 193149: Web Inspector: Audit: create new IDL type for exposing special
functionality in test context
https://bugs.webkit.org/show_bug.cgi?id=193149

Attachment 359007: Patch

https://bugs.webkit.org/attachment.cgi?id=359007&action=review




--- Comment #28 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 359007
  --> https://bugs.webkit.org/attachment.cgi?id=359007
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359007&action=review

r- just because I'd like to see an updated version given all of the things I've
pointed out below.

• The tests look good.
• I'm still not convinced that `AUDIT.Domain` is the best approach for an API
but lets proceed with it for now. I'd have preferred something like something
like "WebInspectorAudit" instead of "AUDIT".

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:1248
> +		91278D5821DEDA9600B57184 /* JSGlobalObjectAuditAgent.h in
Headers */ = {isa = PBXBuildFile; fileRef = 91278D5621DEDA9400B57184 /*
JSGlobalObjectAuditAgent.h */; settings = {ATTRIBUTES = (Private, ); }; };

This fix should not be Private, it isn't and won't be needed outside of
JavaScriptCore.

> Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:87
> +    for (auto& argument : runArguments(executionContextId))
> +	   m_injectedAUDITValue->putDirect(vm,
Identifier::fromString(execState, argument.first), argument.second);

I find the name `runArguments` to be very confusing. It sounds like an action
but it isn't. How about something like `populateAuditObject`. And then you
could just pass in the `m_injectedAUDITValue` instead of this weird and
wasteful intermediate Vector.

> Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:105
> +    StringBuilder functionString;
> +    functionString.appendLiteral("(function(");
> +    if (m_injectedAUDITValue)
> +	   functionString.appendLiteral("AUDIT");
> +    functionString.appendLiteral(") { \"use strict\"; return eval(`(");
> +    functionString.append(test.isolatedCopy().replace('`', "\\`"));
> +    functionString.appendLiteral(")`)(");
> +    if (m_injectedAUDITValue)
> +	   functionString.appendLiteral("AUDIT");
> +    functionString.appendLiteral("); })");

It might be nice to include a comment before this to show what it is doing in a
less confusing way:

    // Perform the following:
    //
    //	   (function(AUDIT) {
    //	       "use strict";
    //	       return eval(`(<escaped-test>)`(AUDIT);
    //	   })

Is there any reason to not include "AUDIT" always? It would just be `undefined`
if not provided below. I'd drop the `m_injectedAUDITValue` branches up here.

> Source/JavaScriptCore/inspector/agents/JSGlobalObjectAuditAgent.h:36
> +class JSGlobalObjectAuditAgent final : public InspectorAuditAgent {

I still think we should remove Audits from being available in JSContexts and
Workers. Maybe it would work with a ESLint style audit, but in general I think
it is not particularly useful there and would just confuse JSContext inspector
users. That would eliminate quite a bit of this new code in JavaScriptCore.
Note we should update the frontend to also not allow the Audits tab for
JSContexts.

> Source/JavaScriptCore/inspector/protocol/Audit.json:22
> +	       "name": "run",
> +	       "description": "Parses and evaluates the given test string and
sends back the result.",
> +	       "parameters": [
> +		   { "name": "test", "type": "string", "description": "Test
string to parse and evaluate." },
> +		   { "name": "contextId", "$ref": "Runtime.ExecutionContextId",
"optional": true, "description": "Specifies in which isolated context to run
the test. Each content script lives in an isolated context and this parameter
may be used to specify one of those contexts. If the parameter is omitted or 0
the evaluation will be performed in the context of the inspected page." }
> +	       ],
> +	       "returns": [
> +		   { "name": "result", "$ref": "Runtime.RemoteObject",
"description": "Evaluation result." },
> +		   { "name": "wasThrown", "type": "boolean", "optional": true,
"description": "True if the result was thrown during the evaluation." }
> +	       ]

It may be important to point out that this puts results into the "audit" object
group. Which the frontend would need to release in order to free up memory from
audits (or navigate the page) and thereby clear the entire injected script.

> Source/JavaScriptCore/inspector/protocol/Audit.json:26
> +	       "description": ""

This could use a description. It seems like this may be necessary to balance
`setup` sometimes.

> Source/WebCore/inspector/InspectorAuditAccessibilityUtilities.h:42
> +    explicit InspectorAuditAccessibilityUtilities();

Can you get away with `= default;` and drop the cpp file?

> Source/WebCore/inspector/WorkerInspectorController.cpp:181
> +    m_agents.append(std::make_unique<WorkerAuditAgent>(workerContext));

Ditto RE not including an Audit Agent in Workers unless there is a clear use
case.

> Source/WebCore/inspector/agents/page/PageAuditAgent.cpp:66
> +	   if (executionContextId)
> +	       errorString = "Internal error: main world execution context not
found."_s;

This should be `if (!executionContextId)` if the error is about the main world
execution context.

> Source/WebCore/inspector/agents/page/PageAuditAgent.cpp:87
> +#define ADD_AUDIT_UTILITIES(name) \
> +		   if (!m_audit##name##Utilities) \
> +		       m_audit##name##Utilities =
InspectorAudit##name##Utilities::create(*this); \
> +		   if (JSC::JSValue inspectorAudit##name##Utilities =
toJS(execState, globalObject, *m_audit##name##Utilities)) \
> +		       arguments.append(std::make_pair("" #name ""_s,
WTFMove(inspectorAudit##name##Utilities)));

I'd prefer an "Object" suffix to a "Utilities" suffix. Meaning the name
"InspectorAudit<Foo>Object" instead of "InspectorAudit<Foo>Utilities".

We already have this pattern already in JavaScriptCore for the C++ backend to
some JavaScript "Object":

    ./JavaScriptCore/runtime/SymbolObject.h
    ./JavaScriptCore/runtime/IntlObject.h
    ./JavaScriptCore/runtime/InspectorInstrumentationObject.h
    ./JavaScriptCore/runtime/ReflectObject.h
    ./JavaScriptCore/runtime/BigIntObject.h
    ./JavaScriptCore/runtime/ConsoleObject.h
    ./JavaScriptCore/runtime/StringObject.h
    ./JavaScriptCore/runtime/JSONObject.h
    ./JavaScriptCore/runtime/MathObject.h
    ...

In contrast to "Utilities" files which are often collections of functions that
may be used to work with something:

    ./WebCore/page/Base64Utilities.h
    ./WebCore/platform/win/GDIUtilities.h
    ./WebCore/platform/network/mac/UTIUtilities.h
    ./WebCore/platform/animation/AnimationUtilities.h
    ./WebCore/platform/ios/wak/WKUtilities.h
    ./WebCore/platform/mac/StringUtilities.h
    ...

This falls more cleanly into the "Object" suffix bucket.

> Source/WebCore/inspector/agents/page/PageAuditAgent.cpp:111
> +void PageAuditAgent::muteConsole()
> +{
> +    InspectorAuditAgent::muteConsole();
> +
> +    PageConsoleClient::mute();
> +}
> +
> +void PageAuditAgent::unmuteConsole()
> +{
> +    PageConsoleClient::unmute();
> +
> +    InspectorAuditAgent::unmuteConsole();
> +}

Style: I'd remove the inner newlines and make these as compact as possible.

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:239
> +	       agentCommandArguments.objectGroup = "audit";

Do we ever clear the `audit` object group anywhere? Does the frontend
clear/trash results anywhere?


More information about the webkit-reviews mailing list