Skip to content
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.

Sign up for GitHub

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

Remove some unnecessary RefCells in AVM1 objects #19734

Merged
merged 2 commits into from
Mar 7, 2025
Merged

Remove some unnecessary RefCells in AVM1 objects #19734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bb7d4ce
avm1: use `Cell` instead of `RefCell` in `ColorMatrixFilter`
moulins Mar 7, 2025
c4b05ee
avm1: Store `AvmString`s in `FileReference`
moulins Mar 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions core/src/avm1/globals/color_matrix_filter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,26 +6,26 @@ use crate::avm1::property_decl::{define_properties_on, Declaration};
use crate::avm1::{Activation, ArrayObject, Error, Object, ScriptObject, TObject, Value};
use crate::string::StringContext;
use gc_arena::{Collect, Gc, Mutation};
use std::cell::RefCell;
use std::cell::Cell;

#[derive(Clone, Debug, Collect)]
#[collect(require_static)]
struct ColorMatrixFilterData {
matrix: RefCell<[f32; 4 * 5]>,
matrix: Cell<[f32; 4 * 5]>,
}

impl From<&ColorMatrixFilterData> for swf::ColorMatrixFilter {
fn from(filter: &ColorMatrixFilterData) -> swf::ColorMatrixFilter {
swf::ColorMatrixFilter {
matrix: *filter.matrix.borrow(),
matrix: filter.matrix.get(),
}
}
}

