-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LibURL+LibWeb: Add a stub for URLPattern.exec, along with some corresponding IDL generator fixes #3237
base: master
Are you sure you want to change the base?
LibURL+LibWeb: Add a stub for URLPattern.exec, along with some corresponding IDL generator fixes #3237
Conversation
832b358
to
2d50623
Compare
I've not looked through all of the changes yet, but looking at the change that adds The WebIDL spec says this (from 2.7. Dictionaries):
|
right, from the specs point of view it is not observable and is the editorial convention they chose. The alternative is to do some tricks in the generared code so that it can handle implicitly an optional argument by overloads or metaprogramming. I am just not sure that is worth the effort since if we slap on a required we can treat the dictionary member like we handle when it is an input 🤔 edit: I think the tradeoff of doing it this way is:
|
2d50623
to
8480b57
Compare
Thinking about this some more, as an idea, a variant of reusing the 'required' keyword might be to have a Tradeoff to me seems to be between extra complexity in the generated code, or some type of hint in the IDL to help our implementation. |
8480b57
to
99b951c
Compare
The URLPattern spec is intended to be implemented inside of LibURL, with LibWeb only responsible for the IDL conversion layer, in a similar manner to how URL is implemented.
This is the return value of a URLPattern after `exec` is called on it. It conveys information about the named (or unammed) regex groups matched for each component of the URL. For example, ``` let p = new URLPattern({ hostname: "{:subdomain.}*example.com" }); const result = pattern.exec({ hostname: "foo.bar.example.com" }); console.log(result.hostname.groups.subdomain); ``` Will log 'foo.bar'.
This fixes a compile error of multiple variables of the same name within the same scope for the URLPattern IDL, which has a dictionary return type that contains multiple dictionaries of the same type. Conveniently, this also makes the complicated generated code of the URLPattern interface easier to read by adding some more structure :^)
120c201
to
1dbbac6
Compare
We were previously assuming that dictionary members were always required when being returned. This is a bit of a weird case, because unlike _input_ dictionaries which the spec marks as required, 'result' dictionaries do not seem to be marked in spec IDL as required. This is still fine from the POV that the spec is written as it states that we should only be putting the values into the dictionary if the value exists. We could do this through some metaprogramming constexpr type checks. For example, if the type in our C++ representation was not an Optional, we can skip the has_value check. Instead of doing that, change the IDL of the result dictionaries to annotate these members so that the IDL generator knows this information up front. While all current cases have every single member returned or not returned, it is conceivable that the spec could have a situation that one member is always returned (and should get marked as required), while the others are optionally returned. Therefore, this new GenerateAsRequired attribute is applied for each individual member.
This is not a very pleasant fix, but matches a similar const_cast that we do to return JS objects returned in a union. Ideally we would 'simply' remove the const from the value being visited in the variant, but that opens up a whole can of worms where we are currently relying on temporary lifetime extension so that interfaces can return a Variant of GC::Ref's to JS::Objects.
Just enough to get the IDL compiling :^)
1dbbac6
to
56ca945
Compare
Sitting on top of #3224. I've split this out as I am not fully convinced on the approach for the last IDL generator fix included in this merge request in particular.
This fixes a further 4 of the ~5-6 issues with the IDL generator for the URLPattern interface, and implements enough of the URLPattern types and IDL generator fixes so that we can get the IDL for URLPattern.exec compiling. Unfortunately, there are still some further compilation issues to fix for this IDL interface, along with a case where it is returning a subtlely wrong results.