[WIP] Implement live NodeList #22264

KiChjang · 2018-11-24T02:01:56Z

Creating this PR just to see what WPT says for now.

This change is

highfive · 2018-11-24T02:01:59Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/nodelist.rs, components/script/dom/document.rs, components/script/dom/bindings/trace.rs
@KiChjang: components/script/dom/nodelist.rs, components/script/dom/document.rs, components/script/dom/bindings/trace.rs

highfive · 2018-11-24T02:02:01Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

KiChjang · 2018-11-24T02:37:18Z

@bors-servo try=wpt

bors-servo · 2018-11-24T02:37:23Z

⌛ Trying commit 90fcb1c with merge 04a2f73...


        Auto merge of #22264 - KiChjang:live-nodelist, r=<try>

[WIP] Implement live NodeList Creating this PR just to see what WPT says for now.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22264)

bors-servo · 2018-11-24T03:06:06Z

💔 Test failed - linux-rel-wpt

KiChjang · 2018-11-25T06:01:30Z

@bors-servo try=wpt

bors-servo · 2018-11-25T06:01:35Z

⌛ Trying commit 896f34d with merge cf254ae...


        Auto merge of #22264 - KiChjang:live-nodelist, r=<try>

[WIP] Implement live NodeList Creating this PR just to see what WPT says for now.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22264)

bors-servo · 2018-11-25T06:21:49Z

💔 Test failed - linux-rel-css

emilio · 2018-11-25T20:11:02Z

components/script/dom/bindings/trace.rs

@@ -372,6 +373,7 @@ unsafe impl<A: JSTraceable, B: JSTraceable, C: JSTraceable> JSTraceable for (A,

 unsafe_no_jsmanaged_fields!(ActiveAttribInfo);
 unsafe_no_jsmanaged_fields!(ActiveUniformInfo);
+unsafe_no_jsmanaged_fields!(Box<dyn Fn(&Node) -> Box<dyn Iterator<Item = DomRoot<Node>>> + 'static>);


Hmm, how is this safe at all?

I had my huge doubts as well, and eventually I created a trait to encapsulate this idea instead.

emilio · 2018-11-25T20:13:15Z

components/script/dom/document.rs

-                .get_attribute(&ns!(), &local_name!("name"))
-                .map_or(false, |attr| &**attr.value() == &*name)
-        })
+        let doc = self.GetDocumentElement();


Why using the document element instead of self? A document is a node as well.

Yeah, I think there wasn't any reason why the document element needs to be used. Let's update this.

emilio

I think we need to come up with something a bit better perf wise... What gecko does is using nsIMutationObserver on the root, which is effectively a combination of the children_changed / attribute_changed/ etc functions.

emilio · 2018-11-25T20:23:22Z

components/script/dom/document.rs

+            Some(doc_node) => NodeList::new_live_list(&self.window, doc_node, move |root| {
+                let name = name.clone();
+                Box::new(
+                    root.traverse_preorder()


Calling this function every time you access an element or the length of the collection is going to be terrible for performance.

The last one needs to be cached.

KiChjang · 2018-12-02T12:02:08Z

@bors-servo try=wpt

bors-servo · 2018-12-02T12:02:14Z

⌛ Trying commit e8e6d57 with merge f1ca1c0...


        Auto merge of #22264 - KiChjang:live-nodelist, r=<try>

[WIP] Implement live NodeList Creating this PR just to see what WPT says for now.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22264)

emilio · 2018-12-08T19:48:51Z

@bors-servo try=wpt

bors-servo · 2018-12-08T19:48:56Z

⌛ Trying commit a7b9be1 with merge b604022...


        Auto merge of #22264 - KiChjang:live-nodelist, r=<try>

[WIP] Implement live NodeList Creating this PR just to see what WPT says for now.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22264)

bors-servo · 2018-12-08T20:16:08Z

💔 Test failed - linux-rel-css

bors-servo · 2019-03-11T20:32:44Z

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


        Implement live NodeList


        Use live list in form control elment's label method


        Introduce LiveListGenerator trait

Use document instead of document element in GetElementsByName


        Cache live DOM node lists

KiChjang · 2019-07-04T16:41:23Z

@bors-servo try=wpt

bors-servo · 2019-07-04T16:41:27Z

⌛ Trying commit cae0f59 with merge 0624356...


        Auto merge of #22264 - KiChjang:live-nodelist, r=<try>

[WIP] Implement live NodeList Creating this PR just to see what WPT says for now.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22264)

bors-servo · 2019-07-04T17:07:56Z

💔 Test failed - linux-rel-css

bors-servo · 2019-07-31T16:13:17Z

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

pshaughn · 2020-01-06T14:47:04Z

I apologize for not noticing this PR when I started working on my own solutions to node list liveness issues. I hope you're not upset that I've been duplicating some of your work. If you want to be the one who continues taking this forward, maybe some of what I've written will help you with the subcases you haven't already solved.

KiChjang · 2020-01-11T19:35:40Z

Fixed by #25435.

KiChjang · 2020-01-11T22:29:32Z

