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

Update rust-mozjs #12954

Merged
merged 1 commit into from Aug 24, 2016
Merged

Update rust-mozjs #12954

merged 1 commit into from Aug 24, 2016

Conversation

@GuillaumeGomez
Copy link
Contributor

GuillaumeGomez commented Aug 20, 2016

This change is Reviewable

@highfive
Copy link

highfive commented Aug 20, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/webdriver_handlers.rs, components/script/dom/bindings/error.rs, components/script/dom/bindings/conversions.rs, components/script/dom/htmlcanvaselement.rs, components/script/script_thread.rs, components/script/devtools.rs
@highfive
Copy link

highfive commented Aug 20, 2016

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!
@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Aug 20, 2016

r? @nox

@highfive highfive assigned nox and unassigned jdm Aug 20, 2016
@nox
Copy link
Member

nox commented Aug 20, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Aug 20, 2016

Trying commit c0a2e36 with merge 53c0862...

bors-servo added a commit that referenced this pull request Aug 20, 2016
Dictionary error

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12954)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 20, 2016

💔 Test failed - linux-dev

@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Aug 20, 2016

@nox: I fixed tidy issues.

@emilio
Copy link
Member

emilio commented Aug 21, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Aug 21, 2016

Trying commit fff48e3 with merge 43151f7...

bors-servo added a commit that referenced this pull request Aug 21, 2016
Dictionary error

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12954)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 21, 2016

💔 Test failed - windows-dev

@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Aug 21, 2016

Retry please (windows tests didn't start apparently?)?

@nox
Copy link
Member

nox commented Aug 21, 2016

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Aug 21, 2016

Trying commit fff48e3 with merge 24ef817...

bors-servo added a commit that referenced this pull request Aug 21, 2016
Dictionary error

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12954)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 21, 2016

💔 Test failed - mac-rel-css

@highfive
Copy link

highfive commented Aug 21, 2016

  ▶ TIMEOUT [expected OK] /geometry-1_dev/html/DOMQuad-001.htm

  ▶ Unexpected subtest result in /geometry-1_dev/html/DOMQuad-001.htm:
  │ TIMEOUT [expected PASS] testConstructor1
  └   → Test timed out
@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Aug 21, 2016

Is this intermittent?

@emilio
Copy link
Member

emilio commented Aug 22, 2016

Nope, and I think those are suspicious enough to deserve a bit of investigation. They're most likely caused by this changeset.

@emilio
Copy link
Member

emilio commented Aug 22, 2016

@nox nox removed the S-awaiting-review label Aug 23, 2016
@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Aug 23, 2016

A generated file: https://gist.github.com/GuillaumeGomez/227ee9b8b3a4d7f23b1193be7fe1a2c4

However I don't understand what you want:

and please add some unions with dictionaries to TestBinding so these paths are tested.

@nox
Copy link
Member

nox commented Aug 23, 2016

Which part do you not understand?

This patch makes Servo cope with unions including a dictionary type, so I want TestBinding (see TestBinding.webidl) to include some stuff with such unions.

@jdm jdm mentioned this pull request Aug 23, 2016
4 of 4 tasks complete
@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Aug 23, 2016

I added @Ms2ger's suggestion.

@jdm jdm mentioned this pull request Aug 23, 2016
4 of 4 tasks complete
@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Aug 23, 2016

Appveyor error is: "error: failed to build archive: bad archive: permission denied". Intermittent failure?

@nox nox changed the title Dictionary error Update rust-mozjs Aug 24, 2016
@nox
Copy link
Member

nox commented Aug 24, 2016

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


Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


components/script/dom/htmlcanvaselement.rs, line 163 [r1] (raw file):

            let attrs = if let Some(webgl_attributes) = attrs {
                if let Ok(ConversionResult::Success(ref attrs)) = unsafe {

You didn't throw in the case of ConversionResult::Failure here.


components/script/dom/bindings/conversions.rs, line 87 [r1] (raw file):

                         -> Result<ConversionResult<Finite<T>>, ()> {
        let result = match FromJSValConvertible::from_jsval(cx, value, option) {
            Ok(ConversionResult::Success(v)) => v,

Missing throw on failure here.


components/script/dom/bindings/codegen/CodegenRust.py, line 5307 [r1] (raw file):

            initParent = ("parent: match try!(%s::%s::new(cx, val)) {\n"
                          "            ConversionResult::Success(v) => v,\n"
                          "            _ => return Err(()),\n"

Missing throw here.


Comments from Reviewable

@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Aug 24, 2016

components/script/dom/htmlcanvaselement.rs, line 163 [r1] (raw file):

Previously, nox (Anthony Ramine) wrote…

You didn't throw in the case of ConversionResult::Failure here.

Because in case of errors, it returns None instead of an error so I tried to keep the same logic.

Comments from Reviewable

@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Aug 24, 2016

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


components/script/dom/bindings/conversions.rs, line 87 [r1] (raw file):

Previously, nox (Anthony Ramine) wrote…

Missing throw on failure here.

Same as above.

components/script/dom/bindings/codegen/CodegenRust.py, line 5307 [r1] (raw file):

Previously, nox (Anthony Ramine) wrote…

Missing throw here.

Same as above.

Comments from Reviewable

@nox
Copy link
Member

nox commented Aug 24, 2016

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


components/script/dom/htmlcanvaselement.rs, line 163 [r1] (raw file):

Previously, GuillaumeGomez (Guillaume Gomez) wrote…

Because in case of errors, it returns None instead of an error so I tried to keep the same logic.

When the trait returns `Err(())`, it threw something. So you need to throw on `ConversionResult::Failure` too otherwise it makes no sense.

components/script/dom/bindings/conversions.rs, line 87 [r1] (raw file):

Previously, GuillaumeGomez (Guillaume Gomez) wrote…

Same as above.

Same.

components/script/dom/bindings/codegen/CodegenRust.py, line 5307 [r1] (raw file):

Previously, GuillaumeGomez (Guillaume Gomez) wrote…

Same as above.

Same.

Comments from Reviewable

@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Aug 24, 2016

Updated.

@nox
Copy link
Member

nox commented Aug 24, 2016

@bors-servo r+


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


Comments from Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented Aug 24, 2016

📌 Commit 2f3f4a5 has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Aug 24, 2016

Testing commit 2f3f4a5 with merge 3c4a08c...

bors-servo added a commit that referenced this pull request Aug 24, 2016
Update rust-mozjs

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12954)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 24, 2016

@bors-servo bors-servo merged commit 2f3f4a5 into servo:master Aug 24, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:dictionary_error branch Aug 24, 2016
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

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