Skip to content
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

Refactor `CommonScriptMsg::Task` to include the name of the task-source #21527

Closed
gterzian opened this issue Aug 27, 2018 · 9 comments
Closed

Refactor `CommonScriptMsg::Task` to include the name of the task-source #21527

gterzian opened this issue Aug 27, 2018 · 9 comments

Comments

@gterzian
Copy link
Member

@gterzian gterzian commented Aug 27, 2018

Message enum is at:

Task(ScriptThreadEventCategory, Box<TaskBox>, Option<PipelineId>),

Name of task sources is found at:

pub enum TaskSourceName {

The CommonScriptMsg::Task should include the task-source name.

Follow up on #21388

See 263ad79#r212802334

@AgustinCB
Copy link
Contributor

@AgustinCB AgustinCB commented Aug 28, 2018

I'd like to give this a shot.

@gterzian
Copy link
Member Author

@gterzian gterzian commented Aug 28, 2018

You're welcome to do so. Depending on when #21388 merges, I will either have to rebase off of your work, or you could integrate yours into the TaskQueue...

The later would require changing fn task_category(&self) -> Option<&ScriptThreadEventCategory> into something like fn task_source_name(&self) -> Option<&TaskSourceName> and updating it's use accordingly...
see https://github.com/servo/servo/pull/21388/files#diff-eac74606d6d2a29782f87f355cd3e0bbR23

Some other methods of QueuedTaskConversion, and their use, would require slight adaptations to use the TaskSourceName instead of the ScriptThreadEventCategory...

For example here, obviously the task-source name could be used directly, instead of doing it via the category: https://github.com/servo/servo/pull/21388/files#diff-eac74606d6d2a29782f87f355cd3e0bbR63

and here too: https://github.com/servo/servo/pull/21388/files#diff-eac74606d6d2a29782f87f355cd3e0bbR89

@AgustinCB
Copy link
Contributor

@AgustinCB AgustinCB commented Aug 28, 2018

Awesome! I'll start working on this in the afternoon. Thanks.

@AgustinCB
Copy link
Contributor

@AgustinCB AgustinCB commented Aug 31, 2018

I just finished the integration previous to your commit. I just rebased and started the other instructions. Hopefully will have a pr in the weekend :)

@gterzian
Copy link
Member Author

@gterzian gterzian commented Aug 31, 2018

Great, looking forward to it.

One thing I didn't mention previously, is that the TaskSourceName also should be added to the QueuedTask

Also, we still want to keep ScriptThreadEventCategory where it is, we just want to use the TaskSourceName instead of it where appropriate(like I mentioned previously)

If anything is unclear, don't hesitate to open a PR with what you have so we can discuss it concretely...

@AgustinCB
Copy link
Contributor

@AgustinCB AgustinCB commented Sep 2, 2018

There are some tests failing with this change that I wasn't expecting. I'm digging into them to understand why.

@AgustinCB AgustinCB mentioned this issue Sep 2, 2018
4 of 5 tasks complete
bors-servo added a commit that referenced this issue Sep 3, 2018
Add task source name

Refactor `CommonScriptMsg::Task` to include `TaskSourceName`.

Sorry for the delay, between doing this after work and the time I spent trying to ramp up on the project, it took me a bit longer than I expected.

Test still don't pass in local, but they fail the same way on master, so I guess it's ok.

I may have forgotten something, I refactored mostly the stuff that the compiler complained about. Please let me know if I missed anything.

I tried to dump my thought process on the commit messages, so feel free to go commit by commit to understand context.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21527

- [ ] There are tests for these changes OR
- [x] These changes do not require tests because it's mostly refactor, no new behavior added.

<!-- 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/21583)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Sep 4, 2018
Add task source name

Refactor `CommonScriptMsg::Task` to include `TaskSourceName`.

Sorry for the delay, between doing this after work and the time I spent trying to ramp up on the project, it took me a bit longer than I expected.

Test still don't pass in local, but they fail the same way on master, so I guess it's ok.

I may have forgotten something, I refactored mostly the stuff that the compiler complained about. Please let me know if I missed anything.

I tried to dump my thought process on the commit messages, so feel free to go commit by commit to understand context.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21527

- [ ] There are tests for these changes OR
- [x] These changes do not require tests because it's mostly refactor, no new behavior added.

<!-- 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/21583)
<!-- Reviewable:end -->
@AgustinCB
Copy link
Contributor

@AgustinCB AgustinCB commented Sep 4, 2018

Sorry for commenting in a closed issue, but I didn't know where else I could ask this:

I'd like to keep contributing to the project. I was thinking in addressing #21589 and #21590. They seem pretty straight forward, so I don't expect to be blocked there for long. What other issues could I take? (keeping in mind that I do this after work and I can sadly allocate 5-10 hours/week to the project).

Thanks!

@gterzian
Copy link
Member Author

@gterzian gterzian commented Sep 5, 2018

@AgustinCB thanks for your help.

I think you might also want to take a look at those issues: https://github.com/servo/servo/issues?q=is%3Aopen+is%3Aissue+label%3AE-candidate-for-mentoring

and https://github.com/servo/servo/issues?q=is%3Aopen+is%3Aissue+label%3AB-interesting-project

I personally recommend this one, #17442 it looks like fun and has a mentor available

@ferjm
Copy link
Member

@ferjm ferjm commented Sep 5, 2018

@AgustinCB you might be interested in #21481 as well, which is similar to #21590

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.