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

Jump to bottom

Refactors some entities from script_thread into script_runtime #10342

Merged
merged 1 commit into from Apr 6, 2016
Merged

Refactors some entities from script_thread into script_runtime #10342

merged 1 commit into from Apr 6, 2016

Conversation

creativcoder
Copy link
Contributor

Fixes #10271.

This change is Reviewable

@highfive
Copy link

highfive commented Apr 1, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/bindings/global.rs, components/script/dom/workerglobalscope.rs, components/script/network_listener.rs, components/script/dom/htmlscriptelement.rs, components/script/dom/filereader.rs, components/script/dom/window.rs, components/script/dom/servohtmlparser.rs, components/script/cors.rs, components/script/dom/document.rs, components/script/dom/bindings/refcounted.rs, components/script/task_source/history_traversal.rs, components/script/dom/xmlhttprequest.rs, components/script/task_source/networking.rs, components/script/task_source/file_reading.rs, components/script/lib.rs, components/script/dom/htmldetailselement.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/script/dom/htmlimageelement.rs, components/script/script_thread.rs, components/script/dom/htmlinputelement.rs, components/script/script_runtime.rs, components/script/dom/worker.rs, components/script/dom/storage.rs, components/script/dom/htmlformelement.rs, components/script/task_source/user_interaction.rs, components/script/dom/bindings/trace.rs, components/script/dom/websocket.rs, components/script/dom/closeevent.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 1, 2016
@highfive
Copy link

highfive commented Apr 1, 2016

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!

@creativcoder creativcoder changed the title For #10271 Refactors some entities from script_thread into script_runtime Refactors some entities from script_thread into script_runtime Apr 1, 2016
@jdm
Copy link
Member

jdm commented Apr 1, 2016

These changes look great! Please also move the things in script_thread.rs that were made public, since they're only used by the other moved code.

@Ms2ger
Copy link
Contributor

Ms2ger commented Apr 1, 2016

Reviewed 28 of 28 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

components/script/lib.rs, line 95 [r1] (raw file):
Let's move this to the places where it's needed instead.

components/script/script_thread.rs, line 117 [r1] (raw file):
Let's add a static function ScriptTask::trace_thread(tr: *mut JSTracer) and move the SCRIPT_THREAD_ROOT.with() call into it. Then move trace_rust_roots into script_runtime too.

components/script/script_thread.rs, line 313 [r1] (raw file):
Are there two of those now? Remove this one.

components/script/script_thread.rs, line 513 [r1] (raw file):
Move this too, along with the thread_locals

components/script/script_thread.rs, line 553 [r1] (raw file):
Ditto

Comments from the review on Reviewable.io

@Ms2ger Ms2ger assigned Ms2ger and unassigned nox Apr 1, 2016
@Ms2ger Ms2ger added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 1, 2016
@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Apr 1, 2016
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Apr 2, 2016
@KiChjang KiChjang removed the S-needs-rebase There are merge conflict errors. label Apr 2, 2016
@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Apr 4, 2016
@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Apr 5, 2016
@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Apr 5, 2016
@Ms2ger
Copy link
Contributor

Ms2ger commented Apr 6, 2016

Reviewed 2 of 6 files at r2, 5 of 8 files at r3, 5 of 5 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

components/script/script_runtime.rs, line 10 [r4] (raw file):
Nit: no {}.

Comments from Reviewable

@Ms2ger
Copy link
Contributor

Ms2ger commented Apr 6, 2016

@bors-servo r+

Thanks!

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@bors-servo
Copy link
Contributor

📌 Commit 2caa9a2 has been approved by Ms2ger

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Apr 6, 2016
@Ms2ger
Copy link
Contributor

Ms2ger commented Apr 6, 2016

@bors-servo p=2

@bors-servo
Copy link
Contributor

⌛ Testing commit 2caa9a2 with merge 883cde4...

bors-servo pushed a commit that referenced this pull request Apr 6, 2016
Auto merge of #10342 - creativcoder:script-runtime-module, r=Ms2ger
883cde4 
Refactors some entities from script_thread into script_runtime

Fixes #10271.

<!-- 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/10342)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor

@bors-servo bors-servo merged commit 2caa9a2 into servo:master Apr 6, 2016
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 6, 2016
metajack added a commit to metajack/servo that referenced this pull request Apr 6, 2016
@metajack
Use better JS engine defaults
93cd0f2 
This adds in preferences for all the SM 39 available options (as
retrieved from Gecko), and uses the same defaults as Gecko. A few
properties are not supported yet, and incremental GC is still always
disabled regardless of the preference setting.

This also adds back in the options that were accidentally deleted when
\servo#10342 was rebased, which moved things from script_thread.rs to
script_runtime.rs.
metajack added a commit to metajack/servo that referenced this pull request Apr 8, 2016
@metajack
Use better JS engine defaults
ebea51c 
This adds in preferences for all the SM 39 available options (as
retrieved from Gecko), and uses the same defaults as Gecko. A few
properties are not supported yet, and incremental GC is still always
disabled regardless of the preference setting.

This also adds back in the options that were accidentally deleted when
\servo#10342 was rebased, which moved things from script_thread.rs to
script_runtime.rs.
metajack added a commit to metajack/servo that referenced this pull request Apr 8, 2016
@metajack
Use better JS engine defaults
1bec193 
This adds in preferences for all the SM 39 available options (as
retrieved from Gecko), and uses the same defaults as Gecko. A few
properties are not supported yet, and incremental GC is still always
disabled regardless of the preference setting.

This also adds back in the options that were accidentally deleted when
\servo#10342 was rebased, which moved things from script_thread.rs to
script_runtime.rs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Ms2ger Ms2ger

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@creativcoder @highfive @jdm @Ms2ger @bors-servo @nox @KiChjang