[DisplayList] DlPath supports generic path dispatching #164753
Conversation
|
Done. There were only 2 which seemed odd. I just went and looked at all of the code that handles clip mutators and noticed that most platforms either NOP a path clip or they just get the bounds and treat it like a clipRect. Well, at least now they have a simpler interface to deal with when we decide to add path support to the other platforms. |
engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc
Outdated
Show resolved
Hide resolved
engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm
Outdated
Show resolved
Hide resolved
Pretty much a refactor, and well covered by other tests, but it does introduce a new public method so there probably should be tests. I'll work on some. |
|
Unit tests now added |
|
This PR should be in its final state now. |
| for (auto it = path.begin(), end = path.end(); it != end; ++it) { | ||
| switch (it.type()) { | ||
| case ComponentType::kContour: { | ||
| const impeller::ContourComponent* contour = it.contour(); | ||
| FML_DCHECK(contour != nullptr); | ||
| if (subpath_needs_close) { | ||
| sk_path.close(); | ||
| receiver.Close(); |
There was a problem hiding this comment.
No action required: It's not in the c++ style guide but when calling virtual methods it's safer to use -> since if you accidentally get a value instead of a reference you'll get incorrect behavior.
There was a problem hiding this comment.
How? -> doesn't even compile.
There was a problem hiding this comment.
You have to switch from using references to using pointers.
There was a problem hiding this comment.
That would add the burden of implicitly accepting null values which must be checked. I don't understand the "value vs reference" issue and the incorrect behavior it leads to.
There was a problem hiding this comment.
When you have a reference in C++ it isn't guaranteed to not be null. You still technically have to consider null values, we just are ignoring that.
There was a problem hiding this comment.
That's a big TIL. I see so many comments here and there that references cannot ever be null, but I'm guessing you can fudge it with *pointer_that_is_null?
There was a problem hiding this comment.
Perhaps clang-tidy might complain (if it is run)?
There was a problem hiding this comment.
but I'm guessing you can fudge it with *pointer_that_is_null?
Yep, exactly and we do this in our codebase all the time. A linter that disallowed references to anything on the heap would be nice, I don't think it exists. That's how I personally use them.
There was a problem hiding this comment.
I'll file this under "wishing C++ would throw if you create a reference by dereferencing a nulllptr".
| const DlScalar weight; | ||
| }; | ||
|
|
||
| class TestPathReceiver : public DlPathReceiver { |
There was a problem hiding this comment.
no action required: This class should just be a gmock.
There was a problem hiding this comment.
I took a quick look at gmock just now, but it seems focused on just asserting that methods are called whereas this class is checking the data they are called with.
There was a problem hiding this comment.
There are hundreds of examples of using gmock to check the arguments of function calls in our codebase. Here is one example:
This is asserting that AcquireFrame with arguments that equal frame_size.
There was a problem hiding this comment.
I don't understand the formatting used there, it looks like it is placing expectations on return values, but my methods are all void and I'm placing expectations on the data they receive. Again, maybe that's in there, but I'd need to study gmock further - likely worth the work.
There was a problem hiding this comment.
Maybe this one is easier to see:
This is asserting that GenTextures is being called with 2 arguments, the first one must be 1 the second can be anything ("_" is a wildcard matcher).
I think this would be a good opportunity to learn how to use gmock. It's a very useful skill and I'm happy to help.
There was a problem hiding this comment.
There are so many similar classes I've written in the unit tests, time to look into that mechanism.
There was a problem hiding this comment.
TIL-gmock. The tests are slightly more wordy, but the intent is outlined in the test itself and the mock receiver class was 100x simpler than the tester harness class.
|
Bumped to ToT as it has been sitting for a week... |
There are different ways to iterate over an SkPath or an impeller::Path and various points in the engine source tree we have boilerplate duplicates of this code to transfer the contents of the DlPath wrapper object into some platform-specific path. This PR adds a dispatch/receiver mechanism to read back the contents of a DlPath - independent of whether it is backed by an SkPath or an impeller::Path - in a simpler form that avoids potential mistakes in the various conversion methods.
See DlPathReceiver and DlPath::Dispatch in the dl_path.h file