In order to provide special functionality within Audit tests, we need a way to expose references to these functions without having them be visible when not running an Audit.
<rdar://problem/46801218>
Created attachment 358347 [details] Patch
(In reply to Devin Rousso from comment #2) > Created attachment 358347 [details] > Patch One thing I was thinking about was instead of only providing `AUDIT` when running within a Page, we could always provide `AUDIT` as an empty object and instead make each of the sub-objects have a separate IDL (e.g. `InspectorAuditDOMUtitlities`, `InspectorAuditAccessibilityUtilities`, etc), adding them to the `AUDIT` object as necessary (e.g. Page vs Worker vs JSContext). I don't feel strongly either way, other than having more IDL this way. Another idea I'd pondered with was to skip the whole IDL idea altogether and just manually create the `AUDIT` object and sub-objects, providing the functions as `JSNativeStdFunction`. The downside of this approach is that we lose the "free" bits of the IDL generator (namely the type checking and conversions).
Comment on attachment 358347 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358347&action=review > Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:76 > + String functionString = makeString("(function(", argumentsString, ") { \"use strict\"; return eval(", test, ")(); })"); Why commas around `test` in the eval? > Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:98 > + return { }; Should JSContexts even have an AuditAgent if we aren't going to expose an AUDIT object to it? > Source/JavaScriptCore/inspector/protocol/Audit.json:4 > + "domain": "Audit", > + "description": "", > + "commands": [ Does this make sense to expose to a "javascript", "worker", or "service-worker" context? I think it makes sense to restrict Audits to a "page" right now. > Source/WebCore/inspector/InspectorController.cpp:152 > + m_agents.append(std::make_unique<PageAuditAgent>(pageContext)); This should go under `createLazyAgents` since there is no need for it to be alive before a frontend connects. > Source/WebCore/inspector/WorkerInspectorController.cpp:81 > + m_agents.append(std::make_unique<WorkerAuditAgent>(workerContext)); Ditto: `createLazyAgents` > Source/WebCore/inspector/agents/page/PageAuditAgent.cpp:86 > + inspectorAuditUtilities = toJSNewlyCreated(execState, globalObject, InspectorAuditUtilities::create()); We could avoid creating an AUDIT object each time. For example, InjectedScriptHost is created once per target. (See calls to InjectedScriptHost::create). > Source/WebInspectorUI/UserInterface/Protocol/Target.js:55 > + console.assert(this.AuditAgent); We wouldn't have an AuditAgent in every target (an older iOS target for example). > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:62 > + WI.auditManager.addEventListener(WI.AuditManager.Event.TestScheduled, this._handleAuditManagerTestScheduled, this); > + WI.auditManager.addEventListener(WI.AuditManager.Event.TestCompleted, this._handleAuditManagerTestCompleted, this); This sounds worth doing separate from this patch, right?
Comment on attachment 358347 [details] Patch Attachment 358347 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10631644 New failing tests: inspector/canvas/create-context-bitmaprenderer.html inspector/canvas/recording-webgl-snapshots.html inspector/canvas/recording-2d.html inspector/canvas/recording-webgl.html inspector/model/stack-trace.html inspector/canvas/create-context-webgl.html
Created attachment 358357 [details] Archive of layout-test-results from ews103 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 358347 [details] Patch Attachment 358347 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10631740 New failing tests: inspector/canvas/create-context-bitmaprenderer.html inspector/canvas/recording-webgl-snapshots.html inspector/canvas/recording-2d.html inspector/canvas/recording-webgl.html inspector/model/stack-trace.html inspector/canvas/create-context-webgl.html
Created attachment 358359 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 358347 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358347&action=review >> Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:76 >> + String functionString = makeString("(function(", argumentsString, ") { \"use strict\"; return eval(", test, ")(); })"); > > Why commas around `test` in the eval? This is adding the `test` variable to the `functionString`, not the literal string "test". One thing to note, however, is that there should be quotes around `test`. String functionString = makeString("(function(", argumentsString, ") { \"use strict\"; return eval(\"", test, "\")(); })"); >> Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:98 >> + return { }; > > Should JSContexts even have an AuditAgent if we aren't going to expose an AUDIT object to it? In the future there might be other things we'd want to make available via an Audit (e.g. promise resolution state) that could be useful when writing tests. I don't have a good reason to disallow Audits in non-Page contexts, especially considering the fact that there's nothing really preventing that from happening.
Comment on attachment 358347 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358347&action=review >> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:62 >> + WI.auditManager.addEventListener(WI.AuditManager.Event.TestCompleted, this._handleAuditManagerTestCompleted, this); > > This sounds worth doing separate from this patch, right? <https://webkit.org/b/193158> Web Inspector: Audit: disable breakpoints when running Audit
Comment on attachment 358347 [details] Patch Attachment 358347 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10632111 New failing tests: inspector/canvas/create-context-bitmaprenderer.html inspector/canvas/recording-webgl-snapshots.html inspector/canvas/recording-2d.html http/wpt/css/css-animations/start-animation-001.html inspector/canvas/recording-webgl.html inspector/model/stack-trace.html inspector/canvas/create-context-webgl.html
Created attachment 358375 [details] Archive of layout-test-results from ews116 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 358380 [details] Patch
Comment on attachment 358380 [details] Patch Attachment 358380 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10634054 New failing tests: inspector/canvas/recording-webgl-snapshots.html inspector/model/stack-trace.html inspector/canvas/recording-webgl.html
Created attachment 358394 [details] Archive of layout-test-results from ews100 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 358380 [details] Patch Attachment 358380 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10634283 New failing tests: inspector/canvas/recording-webgl-snapshots.html inspector/model/stack-trace.html inspector/canvas/recording-webgl.html
Created attachment 358400 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 358380 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358380&action=review > Source/JavaScriptCore/ChangeLog:39 > + Create a version of `evaluate` that accepts a list of arguemnts to be passed in as arguments arguemnts -> arguments
Comment on attachment 358380 [details] Patch Attachment 358380 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10634880 New failing tests: inspector/canvas/recording-webgl-snapshots.html inspector/model/stack-trace.html inspector/canvas/recording-webgl.html
Created attachment 358409 [details] Archive of layout-test-results from ews115 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 358422 [details] Patch
Created attachment 358462 [details] Patch Rework the injected `AUDIT` object to be a plain JavaScript object with special IDL subobjects. This way, we can always provide an `AUDIT` object, and can call the `AuditAgent` functions in such an order as to preserve the `AUDIT` object between tests, allowing for a simple way to share values between runs.
Created attachment 358467 [details] Patch
Created attachment 358843 [details] Patch Add a few extra functions for use by later patches
Comment on attachment 358843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358843&action=review > Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:102 > + functionString.appendLiteral(")`)(); })"); We should also add another "AUDIT" inside the `()` so that if the test function string defines `AUDIT` as the first parameter, it isn't overwritten.
Comment on attachment 358843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358843&action=review > Source/WebCore/inspector/agents/page/PageAuditAgent.cpp:85 > + m_audit##name##Utilities = InspectorAudit##name##Utilities::create(); \ Since both `InspectorAuditAccessibilityUtilites` and `InspectorAuditDOMUtilities` will passed `*this` (in order to check whether a test is currently running, and to throw an error if not), we should make that stub implementation now so the future patches don't have to worry about it.
Created attachment 359007 [details] Patch
Comment on attachment 359007 [details] 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?
Comment on attachment 359007 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359007&action=review > • 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". I like `WebInspectorAudit` over `AUDIT`. It seems bit more "normal", in that we rarely use all-caps except for constants. It's also a bit more "specific" than something general like "audit". >> Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:105 >> + 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. Good point. There's no real reason to not always supply it. >> Source/JavaScriptCore/inspector/protocol/Audit.json:26 >> + "description": "" > > This could use a description. It seems like this may be necessary to balance `setup` sometimes. Ah whoops 😅 >> Source/WebCore/inspector/InspectorAuditAccessibilityUtilities.h:42 >> + explicit InspectorAuditAccessibilityUtilities(); > > Can you get away with `= default;` and drop the cpp file? The cpp file is used by all of the blocked patches, so rather than have them worry about it, I felt that it made sense to include it in the base patch (this way I wouldn't have to worry about what patches landed in what order). >> 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? Ah, good point. I think it's safe to clear them before each time we call `Audit.run`.
Created attachment 359118 [details] Patch
Comment on attachment 359007 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359007&action=review >>> 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? > > Ah, good point. I think it's safe to clear them before each time we call `Audit.run`. Actually, since we have the "Results" folder, we shouldn't ever clear the object group, as we could potentially show a value at any point in the future. Clearing this will happen on page navigation (unless we add a button somewhere).
Comment on attachment 359118 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359118&action=review r=me! Very nice > Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:102 > + options.includeCommandLineAPI = true; Should we be including the command line API?! That means they would have `getEventListeners` and we wouldn't need our (future) DOM additions. I feel like we shouldn't include the command line API.
Comment on attachment 359118 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359118&action=review >> Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:102 >> + options.includeCommandLineAPI = true; > > Should we be including the command line API?! That means they would have `getEventListeners` and we wouldn't need our (future) DOM additions. I feel like we shouldn't include the command line API. Oh! I was trying to allow for `console.*` functions. My mistake.
Created attachment 359120 [details] Patch
Comment on attachment 359120 [details] Patch Attachment 359120 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10753530 New failing tests: inspector/audit/run.html
Created attachment 359124 [details] Archive of layout-test-results from ews103 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 359120 [details] Patch Attachment 359120 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10753677 New failing tests: inspector/audit/run.html
Created attachment 359132 [details] Archive of layout-test-results from ews116 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 359139 [details] Patch
Comment on attachment 359139 [details] Patch Clearing flags on attachment: 359139 Committed r239976: <https://trac.webkit.org/changeset/239976>
All reviewed patches have been landed. Closing bug.
It appears that the changes in https://trac.webkit.org/changeset/239976/webkit has caused inspector/model/remote-object.html to become a constant failure. History: http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fmodel%2Fremote-object.html Diff: --- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/inspector/model/remote-object-expected.txt +++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/inspector/model/remote-object-actual.txt @@ -1729,7 +1729,7 @@ { "_name": "stack", "_type": "string", - "_value": "splitText@[native code]\nglobal code\nevaluateWithSc…ension@[native code]\n\n_wrapCall" + "_value": "splitText@[native code]\nglobal code\nevaluateWithScopeExtension@[native code]\n\n_wrapCall" }, { "_name": "code", I confirmed this issue by reverting this revision locally and running the test. After the revert the test started passing again.
This looks like it could just be a rebaseline, could you confirm?
(In reply to Truitt Savell from comment #43) > This looks like it could just be a rebaseline, could you confirm? <https://trac.webkit.org/changeset/239990/webkit>