Introduce VirtualMethods::attribute_mutated() #7452

nox · 2015-08-30T12:10:22Z

nox · 2015-08-30T12:13:48Z

Amended because of #7412.

jdm · 2015-08-30T13:28:40Z

I'm not keen on the debug_assert changes since we only run tests in release mode right now.

nox · 2015-08-30T13:29:20Z

@jdm Will remove commit.


        Replace many uses of to_ascii_lowercase() by make_ascii_lowercase()


        Replace many uses of Attr::Value() by Attr::value()

The later only borrows the attribute, without copying its value as a string.

jdm · 2015-08-30T15:23:14Z

I like this refactoring. I'd also like to get @Ms2ger's opinion.

Ms2ger · 2015-09-01T07:59:08Z

Will try to finish reviewing today.

Ms2ger · 2015-09-01T14:22:30Z

I hope it won't be too much trouble to match my comments and the code; I reviewed most of it on a train.

     pub fn get_attribute(&self, namespace: &Namespace, local_name: &Atom) -> Option<Root<Attr>> {
         let mut attributes = RootedVec::new();
         self.get_attributes(local_name, &mut attributes);
-        attributes.r().iter()
+        attributes.r().
+        iter()

Not an improvement. (Fixed in another commit? Please fix that.)

+                };
+                // https://github.com/rust-lang/rust/issues/21114
+                is_js

Ugh.

+                self.attrs.borrow().iter().map(|attr| attr.root())

Can we create a method that returns that iterator?

+            attr.namespace() == namespace && attr.local_name() == local_name

The local_name check is much more likely to fail, so check it first.

    after_remove_attr() (i.e. when removing an href attribute from a base element).

I think you mean "e.g." ("for example") rather than "i.e." ("that is").

