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

Implement custom element reactions #17614

Merged
merged 4 commits into from Jul 18, 2017

Conversation

@cbrewster
Copy link
Member

commented Jul 5, 2017

Initial work for implementing custom element reactions: https://html.spec.whatwg.org/multipage/custom-elements.html#custom-element-reactions


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #17433 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Jul 5, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/customelementregistry.rs, components/script/dom/webidls/CustomElementRegistry.webidl, components/script/dom/element.rs, components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/create.rs and 5 more
  • @KiChjang: components/script/dom/customelementregistry.rs, components/script/dom/webidls/CustomElementRegistry.webidl, components/script/dom/element.rs, components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/create.rs and 5 more
@highfive

This comment has been minimized.

Copy link

commented Jul 5, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@jdm

This comment has been minimized.

Copy link
Member

commented Jul 6, 2017

failures:

    size_of::size_div

    size_of::size_element

    size_of::size_htmlelement

    size_of::size_span

@jdm jdm added the S-fails-travis label Jul 6, 2017

@cbrewster cbrewster force-pushed the cbrewster:custom_element_reactions branch from 63015e5 to 787111d Jul 6, 2017

@jdm jdm removed the S-fails-travis label Jul 6, 2017

@cbrewster cbrewster force-pushed the cbrewster:custom_element_reactions branch 2 times, most recently from c9efe47 to 20c8a58 Jul 7, 2017

@cbrewster cbrewster changed the title [WIP] Implement custom element reactions Implement custom element reactions Jul 7, 2017

@cbrewster

This comment has been minimized.

Copy link
Member Author

commented Jul 7, 2017

Ready for review. r? @jdm

I am going to submit the changes for [CEReactions] as a separate PR since it would add quite a lot of noise here. This means that all reactions are triggered via the backup element queue through a microtask. This is why many reaction-related tests are failing, they should be passing after [CEReactions] is implemented.

@cbrewster cbrewster force-pushed the cbrewster:custom_element_reactions branch from 3b21819 to b2b5f06 Jul 10, 2017

@nox

This comment has been minimized.

Copy link
Member

commented Jul 11, 2017

The unit tests are still failing.

@nox nox added the S-fails-travis label Jul 11, 2017

@cbrewster cbrewster force-pushed the cbrewster:custom_element_reactions branch from b2b5f06 to 944ca90 Jul 11, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2017

☔️ The latest upstream changes (presumably #17364) made this pull request unmergeable. Please resolve the merge conflicts.

@@ -221,6 +221,15 @@ unsafe impl<T: JSTraceable> JSTraceable for Vec<T> {
}
}

unsafe impl<T: JSTraceable> JSTraceable for [T] {

This comment has been minimized.

Copy link
@jdm

jdm Jul 12, 2017

Member

This commit can be removed now.

This comment has been minimized.

Copy link
@cbrewster

cbrewster Jul 13, 2017

Author Member

Done.

/// Steps 10.3, 10.4
#[allow(unsafe_code)]
fn get_callbacks(&self, prototype: HandleObject) -> Fallible<LifecycleCallbacks> {
macro_rules! get_callback {

This comment has been minimized.

Copy link
@jdm

jdm Jul 12, 2017

Member

This can be a function that returns Result<Function, Error>, right? I don't see a reason to use a macro here.

This comment has been minimized.

Copy link
@cbrewster

cbrewster Jul 13, 2017

Author Member

Done.

},
_ => return Err(Error::JSFailed),
};
Ok(observed_attributes)

This comment has been minimized.

Copy link
@jdm

jdm Jul 12, 2017

Member

We can return this expression directly:

match conversion {
    Ok(ConversionResult::Success(attributes)) => Ok(attributes),
    Ok(ConversionResult::Failure(error)) => Err(Error::Type(error)),
    Err(()) => Err(Error::JSFailed),
}
MutationObserver::queue_a_mutation_record(owner.upcast::<Node>(), mutation);

let reaction = CallbackReaction::AttributeChanged(name, Some(old_value), Some(new_value), namespace);
ScriptThread::enqueue_callback_reaction(self.owner().unwrap(), reaction);

This comment has been minimized.

Copy link
@jdm

jdm Jul 12, 2017

Member

Use owner instead of self.owner().

This comment has been minimized.

Copy link
@cbrewster

cbrewster Jul 13, 2017

Author Member

Done.

@@ -456,6 +456,196 @@ impl CustomElementDefinition {
}
}

