Bug 199746 – Add API to get all the dependencies of a given JSScript

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Add API to get all the dependencies of a given JSScript

[Keith Miller]

Created attachment 373997 [details]
Patch

[Keith Miller]

Created attachment 373998 [details]
Patch

[Keith Miller]

rdar://problem/51768389

[EWS Watchlist]

Attachment 373998 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2646:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2672:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2692:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2712:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2732:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 373998 [details]
Patch

Attachment 373998 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12723132

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases
apiTests

[Keith Miller]

Created attachment 374020 [details]
Patch

[EWS Watchlist]

Attachment 374020 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2646:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2672:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2692:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2712:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2732:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 374020 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:10
> +        are at the satify phase but for API simplicity we only provide

satify => satisfy

> Source/JavaScriptCore/ChangeLog:15
> +        This patch also fixes an issue where we would allow import specifiers
> +        that didn't start "./" or "/".

why is that bad? "foo" just just be "./foo", right?

> Source/JavaScriptCore/API/JSAPIGlobalObject.mm:72
> +static Expected<URL, String> computeValidImportSpecifier(const URL& base, const String& specifier)

"computeValidImportSpecifier" => "computeURL"?

> Source/JavaScriptCore/API/JSAPIGlobalObject.mm:79
> +    if (!specifier.startsWith('/') && !specifier.startsWith("./") && !specifier.startsWith("../"))
> +        return makeUnexpected(makeString("Module specifier: "_s, specifier, " does not start with \"/\", \"./\", or \"../\"."_s));

I don't see why this is necessary. See comment in changelog

> Source/JavaScriptCore/API/JSContext.mm:163
> +    JSC::JSArray* result = exec->lexicalGlobalObject()->moduleLoader()->dependencyKeysIfEvaluated(exec, JSC::jsString(&vm, [[script sourceURL] absoluteString]));
> +    if (!result) {

Don't you want an exception scope for this?

[Saam Barati]

Comment on attachment 374020 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:15
>> +        that didn't start "./" or "/".
> 
> why is that bad? "foo" just just be "./foo", right?

Please describe why we're doing this? E.g, to be compatible with web. And why the web does it

> Source/JavaScriptCore/ChangeLog:18
> +        Lastly, this patch makes it so that we copy all scripts in the API/tests/testapiScripts directory so
> +        they don't have to be individually added to the xcode project.

👍🏼

[Keith Miller]

Comment on attachment 374020 [details]
Patch

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

>> Source/JavaScriptCore/API/JSContext.mm:163
>> +    if (!result) {
> 
> Don't you want an exception scope for this?

Uhh, I don't think there's any non-internal exceptions that can be thrown right now but maybe it's good practice.

[Keith Miller]

Created attachment 374023 [details]
Patch for landing

[Keith Miller]

Committed r247403: <https://trac.webkit.org/changeset/247403>