@pshaughn No worries! I was asking just to see if anything from this PR was useful to yours. Thanks for picking this up!

highfive assigned SimonSapin Nov 24, 2018

highfive added the S-awaiting-review label Nov 24, 2018

highfive added the S-tests-failed label Nov 24, 2018

KiChjang force-pushed the KiChjang:live-nodelist branch from 90fcb1c to a0d358b Nov 24, 2018

highfive removed the S-tests-failed label Nov 24, 2018

KiChjang force-pushed the KiChjang:live-nodelist branch from a0d358b to 9ca312e Nov 25, 2018

highfive added the S-tests-failed label Nov 25, 2018

KiChjang force-pushed the KiChjang:live-nodelist branch from 896f34d to 50f111e Nov 25, 2018

highfive removed the S-tests-failed label Nov 25, 2018

emilio reviewed Nov 25, 2018

View changes

KiChjang force-pushed the KiChjang:live-nodelist branch 5 times, most recently from 1ba54dc to e8e6d57 Nov 26, 2018

KiChjang force-pushed the KiChjang:live-nodelist branch from 558ceaa to a7b9be1 Dec 7, 2018

highfive added the S-tests-failed label Dec 8, 2018

highfive added the S-needs-rebase label Mar 11, 2019

KiChjang added 4 commits Nov 24, 2018

Implement live NodeList

dcee51d

Use live list in form control elment's label method

9857e6b

Introduce LiveListGenerator trait

233a7ff

Use document instead of document element in GetElementsByName

Cache live DOM node lists

Loading status checks…

cae0f59

KiChjang force-pushed the KiChjang:live-nodelist branch from a7b9be1 to cae0f59 Jul 4, 2019

highfive removed the S-tests-failed label Jul 4, 2019

highfive added the S-tests-failed label Jul 4, 2019

KiChjang removed the S-needs-rebase label Jul 5, 2019

highfive added the S-needs-rebase label Jul 31, 2019

KiChjang mentioned this pull request Jan 6, 2020

RadioNodeList liveness #25435

Merged

4 of 4 tasks complete

pshaughn mentioned this pull request Jan 6, 2020

Make labelable element .labels a live list in tree order #25424

Merged

4 of 4 tasks complete

asajeffrey mentioned this pull request Jan 6, 2020

make Document.getElementsByName a live collection #25428

Merged

4 of 4 tasks complete

KiChjang closed this Jan 11, 2020

KiChjang deleted the KiChjang:live-nodelist branch Jan 11, 2020

servo / servo

[WIP] Implement live NodeList #22264

[WIP] Implement live NodeList #22264

KiChjang commented Nov 24, 2018 •

edited by SimonSapin

highfive commented Nov 24, 2018

highfive commented Nov 24, 2018

KiChjang commented Nov 24, 2018

bors-servo commented Nov 24, 2018

bors-servo commented Nov 24, 2018

KiChjang commented Nov 25, 2018

bors-servo commented Nov 25, 2018

bors-servo commented Nov 25, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio left a comment

This comment has been minimized.

This comment has been minimized.

KiChjang commented Dec 2, 2018

bors-servo commented Dec 2, 2018

emilio commented Dec 8, 2018

bors-servo commented Dec 8, 2018

bors-servo commented Dec 8, 2018

bors-servo commented Mar 11, 2019

KiChjang commented Jul 4, 2019

bors-servo commented Jul 4, 2019

bors-servo commented Jul 4, 2019

bors-servo commented Jul 31, 2019

pshaughn commented Jan 6, 2020

KiChjang commented Jan 11, 2020

KiChjang commented Jan 11, 2020

servo / servo

Join GitHub today

[WIP] Implement live NodeList #22264

[WIP] Implement live NodeList #22264

Conversation

KiChjang commented Nov 24, 2018 • edited by SimonSapin

highfive commented Nov 24, 2018

highfive commented Nov 24, 2018

KiChjang commented Nov 24, 2018

bors-servo commented Nov 24, 2018

bors-servo commented Nov 24, 2018

KiChjang commented Nov 25, 2018

bors-servo commented Nov 25, 2018

bors-servo commented Nov 25, 2018

This comment has been minimized.

emilio Nov 25, 2018

This comment has been minimized.

KiChjang Nov 26, 2018

This comment has been minimized.

emilio Nov 25, 2018

This comment has been minimized.

KiChjang Nov 26, 2018

emilio left a comment

This comment has been minimized.

emilio Nov 25, 2018

This comment has been minimized.

nox Nov 27, 2018

KiChjang commented Dec 2, 2018

bors-servo commented Dec 2, 2018

emilio commented Dec 8, 2018

bors-servo commented Dec 8, 2018

bors-servo commented Dec 8, 2018

bors-servo commented Mar 11, 2019

KiChjang commented Jul 4, 2019

bors-servo commented Jul 4, 2019

bors-servo commented Jul 4, 2019

bors-servo commented Jul 31, 2019

pshaughn commented Jan 6, 2020

KiChjang commented Jan 11, 2020

KiChjang commented Jan 11, 2020

KiChjang commented Nov 24, 2018 •

edited by SimonSapin