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.

Sign up for GitHub

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

Invoke specialized callback behaviour for OnErrorEventHandler. #8430

Merged
merged 2 commits into from Nov 12, 2015
Merged

Invoke specialized callback behaviour for OnErrorEventHandler. #8430

merged 2 commits into from Nov 12, 2015

Conversation

jdm
Copy link
Member

@jdm jdm commented Nov 9, 2015

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 9, 2015
@highfive
Copy link

highfive commented Nov 9, 2015

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@jdm jdm force-pushed the errorhandler branch 2 times, most recently from b23f657 to fbaafef Compare November 9, 2015 16:54
@Ms2ger
Copy link
Contributor

Ms2ger commented Nov 9, 2015

Reviewed 7 of 8 files at r1.
Review status: 7 of 12 files reviewed at latest revision, 2 unresolved discussions.

components/script/dom/eventtarget.rs, line 41 [r1] (raw file):
Any reason to keep these typedefs?

components/script/dom/eventtarget.rs, line 124 [r1] (raw file):
Can you get me some spec links?

Comments from the review on Reviewable.io

@Ms2ger Ms2ger self-assigned this Nov 9, 2015
@bors-servo
Copy link
Contributor

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

@highfive highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 9, 2015
@jdm
Copy link
Member Author

jdm commented Nov 9, 2015

Your very simple requests caused me to add a couple extra assertions to the new test, and that ended up sending me down a rabbit hole of discovery about the various ways that the test was not testing what it thought it was, and additionally how the code for uncompiled event handlers was subtly incorrect in the forward-to-window case!

@jdm jdm removed the S-needs-rebase There are merge conflict errors. label Nov 9, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Nov 10, 2015

You're welcome :)

@jdm jdm mentioned this pull request Nov 10, 2015
Merged
@Ms2ger
Copy link
Contributor

Ms2ger commented Nov 12, 2015

Se we're still eagerly compiling? File an issue on that.

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

components/script/dom/eventtarget.rs, line 150 [r2] (raw file):
File an issue for this too.

components/script/dom/eventtarget.rs, line 266 [r2] (raw file):
How about just putting the &'static [*const c_char] in the local bindings, and calling as_ptr()/len() below?

components/script/dom/htmlbodyelement.rs, line 156 [r2] (raw file):
This seems fishy. Do we really not have a better way to solve this?

tests/wpt/web-platform-tests/html/webappapis/scripting/events/onerroreventhandler-frame.html, line 25 [r2] (raw file):
Put this all in parent.t1.step(function() { ... }); ditto below.

Comments from the review on Reviewable.io

@Ms2ger
Copy link
Contributor

Ms2ger commented Nov 12, 2015

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

Review status: all files reviewed at latest revision, 5 unresolved discussions.

Comments from the review on Reviewable.io

@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 Nov 12, 2015
@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 Nov 12, 2015
@Ms2ger Ms2ger added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 12, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Nov 12, 2015

-S-awaiting-review +S-needs-squash

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

Comments from the review on Reviewable.io

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 12, 2015
@jdm
Copy link
Member Author

jdm commented Nov 12, 2015

@bors-servo: r=Ms2ger

@bors-servo
Copy link
Contributor

📌 Commit 16103fa has been approved by Ms2ger

@highfive highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Nov 12, 2015
@highfive highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 12, 2015
@bors-servo
Copy link
Contributor

⌛ Testing commit 16103fa with merge 1bc36f9...

bors-servo pushed a commit that referenced this pull request Nov 12, 2015
Auto merge of #8430 - jdm:errorhandler, r=Ms2ger
1bc36f9 
Invoke specialized callback behaviour for OnErrorEventHandler.

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

💔 Test failed - linux-rel

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 12, 2015
@jdm
Copy link
Member Author

jdm commented Nov 12, 2015

@bors-servo: retry

@bors-servo
Copy link
Contributor

⌛ Testing commit 16103fa with merge 2212cd8...

bors-servo pushed a commit that referenced this pull request Nov 12, 2015
Auto merge of #8430 - jdm:errorhandler, r=Ms2ger
2212cd8 
Invoke specialized callback behaviour for OnErrorEventHandler.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8430)
<!-- Reviewable:end -->
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Nov 12, 2015
@bors-servo
Copy link
Contributor

💔 Test failed - linux-dev

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 12, 2015
@eefriedman
Copy link
Contributor 

/home/servo/buildbot/slave/linux-dev/build/components/script/dom/eventtarget.rs:14:5: 14:55 error: unresolved import `dom::bindings::global::global_object_for_reflector`. There is no `global_object_for_reflector` in `dom::bindings::global` [E0432]
/home/servo/buildbot/slave/linux-dev/build/components/script/dom/eventtarget.rs:14 use dom::bindings::global::global_object_for_reflector;
                                                                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/servo/buildbot/slave/linux-dev/build/components/script/dom/eventtarget.rs:14:5: 14:55 help: run `rustc --explain E0432` to see a detailed explanation

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Nov 12, 2015
@jdm
Copy link
Member Author

jdm commented Nov 12, 2015

@bors-servo r=Ms2ger

@bors-servo
Copy link
Contributor

📌 Commit c4c0809 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. labels Nov 12, 2015
@bors-servo
Copy link
Contributor

⌛ Testing commit c4c0809 with merge fafc280...

bors-servo pushed a commit that referenced this pull request Nov 12, 2015
Auto merge of #8430 - jdm:errorhandler, r=Ms2ger
fafc280 
Invoke specialized callback behaviour for OnErrorEventHandler.

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

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

@bors-servo bors-servo merged commit c4c0809 into servo:master Nov 12, 2015
@Ms2ger Ms2ger mentioned this pull request Nov 13, 2015
Merged
@jdm jdm mentioned this pull request Nov 28, 2015
Closed
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
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jdm @highfive @Ms2ger @bors-servo @eefriedman