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

remove ScriptListener #7191

Merged
merged 3 commits into from Aug 16, 2015
Merged

remove ScriptListener #7191

merged 3 commits into from Aug 16, 2015

Conversation

@vectorijk
Copy link
Contributor

vectorijk commented Aug 13, 2015

Fix issue #7175 and for code review

Review on Reviewable

@highfive
Copy link

highfive commented Aug 13, 2015

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Ms2ger (or someone else) soon.

@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 13, 2015

-S-awaiting-review +S-awaiting-answer

Thank you for your PR. I have one question below.


Reviewed 7 of 7 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


components/script/script_task.rs, line 1289 [r1] (raw file):
I don't understand why we need these RefMut::map calls.


Comments from the review on Reviewable.io

@vectorijk
Copy link
Contributor Author

vectorijk commented Aug 13, 2015

components/script/script_task.rs, line 1289 [r1] (raw file):
cause self.compositor return type is DOMRefCell<IpcSender<ScriptToCompositorMsg>> and self.compositor.borrow_mut() return type isRefMut<'_, IpcSender<ScriptToCompositorMsg>>. So I use RefMut::map to get IpcSender<T>. Do you have better way to get it?


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Aug 13, 2015

We can just dereference RefMut<T> via * to get &mut T.

@vectorijk
Copy link
Contributor Author

vectorijk commented Aug 13, 2015

@jdm Okay, Got it~ I might try dereference RefMut<T> via * before. But I gave up this method cause I didn't use correct parentheses. And I will revise this for code review later.

@vectorijk
Copy link
Contributor Author

vectorijk commented Aug 13, 2015

@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 14, 2015

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


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


components/script/script_task.rs, line 1289 [r1] (raw file):
Thanks. Could you try without the explicit dereferencing too (i.e., self.compositor.borrow_mut().send(ScriptToCompositorMsg::Exit).unwrap();


Comments from the review on Reviewable.io

@vectorijk
Copy link
Contributor Author

vectorijk commented Aug 14, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Aug 15, 2015

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

@vectorijk vectorijk force-pushed the vectorijk:removeScriptListener branch from 4e70c67 to 270df6b Aug 16, 2015
@vectorijk
Copy link
Contributor Author

vectorijk commented Aug 16, 2015

@Ms2ger @jdm rebased it.

@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 16, 2015

Thank you!

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Aug 16, 2015

📌 Commit 270df6b has been approved by Ms2ger

@bors-servo
Copy link
Contributor

bors-servo commented Aug 16, 2015

Testing commit 270df6b with merge 5ab9aa5...

bors-servo pushed a commit that referenced this pull request Aug 16, 2015
remove ScriptListener

Fix issue #7175 and for code review

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7191)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 16, 2015

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2, mac3

@bors-servo bors-servo merged commit 270df6b into servo:master Aug 16, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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