impl From<swf::ColorMatrixFilter> for ColorMatrixFilterData {
fn from(filter: swf::ColorMatrixFilter) -> ColorMatrixFilterData {
Self {
matrix: RefCell::new(filter.matrix),
matrix: Cell::new(filter.matrix),
}
}
}
Expand All @@ -34,7 +34,7 @@ impl Default for ColorMatrixFilterData {
fn default() -> Self {
Self {
#[rustfmt::skip]
matrix: RefCell::new([
matrix: Cell::new([
1.0, 0.0, 0.0, 0.0, 0.0,
0.0, 1.0, 0.0, 0.0, 0.0,
0.0, 0.0, 1.0, 0.0, 0.0,
Expand Down Expand Up @@ -65,8 +65,12 @@ impl<'gc> ColorMatrixFilter<'gc> {
}

fn matrix(self, activation: &mut Activation<'_, 'gc>) -> Value<'gc> {
// Use `.as_slice_of_cells()` to avoid a copy out of the `Cell`.
// FIXME: use `.as_array_of_cells()` once stabilized.
let matrix: &Cell<[f32]> = &self.0.matrix;
let matrix = matrix.as_slice_of_cells();
ArrayObject::builder(activation)
.with(self.0.matrix.borrow().iter().map(|&v| v.into()))
.with(matrix.iter().map(|v| v.get().into()))
.into()
}

Expand Down Expand Up @@ -98,7 +102,7 @@ impl<'gc> ColorMatrixFilter<'gc> {
_ => (),
}

self.0.matrix.replace(matrix);
self.0.matrix.set(matrix);
Ok(())
}

Expand Down
118 changes: 48 additions & 70 deletions core/src/avm1/globals/file_reference.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ use crate::backend::ui::{FileDialogResult, FileFilter};
use crate::string::{AvmString, StringContext};
use gc_arena::barrier::unlock;
use gc_arena::lock::Lock;
use gc_arena::{Collect, Gc, Mutation};
use gc_arena::{Collect, Gc};
use ruffle_macros::istr;
use url::Url;

// There are two undocumented functions in FileReference: convertToPPT and deleteConvertedPPT.
Expand All @@ -30,58 +31,59 @@ impl<'gc> FileReferenceObject<'gc> {
pub fn init_from_dialog_result(
self,
activation: &mut Activation<'_, 'gc>,
dialog_result: &dyn FileDialogResult,
result: &dyn FileDialogResult,
) {
let mc = activation.gc();
let write = Gc::write(mc, self.0);

self.0.is_initialised.set(true);

let date_proto = activation.context.avm1.prototypes().date_constructor;
if let Some(creation_time) = dialog_result.creation_time() {
if let Some(creation_time) = result.creation_time() {
if let Ok(Value::Object(obj)) = date_proto.construct(
activation,
&[(creation_time.timestamp_millis() as f64).into()],
) {
self.set_creation_date(activation.gc(), Some(obj));
unlock!(write, FileReferenceData, creation_date).set(Some(obj));
}
}

if let Some(modification_time) = dialog_result.modification_time() {
if let Some(modification_time) = result.modification_time() {
if let Ok(Value::Object(obj)) = date_proto.construct(
activation,
&[(modification_time.timestamp_millis() as f64).into()],
) {
self.set_modification_date(activation.gc(), Some(obj));
unlock!(write, FileReferenceData, modification_date).set(Some(obj));
}
}

self.0.file_type.replace(dialog_result.file_type());
self.0.name.replace(dialog_result.file_name());
self.0.size.replace(dialog_result.size());
self.0.creator.replace(dialog_result.creator());
self.0.data.replace(dialog_result.contents().to_vec());
}
let file_type = result.file_type().map(|s| AvmString::new_utf8(mc, s));
unlock!(write, FileReferenceData, file_type).set(file_type);

fn set_creation_date(self, gc: &'gc Mutation<'gc>, creation_date: Option<Object<'gc>>) {
unlock!(Gc::write(gc, self.0), FileReferenceData, creation_date).set(creation_date);
}
let file_name = result.file_name().map(|s| AvmString::new_utf8(mc, s));
unlock!(write, FileReferenceData, name).set(file_name);

let creator = result.creator().map(|s| AvmString::new_utf8(mc, s));
unlock!(write, FileReferenceData, creator).set(creator);

fn set_modification_date(self, gc: &'gc Mutation<'gc>, modification_date: Option<Object<'gc>>) {
unlock!(Gc::write(gc, self.0), FileReferenceData, modification_date).set(modification_date);
self.0.size.replace(result.size());
self.0.data.replace(result.contents().to_vec());
}
}

#[derive(Default, Clone, Collect)]
#[derive(Clone, Default, Collect)]
#[collect(no_drop)]
pub struct FileReferenceData<'gc> {
/// Has this object been initialised from a dialog
is_initialised: Cell<bool>,

creation_date: Lock<Option<Object<'gc>>>,
creator: RefCell<Option<String>>,
creator: Lock<Option<AvmString<'gc>>>,
modification_date: Lock<Option<Object<'gc>>>,
name: RefCell<Option<String>>,
post_data: RefCell<String>,
name: Lock<Option<AvmString<'gc>>>,
post_data: Lock<Option<AvmString<'gc>>>,
size: Cell<Option<u64>>,
file_type: RefCell<Option<String>>,
file_type: Lock<Option<AvmString<'gc>>>,

/// The contents of the referenced file
/// We track this here so that it can be referenced in FileReference.upload
Expand Down Expand Up @@ -110,30 +112,21 @@ pub fn creation_date<'gc>(
_args: &[Value<'gc>],
) -> Result<Value<'gc>, Error<'gc>> {
if let NativeObject::FileReference(file_ref) = this.native() {
return Ok(file_ref
.0
.creation_date
.get()
.map_or(Value::Undefined, |x| x.into()));
let creation_date = file_ref.0.creation_date.get();
return Ok(creation_date.map_or(Value::Undefined, Into::into));
}

Ok(Value::Undefined)
}

pub fn creator<'gc>(
activation: &mut Activation<'_, 'gc>,
_activation: &mut Activation<'_, 'gc>,
this: Object<'gc>,
_args: &[Value<'gc>],
) -> Result<Value<'gc>, Error<'gc>> {
if let NativeObject::FileReference(file_ref) = this.native() {
return Ok(file_ref
.0
.creator
.borrow()
.as_ref()
.map_or(Value::Undefined, |x| {
AvmString::new_utf8(activation.gc(), x).into()
}));
let creator = file_ref.0.creator.get();
return Ok(creator.map_or(Value::Undefined, Into::into));
}

Ok(Value::Undefined)
Expand All @@ -145,30 +138,20 @@ pub fn modification_date<'gc>(
_args: &[Value<'gc>],
) -> Result<Value<'gc>, Error<'gc>> {
if let NativeObject::FileReference(file_ref) = this.native() {
return Ok(file_ref
.0
.modification_date
.get()
.map_or(Value::Undefined, |x| x.into()));
let modification_date = file_ref.0.modification_date.get();
return Ok(modification_date.map_or(Value::Undefined, Into::into));
}

Ok(Value::Undefined)
}

pub fn name<'gc>(
activation: &mut Activation<'_, 'gc>,
_activation: &mut Activation<'_, 'gc>,
this: Object<'gc>,
_args: &[Value<'gc>],
) -> Result<Value<'gc>, Error<'gc>> {
if let NativeObject::FileReference(file_ref) = this.native() {
return Ok(file_ref
.0
.name
.borrow()
.as_ref()
.map_or(Value::Undefined, |x| {
AvmString::new_utf8(activation.gc(), x).into()
}));
return Ok(file_ref.0.name.get().map_or(Value::Undefined, Into::into));
}

Ok(Value::Undefined)
Expand All @@ -180,9 +163,8 @@ pub fn post_data<'gc>(
_args: &[Value<'gc>],
) -> Result<Value<'gc>, Error<'gc>> {
if let NativeObject::FileReference(file_ref) = this.native() {
return Ok(
AvmString::new_utf8(activation.gc(), file_ref.0.post_data.borrow().clone()).into(),
);
let post_data = file_ref.0.post_data.get();
return Ok(post_data.unwrap_or_else(|| istr!("")).into());
}

Ok(Value::Undefined)
Expand All @@ -199,7 +181,8 @@ pub fn set_post_data<'gc>(
.coerce_to_string(activation)?;

if let NativeObject::FileReference(file_ref) = this.native() {
file_ref.0.post_data.replace(post_data.to_string());
let write = Gc::write(activation.gc(), file_ref.0);
unlock!(write, FileReferenceData, post_data).set(Some(post_data));
}

Ok(Value::Undefined)
Expand All @@ -211,26 +194,21 @@ pub fn size<'gc>(
_args: &[Value<'gc>],
) -> Result<Value<'gc>, Error<'gc>> {
if let NativeObject::FileReference(file_ref) = this.native() {
return Ok(file_ref.0.size.get().map_or(Value::Undefined, |x| x.into()));
let size = file_ref.0.size.get();
return Ok(size.map_or(Value::Undefined, Into::into));
}

Ok(Value::Undefined)
}

pub fn file_type<'gc>(
activation: &mut Activation<'_, 'gc>,
_activation: &mut Activation<'_, 'gc>,
this: Object<'gc>,
_args: &[Value<'gc>],
) -> Result<Value<'gc>, Error<'gc>> {
if let NativeObject::FileReference(file_ref) = this.native() {
return Ok(file_ref
.0
.file_type
.borrow()
.as_ref()
.map_or(Value::Undefined, |x| {
AvmString::new_utf8(activation.gc(), x).into()
}));
let file_type = file_ref.0.file_type.get();
return Ok(file_type.map_or(Value::Undefined, Into::into));
}

Ok(Value::Undefined)
Expand Down Expand Up @@ -401,17 +379,17 @@ pub fn upload<'gc>(
_ => return Ok(false.into()),
}

let file_name = match file_reference.0.name.get() {
Some(name) => name.to_string(),
None => "file".to_string(),
};

let process = activation.context.load_manager.upload_file(
activation.context.player.clone(),
this,
url_string,
file_reference.0.data.borrow().clone(),
file_reference
.0
.name
.borrow()
.clone()
.unwrap_or_else(|| "file".to_string()),
file_name,
);

activation.context.navigator.spawn_future(process);
Expand Down
Loading