+    pub fn uint(&self) -> Option<u32> {

With the helper traits gone, it's no longer a pain to document these.

+        mem::swap(&mut *self.value.borrow_mut(), &mut value);

Feels more like a mem::replace.

-        self.do_set_attribute(qname.local, value, name, qname.ns, prefix, |_| false)
+        let in_empty_ns = qname.ns == ns!("");
+        let attr = Attr::new(&window, qname.local, value, name, qname.ns, prefix, Some(self));
+        self.attrs.borrow_mut().push(JS::from_rooted(&attr));

I kinda liked having a single place where we pushed into attrs.

-    pub fn do_set_attribute<F>(&self,
+    fn set_first_matching_attribute<F>(&self,

Reviewing tends to be easier if you split renames into their own commit.

+                                       find: F)
+                                       where F: Fn(&Attr)
+                                       -> bool {

                                       find: F)
    where F: Fn(&Attr) -> bool
{

Your formatting is extremely confusing. I thought you'd managed to stick a where clause between arguments and return type.

+        if let Some(attr) = attr {
+            attr.set_value(value, self);
+        } else {

Seems like a case for match.

             let attr = (*self.attrs.borrow())[idx].root();
-            if attr.r().namespace() == &ns!("") {
-                vtable_for(&NodeCast::from_ref(self)).before_remove_attr(attr.r());
-            }
-
             self.attrs.borrow_mut().remove(idx);

let attr = self.attrs.borrow_mut().remove(idx);

-            &atom!("style") => {
+            &atom!(style) => {

Did that really need to pollute this commit?

+                static FORWARDED_EVENTS: &'static [&'static str] =
+                    &["onfocus", "onload", "onscroll", "onafterprint", "onbeforeprint",

Followup: this should use atoms.

(And I already found two other pre-existing bugs in this code, yay!)

+        if *attr.local_name() == atom!(disabled) {
+            let node = NodeCast::from_ref(self);
+            match mutation {
+                AttributeMutation::Set(Some(_)) => {}

I guess this is correct, but now I want a test for this case.

+            let children = node.children().filter(|node| {
+                if found_legend {
+                    true
+                } else if node.is_htmllegendelement() {
+                    found_legend = true;

I tend to prefer using those methods only with pure closures, but I guess this is fine.

-            _ => ()
+            _ => {},

I prefer ().

+        if attr.local_name() == &atom!(data) && mutation != AttributeMutation::Removed {
+            self.process_data_url();
         }

Please keep the match.

+        if attr.local_name() == &atom!(disabled) {
+            let options = NodeCast::from_ref(self).children().filter(|child| {
+                child.is_htmloptionelement()
+            });

Ditto. Also elsewhere.

Why pass the Attr to attribute_mutated at all? Wouldn't it be better to pass the value in the enum, and the local name explicitly? mutation.new_value(attr) is pretty awkward.

nox · 2015-09-01T14:32:19Z

after_remove_attr() (i.e. when removing an href attribute from a base element).
I think you mean "e.g." ("for example") rather than "i.e." ("that is").

There are no other examples of such a use, but I will reword it.

nox · 2015-09-01T14:42:44Z

Why pass the Attr to attribute_mutated at all? Wouldn't it be better to pass the value in the enum, and the local name explicitly? mutation.new_value(attr) is pretty awkward.

I find mutation.new_value(attr) the opposite of awkward, this is my favourite part of the whole refactoring. Passing the value in the enum would mean borrowing it or cloning it from the set attribute and I didn't want that, in many cases we don't even access it.

nox · 2015-09-01T14:49:11Z

Review status: 0 of 31 files reviewed at latest revision, 18 unresolved discussions, all commit checks successful.

^{components/script/dom/attr.rs, line 92 [r4] (raw file):}

With the helper traits gone, it's no longer a pain to document these.

Will do.

^{components/script/dom/element.rs, line 829 [r3] (raw file):}

Can we create a method that returns that iterator?

Will make a private one.

^{components/script/dom/element.rs, line 830 [r3] (raw file):}

The local_name check is much more likely to fail, so check it first.

Will do.

^{components/script/dom/element.rs, line 833 [r2] (raw file):}

Not an improvement. (Fixed in another commit? Please fix that.)

This is not in one of my commits.

^{components/script/dom/element.rs, line 912 [r4] (raw file):}

I kinda liked having a single place where we pushed into attrs.

I guess I could make a push_new_attr() method that calls Attr::new(), pushes it and handles the virtual method.

^{components/script/dom/element.rs, line 1457 [r4] (raw file):}

Did that really need to pollute this commit?

This is quite the nit, I didn't even expected those would be on the same indent level, and most lines around it changed anyway. Do I really need to address this?

^{components/script/dom/htmlbodyelement.rs, line 141 [r4] (raw file):}

Followup: this should use atoms.

Will file.

^{components/script/dom/htmlbuttonelement.rs, line 143 [r4] (raw file):}

I guess this is correct, but now I want a test for this case.

Will write one.

^{components/script/dom/htmlfieldsetelement.rs, line 93 [r4] (raw file):}
Note to self: put back match here.

^{components/script/dom/htmlfieldsetelement.rs, line 109 [r4] (raw file):}

I tend to prefer using those methods only with pure closures, but I guess this is fine.

^{components/script/dom/htmlfontelement.rs, line 65 [r4] (raw file):}
Note to self: put back match here.

^{components/script/dom/htmliframeelement.rs, line 384 [r4] (raw file):}

I prefer ().

± git grep -F "=> {}" | wc -l
      188
± git grep -F "_ => ()" | wc -l
      118

^{components/script/dom/htmlimageelement.rs, line 305 [r4] (raw file):}
Note to self: put back match here.

^{components/script/dom/htmlobjectelement.rs, line 106 [r4] (raw file):}

Please keep the match.

Will do.

^{components/script/dom/htmloptionelement.rs, line 136 [r4] (raw file):}
Note to self: put back match here.

^{components/script/dom/htmlscriptelement.rs, line 537 [r4] (raw file):}
Note to self: put back match here.

^{components/script/dom/htmltablerowelement.rs, line 67 [r4] (raw file):}
Note to self: put back match here.

^{components/script/dom/htmltablesectionelement.rs, line 66 [r4] (raw file):}
Note to self: put back match here.

Comments from the review on Reviewable.io


        Remove Element::get_attributes()

nox · 2015-09-01T16:32:38Z

Review status: 0 of 31 files reviewed at latest revision, 19 unresolved discussions, all commit checks successful.

^{components/script/dom/attr.rs, line 92 [r4] (raw file):}
Done.

^{components/script/dom/attr.rs, line 243 [r4] (raw file):}

Feels more like a mem::replace.

Feels like a mem::swap, to use mem::replace I would need to either mutably borrow the value twice, or to bind it to a mutable local variable in an inner block to make sure it is relinquished afterwards.

^{components/script/dom/element.rs, line 829 [r3] (raw file):}
Actually I can't, the iterator is behid a Ref<_>.

^{components/script/dom/element.rs, line 830 [r3] (raw file):}
Done.

^{components/script/dom/element.rs, line 912 [r4] (raw file):}
Done.

^{components/script/dom/htmlfieldsetelement.rs, line 93 [r4] (raw file):}
Done.

^{components/script/dom/htmlfontelement.rs, line 65 [r4] (raw file):}
Done.

^{components/script/dom/htmlimageelement.rs, line 305 [r4] (raw file):}
Done.

^{components/script/dom/htmlobjectelement.rs, line 106 [r4] (raw file):}
Done.

^{components/script/dom/htmloptionelement.rs, line 136 [r4] (raw file):}
Done.

^{components/script/dom/htmlscriptelement.rs, line 537 [r4] (raw file):}
Done.

^{components/script/dom/htmltablerowelement.rs, line 67 [r4] (raw file):}
Done.

^{components/script/dom/htmltablesectionelement.rs, line 66 [r4] (raw file):}
Done.

Comments from the review on Reviewable.io

bors-servo · 2015-09-02T11:16:48Z

⌛ Testing commit e91434d with merge 2607a8a...


        Auto merge of #7452 - nox:cleanup-attributes, r=Ms2ger

Introduce VirtualMethods::attribute_mutated()  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7452)

bors-servo · 2015-09-02T11:41:00Z

💔 Test failed - linux-rel

jdm · 2015-09-02T13:03:58Z



/_mozilla/mozilla/enabled_disabled_selectors.html
-------------------------------------------------
FAIL JS tests (Option)
/html/semantics/selectors/pseudo-classes/disabled.html
------------------------------------------------------
FAIL ':disabled' should match only disabled elements
FAIL ':disabled' should not match elements whose disabled attribute has been removed
FAIL ':disabled' should also match elements whose disabled attribute has been set
FAIL ':disabled' should also match elements whose disabled attribute has been set twice
FAIL ':disabled' should also match disabled elements whose type has changed
FAIL ':disabled' should not match elements not in the document

nox · 2015-09-02T13:04:47Z

Failed rebase, probably, I had ran these tests before. :( Will look at.


        Introduce VirtualMethods::attribute_mutated()

This replaces before_remove_attr(), after_remove_attr() and after_set_attr(). The virtual method takes the mutated attribute and an AttributeMutation value to disambiguate between "attribute is changed", "attribute is added" and "attribute is removed". In the case of "attribute is changed", the mutation value contains a reference to the old value of the mutated attribute, which is used to unregister outdated named elements when the "id" attribute is changed on an element. This greatly simplifies the handling of attributes, which in many cases don't have any specific behaviour whether they are removed or changed or added. It also fixes a few bugs where things were put in before_remove_attr() instead of after_remove_attr() (e.g. when removing an href attribute from a base element). A few helper functions in Element were also renamed and made private.

nox · 2015-09-02T13:46:42Z

I had broken HTMLOptGroup. I added a line in the associated test to better improve it.

nox · 2015-09-02T13:48:46Z

@bors-servo r+ r=jdm r=Ms2ger

bors-servo · 2015-09-02T13:48:47Z

📌 Commit 58e1bd0 has been approved by Ms2ger

bors-servo · 2015-09-02T13:48:47Z

📌 Commit 58e1bd0 has been approved by jdm

bors-servo · 2015-09-02T13:48:47Z

📌 Commit 58e1bd0 has been approved by nox

bors-servo · 2015-09-02T14:14:39Z

⌛ Testing commit 58e1bd0 with merge eaf90c0...


        Auto merge of #7452 - nox:cleanup-attributes, r=nox

Introduce VirtualMethods::attribute_mutated()  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7452)

bors-servo · 2015-09-02T14:44:26Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

nox added A-content/dom I-refactor labels Aug 30, 2015

highfive added the S-awaiting-review label Aug 30, 2015

nox force-pushed the nox:cleanup-attributes branch from 4ffbaed to d2838b5 Aug 30, 2015

nox added 2 commits Aug 27, 2015

Replace many uses of to_ascii_lowercase() by make_ascii_lowercase()

105d990

Replace many uses of Attr::Value() by Attr::value()

51418fc

The later only borrows the attribute, without copying its value as a string.

nox force-pushed the nox:cleanup-attributes branch from d2838b5 to d61e884 Aug 30, 2015

frewsxcv mentioned this pull request Aug 31, 2015

Implement obsolete IDL attributes for <font> element #7472

Closed

Ms2ger self-assigned this Sep 1, 2015

jdm added S-needs-code-changes and removed S-awaiting-review labels Sep 1, 2015

nox assigned jdm and Ms2ger and unassigned Ms2ger and jdm Sep 1, 2015

Remove Element::get_attributes()

5672142

nox force-pushed the nox:cleanup-attributes branch from d61e884 to 740f278 Sep 1, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 1, 2015

nox mentioned this pull request Sep 1, 2015

Forwarded events should be referenced as atoms in HTMLBodyElement::attribute_mutated() #7495

Closed

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 2, 2015

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 2, 2015

nox force-pushed the nox:cleanup-attributes branch from e91434d to 58e1bd0 Sep 2, 2015

highfive added S-awaiting-review and removed S-tests-failed labels Sep 2, 2015

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 2, 2015

nox mentioned this pull request Sep 2, 2015

make AttrVal tokens() and atom() return or panic #7511

Merged

bors-servo merged commit 58e1bd0 into servo:master Sep 2, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

This was referenced Sep 2, 2015

Adds support for input element's maxlength attr #7323

Merged

Use cached element ID in LayoutElement::get_id #7068

Closed

nox deleted the nox:cleanup-attributes branch Sep 11, 2015

servo / servo

Introduce VirtualMethods::attribute_mutated() #7452

Introduce VirtualMethods::attribute_mutated() #7452

nox commented Aug 30, 2015

nox commented Aug 30, 2015

jdm commented Aug 30, 2015

nox commented Aug 30, 2015

jdm commented Aug 30, 2015

Ms2ger commented Sep 1, 2015

Ms2ger commented Sep 1, 2015

nox commented Sep 1, 2015

nox commented Sep 1, 2015

nox commented Sep 1, 2015

nox commented Sep 1, 2015

bors-servo commented Sep 2, 2015

bors-servo commented Sep 2, 2015

jdm commented Sep 2, 2015

nox commented Sep 2, 2015

nox commented Sep 2, 2015

nox commented Sep 2, 2015

bors-servo commented Sep 2, 2015

bors-servo commented Sep 2, 2015

bors-servo commented Sep 2, 2015

bors-servo commented Sep 2, 2015

bors-servo commented Sep 2, 2015

servo / servo

Join GitHub today

Introduce VirtualMethods::attribute_mutated() #7452

Introduce VirtualMethods::attribute_mutated() #7452

Conversation

nox commented Aug 30, 2015

nox commented Aug 30, 2015

jdm commented Aug 30, 2015

nox commented Aug 30, 2015

jdm commented Aug 30, 2015

Ms2ger commented Sep 1, 2015

Ms2ger commented Sep 1, 2015

nox commented Sep 1, 2015

nox commented Sep 1, 2015

nox commented Sep 1, 2015

nox commented Sep 1, 2015

bors-servo commented Sep 2, 2015

bors-servo commented Sep 2, 2015

jdm commented Sep 2, 2015

nox commented Sep 2, 2015

nox commented Sep 2, 2015

nox commented Sep 2, 2015

bors-servo commented Sep 2, 2015

bors-servo commented Sep 2, 2015

bors-servo commented Sep 2, 2015

bors-servo commented Sep 2, 2015

bors-servo commented Sep 2, 2015