-
Notifications
You must be signed in to change notification settings - Fork 6k
[dart:ui] add an explicit API for loading assets #21153
Changes from all commits
78941c9
31c26c9
226be4a
f284e47
79833b7
87dd5e1
bcf4c84
b5892d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4960,6 +4960,22 @@ class ImmutableBuffer extends NativeFieldWrapperClass2 { | |
} | ||
void _init(Uint8List list, _Callback<void> callback) native 'ImmutableBuffer_init'; | ||
|
||
/// Create a buffer from the asset with key [assetKey]. | ||
/// | ||
/// Returns `null` if the asset does not exist. | ||
static ImmutableBuffer? fromAsset(String assetKey) { | ||
final ImmutableBuffer buffer = ImmutableBuffer._(0); | ||
bool success = false; | ||
_fromAsset(assetKey, buffer, (void result) { | ||
success = true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is racy - see below. We want to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this actually racy? I'm not really familiar with the native interop but it appears to be fully synchronous to me There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If someone sets the return value in the C++ side before calling the completer here I believe it will have unexpected results. |
||
}); | ||
if (!success) { | ||
return null; | ||
} | ||
return buffer; | ||
} | ||
static void _fromAsset(String assetKey, ImmutableBuffer buffer, _Callback<void> callback) native 'ImmutableBuffer_fromAsset'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The more typical signature for this is It doesn't need to be static, and if the callback calls back with an int it could return the length. |
||
|
||
/// The length, in bytes, of the underlying data. | ||
final int length; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,8 @@ | |
#include "third_party/tonic/converter/dart_converter.h" | ||
#include "third_party/tonic/dart_args.h" | ||
#include "third_party/tonic/dart_binding_macros.h" | ||
#include "flutter/lib/ui/ui_dart_state.h" | ||
#include "flutter/lib/ui/window/platform_configuration.h" | ||
|
||
#if OS_ANDROID | ||
#include <sys/mman.h> | ||
|
@@ -30,6 +32,9 @@ ImmutableBuffer::~ImmutableBuffer() {} | |
void ImmutableBuffer::RegisterNatives(tonic::DartLibraryNatives* natives) { | ||
natives->Register({{"ImmutableBuffer_init", ImmutableBuffer::init, 3, true}, | ||
FOR_EACH_BINDING(DART_REGISTER_NATIVE)}); | ||
natives->Register({{"ImmutableBuffer_fromAsset", ImmutableBuffer::fromAsset, 3, true}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we do it this way, we don't have to pass the |
||
FOR_EACH_BINDING(DART_REGISTER_NATIVE)}); | ||
|
||
} | ||
|
||
void ImmutableBuffer::init(Dart_NativeArguments args) { | ||
|
@@ -53,6 +58,49 @@ size_t ImmutableBuffer::GetAllocationSize() const { | |
return sizeof(ImmutableBuffer) + data_->size(); | ||
} | ||
|
||
void ImmutableBuffer::fromAsset(Dart_NativeArguments args) { | ||
UIDartState::ThrowIfUIOperationsProhibited(); | ||
Dart_Handle callback_handle = Dart_GetNativeArgument(args, 2); | ||
if (!Dart_IsClosure(callback_handle)) { | ||
Dart_SetReturnValue(args, tonic::ToDart("Callback must be a function")); | ||
return; | ||
} | ||
Dart_Handle asset_name_handle = Dart_GetNativeArgument(args, 0); | ||
uint8_t* chars = nullptr; | ||
intptr_t asset_length = 0; | ||
Dart_Handle result = | ||
Dart_StringToUTF8(asset_name_handle, &chars, &asset_length); | ||
if (Dart_IsError(result)) { | ||
Dart_PropagateError(result); | ||
return; | ||
Comment on lines
+74
to
+75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we try to avoid this and instead use a String error. |
||
} | ||
Dart_Handle immutable_buffer = Dart_GetNativeArgument(args, 1); | ||
|
||
std::string asset_name = std::string{reinterpret_cast<const char*>(chars), | ||
static_cast<size_t>(asset_length)}; | ||
|
||
std::shared_ptr<AssetManager> asset_manager = UIDartState::Current() | ||
->platform_configuration() | ||
->client() | ||
->GetAssetManager(); | ||
std::unique_ptr<fml::Mapping> data = asset_manager->GetAsMapping(asset_name); | ||
|
||
if (data == nullptr) { | ||
return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Return a Dart String here with something like "Asset not found". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, Dart_SetReturnValue There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The contract for asset loading is already that we return null if it isn't found, and the framework can turn this into an exception |
||
} | ||
const void* bytes = static_cast<const void*>(data->GetMapping()); | ||
void* peer = reinterpret_cast<void*>(data.release()); | ||
auto size = data->GetSize(); | ||
SkData::ReleaseProc proc = [](const void* ptr, void* context) { | ||
// TODO: what do | ||
}; | ||
|
||
auto sk_data = SkData::MakeWithProc(bytes, size, proc, peer); | ||
Comment on lines
+94
to
+98
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might want to just store the mapping object in our heap which should keep it alive, and then There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will that be enough to keep the Mapping pointer alive? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The mapping pointer should remain valid as long as the |
||
auto buffer = fml::MakeRefCounted<ImmutableBuffer>(sk_data); | ||
buffer->AssociateWithDartWrapper(immutable_buffer); | ||
tonic::DartInvoke(callback_handle, {Dart_TypeVoid()}); | ||
} | ||
|
||
#if OS_ANDROID | ||
|
||
// Compressed image buffers are allocated on the UI thread but are deleted on a | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
// Copyright 2013 The Flutter Authors. All rights reserved. | ||
// Use of this source code is governed by a BSD-style license that can be | ||
// found in the LICENSE file. | ||
|
||
// @dart = 2.10 | ||
part of ui; | ||
|
||
/// Load the asset bytes specified by [assetKey]. | ||
/// | ||
/// The [assetKey] is generally the filepath of the asset which is bundled | ||
/// into the flutter application. This API is not supported on the | ||
/// Web. | ||
/// | ||
/// If the [assetKey] does not correspond to a real asset, returns `null`. | ||
ByteData? loadAsset(String assetKey) { | ||
return null; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
// Copyright 2013 The Flutter Authors. All rights reserved. | ||
// Use of this source code is governed by a BSD-style license that can be | ||
// found in the LICENSE file. | ||
|
||
// @dart = 2.6 | ||
import 'dart:ui' as ui; | ||
|
||
import 'package:test/test.dart'; | ||
|
||
void main() { | ||
test('Loading an asset that does not exist returns null', () { | ||
expect(ui.loadAsset('ThisDoesNotExist'), null); | ||
}); | ||
|
||
test('returns the bytes of a bundled asset', () { | ||
expect(ui.loadAsset('FontManifest.json'), isNotNull); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'd want to make this a factory so we could have
_fromAsset
return the length of the asset in bytes, and set the lenght properly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we could make the
length
a getter on a private field that is mutable.