This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Add a separate target for Dart coverter on FML types. #14011
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cbracken
reviewed
Nov 26, 2019
| DestroyShell(std::move(shell)); | ||
| } | ||
|
|
||
| static bool MemsetPatternSetOrCheck(uint8_t* buffer, |
Member
There was a problem hiding this comment.
Add a short doc comment, since it's a utility. Particularly, document that if set_or_checkis true, you set, otherwise you check.
Member
Author
There was a problem hiding this comment.
Done. Made the bool an enum.
shell/common/shell_unittests.cc
Outdated
| uint8_t* start = buffer; | ||
| uint8_t* p = buffer; | ||
|
|
||
| while ((start + size) - p >= 4) { |
Member
There was a problem hiding this comment.
nit: Pretty obvious what this is, but maybe pattern and a constexpr size_t pattern_len = 4;?
cbracken
reviewed
Nov 26, 2019
shell/common/shell_unittests.cc
Outdated
|
|
||
| uint8_t* buffer = static_cast<uint8_t*>(::malloc(buffer_size)); | ||
| ASSERT_NE(buffer, nullptr); | ||
| ASSERT_TRUE(MemsetPatternSetOrCheck(buffer, buffer_size, true /* set */)); |
Member
There was a problem hiding this comment.
Wonder if you should just chuck in an two-valued enum instead?
cbracken
reviewed
Nov 26, 2019
|
|
||
| fml::UniqueFD OpenFixture(std::string fixture_name); | ||
|
|
||
| std::unique_ptr<fml::Mapping> OpenFixtureAsMapping(std::string fixture_name); |
Member
There was a problem hiding this comment.
Consider documenting this (and the surrounding prototypes while you're in here).
cbracken
reviewed
Nov 26, 2019
| return nullptr; | ||
| } | ||
|
|
||
| auto mapping_buffer = ::malloc(length); |
Member
There was a problem hiding this comment.
For a zero-length list this can return null, in which case we'll handle as an OOM below.
Member
Author
There was a problem hiding this comment.
Done. Good catch. Returns a valid zero sized mapping now.
chinmaygarde
commented
Nov 26, 2019
Member
Author
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
Addressed all comments. PTAL.
| DestroyShell(std::move(shell)); | ||
| } | ||
|
|
||
| static bool MemsetPatternSetOrCheck(uint8_t* buffer, |
Member
Author
There was a problem hiding this comment.
Done. Made the bool an enum.
shell/common/shell_unittests.cc
Outdated
| uint8_t* start = buffer; | ||
| uint8_t* p = buffer; | ||
|
|
||
| while ((start + size) - p >= 4) { |
shell/common/shell_unittests.cc
Outdated
|
|
||
| uint8_t* buffer = static_cast<uint8_t*>(::malloc(buffer_size)); | ||
| ASSERT_NE(buffer, nullptr); | ||
| ASSERT_TRUE(MemsetPatternSetOrCheck(buffer, buffer_size, true /* set */)); |
|
|
||
| fml::UniqueFD OpenFixture(std::string fixture_name); | ||
|
|
||
| std::unique_ptr<fml::Mapping> OpenFixtureAsMapping(std::string fixture_name); |
| return nullptr; | ||
| } | ||
|
|
||
| auto mapping_buffer = ::malloc(length); |
Member
Author
There was a problem hiding this comment.
Done. Good catch. Returns a valid zero sized mapping now.
The converters are still in a separate target that must be included manually. This allows targets that depend on FML but not Dart runtime not have to depend on the runtime. Adds a test that includes this target and tests image decompression from assets. There is also a test for the standalone DartConvertor in shell_unittests but not in fml_unittests be cause FML uni-tests cannot yet launch a VM. I will work on adding fixtures for those.
chinmaygarde
force-pushed
the
fml_dart_converter
branch
from
af15f56 to
39b6912
Compare
Member
|
|
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 27, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 27, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 27, 2019
auto-submit bot
pushed a commit
that referenced
this pull request
Nov 22, 2023
This is more-or-less a revert of #14011 This code never ended up being used outside of tests, and it's not how we handle asset loading at this point anyway. I was hopeful we could kill off all runtime dependencies on Dart in `FML` when looking at this, but it looks like trace_event.h still wants to import dart_api_tools.h for some Dart enum types. This may or may not matter if we ever want to build FML for web/wasm. /cc @eyebrowsoffire. If we really need to do that, we can refactor the trace event stuff so that it has a web and Dart implementation that's selected at build time.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The converters are still in a separate target that must be included manually. This allows targets that depend on FML but not Dart runtime not have to depend on the runtime.
Adds a test that includes this target and tests image decompression from assets. There is also a test for the standalone DartConvertor in shell_unittests but not in fml_unittests be cause FML unit-tests cannot yet launch a VM. I will work on adding fixtures for those.
A potential future improvement could be that we start using typed data buffer on mapping that are given to us so that there are no copies.