Html options collection#13129 #13380

splav · 2016-09-22T19:22:48Z

Implement HTMLOptionsCollection and related HTMLSelectElement items

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Implement HTMLOptionsCollection #13129 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-09-22T19:22:51Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/htmloptionscollection.rs, components/script/dom/webidls/HTMLOptionsCollection.webidl, components/script/dom/webidls/HTMLSelectElement.webidl, components/script/dom/mod.rs, components/script/dom/htmlselectelement.rs

KiChjang · 2016-09-22T19:53:41Z

components/script/dom/htmloptionscollection.rs

+#[dom_struct]
+pub struct HTMLOptionsCollection {
+    collection: HTMLCollection,
+    root: MutHeap<JS<Node>>


Why is this necessary? The parent class already has this field.

KiChjang · 2016-09-22T20:00:35Z

components/script/dom/htmlselectelement.rs

@@ -28,7 +32,8 @@ use style::element_state::*;

 #[dom_struct]
 pub struct HTMLSelectElement {
-    htmlelement: HTMLElement
+    htmlelement: HTMLElement,
+    options: MutNullableHeap<JS<HTMLOptionsCollection>>,


This shouldn't be nullable.

Why not? It's done so in HTMLFormElement.

It's expensive to create an HTMLFormControlsCollection for HTMLFormElement, but for HTMLSelectElement, creating an HTMLOptionsCollection is just a straightforward filter, hence it isn't necessary.

Looks like this isn't possible to create an HTMLOptionsCollection during the creation of HTMLSelectElement. Ignore my comment and leave it as-is.

KiChjang · 2016-09-22T20:01:12Z

components/script/dom/htmlselectelement.rs

+        let options = HTMLOptionsCollection::new(window.r(), self.upcast(), filter);
+        self.options.set(Some(&options));
+        options
+      }


nit: Indentation.

KiChjang · 2016-09-22T20:01:42Z

components/script/dom/htmlselectelement.rs

+
+    // https://html.spec.whatwg.org/multipage/#dom-select-item
+    fn Item(&self, index: u32) -> Option<Root<Element>> {
+        self.Options().IndexedGetter(index)


Why is this not Item(index) instead?

There is no such method for HTMLOptionsCollection.

KiChjang · 2016-09-22T20:08:04Z

components/script/dom/htmloptionscollection.rs

+
+    // https://html.spec.whatwg.org/multipage/#dom-htmloptionscollection-add
+    fn Add(&self, element: HTMLOptionElementOrHTMLOptGroupElement, before: Option<HTMLElementOrLong >) -> ErrorResult {
+        // TODO


What is this TODO doing here?

KiChjang · 2016-09-22T20:24:34Z

components/script/dom/htmloptionscollection.rs

+            let name = QualName::new(ns!(html), name_atom.clone());
+            let element = Element::create(name, None, document.r(), ElementCreator::ScriptCreated);
+            let node = element.r().upcast::<Node>();
+            try!(Node::pre_insert(node, root.r(), None));


This looks pretty messy, you can achieve the same simply by using the AppendChild WebIDL function on Node and HTMLOptionElement::new.

KiChjang · 2016-09-22T20:36:40Z

components/script/dom/htmlselectelement.rs

+//     // https://html.spec.whatwg.org/multipage/#dom-select-setter
+//     fn IndexedSetter(&self, index: u32, value: Option<&HTMLOptionElement>) -> ErrorResult {
+//         self.Options().IndexedSetter(index, value)
+//     }


Why is this commented out?

KiChjang · 2016-09-22T20:36:49Z

components/script/dom/htmlselectelement.rs

+//     // https://html.spec.whatwg.org/multipage/#dom-select-remove
+//     fn Remove(&self, index: i32) -> ErrorResult {
+//         self.Options().Remove(index)
+//     }


Why is this commented out?

KiChjang · 2016-09-22T20:37:11Z

components/script/dom/htmloptionscollection.rs

+            element.r().Remove();
+            Ok(())
+        } else {
+            Err(Error::NotFound)


The spec doesn't say that you should throw a NotFoundError in this case, instead it says to abort the algorithm.

KiChjang · 2016-09-22T20:52:56Z

components/script/dom/htmloptionscollection.rs

+        if delta < 0 {
+            // new length is lower - deleting last option elements
+            for index in (length..current_length).rev() {
+                self.Remove(index as i32).unwrap()


Unwrapping is not fine here, since it can fail. If you don't care about the result of removing, you can do let _ = self.Remove(index as i32);

KiChjang · 2016-09-22T21:01:37Z

components/script/dom/htmlselectelement.rs

-            .filter_map(Root::downcast::<HTMLOptionElement>)
-            .nth(index as usize)
-            .map(|item| Root::from_ref(item.upcast()))
+        self.Options().IndexedGetter(index)


Why is this not self.Options().Item(index)?

Looks like you ignored my review comment here.

Same as the previous similar - there is no such method (Item) for HTMLOptionsCollection.

KiChjang · 2016-09-24T07:18:06Z

components/script/dom/htmloptionscollection.rs

+    fn add_new_elements(&self, count: u32) -> ErrorResult {
+        let root = self.collection.get_root();
+        let document = document_from_node(root.r());
+        let name_atom = Atom::from(DOMString::from("option".to_owned()));


let name_atom = atom!("option");

splav · 2016-09-24T17:40:42Z

@KiChjang fixed latest one, fixed indentation in idl file.

splav · 2016-09-24T18:14:18Z

@KiChjang as for Item method - how are they used from JS? The collection should inherit these methods from HTMLCollection, but binding generator does not require them for HTMLOptionsCollection. Should I implement it anyway?

KiChjang · 2016-09-25T06:47:23Z

What you need to do is to .upcast::<HTMLCollection>() the resulting HTMLOptionsCollection, and call Item() on it.

KiChjang · 2016-09-25T06:49:05Z

components/script/dom/htmloptionscollection.rs

+    fn add_new_elements(&self, count: u32) -> ErrorResult {
+        let root = self.collection.get_root();
+        let document = document_from_node(root.r());
+        let name_atom = atom!("option");


I missed this in my previous review, but name_atom is completely unnecessary. You can just call atom!("option") directly in HTMLOptionElement::new.

KiChjang · 2016-09-25T06:50:41Z

components/script/dom/htmloptionscollection.rs

+        for _ in 0..count {
+            let element = HTMLOptionElement::new(name_atom.clone(), None, document.r());
+            let node = element.upcast::<Node>();
+            try!(root.AppendChild(node).map(|_| ()));


Why is the map necessary?

AppentChild returns Fallable

Presumably this function returns ErrorResult, which is Fallible<(), Error>.

Yes, AppendChild returns Fallible<Root<Node>, Error> and the function returns Fallible<(), Error> so I had to map Root<Node> to ().

KiChjang · 2016-09-25T06:53:52Z

components/script/dom/htmlcollection.rs

@@ -229,6 +229,9 @@ impl HTMLCollection {
        }
    }

+    pub fn get_root(&self) -> Root<Node> {


Let's call this root_node instead.

KiChjang · 2016-09-25T07:03:32Z

components/script/dom/htmloptionscollection.rs

+
+        let node = match element {
+            HTMLOptionElementOrHTMLOptGroupElement::HTMLOptionElement(ref element) => element.upcast::<Node>(),
+            HTMLOptionElementOrHTMLOptGroupElement::HTMLOptGroupElement(ref element) => element.upcast::<Node>(),


HTMLOptionElementOrHTMLOptGroupElement::HTMLOptionElement(ref el) | HTMLOptionElementOrHTMLOptGroupElement::HTMLOptGroupElement(ref el) => el.upcast::<Node>()

I'm not sure the above works, but if not, use the following:

let node: &Node = match element { // same as above, but use element.upcast() instead of element.upcast::<Node>() }

I don't believe either of those will work - match arms that are combined must contain the exact same types.
edit: oh, I think I'm misreading your second suggestion. If "as above" means the original code, then that's fine.

KiChjang · 2016-09-25T07:07:16Z

components/script/dom/htmloptionscollection.rs

+
+    fn add_new_elements(&self, count: u32) -> ErrorResult {
+        let root = self.collection.get_root();
+        let document = document_from_node(root.r());


Not sure why it has to be root.r(), but self works here.

Self is not derived from Node. Base 'class' is HTMLCollection.

bors-servo · 2016-09-26T15:41:46Z

🔒 Merge conflict

bors-servo · 2016-09-26T15:43:02Z

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

…o#13333

…ement

…ction

jdm · 2016-09-26T19:44:28Z

@bors-servo: r=KiChjang

bors-servo · 2016-09-26T19:44:29Z

📌 Commit 6d5791f has been approved by KiChjang

jdm · 2016-09-26T20:27:01Z

@bors-servo: r=KiChjang

bors-servo · 2016-09-26T20:27:02Z

📌 Commit 80c70e3 has been approved by KiChjang

bors-servo · 2016-09-26T20:49:46Z

⌛ Testing commit 80c70e3 with merge 7de13a6...

Html options collection#13129  Implement HTMLOptionsCollection and related HTMLSelectElement items ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13129 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13380)

bors-servo · 2016-09-26T21:56:01Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

highfive assigned asajeffrey Sep 22, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 22, 2016

jdm added the S-needs-rebase There are merge conflict errors. label Sep 22, 2016

splav force-pushed the HTMLOptionsCollection#13129 branch from 7340655 to 26659af Compare September 22, 2016 20:53

KiChjang suggested changes Sep 22, 2016

View reviewed changes

KiChjang added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Sep 22, 2016

KiChjang reviewed Sep 22, 2016

View reviewed changes

KiChjang assigned KiChjang and unassigned asajeffrey Sep 22, 2016

splav force-pushed the HTMLOptionsCollection#13129 branch from 26659af to 47aaf09 Compare September 23, 2016 22:33

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Sep 23, 2016

KiChjang reviewed Sep 24, 2016

View reviewed changes

splav force-pushed the HTMLOptionsCollection#13129 branch 3 times, most recently from 3dfce35 to c8574ec Compare September 24, 2016 16:50

KiChjang reviewed Sep 25, 2016

View reviewed changes

KiChjang added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Sep 25, 2016

splav force-pushed the HTMLOptionsCollection#13129 branch from c8574ec to 0245c9c Compare September 25, 2016 14:42

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-tests-failed The changes caused existing tests to fail. labels Sep 26, 2016

highfive added the S-needs-rebase There are merge conflict errors. label Sep 26, 2016

splav added 3 commits September 26, 2016 21:49

remove link to servo#5876 due to everything has been fixed in PR serv…

6bda471

…o#13333

implement HTMLOptionsCollection and add related items to HTMLSelectEl…

cf284cc

…ement

fix existing test expectations and add new tests for HTMLOptionsColle…

2173545

…ction

splav force-pushed the HTMLOptionsCollection#13129 branch from de23c13 to 6d5791f Compare September 26, 2016 18:56

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Sep 26, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Sep 26, 2016

add HTMLOptionsCollection to public interfaces lists

80c70e3

splav force-pushed the HTMLOptionsCollection#13129 branch from 6d5791f to 80c70e3 Compare September 26, 2016 20:20

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Sep 26, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Sep 26, 2016

bors-servo merged commit 80c70e3 into servo:master Sep 26, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Sep 26, 2016

splav deleted the HTMLOptionsCollection#13129 branch September 27, 2016 04:45

Html options collection#13129 #13380

Html options collection#13129 #13380

Conversation

splav commented Sep 22, 2016 • edited by larsbergstrom

highfive commented Sep 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KiChjang Sep 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KiChjang Sep 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

splav commented Sep 24, 2016

splav commented Sep 24, 2016

KiChjang commented Sep 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

splav Sep 25, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KiChjang Sep 25, 2016 • edited

Choose a reason for hiding this comment

jdm Sep 25, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors-servo commented Sep 26, 2016

bors-servo commented Sep 26, 2016

jdm commented Sep 26, 2016

bors-servo commented Sep 26, 2016

jdm commented Sep 26, 2016

bors-servo commented Sep 26, 2016

bors-servo commented Sep 26, 2016

bors-servo commented Sep 26, 2016

splav commented Sep 22, 2016 •

edited by larsbergstrom

KiChjang Sep 22, 2016 •

edited

KiChjang Sep 22, 2016 •

edited

splav Sep 25, 2016 •

edited

KiChjang Sep 25, 2016 •

edited

jdm Sep 25, 2016 •

edited