Bug 180731

<rdar://problem/36298748>

Created attachment 349493 [details] [PATCH] WIP I'm going to wait on BigInt being enabled by default in JavaScriptCore but here is some initial work that gets us most of the way there. • BigInt RemoteObject support ✔︎ • BigInt TypeProfiler support ✔︎ • BigInt CodeMirror support ✔︎ (via <https://github.com/codemirror/CodeMirror/pull/5411>) • BigInt Esprima support (not yet implemented upstream, might want to implement it) • Inspector tests for BigInt (awaiting feature getting enabled by default)

Comment on attachment 349493 [details] [PATCH] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=349493&action=review > Source/WebInspectorUI/UserInterface/External/CodeMirror/javascript.js:115 > + } else if (ch == "0" && stream.match(/^(?:x[\da-f]+|o[0-7]+|b[01]+)n?/i)) { I thought we tried to avoid editing CodeMirror, since any additions we make may get overridden in the future? Is there a GitHub issue to go along with this?

(In reply to Devin Rousso from comment #3) > Comment on attachment 349493 [details] > [PATCH] WIP > > View in context: > https://bugs.webkit.org/attachment.cgi?id=349493&action=review > > > Source/WebInspectorUI/UserInterface/External/CodeMirror/javascript.js:115 > > + } else if (ch == "0" && stream.match(/^(?:x[\da-f]+|o[0-7]+|b[01]+)n?/i)) { > > I thought we tried to avoid editing CodeMirror, since any additions we make > may get overridden in the future? Is there a GitHub issue to go along with > this? See the previous comment: BigInt CodeMirror support (via <https://github.com/codemirror/CodeMirror/pull/5411>) I didn't want to update CodeMirror wholesale, but this is a cherry-picked commit in upstream that applies cleanly to our version.

Esprima support will be added in: https://bugs.webkit.org/show_bug.cgi?id=200796

Created attachment 376476 [details] [IMAGE] Console - ObjectView with BigInt

Created attachment 376477 [details] [IMAGE] Debugger - TypeProfiler with BigInt

Created attachment 376478 [details] [IMAGE] Debugger - Popover with BigInt

Created attachment 376479 [details] [IMAGE] Timelines - Heap Snapshot with BigInt

We can enable Web Inspector support for BigInt now to be ready for it if/when it does get enabled.

Created attachment 376480 [details] [PATCH] Proposed Fix

Created attachment 376538 [details] [PATCH] Proposed Fix

Created attachment 376578 [details] [PATCH] Proposed Fix - New Color

Created attachment 376579 [details] [IMAGE] Light Mode

Created attachment 376580 [details] [IMAGE] Dark Mode

Comment on attachment 376578 [details] [PATCH] Proposed Fix - New Color View in context: https://bugs.webkit.org/attachment.cgi?id=376578&action=review > LayoutTests/inspector/model/remote-object-expected.txt:153 > + "_value": "<filtered bigint>" Odd that the filter only happens here, and not for `456n` below 🤔 that seems like a bug > LayoutTests/inspector/model/resources/remote-object-utilities.js:24 > + return "<filtered bigint>"; Perhaps instead of "filtering" the bigint, we could `toString()` it? That way, we still get some identifiable/recognizable data in the output. > Source/JavaScriptCore/inspector/InjectedScriptSource.js:47 > + if (typeof obj === "bigint") Why not use the `isBigInt` you define below? > Source/JavaScriptCore/inspector/InjectedScriptSource.js:49 > Style: extra newline > Source/JavaScriptCore/inspector/InjectedScriptSource.js:912 > + delete this.value; I'd prefer we avoided using `delete`. Could we split this into the checks above? ``` // `undefined` and `BigInt` are not JSON serializable. if (this.type !== "undefined" && this.type !== "bigint") this.value = object; ... // Provide user-friendly number values. if (this.type === "number" || this.type === "bigint") this.description = toStringDescription(object) ``` > Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.cpp:190 > + JSBigInt* bigInt = asBigInt(cell); > + resultString = JSBigInt::tryGetString(vm, bigInt, 10); I'd just inline this, as it's pretty short. ``` resultString = JSBigInt::tryGetString(vm, asBigInt(cell), 10); ``` > Source/JavaScriptCore/inspector/protocol/Runtime.json:160 > + { "name": "isBigInt", "type": "boolean", "description": "Indicates if this type description has been type BigInt." } I wish all of these were optional :( > Source/JavaScriptCore/runtime/JSBigInt.cpp:63 > const ClassInfo JSBigInt::s_info = > - { "JSBigInt", nullptr, nullptr, nullptr, CREATE_METHOD_TABLE(JSBigInt) }; > + { "BigInt", nullptr, nullptr, nullptr, CREATE_METHOD_TABLE(JSBigInt) }; Style: most other files like this one have this on one line. > Source/JavaScriptCore/runtime/RuntimeType.cpp:83 > + if (type == TypeSymbol) > + return "Symbol"_s; Yikes! How long has this been missing? Actually, is this function even used by anything? > Source/WebInspectorUI/UserInterface/Images/TypeBigInt.svg:6 > + <path fill="hsl(192, 50%, 75%)" d="M 13 1 L 3 1 C 1.9 1 1 1.9 1 3 L 1 13 C 1 14.1 1.9 15 3 15 L 13 15 C 14.1 15 15 14.1 15 13 L 15 3 C 15 1.9 14.1 1 13 1 L 13 1 Z"/> > + <path fill="hsl(192, 48%, 45%)" d="M 13 1 L 3 1 C 1.9 1 1 1.9 1 3 L 1 13 C 1 14.1 1.9 15 3 15 L 13 15 C 14.1 15 15 14.1 15 13 L 15 3 C 15 1.9 14.1 1 13 1 M 13 2 C 13.552 2 14 2.449 14 3 L 14 13 C 14 13.552 13.552 14 13 14 L 3 14 C 2.449 14 2 13.552 2 13 L 2 3 C 2 2.449 2.449 2 3 2 L 13 2"/> > + <path fill="hsl(192, 40%, 50%)" d="M 7.7266 9.8457 L 7.7266 9.5417 C 7.7836 9.568701 7.8366 9.5987 7.8856 9.6337 C 7.9926 9.7117 8.0186 9.7607 8.025599 9.7997 C 7.9596 9.8147 7.8636 9.8317 7.7266 9.8457 M 7.9306 3.0457 L 5.0876 3.0457 L 4.0876 3.0457 L 4.0876 4.0457 L 4.0876 11.999701 L 4.0876 12.999701 L 5.0876 12.999701 L 8.190599 12.999701 C 9.2586 12.999701 10.1216 12.7637 10.7536 12.2987 C 11.4706 11.7637 11.8476 10.9817 11.8476 10.0337 C 11.8476 9.419701 11.6646 8.485701 10.7176 7.7477 C 11.3386 7.1267 11.5086 6.4107 11.5086 5.8137 C 11.5086 4.9037 11.1306 4.1627 10.4156 3.6727 C 9.8006 3.2507 8.987599 3.0457 7.9306 3.0457 M 6.7266 10.8717 L 7.1006 10.8717 C 7.8306 10.8717 8.3356 10.7967 8.6116 10.6477 C 8.8896 10.499701 9.027599 10.228701 9.027599 9.8347 C 9.027599 9.428699 8.841599 9.0927 8.4716 8.8237 C 8.0996 8.554701 7.6316 8.4207 7.0656 8.4207 L 6.7266 8.4207 L 6.7266 10.8717 M 6.7266 7.3567 L 7.1396 7.3567 C 7.6286 7.3567 8.0186 7.2417 8.3056 7.0127 C 8.5956 6.782701 8.7386 6.471701 8.7386 6.077701 C 8.7386 5.7117 8.6226 5.4637 8.3926 5.3337 C 8.1586 5.2027 7.7216 5.1377 7.0776 5.1377 L 6.7266 5.1377 L 6.7266 7.3567 M 7.9306 4.0457 C 8.770599 4.0457 9.4116 4.1967 9.850599 4.4977 C 10.2896 4.7987 10.5086 5.2377 10.5086 5.8137 C 10.5086 6.7707 9.8386 7.447701 8.501599 7.8457 C 10.0656 8.207701 10.8476 8.936701 10.8476 10.0337 C 10.8476 10.6677 10.6186 11.1527 10.1616 11.492701 C 9.7036 11.8297 9.0456 11.999701 8.190599 11.999701 L 5.0876 11.999701 L 5.0876 4.0457 L 7.9306 4.0457"/> We normally use `rgb` colors in our SVG images. I'm not against using `hsl` instead (I prefer `hsl`), but we should decide whether to be consistent with `rgb` or switch everything (or just new images) to `hsl`. > Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:71 > + console.assert(value === undefined); I'd use `this._value` since you've assigned `value` to it. > Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:72 > + console.assert(description.endsWith("n")); Ditto (71) with `this._description`. > Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:73 > + if (globalThis.BigInt) Given that Web Inspector is _always_ a web page, perhaps we should be consistent in using `window`? > Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:74 > + this._value = globalThis.BigInt(description.substring(0, description.length - 1)); Our normal style is to do a check with `window.XXX` and then just use it "normally" if true (e.g. `new XXX` or `XXX(...)`). > Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:95 > + console.assert(globalThis.BigInt); Ditto (73). > Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:98 > + return new WI.RemoteObject(undefined, undefined, "bigint", undefined, undefined, description, undefined, undefined, undefined); It's cases like these that make me prefer using an `options = {}` style constructor (>_<) > Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:307 > + Style: extra newline > Source/WebInspectorUI/UserInterface/Views/HeapSnapshotInstanceDataGridNode.js:302 > + if (this._node.className === "BigInt" && globalThis.BigInt) { Ditto (RemoteObject.js:73). > Source/WebInspectorUI/UserInterface/Views/HeapSnapshotInstanceDataGridNode.js:306 > + let primitiveRemoteObject = WI.RemoteObject.fromPrimitiveValue(string); If `!window.BigInt`, won't this trigger the `console.assert(value === undefined);` in `WI.RemoteObject`'s constructor? > Source/WebInspectorUI/UserInterface/Views/Variables.css:106 > + --syntax-highlight-bigint-color: hsl(192, 100%, 38%); I personally think this is a bit too bright, and clashes with Symbol too much. I'd rather have `BigInt` be closer to `Number`, as that's the "closest" type. I think the "difference" between `String` and `RegExp` is a good example of this. > Source/WebInspectorUI/UserInterface/Views/Variables.css:286 > + --syntax-highlight-bigint-color: hsl(195, 100%, 85%); Ditto (106). > Source/WebInspectorUI/UserInterface/Views/Variables.css:-291 > - --syntax-highlight-regex-group-color: var(--text-color-gray-medium); Nice! The orange looks WAY better =D

Comment on attachment 376578 [details] [PATCH] Proposed Fix - New Color View in context: https://bugs.webkit.org/attachment.cgi?id=376578&action=review >> LayoutTests/inspector/model/remote-object-expected.txt:153 >> + "_value": "<filtered bigint>" > > Odd that the filter only happens here, and not for `456n` below 🤔 that seems like a bug This is the `value`. Previews are always strings. >> Source/JavaScriptCore/runtime/RuntimeType.cpp:83 >> + return "Symbol"_s; > > Yikes! How long has this been missing? Actually, is this function even used by anything? Doesn't appear to be used, might only have been for debugging. >> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:71 >> + console.assert(value === undefined); > > I'd use `this._value` since you've assigned `value` to it. I want to be checking the parameters that came in and not a potentially modified member variable. >> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:73 >> + if (globalThis.BigInt) > > Given that Web Inspector is _always_ a web page, perhaps we should be consistent in using `window`? I'd rather use globalThis since this is a feature of a JavaScript environment not a Web environment, but I can switch to window. >> Source/WebInspectorUI/UserInterface/Views/HeapSnapshotInstanceDataGridNode.js:306 >> + let primitiveRemoteObject = WI.RemoteObject.fromPrimitiveValue(string); > > If `!window.BigInt`, won't this trigger the `console.assert(value === undefined);` in `WI.RemoteObject`'s constructor? Ahh yes, I think I can remove the !BigInt check now that I support that in RemoteObject. >> Source/WebInspectorUI/UserInterface/Views/Variables.css:106 >> + --syntax-highlight-bigint-color: hsl(192, 100%, 38%); > > I personally think this is a bit too bright, and clashes with Symbol too much. I'd rather have `BigInt` be closer to `Number`, as that's the "closest" type. I think the "difference" between `String` and `RegExp` is a good example of this. The entire reason for having a separate color is to not be very close to number, since we want to be able to quickly see when BigInt and Number are being used nearby, since that may result in TypeErrors. >> Source/WebInspectorUI/UserInterface/Views/Variables.css:-291 >> - --syntax-highlight-regex-group-color: var(--text-color-gray-medium); > > Nice! The orange looks WAY better =D This was just removed because it was the same value in light mode. This is for regex groups.

Created attachment 376703 [details] [PATCH] Proposed Fix

Comment on attachment 376703 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=376703&action=review r=me, nice work! > Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:76 > + this._value = `${description} [BigInt Not Enabled in Web Inspector]`; Should "Not Enabled" be capitalized? Wouldn't this always be shown for WK1? If so, perhaps we should just leave the `_value` as a string.

> > Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:76 > > + this._value = `${description} [BigInt Not Enabled in Web Inspector]`; > > Should "Not Enabled" be capitalized? Wouldn't this always be shown for WK1? > If so, perhaps we should just leave the `_value` as a string. It'll be the case in WK1 until we enable BigInt by default. This `value` doesn't really get seen or used anywhere so this isn't a problem. Maybe we don't even create it at all. I don't believe this hits WK1 tests because it is using the same process as the DumpRenderTree which gets the JSC::Option enabled.

Comment on attachment 376703 [details] [PATCH] Proposed Fix Clearing flags on attachment: 376703 Committed r248898: <https://trac.webkit.org/changeset/248898>

All reviewed patches have been landed. Closing bug.

This caused inspector/timeline/line-column.html to fail on the bots, but it looks like it may just be a rebaseline: --- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/inspector/timeline/line-column-expected.txt +++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/inspector/timeline/line-column-actual.txt @@ -45,7 +45,7 @@ "functionName": "", "url": "", "scriptId": "<filtered>", - "lineNumber": 138, + "lineNumber": 141, "columnNumber": 97 } ], @@ -107,7 +107,7 @@ "functionName": "", "url": "", "scriptId": "<filtered>", - "lineNumber": 138, + "lineNumber": 141, "columnNumber": 97 } ], @@ -160,7 +160,7 @@ "functionName": "", "url": "", "scriptId": "<filtered>", - "lineNumber": 138, + "lineNumber": 141, "columnNumber": 97 } ], @@ -220,7 +220,7 @@ "functionName": "", "url": "", "scriptId": "<filtered>", - "lineNumber": 138, + "lineNumber": 141, "columnNumber": 97 } ],

(In reply to Ryan Haddad from comment #25) > This caused inspector/timeline/line-column.html to fail on the bots, but it > looks like it may just be a rebaseline: Rebaselined in https://trac.webkit.org/changeset/248908/webkit

(In reply to Ryan Haddad from comment #26) > (In reply to Ryan Haddad from comment #25) > > This caused inspector/timeline/line-column.html to fail on the bots, but it > > looks like it may just be a rebaseline: > > Rebaselined in https://trac.webkit.org/changeset/248908/webkit Thanks!