Implement ask_for_reset for HTMLSelectElement. #7963

dagnir · 2015-10-10T22:36:01Z

highfive · 2015-10-10T22:36:09Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @SimonSapin (or someone else) soon.

frewsxcv · 2015-10-10T22:38:13Z

ask_for_reset (the function you just created) should get called here

dagnir · 2015-10-10T23:05:21Z

Hmmm ask_for_reset calls HTMLOptionElement::SetSeledcted so that will make them mutually recursive. Is this intentional? Or should I be setting HTMLOptionElement::selectedness directly somehow?

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks pending.

Comments from the review on Reviewable.io

frewsxcv · 2015-10-11T00:27:26Z

Or should I be setting HTMLOptionElement::selectedness directly somehow?

You could add a set_selectedness method on HTMLOptionElement so that you could set it directly

Comments from the review on Reviewable.io

frewsxcv · 2015-10-11T01:36:44Z

^{components/script/dom/htmloptionelement.rs, line 153 [r4] (raw file):}
Instead of doing .find(|a| a.is_htmlselectelement) and then HTMLSelectElementCast::to_root(..).unwrap, use filter_map with HTMLSelectElementCast::to_root

http://doc.servo.org/std/iter/trait.Iterator.html#method.filter_map

Let me know if you need help

Comments from the review on Reviewable.io

dagnir · 2015-10-11T01:51:15Z

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

^{components/script/dom/htmloptionelement.rs, line 153 [r4] (raw file):}
Thanks, changed to filter_map.

Comments from the review on Reviewable.io

frewsxcv · 2015-10-11T02:54:19Z

^{components/script/dom/htmlselectelement.rs, line 78 [r5] (raw file):}
This could be simplified to is_none and get_enabled_state

Comments from the review on Reviewable.io

frewsxcv · 2015-10-11T03:11:25Z

^{components/script/dom/htmlselectelement.rs, line 72 [r5] (raw file):}
Since we implement Deref on Root, you don't need to explicitly call .r(). It's the same reason one can do String::from("hello").len() even though len() is a &str method (String Derefs to &str)

Comments from the review on Reviewable.io

frewsxcv · 2015-10-11T21:06:43Z

^{components/script/dom/htmlselectelement.rs, line 78 [r5] (raw file):}
Actually, get_enabled_state might be something different

Comments from the review on Reviewable.io

dagnir · 2015-10-12T02:36:34Z

Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

^{components/script/dom/htmlselectelement.rs, line 72 [r5] (raw file):}
Removed unneeded calls to r().

^{components/script/dom/htmlselectelement.rs, line 78 [r5] (raw file):}
Simplified check.

Comments from the review on Reviewable.io

frewsxcv · 2015-10-13T00:12:38Z

FYI, the latest commit doesn't pass the style checker, one of the lines is too long

https://travis-ci.org/servo/servo/jobs/84851573

dagnir · 2015-10-13T18:16:09Z

Hi @frewsxcv, is there anything else needed for this PR?

frewsxcv · 2015-10-13T19:04:12Z

Have you tried running the web-platform-tests (test-wpt) test suite to see if any tests unexpectedly pass or fail. If you need help doing that, check out this link). If after running the tests, nothing changes, a regression test will need to be written to make sure algorithm this doesn't break in the future. If you need help with any of this or want clarification, please let me know, I'll be happy to help! I'm currently doing IRL non-Servo work at the moment, but when I get home I'll actually read through the spec and make sure your implementation looks correct.

dagnir · 2015-10-13T22:04:09Z

I'm running test-wpt now. If you would show me how to get started with writing the regression test for this, I'd appreciate it!

Thanks!

frewsxcv · 2015-10-14T00:29:36Z

