JS Error Event Interface (NCSU Team: ronak, shreya, sayali) v 1.0

[RonakNisher]

Resolved all compilation errors

[hoppipolla-critic-bot]

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

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.

[jdm]

Instead of opening multiple pull requests, can you continue adding commits to the previously-opened one so that our review tool can be used effectively?

[jdm]

Getting better! Please be careful about matching the code style in other files, especially in regards to consistent indentation and spacing. Also be sure to run ./mach test-tidy to catch problems like missing license headers and trailing whitespace early!

[jdm]

Much improved! I've left some more comments on Critic again, as there are still some formatting problems.

[jdm]

Very close! There are only three remaining issues; once they're fixed, these changes can be rebased against master and merged :)

[jdm]

Yay! Please rebase all of these changes on top of the latest code and squash them into a single commit (https://ariejan.net/2011/07/05/git-squash-your-latests-commits-into-one/), and this is ready to merge!

[Ms2ger]

@RonakNisher: initErrorEvent no longer exists, please remove it. Also make sure to run web-platform-tests and update expected failures where necessary (documentation under /tests/wpt/README.md).

[RonakNisher]

@Ms2ger , Removing the initErrorEvent caused compile time errors.

It says , "not all trait items implemented, missing: InitErrorEvent "

[jdm]

@RonakNisher You will need to remove both the Rust implementation and the WebIDL method declaration.

[jdm]

@RonakNisher The problem is that you're using a master revision from October 15, so the dom_struct attribute does not exist yet. Similar DOM structures use the cocktail of jstraceable, privatize, and must_root, as your original commit did.

[RonakNisher]

@jdm , Should I update my master revision ?

[jdm]

It appears that you did.

[RonakNisher]

@jdm, yeah I had to. I thought the error was due to a mismatch in my local servo so I pulled in the latest changes. As of now, servo is compiling but I'm unsure of the way forward with removing the InitErrorEvent method. Its being called directly in the "new" method. I'm not sure how can I use this while calling the error event

[RonakNisher]

@jdm ,@Ms2ger : Removed the initErrorEvent. I've run the web-platform-tests too but I could not find anything to add / edit in the .ini file in Metadata. It showed that it passed the test when the expectation was FAIL. should we be adding these too ?
(Here is the link to the partial output of running the web-platform-tests http://pastebin.com/j6KtRz5F )

[jdm]

@RonakNisher: When there's an expected failure but the test passes, the annotation should be removed from the ini file, eg:

[ErrorEvent interface: existence and properties of interface object]
  expected: FAIL

should have both lines deleted.

[jdm]

@RonakNisher Also, could you rebase the Critic review onto afc144a by following the steps at https://github.com/servo/servo/wiki/Github-&-Critic-PR-handling-101? That will allow me to leave a few more review comments.

[jdm]

To be specific, starting at the Use the "Rebase Branch" step and using the revision that I specified; it's the SHA1 of the parent that the steps talk about.

[jdm]

Thanks for doing the rebase! Don't forget to press the "Enable tracking" button, too.

[jdm]

Super close! Two last small issues remaining :)

[jdm]

Squash and I'll merge! https://ariejan.net/2011/07/05/git-squash-your-latests-commits-into-one/

[jdm]

Sorry @RonakNisher, the extra attributes have to go:

dom/errorevent.rs:26:1: 26:15 error: conflicting implementations for trait `dom::bindings::trace::JSTraceable` [E0119]
dom/errorevent.rs:26 #[jstraceable]
                     ^~~~~~~~~~~~~~
note: in expansion of #[jstraceable]
dom/errorevent.rs:26:1: 26:15 note: expansion site

Please remove the attributes that are not dom_struct, amend the commit (git commit -a --amend) and force push the change.

[jdm]

@RonakNisher It appears Shreya's changes got mixed into this branch, and I did not notice that you had updated it to remove the attributes that were causing problems before. Could you rebase your first commit against master, and reset the branch so Shreya's changes aren't included? This should be ready to merge when that happens.

[RonakNisher]

@jdm , done. I'm getting better at undoing changes on git :P

[jdm]

Sorry, the code changed under you:

dom/errorevent.rs:53:28: 53:34 error: mismatched types: expected `dom::bindings::global::GlobalRef<'_>`, found `&dom::bindings::global::GlobalRef<'_>` (expected enum dom::bindings::global::GlobalRef, found &-ptr)
dom/errorevent.rs:53                            global,
                                                ^~~~~~

I'll make the fix and merge this myself in ~4 hours unless you get to it first.

[jdm]

Merged in #4230. Hooray!

[RonakNisher]

Awesome. Thank you so much for your help :)

On 10:46AM, Fri, Dec 5, 2014 Josh Matthews notifications@github.com wrote:

Closed #3822 #3822.

—
Reply to this email directly or view it on GitHub
#3822 (comment).