Implement user interaction task source #10714

KiChjang · 2016-04-19T18:08:45Z

Part of #7959.

This change is

highfive · 2016-04-19T18:08:48Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/htmltextareaelement.rs, components/script/dom/bindings/global.rs, components/script/dom/htmldetailselement.rs, components/script/task_source/user_interaction.rs, components/script/script_thread.rs, components/script/dom/htmlinputelement.rs, components/script/dom/window.rs, components/script/task_source/dom_manipulation.rs

highfive · 2016-04-19T18:08:49Z

Warning

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

Ms2ger · 2016-04-21T13:20:09Z

components/script/dom/bindings/global.rs

@@ -175,10 +176,10 @@ impl<'a> GlobalRef<'a> {

    /// `ScriptChan` used to send messages to the event loop of this global's
    /// thread.
-    pub fn user_interaction_task_source(&self) -> Box<ScriptChan + Send> {
+    pub fn user_interaction_task_source(&self) -> Box<TaskSource<UserInteractionTask> + Send> {


Let's remove this function.

Why though? Is it because workers are not supposed to have the concept of task sources at all? If so, I might just remove all other task sources for GlobalRoot.

Because workers don't have user interaction. The others are likely used by workers as well as the main thread.

KiChjang · 2016-04-25T05:59:14Z

User interaction task source removed from global.rs.

Ms2ger · 2016-04-25T07:49:18Z

-S-awaiting-review +S-needs-code-changes

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

components/script/script_thread.rs, line 927 [r2] (raw file):
Unrelated: revert or stick in a separate commit.

components/script/dom/htmldetailselement.rs, line 88 [r2] (raw file):
Unrelated: revert or stick in a separate commit.

components/script/dom/htmlinputelement.rs, line 552 [r2] (raw file):
You're firing an event that we didn't before; this change should go in a separate commit and have a test.

components/script/dom/htmltextareaelement.rs, line 33 [r2] (raw file):
Random unnecessary change; revert.

components/script/dom/htmltextareaelement.rs, line 266 [r2] (raw file):
So much copy/paste around here that you forgot to rename this binding.

components/script/dom/htmltextareaelement.rs, line 268 [r2] (raw file):
You're firing an event that we didn't before; this change should go in a separate commit and have a test.

components/script/dom/htmltextareaelement.rs, line 374 [r2] (raw file):
Definitely too many clone()s here (and everywhere you copy/pasted this code).

components/script/dom/bindings/global.rs, line 29 [r2] (raw file):
Unused?

components/script/task_source/dom_manipulation.rs, line 43 [r2] (raw file):
Unrelated: revert or stick in a separate commit.

components/script/task_source/user_interaction.rs, line 19 [r2] (raw file):
I think in this case I'd prefer having a queue_event method that takes &Node or &EventTarget.

components/script/task_source/user_interaction.rs, line 20 [r2] (raw file):
So to be clear: this commit does not change anything about the scheduling of tasks: every message still ends up in the same queue? (This should be mentioned in the commit message.)

components/script/task_source/user_interaction.rs, line 30 [r2] (raw file):
I think I'd put the target first, rather than between the fields that define the properties of the event.

components/script/task_source/user_interaction.rs, line 32 [r2] (raw file):
Unused, remove

Comments from Reviewable

KiChjang · 2016-04-26T00:11:13Z

Review status: 1 of 8 files reviewed at latest revision, 13 unresolved discussions.

components/script/task_source/user_interaction.rs, line 19 [r2] (raw file):
Not sure if DOMManipulationTaskSource should be changed as well...

components/script/task_source/user_interaction.rs, line 20 [r2] (raw file):
Yes, every task here is still being routed to the script_thread main loop queue right now, we don't have separate event loops for each task sources yet.

Comments from Reviewable

KiChjang · 2016-04-26T01:38:19Z

Review status: 1 of 8 files reviewed at latest revision, 13 unresolved discussions.

components/script/dom/htmltextareaelement.rs, line 374 [r2] (raw file):
Not too sure what to do about this... @jdm any thoughts in improving this? This channel is only used for creating a new Trusted struct, so I'm thinking maybe that we allow it to accept a TaskSource as well?

components/script/task_source/user_interaction.rs, line 30 [r2] (raw file):
Should I change the definitions in DOMManipulationTask as well?

Comments from Reviewable

jdm · 2016-04-26T13:39:14Z

Review status: 1 of 8 files reviewed at latest revision, 13 unresolved discussions.

components/script/dom/htmltextareaelement.rs, line 374 [r2] (raw file):
This is the state of the art for obtaining a Box<ScriptChan + Send> from the window's main thread sender. Our choices are to 1) add a new API to Window that does this for us (like GlobalRef::script_chan), 2) obtain a GlobalRef value and use GlobalRef::script_chan, 3) modify Trusted::new to use the global of the passed object and do this internally so that callers don't have to.

Comments from Reviewable

bors-servo · 2016-04-27T11:24:53Z

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

bors-servo · 2016-05-11T14:24:36Z

📌 Commit 9617ca5 has been approved by Ms2ger


        Auto merge of #10714 - KiChjang:user-interaction-task, r=Ms2ger

Implement user interaction task source Part of #7959.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10714)

bors-servo · 2016-05-11T14:49:03Z

⌛ Testing commit 9617ca5 with merge 2b6cc6f...

bors-servo · 2016-05-11T15:33:12Z

💔 Test failed - mac-rel-wpt

highfive · 2016-05-11T15:33:16Z

  ▶ TIMEOUT [expected OK] /html/semantics/forms/textfieldselection/textfieldselection-setSelectionRange.html

  ▶ Unexpected subtest result in /html/semantics/forms/textfieldselection/textfieldselection-setSelectionRange.html:
  │ TIMEOUT [expected PASS] input setSelectionRange fires a select event
  └   → Test timed out

  ▶ Unexpected subtest result in /html/semantics/forms/textfieldselection/textfieldselection-setSelectionRange.html:
  │ TIMEOUT [expected PASS] textarea setSelectionRange fires a select event
  └   → Test timed out

KiChjang · 2016-05-11T17:50:25Z

This is not reassuring, since it looks as if the select event was never fired, causing the tests to time out.

highfive · 2016-05-12T01:15:50Z

New code was committed to pull request.

highfive · 2016-05-12T14:41:09Z

New code was committed to pull request.


        Fire 'select' event in SetSelectionRange

highfive · 2016-05-12T14:44:10Z

New code was committed to pull request.

KiChjang · 2016-05-12T14:51:12Z

@bors-servo r=Ms2ger

bors-servo · 2016-05-12T14:51:13Z

📌 Commit f60de52 has been approved by Ms2ger

bors-servo · 2016-05-12T16:00:16Z

⌛ Testing commit f60de52 with merge 4214187...


        Auto merge of #10714 - KiChjang:user-interaction-task, r=Ms2ger

Implement user interaction task source Part of #7959.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10714)

bors-servo · 2016-05-12T16:42:20Z

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

highfive assigned KiChjang Apr 19, 2016

highfive added the S-awaiting-review label Apr 19, 2016

Ms2ger reviewed Apr 21, 2016
View changes

Ms2ger assigned Ms2ger and unassigned KiChjang Apr 22, 2016

Ms2ger added S-needs-code-changes and removed S-awaiting-review labels Apr 22, 2016

KiChjang force-pushed the KiChjang:user-interaction-task branch from ae394d8 to 0ade487 Apr 25, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Apr 25, 2016

KiChjang force-pushed the KiChjang:user-interaction-task branch from 0ade487 to 51e7a38 Apr 25, 2016

Ms2ger added S-needs-code-changes and removed S-awaiting-review labels Apr 25, 2016

KiChjang force-pushed the KiChjang:user-interaction-task branch from 51e7a38 to 7557132 Apr 25, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Apr 25, 2016

KiChjang force-pushed the KiChjang:user-interaction-task branch 4 times, most recently from 8a69f00 to 6ba0ff4 Apr 25, 2016

KiChjang force-pushed the KiChjang:user-interaction-task branch 2 times, most recently from 4cadfb5 to 15c98a4 Apr 26, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels May 11, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels May 11, 2016

KiChjang force-pushed the KiChjang:user-interaction-task branch from 9617ca5 to f7d5de9 May 12, 2016

highfive removed the S-tests-failed label May 12, 2016

highfive added the S-awaiting-review label May 12, 2016

KiChjang force-pushed the KiChjang:user-interaction-task branch from f7d5de9 to a1a2b30 May 12, 2016

Fire 'select' event in SetSelectionRange

Loading status checks…

f60de52

KiChjang force-pushed the KiChjang:user-interaction-task branch from a1a2b30 to f60de52 May 12, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels May 12, 2016

bors-servo merged commit f60de52 into servo:master May 12, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label May 12, 2016

bors-servo mentioned this pull request May 12, 2016

HTML5 Form Validation - Subsequent Steps #10843

Closed

KiChjang deleted the KiChjang:user-interaction-task branch Oct 19, 2017

servo / servo

Implement user interaction task source #10714

Implement user interaction task source #10714

KiChjang commented Apr 19, 2016

highfive commented Apr 19, 2016

highfive commented Apr 19, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KiChjang commented Apr 25, 2016

Ms2ger commented Apr 25, 2016

KiChjang commented Apr 26, 2016

KiChjang commented Apr 26, 2016

jdm commented Apr 26, 2016

bors-servo commented Apr 27, 2016

bors-servo commented May 11, 2016

bors-servo commented May 11, 2016

bors-servo commented May 11, 2016

highfive commented May 11, 2016

KiChjang commented May 11, 2016

highfive commented May 12, 2016

highfive commented May 12, 2016

highfive commented May 12, 2016

KiChjang commented May 12, 2016

bors-servo commented May 12, 2016

bors-servo commented May 12, 2016

bors-servo commented May 12, 2016

servo / servo

Join GitHub today

Implement user interaction task source #10714

Implement user interaction task source #10714

Conversation

KiChjang commented Apr 19, 2016

highfive commented Apr 19, 2016

highfive commented Apr 19, 2016

This comment has been minimized.

Ms2ger Apr 21, 2016

This comment has been minimized.

KiChjang Apr 22, 2016

This comment has been minimized.

Ms2ger Apr 24, 2016

KiChjang commented Apr 25, 2016

Ms2ger commented Apr 25, 2016

KiChjang commented Apr 26, 2016

KiChjang commented Apr 26, 2016

jdm commented Apr 26, 2016

bors-servo commented Apr 27, 2016

bors-servo commented May 11, 2016

bors-servo commented May 11, 2016

bors-servo commented May 11, 2016

highfive commented May 11, 2016

KiChjang commented May 11, 2016

highfive commented May 12, 2016

highfive commented May 12, 2016

highfive commented May 12, 2016

KiChjang commented May 12, 2016

bors-servo commented May 12, 2016

bors-servo commented May 12, 2016

bors-servo commented May 12, 2016