Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unify Window task sources and task cancellers API #21154

Closed
gterzian opened this issue Jul 10, 2018 · 3 comments
Closed

Unify Window task sources and task cancellers API #21154

gterzian opened this issue Jul 10, 2018 · 3 comments

Comments

@gterzian
Copy link
Member

@gterzian gterzian commented Jul 10, 2018 •
edited

#21126 (comment)

we should do something to avoid all of the duplication of calling foo_task_source and then getting a canceller where it's easy to end up with one for a different source. Possibly the foo_task_source APIs could return both a source and a canceller at the same time.

Note there are two current use-cases:

  1. A script-thread task is enqueued 'directly from the main script-thread/event-loop', like in

    servo/components/script/dom/htmldetailselement.rs

    Line 72 in a9022be

    let _ = window.dom_manipulation_task_source().queue(
    where TaskSource.queue is called, and the task source itself is responsible for 'selecting' the right task canceller for the queued task.
  2. A script-thread task is enqueued 'from code running in parallel', like in

    servo/components/script/dom/filereader.rs

    Line 491 in 2bc70e7

    task_source.queue_with_canceller(task, &canceller).unwrap();
    where queue_with_canceller is called directly from the parallel steps of the algorithm.
@gterzian gterzian mentioned this issue Jul 10, 2018
Merged
0 of 5 tasks complete
@gterzian
Copy link
Member Author

@gterzian gterzian commented Sep 21, 2018

@AgustinCB This one could be of interest to you as well, given your prior work with task-sources(or perhaps you'd rather do other things actually :) )

It's not completely clear what such "unified API" should look like, so this will require some initial exploration with the code...
@AgustinCB
Copy link
Contributor

@AgustinCB AgustinCB commented Sep 21, 2018

Awesome! Will give it a try. Thanks for letting me know!
@AgustinCB
Copy link
Contributor

@AgustinCB AgustinCB commented Sep 23, 2018

I'll have a PR for this tomorrow, hopefully.
@AgustinCB AgustinCB mentioned this issue Sep 24, 2018
Merged
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Oct 3, 2018
@bors-servo
Auto merge of #21804 - AgustinCB:unify-task-source-canceller-api, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
594dd5e 
Unify the task source and task canceller API

To do so, I created a struct `TaskManagement(TaskSource,
TaskCanceller)` and made `*_task_source` return that instead of just
the task source.

Next, I refactored all places in which `task_canceller` by basically
removing them in favour of a previously called `*_task_source`.

I tried to make `task_canceller` a private method in `Window`, with the
hope of enforcing the use of `*_task_source`. However, it's used in
components/script/dom/globalscope.rs:575 in such a way that will make it
harder to avoid. I decided to leave it that way.

It'd be possible to unify `*_task_source` in such a way that we would
have only one method. However, I decided not to do it because one of the
`TaskSource` implementations is special:
`history_traversal_task_source`. Not wanting to over complicate things,
I decided to leave the structure this way.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21154 (github issue number if applicable).

- [x] These changes do not require tests because it's refactoring code that should already be tested.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21804)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Oct 4, 2018
@bors-servo
Auto merge of #21804 - AgustinCB:unify-task-source-canceller-api, r=<…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
f5df185 
…try>

Unify the task source and task canceller API

To do so, I created a struct `TaskManagement(TaskSource,
TaskCanceller)` and made `*_task_source` return that instead of just
the task source.

Next, I refactored all places in which `task_canceller` by basically
removing them in favour of a previously called `*_task_source`.

I tried to make `task_canceller` a private method in `Window`, with the
hope of enforcing the use of `*_task_source`. However, it's used in
components/script/dom/globalscope.rs:575 in such a way that will make it
harder to avoid. I decided to leave it that way.

It'd be possible to unify `*_task_source` in such a way that we would
have only one method. However, I decided not to do it because one of the
`TaskSource` implementations is special:
`history_traversal_task_source`. Not wanting to over complicate things,
I decided to leave the structure this way.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21154 (github issue number if applicable).

- [x] These changes do not require tests because it's refactoring code that should already be tested.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21804)
<!-- Reviewable:end -->
@AgustinCB AgustinCB mentioned this issue Oct 12, 2018
Open
bors-servo added a commit that referenced this issue Nov 13, 2018
@bors-servo
Auto merge of #21804 - AgustinCB:unify-task-source-canceller-api, r=<…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
8b47358 
…try>

Unify the task source and task canceller API

To do so, I created a struct `TaskManagement(TaskSource,
TaskCanceller)` and made `*_task_source` return that instead of just
the task source.

Next, I refactored all places in which `task_canceller` by basically
removing them in favour of a previously called `*_task_source`.

I tried to make `task_canceller` a private method in `Window`, with the
hope of enforcing the use of `*_task_source`. However, it's used in
components/script/dom/globalscope.rs:575 in such a way that will make it
harder to avoid. I decided to leave it that way.

It'd be possible to unify `*_task_source` in such a way that we would
have only one method. However, I decided not to do it because one of the
`TaskSource` implementations is special:
`history_traversal_task_source`. Not wanting to over complicate things,
I decided to leave the structure this way.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21154 (github issue number if applicable).

- [x] These changes do not require tests because it's refactoring code that should already be tested.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21804)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Nov 16, 2018
@bors-servo
Auto merge of #21804 - AgustinCB:unify-task-source-canceller-api, r=g…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
a2937d9 
…terzian

Unify the task source and task canceller API

To do so, I created a struct `TaskManagement(TaskSource,
TaskCanceller)` and made `*_task_source` return that instead of just
the task source.

Next, I refactored all places in which `task_canceller` by basically
removing them in favour of a previously called `*_task_source`.

I tried to make `task_canceller` a private method in `Window`, with the
hope of enforcing the use of `*_task_source`. However, it's used in
components/script/dom/globalscope.rs:575 in such a way that will make it
harder to avoid. I decided to leave it that way.

It'd be possible to unify `*_task_source` in such a way that we would
have only one method. However, I decided not to do it because one of the
`TaskSource` implementations is special:
`history_traversal_task_source`. Not wanting to over complicate things,
I decided to leave the structure this way.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21154 (github issue number if applicable).

- [x] These changes do not require tests because it's refactoring code that should already be tested.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21804)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Unify the task source and task canceller API
2 participants
@AgustinCB @gterzian
You can’t perform that action at this time.