#[derive(HeapSizeOf, JSTraceable)]
pub enum CustomElementReaction {

This comment has been minimized.

Copy link
@jdm

jdm Jul 12, 2017

Member

Add #[must_root] to this.

This comment has been minimized.

Copy link
@cbrewster

cbrewster Jul 13, 2017

Author Member

Done.

fn invoke_reactions(&self) {
// Steps 1-2
for element in self.queue.borrow().iter() {
element.invoke_reactions()

This comment has been minimized.

Copy link
@jdm

jdm Jul 12, 2017

Member

Can these reactions end up appending an element to this queue?

This comment has been minimized.

Copy link
@cbrewster

cbrewster Jul 13, 2017

Author Member

I do not believe so, there are two queues an element can be added to:

  1. The current element queue.
  2. The backup element queue.

For the first case, the current element queue is popped from the custom element reaction stack thus it is no longer the current element queue which means no element will be added to it.

The second case is the backup element queue has the processing_backup_element_queue flag which prevents elements being enqueued until the flag is set to NotProcessing.

This comment has been minimized.

Copy link
@cbrewster

cbrewster Jul 13, 2017

Author Member

Oh actually in the second case, an element can be appended to the backup element queue is being invoked. The flag check occurs after the element is appended to the queue. This is what causes the crash in the test expectations.

I want to change it so the queue is not borrowed for the duration of the loop. Instead if element are popped and processed, new element can be appended to the queue. But I always run into unrooted must root errors. any ideas?

while let Some(element) = self.queue.borrow_mut().pop_front().map(|e| Root::from_ref(&*e)) {
    element.invoke_reactions()
}

This comment has been minimized.

Copy link
@cbrewster

cbrewster Jul 17, 2017

Author Member

Avoiding the closure appears to appease the lint

self.queue.borrow_mut().pop_front().as_ref().map(JS::deref).map(Root::from_ref)
pub fn invoke_reactions(&self) {
let mut reaction_queue = self.custom_element_reaction_queue.borrow_mut();
while let Some(reaction) = reaction_queue.pop_front() {
reaction.invoke(Root::from_ref(self));

This comment has been minimized.

Copy link
@jdm

jdm Jul 12, 2017

Member

Can these reactions end up appending reactions to the queue?

This comment has been minimized.

Copy link
@cbrewster

cbrewster Jul 13, 2017

Author Member

See other comment

@@ -313,6 +315,11 @@ impl Node {
// e.g. when removing a <form>.
vtable_for(&&*node).unbind_from_tree(&context);
node.style_and_layout_data.get().map(|d| node.dispose(d));
if let Some(element) = node.downcast::<Element>() {

This comment has been minimized.

Copy link
@jdm
for descendant in node.traverse_preorder() {
// Step 3.2.
if let Some(element) = node.downcast::<Element>() {
if element.get_custom_element_definition().is_some() {

This comment has been minimized.

Copy link
@jdm

jdm Jul 12, 2017

Member

We may want to write a Node::as_custom_element helper which combines the downcast and custom check into one.

This comment has been minimized.

Copy link
@cbrewster

cbrewster Jul 13, 2017

Author Member

Done.

@@ -1,5 +1,6 @@
[reaction-timing.html]
type: testharness
expected: CRASH

This comment has been minimized.

Copy link
@jdm
cbrewster added 2 commits Jun 19, 2017
Store lifecycle callbacks when CEs are defined
This implements steps 10.3 - 10.4 of the CustomElementRegistry.define
steps.
Get observed attributes when a CE is defined
Implements steps 10.5 - 10.6 of CustomElementRegistry.define

@cbrewster cbrewster force-pushed the cbrewster:custom_element_reactions branch from 944ca90 to 30afac6 Jul 17, 2017

@cbrewster cbrewster force-pushed the cbrewster:custom_element_reactions branch from 30afac6 to 348251b Jul 17, 2017

@cbrewster cbrewster referenced this pull request Jul 17, 2017
3 of 5 tasks complete
@jdm
jdm approved these changes Jul 17, 2017
while let Some(element) = self.next_element() {
element.invoke_reactions()
}
self.queue.borrow_mut().clear();

This comment has been minimized.

Copy link
@jdm

jdm Jul 17, 2017

Member

This shouldn't be necessary since we now pop, right?

@jdm

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

Looks like the sizeof unit tests are failing again, too.

@cbrewster cbrewster force-pushed the cbrewster:custom_element_reactions branch from 348251b to 9b587a4 Jul 18, 2017

@cbrewster

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2017

@bors-servo r=jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2017

📌 Commit 9b587a4 has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2017

⌛️ Testing commit 9b587a4 with merge 7b13444...

bors-servo added a commit that referenced this pull request Jul 18, 2017
Auto merge of #17614 - cbrewster:custom_element_reactions, r=jdm
Implement custom element reactions

<!-- Please describe your changes on the following line: -->
Initial work for implementing custom element reactions: https://html.spec.whatwg.org/multipage/custom-elements.html#custom-element-reactions

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #17433 (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17614)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2017

@bors-servo bors-servo merged commit 9b587a4 into servo:master Jul 18, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
bors-servo added a commit that referenced this pull request Jul 18, 2017
Auto merge of #17761 - cbrewster:ce_reactions, r=jdm
Add [CEReactions] to webidls

<!-- Please describe your changes on the following line: -->
Relies on #17614

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17761)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Jul 18, 2017
Auto merge of #17761 - cbrewster:ce_reactions, r=jdm
Add [CEReactions] to webidls

<!-- Please describe your changes on the following line: -->
Relies on #17614

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17761)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.