Reland "dart:ui conversion from native to FfiNative" (#33116)" #34700
Conversation
This reverts commit 6f19a3e. Attempts to resolve merge conflicts and update modified/newly added methods, and fixes the incorrect argument counts on some of the Path methods.
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Looks like there's a segfault in a |
|
I had an incorrect |
|
Filed dart-lang/sdk#49471 |
|
CI is passing - this is ready for review. |
|
test-exempt: the existing tests should in theory cover this. |
| V(Path, relativeArcToPoint, 8) \ | ||
| V(Path, relativeConicTo, 6) \ |
There was a problem hiding this comment.
I've fixed these compared to last time.
| &CLASS::METHOD>::Call); \ | ||
| } | ||
|
|
||
| void* ResolveFfiNativeFunction(const char* name, uintptr_t args) { |
There was a problem hiding this comment.
Can this be a hash table lookup instead?
There was a problem hiding this comment.
Also, does this get cached by the Dart VM, or do we pay this lookup cost on every call?
There was a problem hiding this comment.
I had raised this and Jason ended up saying: #29607 (comment)
I think we should still do this, but I'm slightly inclined to do it in a separate PR. That said we could do it here.
There was a problem hiding this comment.
I did a test implementation of map-based lookup a while ago while reviewing the original version of this PR. I plan to revive it after this PR lands.
The lookup is cached by Dart and is only done once for each native function.
| V(SceneBuilder, setCheckerboardRasterCacheImages) \ | ||
| V(SceneBuilder, build) | ||
|
|
||
| FOR_EACH_BINDING(DART_NATIVE_CALLBACK) |
There was a problem hiding this comment.
Does the DART_NATIVE_CALLBACK macro (and possibly other macros) become dead with this change? Can they be removed?
There was a problem hiding this comment.
They're still used in the Fuchsia embedding apparently.
| // trying to resolve, an exception will be thrown. | ||
| #define FFI_FUNCTION_LIST(V) \ | ||
| /* Constructors */ \ | ||
| V(Canvas::Create, 6) \ |
There was a problem hiding this comment.
This list should be alphabetized. But is there any way that these lists could go back into the separate files? That seemed easier to manage than this giant list.
There was a problem hiding this comment.
They are alphabetized within their subsections (e.g. all constructors, all "other").
I don't have a strong preference either way here. This was part of Clement's original refactor.
lib/ui/dart_ui.cc
Outdated
| // If there is a mismatch between names or parameter count an @FfiNative is | ||
| // trying to resolve, an exception will be thrown. | ||
| #define FFI_METHOD_LIST(V) \ | ||
| V(Image, dispose, 1) \ |
There was a problem hiding this comment.
Same comment about at least alphabetizing, and considering splitting back into separate files if possible.
There was a problem hiding this comment.
I moved Image to the right section, everything else looked alphabetized.
I don't have a strong preference either way for whether we move these to separate files, but I'm somewhat inclined to get this landed first before it ends up with merge conflicts.
|
Talked to Zach offline, going to try to land this and follow up on the nits. |
dnfield
added
the
autosubmit
|
Golem said this negatively impacted code size. |
4 participants
This reverts commit 6f19a3e.
Attempts to resolve merge conflicts and update modified/newly added
methods, and fixes the incorrect argument counts
on some of the Path methods.