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

Various CEF refactoring. #6435

Merged
merged 12 commits into from Jun 20, 2015
Merged

Various CEF refactoring. #6435

merged 12 commits into from Jun 20, 2015

Conversation

@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 20, 2015

Review on Reviewable

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Jun 20, 2015

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

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.

@zmike
Copy link
Contributor

zmike commented Jun 20, 2015

Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


ports/cef/string_map.rs, line 65 [r11] (raw file):
Maybe slightly outside the scope of this patchset, but I think it should just return the value of this function here instead of 1.


ports/cef/string_map.rs, line 84 [r11] (raw file):
Same as above.


Comments from the review on Reviewable.io

@zmike
Copy link
Contributor

zmike commented Jun 20, 2015

Reviewed 1 of 1 files at r1, 1 of 1 files at r5, 1 of 1 files at r7, 1 of 2 files at r8, 1 of 1 files at r10, 1 of 1 files at r11.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


Comments from the review on Reviewable.io

@zmike
Copy link
Contributor

zmike commented Jun 20, 2015

Okay, it seems we've had some regressions in CEF string APIs (or maybe rust?) since I last ran full unit tests.

Now we have errors which were not present earlier, seen here: https://gist.github.com/zmike/9115f37d88380e5e9fc9

Given that I've verified the errors were not introduced in this patchset, I'll still give r+ here, but I guess now I should also create another ticket...

@zmike
Copy link
Contributor

zmike commented Jun 20, 2015

Reviewed 1 of 1 files at r12.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


Comments from the review on Reviewable.io

@Ms2ger
Copy link
Contributor Author

Ms2ger commented Jun 20, 2015

@bors-servo r=zmike

@bors-servo
Copy link
Contributor

bors-servo commented Jun 20, 2015

📌 Commit da53a9c has been approved by zmike

@bors-servo
Copy link
Contributor

bors-servo commented Jun 20, 2015

Testing commit da53a9c with merge 6e04c12...

bors-servo pushed a commit that referenced this pull request Jun 20, 2015
bors-servo
Various CEF refactoring.



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

bors-servo commented Jun 20, 2015

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

@bors-servo bors-servo merged commit da53a9c into servo:master Jun 20, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@Ms2ger Ms2ger deleted the Ms2ger:cef branch Jun 21, 2015
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.