Uh oh!
There was an error while loading. Please reload this page.
[wasm][debugger] Run getter using Runtime.GetProperties - #62857
Conversation
…s it's done by chrome.
ghost
commented
Dec 15, 2021
Tagging subscribers to this area: @thaystg Issue DetailsImplementing support on running getters using Runtime.GetProperties as it's done by CDP. Behavior on VS (not related to this PR) Bahavior on Chrome (using this PR)
|
| { | ||
| public string Scheme { get; } | ||
| public string Value { get; } | ||
| public string SubScheme { get; } |
There was a problem hiding this comment.
I think it would be useful to add a deconstructor which returns int values?
https://docs.microsoft.com/en-us/dotnet/csharp/fundamentals/functional/deconstruct#user-defined-types
public void Deconstruct(out string scheme, out int value, out string? subscheme, out int? subvalue)
{..}
.. and use it as (example):
var(_,value,subscheme, _)=dotnetObjectId;int value - so the callers don't have to worry about int.Parse failing. And IIUC, the string value should always parse as an int.
And int? subValue as a null` value would indicate a missing value.
There was a problem hiding this comment.
Also, we should maybe use an enum for this, even if it has only one value MethodId to start with. That would avoid callers having to check for == "methodId", and having any typos.
| SubValue = subvalue; | ||
| } | ||
| public override string ToString() => $"dotnet:{Scheme}:{Value}"; |
| } | ||
| public DotnetObjectId(string scheme, string value) | ||
| public DotnetObjectId(string scheme, string value, string subscheme = "", string subvalue = "") |
There was a problem hiding this comment.
wouldn't null be better default values for these?
| public async Task<JObject> InvokeMethodInObject(int objectId, int methodId, string varName, CancellationToken token) | ||
| { | ||
| using var commandParamsObjWriter = new MonoBinaryWriter(); | ||
| commandParamsObjWriter.Write(ElementType.Class, objectId); |
| GetJ = TGetter("GetJ"), | ||
| GetI = TGetter("GetI"), | ||
| GetK = TGetter("GetK") | ||
| GetJ = TGetter("GetJ", TNumber(20)), |
There was a problem hiding this comment.
Should other tests with TGetter also be updated, like the datetime ones?
| string[] parts = id.Split(":", 3); | ||
| string[] parts = id.Split(":", 5); | ||
| if (parts.Length < 3) |
There was a problem hiding this comment.
Please, change to:
if (parts.Length < 3 || parts.Length == 4)
because when running DebuggerTests.ArrayTests.InvalidArrayId we are passing 4 part ID:
dotnet:array:{ "arrayId": 234980 }.
After split we are left with 4 elements so the condition is false and we are reaching for non-existing element: parts[4] in the following lines. Without this change the test is failing.
There was a problem hiding this comment.
I would suggest doing something like https://github.com/dotnet/runtime/blob/release/5.0/src/mono/wasm/runtime/library_mono.js#L508-L512 . 3 parts, dotnet, scheme, and the rest. How to parse the rest part would depend on the scheme.
Implementing support on running getters using Runtime.GetProperties as it's done by CDP.
Behavior on VS (not related to this PR)
Bahavior on Chrome (using this PR)
Fixes#62774