Add comments for the "Constructing the form data set" algorithm

[askobara]

Fixes #7852

[wafflespeanut]

Nice, this should be helpful. Some nits...

---

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

components/script/dom/htmlinputelement.rs, line 445 [r1] (raw file):
Nit: I think it'd be clean if we have the if inside the match arm, and move the comment to the top of the arm.

---

components/script/dom/htmlinputelement.rs, line 448 [r1] (raw file):
Nit: ditto (since there's only one if), and we can forget the braces :)

---

Comments from the review on Reviewable.io

[nox]

/home/travis/build/servo/servo/components/script/dom/htmlinputelement.rs:440:9: 448:10 error: non-exhaustive patterns: `Atom { .. }` not covered [E0004]
/home/travis/build/servo/servo/components/script/dom/htmlinputelement.rs:440         match ty {
/home/travis/build/servo/servo/components/script/dom/htmlinputelement.rs:441             // Step 3.1: it's a button but it is not submitter.
/home/travis/build/servo/servo/components/script/dom/htmlinputelement.rs:442             atom!("submit") | atom!("button") | atom!("reset") if !is_submitter => return None,
/home/travis/build/servo/servo/components/script/dom/htmlinputelement.rs:443             // Step 3.1: it's the "Checkbox" or "Radio Button" and whose checkedness is false.
/home/travis/build/servo/servo/components/script/dom/htmlinputelement.rs:444             atom!("radio") | atom!("checkbox") if !self.Checked() || name.is_empty() => return None,
/home/travis/build/servo/servo/components/script/dom/htmlinputelement.rs:445             atom!("image") | atom!("file") => return None, // Unimplemented
                                                                             ...
/home/travis/build/servo/servo/components/script/dom/htmlinputelement.rs:440:9: 448:10 help: run `rustc --explain E0004` to see a detailed explanation

[KiChjang]

This part here is what's generating the compilation error. You removed the _ match case and added a pattern guard here.

This has different semantics than the code you deleted. Previously it meant "match against all other values of ty, and if their name is empty, return None". Right now it means "match against all other values of ty that has their name empty, and return None". See the difference?

[wafflespeanut]

Yep, sorry I should've been more specific. I meant something like,

match foo {
    foobar => (),
    _ => if thing {
        return None;
    }
}

(braces are unnecessary)

[askobara]

yeah, sorry guys, I'm hurried
thanks @KiChjang for the detailed answer and @wafflespeanut for the example of code, now I hope everything is fine

[wafflespeanut]

@bors-servo try

[KiChjang]

@bors-servo try

[bors-servo]

⌛ Trying commit 8d2f9fc with merge 11cdb56...

[Manishearth]

@bors-servo delegate=Wafflespeanut

[bors-servo]

✌️ @wafflespeanut can now approve this pull request

[Manishearth]

Review status: 1 of 2 files reviewed at latest revision, 3 unresolved discussions.

---

components/script/dom/htmlinputelement.rs, line 445 [r1] (raw file):
If you move the if inside the match arm, the match arm is unconditionally executed. We want it to be able to try the next arms if is_submitter is true

---

Comments from the review on Reviewable.io

[wafflespeanut]

@Manishearth You forgot to mention that the current changes are fine :)

[wafflespeanut]

@bors-servo r+

[bors-servo]

📌 Commit 8d2f9fc has been approved by Wafflespeanut

[KiChjang]

@bors-servo try-

[KiChjang]

@bors-servo try- force

[Manishearth]

@bors-servo try- r=waffles retry force

[bors-servo]

📌 Commit 8d2f9fc has been approved by waffles

[bors-servo]

⌛ Testing commit 8d2f9fc with merge 1d7f296...

[bors-servo]

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