(to anyone who is reading this PR and wondering why we haven't responded, we just chatted 1 on 1 on IRC)

bors-servo · 2015-10-14T20:48:46Z

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

dagnir · 2015-10-17T23:31:55Z

@frewsxcv, I've added some tests, please take a look. Only the third one passes at the moment because ask_for_reset doesn't get called on node insert and remove.

eefriedman · 2015-10-18T01:33:42Z

The relevant hooks for node insert and remove are bind_to_tree() and unbind_from_tree().

dagnir · 2015-10-18T09:38:55Z

Thanks!

frewsxcv · 2015-10-18T15:04:29Z

FYI, in case you missed it, there are merge conflicts that will eventually need to be addressed


        Add fixes based on review.

- Whitespace and indentation issues - call as_for_reset on option insert - add link to 'pick' in standard

dagnir · 2015-10-27T21:55:43Z

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

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

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

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

^{components/script/dom/htmlselectelement.rs, line 73 [r14] (raw file):}
From what I understood from @frewsxcv's comment here: https://reviewable.io/reviews/servo/servo/7963#-K1MJVz-efchWQhrLrCG, this was preferable, unless I misunderstood

^{components/script/dom/htmlselectelement.rs, line 85 [r14] (raw file):}
Done.

Comments from the review on Reviewable.io

eefriedman · 2015-10-27T22:04:21Z

Review status: 0 of 5 files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.

^{components/script/dom/htmlselectelement.rs, line 73 [r14] (raw file):}
if let is generally preferred over is_none()+unwrap(); I don't think that review comment was specifically suggesting the opposite.

Comments from the review on Reviewable.io

dagnir · 2015-10-27T22:09:06Z

Review status: 0 of 5 files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.

^{components/script/dom/htmlselectelement.rs, line 73 [r14] (raw file):}
Hmm is it possible you saw an outdated revision? This is what it currently looks like dagnir@b5e991c

Comments from the review on Reviewable.io

eefriedman · 2015-10-27T22:14:14Z

Review status: 0 of 5 files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.

^{components/script/dom/htmlselectelement.rs, line 73 [r14] (raw file):}
Oh, right, sorry, I somehow got flipped to the wrong revision just now. Still, if let is generally preferred over match.

Comments from the review on Reviewable.io

frewsxcv · 2015-10-27T22:18:59Z

^{components/script/dom/htmlselectelement.rs, line 73 [r14] (raw file):}
"Oh, right, sorry, I somehow got flipped to the wrong revision just now. Still, if let is generally preferred over match."

I disagree, I think they're both perfectly valid. match gives you the benefit of erroring when all cases are not covered.

Comments from the review on Reviewable.io

frewsxcv · 2015-10-27T22:19:35Z

Thanks for looking at this @eefriedman, I've been busy these past few days and haven't had time to review this.

eefriedman · 2015-10-27T22:45:29Z

I disagree, I think they're both perfectly valid. match gives you the benefit of erroring when all cases are not covered.

You're right. I'll narrow it a bit; "if let" is preferred over "match" for Option<T>.

dagnir · 2015-10-27T23:54:22Z

@frewsxcv @eefriedman okay to leave, or should I change it?

eefriedman · 2015-10-28T00:15:29Z

Please change it. Also, please refactor the identical code in SetSelected and ask_for_reset.

dagnir · 2015-10-28T00:36:24Z

@eefriedman I'm not sure what identical code you're referring to; unless you mean the option element traversal in ask_for_reset and pick_option?

eefriedman · 2015-10-28T00:59:28Z

SetSelected:

+        if let Some(select) = self.upcast::<Node>().ancestors()
+                .filter_map(Root::downcast::<HTMLSelectElement>).next() {
+            if selected {
+                select.pick_option(self);
+            }
+            select.ask_for_reset();
+        }

bind_to_tree:

+        if let Some(select) = self.upcast::<Node>().ancestors()
+                .filter_map(Root::downcast::<HTMLSelectElement>)
+                .next() {
+            if self.Selected() {
+                select.pick_option(self);
+            }
+            select.ask_for_reset();
+        }

eefriedman · 2015-10-28T01:03:50Z

Err, oops, I meant SetSelected and bind_to_tree.


        Add fixes based on review.

- Use if let instead of match for Option - Refactor common code into pick_if_selected_and_reset

dagnir · 2015-10-28T01:24:45Z

Thanks, done.

Review status: 0 of 5 files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.

Comments from the review on Reviewable.io

eefriedman · 2015-10-28T02:15:01Z

@bors-servo r+

bors-servo · 2015-10-28T02:15:01Z

📌 Commit 4849033 has been approved by eefriedman

bors-servo · 2015-10-28T02:15:03Z

⌛ Testing commit 4849033 with merge 3951c57...


        Auto merge of #7963 - dagnir:issue-7774, r=eefriedman

Implement ask_for_reset for HTMLSelectElement. Fixes #7774  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7963)

bors-servo · 2015-10-28T02:50:21Z

💔 Test failed - mac-rel-css

frewsxcv · 2015-10-28T02:55:11Z

@bors-servo retry

#8236

bors-servo · 2015-10-28T02:55:12Z

⚡ Previous build results for android, gonk, linux-dev, linux-rel, mac-dev-ref-unit are reusable. Rebuilding only mac-rel-css, mac-rel-wpt...

bors-servo · 2015-10-28T03:22:05Z

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

Ms2ger · 2015-10-28T14:17:57Z

For future reference:

"if let" is preferred over "match" for Option<T>

is not true. It is preferred only for (some) cases where only a single variant has code associated with it.

highfive added the S-awaiting-review label Oct 10, 2015

jdm assigned frewsxcv Oct 13, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Oct 14, 2015

dagnir force-pushed the dagnir:issue-7774 branch 2 times, most recently from 369d5cb to e1fbad2 Oct 17, 2015

dagnir force-pushed the dagnir:issue-7774 branch 3 times, most recently from 0a1ced2 to 2855e2d Oct 18, 2015

highfive removed the S-needs-rebase label Oct 18, 2015

Add fixes based on review.

Loading status checks…

92e0083

- Whitespace and indentation issues - call as_for_reset on option insert - add link to 'pick' in standard

Add fixes based on review.

Loading status checks…

4849033

- Use if let instead of match for Option - Refactor common code into pick_if_selected_and_reset

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 28, 2015

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 28, 2015

bors-servo merged commit 4849033 into servo:master Oct 28, 2015
2 checks passed

2 checks passed

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

homu Test successful
Details

SimonSapin removed the S-tests-failed label Jan 27, 2016

servo / servo

Join GitHub today

Implement ask_for_reset for HTMLSelectElement. #7963

Implement ask_for_reset for HTMLSelectElement. #7963

Conversation

dagnir commented Oct 10, 2015

highfive commented Oct 10, 2015

frewsxcv commented Oct 10, 2015

dagnir commented Oct 10, 2015

frewsxcv commented Oct 11, 2015

frewsxcv commented Oct 11, 2015

dagnir commented Oct 11, 2015

frewsxcv commented Oct 11, 2015

frewsxcv commented Oct 11, 2015

frewsxcv commented Oct 11, 2015

dagnir commented Oct 12, 2015

frewsxcv commented Oct 13, 2015

dagnir commented Oct 13, 2015

frewsxcv commented Oct 13, 2015

dagnir commented Oct 13, 2015

frewsxcv commented Oct 14, 2015

bors-servo commented Oct 14, 2015

dagnir commented Oct 17, 2015

eefriedman commented Oct 18, 2015

dagnir commented Oct 18, 2015

frewsxcv commented Oct 18, 2015

dagnir commented Oct 27, 2015

eefriedman commented Oct 27, 2015

dagnir commented Oct 27, 2015

eefriedman commented Oct 27, 2015

frewsxcv commented Oct 27, 2015

frewsxcv commented Oct 27, 2015

eefriedman commented Oct 27, 2015

dagnir commented Oct 27, 2015

eefriedman commented Oct 28, 2015

dagnir commented Oct 28, 2015

eefriedman commented Oct 28, 2015

eefriedman commented Oct 28, 2015

dagnir commented Oct 28, 2015

eefriedman commented Oct 28, 2015

bors-servo commented Oct 28, 2015

bors-servo commented Oct 28, 2015

bors-servo commented Oct 28, 2015

frewsxcv commented Oct 28, 2015

bors-servo commented Oct 28, 2015

bors-servo commented Oct 28, 2015

Ms2ger commented Oct 28, 2015