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.

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

Split getter for mutation_observers() into two methods. #23529

Merged
merged 1 commit into from Jun 10, 2019
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Split getter for mutation_observers() into two methods.

registered_mutation_observers() returns immutable references and does
not init mutation observers.

registered_mutation_observers_mut() lazily initializes raredata if it
does not exist.

Updated code that uses this methods to call appropriate method when
mutation is not necessary.
  • Loading branch information
gatoWololo committed Jun 6, 2019
commit 3df8d6891e0fe47d4af3d7ee59ba05ea64ee628c
@@ -132,7 +132,12 @@ impl MutationObserver {

// Step 2 & 3
for node in target.inclusive_ancestors(ShadowIncluding::No) {
for registered in &*node.registered_mutation_observers() {
let registered = node.registered_mutation_observers();
This conversation was marked as resolved by gatoWololo

This comment has been minimized.

Copy link
@KiChjang

KiChjang Jun 7, 2019

Member
Suggested change
let registered = node.registered_mutation_observers();
let registered = match node.registered_mutation_observers() {
Some(reg) => reg,
None => continue,
};

This comment has been minimized.

Copy link
@gatoWololo

gatoWololo Jun 7, 2019

Author Contributor

Thanks! This will be pushed.

This comment has been minimized.

Copy link
@KiChjang

KiChjang Jun 10, 2019

Member

Looks like this was never fixed?

if registered.is_none() {
continue;
}

for registered in &*registered.unwrap() {
if &*node != target && !registered.options.subtree {
continue;
}
@@ -297,7 +302,7 @@ impl MutationObserverMethods for MutationObserver {
// Step 7
let add_new_observer = {
let mut replaced = false;
for registered in &mut *target.registered_mutation_observers() {
for registered in &mut *target.registered_mutation_observers_mut() {
if &*registered.observer as *const MutationObserver !=
self as *const MutationObserver
{
@@ -451,18 +451,25 @@ impl Node {
cmp & NodeConstants::DOCUMENT_POSITION_PRECEDING != 0
}

/// Return all registered mutation observers for this node.
/// XXX(ferjm) This should probably be split into two functions,
/// `registered_mutation_observers`, which returns an Option or
/// an empty slice or something, and doesn't create the rare data,
/// and `registered_mutation_observers_mut`, which does lazily
/// initialize the raredata.
pub fn registered_mutation_observers(&self) -> RefMut<Vec<RegisteredObserver>> {
/// Return all registered mutation observers for this node. Lazily initialize the
/// raredata if it does not exist.
pub fn registered_mutation_observers_mut(&self) -> RefMut<Vec<RegisteredObserver>> {
RefMut::map(self.ensure_rare_data(), |rare_data| {
&mut rare_data.mutation_observers
})
}

pub fn registered_mutation_observers(&self) -> Option<Ref<Vec<RegisteredObserver>>> {
let rare_data: Ref<_> = self.rare_data.borrow();

if rare_data.is_none() {
return None;
}
Some(Ref::map(rare_data, |rare_data| {
This conversation was marked as resolved by KiChjang

This comment has been minimized.

Copy link
@KiChjang

KiChjang Jun 7, 2019

Member

The check above can be removed and this line can be changed to:

Suggested change
Some(Ref::map(rare_data, |rare_data| {
Some(Ref::map(rare_data?, |rare_data| {

This comment has been minimized.

Copy link
@gatoWololo

gatoWololo Jun 7, 2019

Author Contributor

The type of rare_data is Ref<Option<_>>. So we cannot do rare_data?:

error[E0277]: the `?` operator can only be applied to values that implement `std::ops::Try`
   --> components/script/dom/node.rs:465:23
    |
465 |         Some(Ref::map(rare_data?, |rare_data| {
    |                       ^^^^^^^^^^ the `?` operator cannot be applied to type `std::cell::Ref<'_, std::option::Option<std::boxed::Box<dom::raredata::NodeRareData>>>`

This comment has been minimized.

Copy link
@gatoWololo

gatoWololo Jun 7, 2019

Author Contributor

I tried a few other things, but working with the Ref<_> is annoying.

This comment has been minimized.

Copy link
@jdm

jdm Jun 7, 2019

Member

Yeah, it tends to inhibit some common patterns. I wouldn't spend any more time trying to clean it up.

&rare_data.as_ref().unwrap().mutation_observers
}))
}

/// Add a new mutation observer for a given node.
pub fn add_mutation_observer(&self, observer: RegisteredObserver) {
self.ensure_rare_data().mutation_observers.push(observer);
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.