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

Implement Msg::Status with serialized url upon mouseover #6181

Merged
merged 1 commit into from Jul 30, 2015

Conversation

@brunoabinader
Copy link
Contributor

brunoabinader commented May 25, 2015

Credits for Mike Blumenkrantz (@zmike), I just rebased against trunk and
fixed the url serialization.

Fixes #6178.

Review on Reviewable

@zmike
Copy link
Contributor

zmike commented May 25, 2015

Hooray!

@larsbergstrom
Copy link
Contributor

larsbergstrom commented May 26, 2015

Review status: 9 of 10 files reviewed, all discussions resolved, all commit checks successful.
Reviewed files:

  • components/compositing/compositor.rs @ r1
  • components/compositing/compositor_task.rs @ r1
  • components/compositing/constellation.rs @ r1
  • components/compositing/headless.rs @ r1
  • components/compositing/windowing.rs @ r1
  • components/msg/constellation_msg.rs @ r1
  • ports/cef/window.rs @ r1
  • ports/glutin/window.rs @ r1
  • ports/gonk/src/window.rs @ r1

Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented May 26, 2015

Review status: all files reviewed, 6 unresolved discussions, all commit checks successful.
Reviewed files:

  • components/script/dom/document.rs @ r1

components/script/dom/document.rs, line 655 [r1] (raw file):
Let's add a let target = target.root(); and clean up these two lines.


components/script/dom/document.rs, line 660 [r1] (raw file):
Let's add a loop to the MouseMoveEvent handler in script_task.rs instead of doing this work here.


components/script/dom/document.rs, line 698 [r1] (raw file):
Let's move this whole loop to the MouseMoveEvent handler in script_task.rs instead of adding it here.


components/script/dom/document.rs, line 703 [r1] (raw file):
ElementCast::to_ref(anchor.r()).unwrap();


components/script/dom/document.rs, line 712 [r1] (raw file):
We don't need any of this.


components/script/dom/document.rs, line 721 [r1] (raw file):
let status = attr.map(|href| { instead of the match.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented May 27, 2015

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

@bors-servo
Copy link
Contributor

bors-servo commented Jun 3, 2015

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

@metajack
Copy link
Contributor

metajack commented Jul 29, 2015

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Jul 29, 2015

Reviewed 1 of 11 files at r2.
Review status: 1 of 10 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


components/script/script_task.rs, line 1426 [r2] (raw file):
tiny nit: Please remove the one extra newline.


Comments from the review on Reviewable.io

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Jul 29, 2015

Reviewed 8 of 11 files at r2.
Review status: 9 of 10 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from the review on Reviewable.io

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Jul 29, 2015

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


Comments from the review on Reviewable.io

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Jul 29, 2015

@metajack GH still thinks it needs a rebase? Plus the one nit :-)

@metajack
Copy link
Contributor

metajack commented Jul 29, 2015

@brunoabinader
Copy link
Contributor Author

brunoabinader commented Jul 30, 2015

@metajack pong! I've rebased against master and fixed the nit 👍


Review status: 0 of 10 files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Jul 30, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Jul 30, 2015

📌 Commit 54494cd has been approved by larsbergstrom

@brunoabinader
Copy link
Contributor Author

brunoabinader commented Jul 30, 2015

Please do not merge yet, I found an issue in script_task related to misusage of root(). Fixing it now.

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Jul 30, 2015

@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 30, 2015

@bors-servo r=larsbergstrom

@bors-servo
Copy link
Contributor

bors-servo commented Jul 30, 2015

📌 Commit a08bd8b has been approved by larsbergstrom

@bors-servo
Copy link
Contributor

bors-servo commented Jul 30, 2015

Testing commit a08bd8b with merge 120fc18...

bors-servo pushed a commit that referenced this pull request Jul 30, 2015
Implement Msg::Status with serialized url upon mouseover

Credits for Mike Blumenkrantz (@zmike), I just rebased against trunk and
fixed the url serialization.

Fixes #6178.

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

bors-servo commented Jul 30, 2015

💔 Test failed - linux1

Credits for Mike Blumenkrantz (@zmike), I just rebased against trunk and
fixed the url serialization.

Fixes #6178.
@brunoabinader
Copy link
Contributor Author

brunoabinader commented Jul 30, 2015

💔 Test failed - linux1

Oops, that one didn't fired up on my OSX build. I've added a stub for glutin.

@brunoabinader
Copy link
Contributor Author

brunoabinader commented Jul 30, 2015

@bors-servo r=larsbergstrom

@bors-servo
Copy link
Contributor

bors-servo commented Jul 30, 2015

📌 Commit b3927d5 has been approved by larsbergstrom

@bors-servo
Copy link
Contributor

bors-servo commented Jul 30, 2015

Testing commit b3927d5 with merge 82e476f...

bors-servo pushed a commit that referenced this pull request Jul 30, 2015
Implement Msg::Status with serialized url upon mouseover

Credits for Mike Blumenkrantz (@zmike), I just rebased against trunk and
fixed the url serialization.

Fixes #6178.

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

bors-servo commented Jul 30, 2015

💔 Test failed - linux3

@jdm
Copy link
Member

jdm commented Jul 30, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Jul 30, 2015

Previous build results for android, gonk, linux2, mac1, mac2, mac3 are reusable. Rebuilding only linux1, linux3...

@bors-servo
Copy link
Contributor

bors-servo commented Jul 30, 2015

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

@bors-servo bors-servo merged commit b3927d5 into servo:master Jul 30, 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.

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