Adding all the relevant mutations #21680

paavininanda · 2018-09-11T20:03:41Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix a subset of #11416

These changes do not require tests

This change is

highfive · 2018-09-11T20:03:47Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/document.rs, components/script/dom/htmlimageelement.rs, components/script/dom/window.rs, components/script/dom/htmlsourceelement.rs
@KiChjang: components/script/dom/document.rs, components/script/dom/htmlimageelement.rs, components/script/dom/window.rs, components/script/dom/htmlsourceelement.rs

paavininanda · 2018-09-13T08:14:02Z

@jdm No new test passed after adding the relevant mutations. Is there something that I am missing?

jdm · 2018-09-13T14:35:28Z

components/script/dom/htmlimageelement.rs

-            &local_name!("src") => self.update_the_image_data(),
-            &local_name!("srcset") => self.update_the_image_data(),
+            &local_name!("src") | &local_name!("srcset")  |
+            &local_name!("width") | &local_name!("crossorigin") => self.update_the_image_data(),


This should include sizes according to https://html.spec.whatwg.org/multipage/images.html#reacting-to-dom-mutations

jdm · 2018-09-13T14:35:28Z

components/script/dom/htmlimageelement.rs

-            &local_name!("src") => self.update_the_image_data(),
-            &local_name!("srcset") => self.update_the_image_data(),
+            &local_name!("src") | &local_name!("srcset")  |
+            &local_name!("width") | &local_name!("crossorigin") => self.update_the_image_data(),
            _ => {},
        }
        if let Some(parent) = self.upcast::<Node>().GetParentElement() {


I don't think we want to update the image data whenever any attribute was modified and the parent is a picture element. I'm not sure why this code is here, so let's remove it.

jdm · 2018-09-13T14:35:28Z

components/script/dom/htmlimageelement.rs

@@ -1528,6 +1528,15 @@ impl VirtualMethods for HTMLImageElement {
        self.super_type().unwrap().unbind_from_tree(context);
        let document = document_from_node(self);
        document.unregister_responsive_image(self);
+        if let Some(parent) = self.upcast::<Node>().GetParentElement() {


I think we'll need to check context.parent here instead. Removing the node from the tree may mean that it has no parent element according to the Node APIs. This will address the " removed from a picture parent element" mutation.

jdm · 2018-09-13T14:35:28Z

components/script/dom/htmlimageelement.rs

+        if let Some(parent) = self.upcast::<Node>().GetParentElement() {
+            if parent.is::<HTMLPictureElement>() {
+                if let Some(previous_sibling) = self.upcast::<Node>().GetPreviousSibling() {
+                    if previous_sibling.is::<HTMLSourceElement> () {


Is this trying to address "The element's parent is a picture element and a source element that was a previous sibling is removed."? We need to check for that state in the HTMLSourceElement::unbind_from_tree instead.

Yes, I tried to do it here because of the discussion that we had on Gitter. Will change this 👍

jdm · 2018-09-13T14:35:28Z

components/script/dom/htmlsourceelement.rs

+        match attr.local_name() {
+            &local_name!("srcset") | &local_name!("sizes")  |
+            &local_name!("media") | &local_name!("type") => {
+                if let Some(next_sibling) = self.upcast::<Node>().GetNextSibling() {


We will need to iterate over self.upcast::<Node>().following_siblings() instead of just getting the immediately following sibling.

components/script/dom/htmlsourceelement.rs

jdm · 2018-09-13T14:35:28Z

components/script/dom/document.rs

+            .map_or(false, |p| p.is::<HTMLPictureElement>());
+        if is_parent_picture {
+            img.update_the_image_data();
+        }


I don't think it makes sense to implement these mutation checks here. It splits the implementation between multiple files so it's harder to understand how the pieces fit together.

jdm · 2018-09-13T14:35:28Z

components/script/dom/document.rs

    }

    pub fn unregister_responsive_image(&self, img: &HTMLImageElement) {
        let index = self.responsive_images.borrow().iter().position(|x| **x == *img);
        if let Some(i) = index {
            self.responsive_images.borrow_mut().remove(i);
+            let elem = img.upcast::<Element>();


jdm

Closer!

jdm · 2018-09-14T19:23:54Z

components/script/dom/htmlsourceelement.rs

-                    if let Some(htmlimageelement_sibling) = next_sibling.downcast::<HTMLImageElement>() {
-                        htmlimageelement_sibling.update_the_image_data();
+                let mut next_siblings_iterator = self.upcast::<Node>().following_siblings();
+                if let Some(next_sibling) = next_siblings_iterator.next() {


You will want to use a for loop to iterate over all the siblings, not just check the immediate sibling.

Oops, missed the loop by mistake.

jdm · 2018-09-14T19:23:54Z

components/script/dom/htmlsourceelement.rs

+            &local_name!("srcset") | &local_name!("sizes")  |
+            &local_name!("media") | &local_name!("type") => {
+                let mut next_siblings_iterator = self.upcast::<Node>().following_siblings();
+                if let Some(next_sibling) = next_siblings_iterator.next() {


You will want to use a for loop to iterate over the siblings, rather than only checking the immediate sibling.

jdm · 2018-09-14T19:23:54Z

components/script/dom/htmlsourceelement.rs

    /// <https://html.spec.whatwg.org/multipage/#the-source-element:nodes-are-inserted>
    fn bind_to_tree(&self, tree_in_doc: bool) {
        self.super_type().unwrap().bind_to_tree(tree_in_doc);
        let parent = self.upcast::<Node>().GetParentNode().unwrap();
        if let Some(media) = parent.downcast::<HTMLMediaElement>() {
            media.handle_source_child_insertion();
        }
+
+        let mut next_siblings_iterator = self.upcast::<Node>().following_siblings();
+        if let Some(next_sibling) = next_siblings_iterator.next() {


It would be worth creating a helper function for the sibling iterator check to avoid duplicating this code.

jdm · 2018-09-14T19:23:54Z

components/script/dom/htmlsourceelement.rs

+    fn unbind_from_tree(&self, context: &UnbindContext) {
+        self.super_type().unwrap().unbind_from_tree(context);
+
+        let mut next_siblings_iterator = self.upcast::<Node>().following_siblings();


Because we're unbinding, we need to be careful. You will want to use the next_sibling member of UnbindContext and call inclusive_next_sibling on that node instead, then loop over the iterator.

There is a prev_sibling in UnbindContext but not a next_sibling! I was trying to find it here - https://doc.servo.org/script/dom/node/struct.UnbindContext.html

Go ahead and add one to UnbindContext, in that case!

jdm · 2018-09-17T16:44:12Z

This looks great! Please also remove

servo/tests/wpt/metadata/html/semantics/embedded-content/the-img-element/relevant-mutations.html.ini

Line 4 in 9a83ab6

disabled: unstable failures and timeouts in release builds

and update the test expectations for that test!

jdm · 2018-09-17T16:54:01Z

Nice solution using impl Iterator, by the way!

jdm · 2018-09-18T13:13:41Z

@bors-servo r+

bors-servo · 2018-09-18T13:13:42Z

📌 Commit 11e2ccd has been approved by jdm

bors-servo · 2018-09-20T06:07:03Z

⌛ Testing commit e5e863d with merge ce9ca37...


        Auto merge of #21680 - paavininanda:relevant_mut, r=jdm

Adding all the relevant mutations  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix a subset of #11416  - [x] These changes do not require tests    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21680)

bors-servo · 2018-09-20T06:25:58Z

💔 Test failed - linux-rel-css

paavininanda · 2018-09-20T07:21:32Z

@jdm These all seem to be intermittent failures!

jdm · 2018-09-20T12:37:54Z

I strongly suspect that these test result changes are not intermittent but triggered by #21751.

jdm · 2018-09-20T12:39:16Z

Indeed, the same result appeared on both mac and linux:


        Adding relevant mutations and new test results

paavininanda · 2018-09-20T18:31:47Z

@bors-servo r=jdm

bors-servo · 2018-09-20T18:31:47Z

📌 Commit 3f24d67 has been approved by jdm

bors-servo · 2018-09-20T20:41:33Z

⌛ Testing commit 3f24d67 with merge f813efd...


        Auto merge of #21680 - paavininanda:relevant_mut, r=jdm

Adding all the relevant mutations  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix a subset of #11416  - [x] These changes do not require tests    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21680)

bors-servo · 2018-09-20T21:00:44Z

💔 Test failed - linux-rel-css

jdm · 2018-09-20T21:20:38Z

@bors-servo retry

#21770 #13480

bors-servo · 2018-09-20T22:16:57Z

⌛ Testing commit 3f24d67 with merge fc0c191...


        Auto merge of #21680 - paavininanda:relevant_mut, r=jdm

Adding all the relevant mutations  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix a subset of #11416  - [x] These changes do not require tests    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21680)

bors-servo · 2018-09-20T23:21:51Z

☀️ Test successful - android, android-x86, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing fc0c191 to master...

highfive assigned ferjm Sep 11, 2018

highfive added the S-awaiting-review label Sep 11, 2018

jdm assigned jdm and unassigned ferjm Sep 11, 2018

paavininanda force-pushed the paavininanda:relevant_mut branch 3 times, most recently from 0c572aa to 8099b63 Sep 12, 2018

paavininanda changed the title ~~Relevant mutations [WIP]~~ Adding all the relevant mutations Sep 12, 2018

paavininanda force-pushed the paavininanda:relevant_mut branch from 8099b63 to f3bad89 Sep 12, 2018

jdm requested changes Sep 13, 2018

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Sep 13, 2018

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 14, 2018

jdm requested changes Sep 14, 2018

View changes

paavininanda force-pushed the paavininanda:relevant_mut branch from 033c021 to 9e54f4a Sep 14, 2018

jdm added S-needs-code-changes and removed S-awaiting-review labels Sep 17, 2018

jdm approved these changes Sep 17, 2018

View changes

paavininanda force-pushed the paavininanda:relevant_mut branch from 9e54f4a to 11e2ccd Sep 18, 2018

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 18, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 18, 2018

highfive added the S-awaiting-merge label Sep 20, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 20, 2018

Adding relevant mutations and new test results

Loading status checks…

3f24d67

paavininanda force-pushed the paavininanda:relevant_mut branch from e5e863d to 3f24d67 Sep 20, 2018

highfive added S-awaiting-review and removed S-tests-failed labels Sep 20, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 20, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 20, 2018

highfive added S-awaiting-merge and removed S-tests-failed labels Sep 20, 2018

highfive removed the S-awaiting-merge label Sep 20, 2018

servo / servo

Adding all the relevant mutations #21680

Adding all the relevant mutations #21680

paavininanda commented Sep 11, 2018 •

edited by SimonSapin

highfive commented Sep 11, 2018

paavininanda commented Sep 13, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Sep 17, 2018

jdm commented Sep 17, 2018

jdm commented Sep 18, 2018

bors-servo commented Sep 18, 2018

bors-servo commented Sep 20, 2018

bors-servo commented Sep 20, 2018

paavininanda commented Sep 20, 2018

jdm commented Sep 20, 2018

jdm commented Sep 20, 2018

paavininanda commented Sep 20, 2018

bors-servo commented Sep 20, 2018

bors-servo commented Sep 20, 2018

bors-servo commented Sep 20, 2018

jdm commented Sep 20, 2018

bors-servo commented Sep 20, 2018

bors-servo commented Sep 20, 2018

servo / servo

Join GitHub today

Adding all the relevant mutations #21680

Adding all the relevant mutations #21680

Conversation

paavininanda commented Sep 11, 2018 • edited by SimonSapin

highfive commented Sep 11, 2018

paavininanda commented Sep 13, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Sep 17, 2018

jdm commented Sep 17, 2018

jdm commented Sep 18, 2018

bors-servo commented Sep 18, 2018

bors-servo commented Sep 20, 2018

bors-servo commented Sep 20, 2018

paavininanda commented Sep 20, 2018

jdm commented Sep 20, 2018

jdm commented Sep 20, 2018

paavininanda commented Sep 20, 2018

bors-servo commented Sep 20, 2018

bors-servo commented Sep 20, 2018

bors-servo commented Sep 20, 2018

jdm commented Sep 20, 2018

bors-servo commented Sep 20, 2018

bors-servo commented Sep 20, 2018

paavininanda commented Sep 11, 2018 •

edited by SimonSapin