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

Refactor resource thread code #11189

Merged
merged 1 commit into from May 20, 2016
Merged

Refactor resource thread code #11189

merged 1 commit into from May 20, 2016
+213 −179

Conversation

@izgzhen
Copy link
Contributor

izgzhen commented May 15, 2016 •
edited

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes do not require tests because it is refactoring

Related to #11131

This change is Reviewable
@highfive
Copy link

highfive commented May 15, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/net/filemanager_thread.rs, components/script/dom/document.rs, components/net/resource_thread.rs, components/script/dom/storage.rs, components/script/dom/xmlhttprequest.rs, components/net_traits/lib.rs, components/net_traits/lib.rs, components/script/script_thread.rs, components/script/dom/websocket.rs, components/script/dom/window.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/net/image_cache_thread.rs
@highfive
Copy link

highfive commented May 15, 2016

warning Warning warning

  • These commits modify net and script code, but no tests are modified. Please consider adding a test!
@izgzhen
Copy link
Contributor Author

izgzhen commented May 15, 2016

Discussed on IRC with @Manishearth here, thinking that this will improve the maintainability of code.
@wafflespeanut
Copy link
Member

wafflespeanut commented May 15, 2016

If we're going to rename ControlMsg to ResourceMsg, then we should also update the unit tests for net.
@izgzhen
Copy link
Contributor Author

izgzhen commented May 15, 2016

@wafflespeanut By ./mach test-unit? I will take a look, thanks!
@izgzhen izgzhen force-pushed the izgzhen:refactor-resource-thread branch from 37ed2c2 to e60ba64 May 15, 2016
@highfive
Copy link

highfive commented May 15, 2016

New code was committed to pull request.
@bors-servo
Copy link
Contributor

bors-servo commented May 15, 2016

☔ The latest upstream changes (presumably #11192) made this pull request unmergeable. Please resolve the merge conflicts.
@izgzhen izgzhen force-pushed the izgzhen:refactor-resource-thread branch from e60ba64 to 5922262 May 15, 2016
@nox nox removed the S-needs-rebase label May 16, 2016
@nox
Copy link
Member

nox commented May 16, 2016

Could you add a summary of your changes in your commit message?
@izgzhen
Copy link
Contributor Author

izgzhen commented May 16, 2016

@nox ok
@izgzhen izgzhen force-pushed the izgzhen:refactor-resource-thread branch from 5922262 to 8b07e68 May 16, 2016
@highfive
Copy link

highfive commented May 16, 2016

New code was committed to pull request.
@nox
Copy link
Member

nox commented May 16, 2016

This adds an intermediate step to request things to the storage thread. AFAICT @Manishearth was suggesting to put all three channels in the same structure and implement a send method on it that would dispatch messages to the right thread, not putting storage and filemanager behind the resource thread.
@nox nox assigned nox and unassigned jdm May 16, 2016
@izgzhen
Copy link
Contributor Author

izgzhen commented May 16, 2016

@nox, thanks for feedback, I remember @Manishearth suggested that but I am not sure whether this design will be changed later on so I put it off. Using send looks like a great idea.
@izgzhen izgzhen force-pushed the izgzhen:refactor-resource-thread branch from 8b07e68 to ba12177 May 17, 2016
@highfive
Copy link

highfive commented May 17, 2016

New code was committed to pull request.
@KiChjang
Copy link
Member

KiChjang commented May 17, 2016

This fails at building unit tests:

/home/travis/build/servo/servo/tests/unit/net/resource_thread.rs:6:5: 6:46 error: unresolved import `net::resource_thread::new_resource_thread`. There is no `new_resource_thread` in `net::resource_thread`. Did you mean to use `new_resource_threads`? [E0432]

/home/travis/build/servo/servo/tests/unit/net/resource_thread.rs:6 use net::resource_thread::new_resource_thread;

                                                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

/home/travis/build/servo/servo/tests/unit/net/resource_thread.rs:6:5: 6:46 help: run `rustc --explain E0432` to see a detailed explanation

error: aborting due to previous error
@izgzhen izgzhen force-pushed the izgzhen:refactor-resource-thread branch from ba12177 to a3efc65 May 17, 2016
@highfive
Copy link

highfive commented May 17, 2016

New code was committed to pull request.
@nox
Copy link
Member

nox commented May 19, 2016

@bors-servo r+ p=1
@bors-servo
Copy link
Contributor

bors-servo commented May 19, 2016

📌 Commit d43203d has been approved by nox
@nox
Copy link
Member

nox commented May 19, 2016

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented May 19, 2016

⌛ Testing commit d43203d with merge 68a4a6e...
bors-servo added a commit that referenced this pull request May 19, 2016
@bors-servo
Auto merge of #11189 - izgzhen:refactor-resource-thread, r=nox
Loading status checks…
68a4a6e 
Refactor resource thread code

- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

- [x] These changes do not require tests because it is refactoring

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11189)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 19, 2016

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows
@bors-servo
Copy link
Contributor

bors-servo commented May 19, 2016

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward
@bors-servo
Copy link
Contributor

bors-servo commented May 19, 2016

☔ The latest upstream changes (presumably #11270) made this pull request unmergeable. Please resolve the merge conflicts.
@izgzhen
Implement trait-based ResourceThreads and clean up related naming issues
Loading status checks…
a51db4c 
Changes include:

- Introduce an IpcSend trait to abstract over a collection of IpcSenders
- Implement ResourceThreads collection to abstract the resource-related
  sub threads across the component
- Rename original ResourceThread and ControlMsg into an unifed CoreResource__
  to accommodate above changes and avoid confusions
@izgzhen izgzhen force-pushed the izgzhen:refactor-resource-thread branch from d43203d to a51db4c May 20, 2016
@Manishearth
Copy link
Member

Manishearth commented May 20, 2016

@bors r=nox
@Manishearth
Copy link
Member

Manishearth commented May 20, 2016

@bors-servo r=nox
@bors-servo
Copy link
Contributor

bors-servo commented May 20, 2016

📌 Commit a51db4c has been approved by nox
@bors-servo
Copy link
Contributor

bors-servo commented May 20, 2016

⌛ Testing commit a51db4c with merge bcea0ad...
bors-servo added a commit that referenced this pull request May 20, 2016
@bors-servo
Auto merge of #11189 - izgzhen:refactor-resource-thread, r=nox
Loading status checks…
bcea0ad 
Refactor resource thread code

- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

- [x] These changes do not require tests because it is refactoring

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11189)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 20, 2016

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows
@bors-servo bors-servo merged commit a51db4c into servo:master May 20, 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
This was referenced May 20, 2016
Merged
Merged
Closed
@izgzhen izgzhen mentioned this pull request May 20, 2016
Merged
4 of 4 tasks complete
bors-servo added a commit that referenced this pull request May 23, 2016
@bors-servo
Auto merge of #11225 - izgzhen:patch-input-element-file, r=Manishearth
Loading status checks…
7cea4eb 
Implement file related functionalities in htmlinputelement and related

- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy --faster` does not report any errors
- [x] These changes is related to #11131
- [x] These changes do not require tests because it is a partial implementation

1. Improve the `filemanager_thread` by adding type string and create `SelectedFile`
2. Fill several gaps in `htmlinputelement` implementation related to file type
3. Improve the `File` interface to accommodate the above changes
4. Integrate changes introduced by PR #11189

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11225)
<!-- Reviewable:end -->
@antrik antrik mentioned this pull request May 24, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nox nox

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@izgzhen @highfive @wafflespeanut @bors-servo @nox @KiChjang @Manishearth @jdm
You can’t perform that action at this time.