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

Move body of ScriptTask::handle_mouse_move_event into a method on #5032

Closed
wants to merge 1 commit into from

Conversation

@JIoJIaJIu
Copy link
Contributor

JIoJIaJIu commented Feb 23, 2015

Document #4984

@highfive
Copy link

highfive commented Feb 23, 2015

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

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Feb 23, 2015

Critic review: https://critic.hoppipolla.co.uk/r/4075

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

let mouse_over_targets = &mut *self.mouse_over_targets.borrow_mut();

if document.r().handle_mouse_move_event(self.js_runtime.ptr, point, mouse_over_targets) {

This comment has been minimized.

Copy link
@kmcallister

kmcallister Feb 23, 2015

Contributor

Nit: don't need a blank line here

@@ -450,6 +452,91 @@ impl<'a> DocumentHelpers<'a> for JSRef<'a, Document> {
None => {}
}
}

fn handle_mouse_move_event (self, js_runtime: *mut JSRuntime, point: Point2D<f32>, mouse_over_targets: &mut Option<Vec<JS<Node>>>) -> bool{

This comment has been minimized.

Copy link
@kmcallister

kmcallister Feb 23, 2015

Contributor

Nit: space before (

This comment has been minimized.

Copy link
@kmcallister

kmcallister Feb 23, 2015

Contributor

Also can you wrap this line, and the line added in the trait, to fit within 100 columns (ideally, 80)?

@kmcallister
Copy link
Contributor

kmcallister commented Feb 23, 2015

r=me with those syntax tweaks

@JIoJIaJIu
Copy link
Contributor Author

JIoJIaJIu commented Feb 24, 2015

Can you give me advise how to wrap func, please?

@jdm
Copy link
Member

jdm commented Feb 24, 2015

Just move arguments that go beyond 80 characters on to the next line, starting in the same column as the arguments on the previous line.

JIoJIaJIu added a commit to JIoJIaJIu/servo that referenced this pull request Feb 25, 2015
@JIoJIaJIu JIoJIaJIu force-pushed the JIoJIaJIu:master branch from e4be675 to 1eef07c Feb 25, 2015
@JIoJIaJIu
Copy link
Contributor Author

JIoJIaJIu commented Feb 25, 2015

Omg, excuse me)

@JIoJIaJIu
Copy link
Contributor Author

JIoJIaJIu commented Feb 25, 2015

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend)

But when im trying to make pull ustream master i've got merge conflict.
What is the best way to avoid this? Thx

@jdm
Copy link
Member

jdm commented Feb 25, 2015

Ah, the best solution there is to use branches when making commits. I leave master tracking the upstream master, and rebase my feature branches based on it.

@jdm
Copy link
Member

jdm commented Feb 25, 2015

You could do

git reset --hard 1eef07c
git checkout -b move_fn
git checkout master
git reset --hard f0b5794e44a2a28212aaf3fac4c340a3f32a8515

and then you should have a master branch that can be pulled from upstream again. You'll need to make a new pull request for the new move_fn branch, though.

JIoJIaJIu added a commit to JIoJIaJIu/servo that referenced this pull request Feb 26, 2015
@JIoJIaJIu
Copy link
Contributor Author

JIoJIaJIu commented Feb 26, 2015

thank you @jdm
#5073

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.